|
Size: 5644
Comment: recat
|
Size: 6732
Comment:
|
| Deletions are marked like this. | Additions are marked like this. |
| Line 1: | Line 1: |
| == Contributing changes to Mercurial == [[MercurialDevelopmentProcess|Mercurial development process]] resembles the one described in KernelPractice. As most free software projects, contributions and feedback are very welcome and even encouraged, but, to make easier to manage those contributions and keep a clean and maintainable code base, these contributions need to follow a review process before entering the main [[Repository|repo]] (see DeveloperRepos). |
#pragma section-numbers 2 = Contributing Changes = How to help us improve Mercurial's code. |
| Line 4: | Line 5: |
| These are some useful recommendations to increase your chances of getting your contributions accepted (see also '''SuccessfulPatch'''): | /!\ This page is intended for (aspiring) developers. |
| Line 6: | Line 7: |
| * '''Make sure your description of what the [[Patch|patch]] addresses is complete'''. * If using {{{hg export}}}, add the ''description along with the exported patch''. * Make also sure your ''description starts with a reasonable one-line summary''. The first line should be like ''<command/module/topic>: <short description> (optional issue number)'', no more than 80 characters in total, no trailing period, and followed by an empty line. * Make sure the description is complete and can be read and understood without following external references and without reading the actual patch. Describe why the patch is needed, what the intent with the patch is, and what the consequence of the patch is. Small examples and tables can be useful. * '''Send your [[ChangeSet|changes]] as patches to the [[MailingList|mailing list]]''', so other people can comment on them and '''review''' what you're doing. * '''Use [[Export|hg export]] or the [[PatchbombExtension|patchbomb extension]]''' to create and send patches, where possible. * This will get your name in the changelog without any extra editing and, with patchbomb, email messages will be nicely formatted. * We prefer inlined patches over attached patches. It allows reviewer to quote the code when commenting on your patch. * '''Send only one patch per email''', as some review tools and processes assume this. * '''Inline patches are preferable to MIME-attached copies of patches''', because many mailers cannot quote attachments. Inline patches are thus easier to review. Be warned that many mailers will by default screw up the white space of inline patches, so they will not apply cleanly. This is usually easy to work around, for example by using the patchbomb extension or turning off flowed text in mailers such as Alpine. Note that ''inline patch'' '''doesn't''' mean ''inline attachment'' so don't use `-i` with `hg email`. * '''Make sure the patches apply cleanly against the current [[Tip|tip]]''' (either the main or crew repositories are ok). * '''Each patch should make sense in and of itself, and not contain pieces of unrelated stuff''' that you noticed and decided to fix up. * '''Follow the prevailing coding style''' in the code you're modifying, not what you think it ''ought'' to be. See '''BasicCodingStyle''' for a detailed list. * Patches ''should not contain tab characters''. * Patches ''should not contain trailing white space''. * If you are ''cleaning up white space, do it in a self-contained patch'' that contains no functional changes. * '''Run the [[WritingTests|test suite]]'''. Changes breaking the tests are usually integrated with the changes to fix the tests, so that the test suite will run clean in each revision. To test against main or crew, you can clone these repositories somewhere, run `make local` inside the repository, and then call the binary `hg` at the top level of the repository. In Unix you can make a symbolic link from somewhere in your path, such as a location in `/usr/local/bin` to this binary. For example: {{{ ln -s /usr/local/src/mercurial/crew/hg /usr/local/bin/hg-crew |
<<TableOfContents>> == The basics: patches by email == /!\ Don't send your patch to the BugTracker - it can't be reviewed there, so it won't go anywhere! We like to receive changes as patches by email. This allows us to review patches, give feedback, and track which patches need attention. * Patches go to [[MailingLists|mercurial-devel@selenic.com]] - '''no subscription necessary!''' * Patch emails should have '''[PATCH]''' in the subject followed by a summary (not included in the patch) * We prefer patches '''in the message body''' so we can review them (no attachments or URLs!) * Patches should be in the '`# HG changeset patch`' form output by '`hg export`' - unified diff with author and full patch description * Sending a patch implies granting permission to use it in our project under an appropriate [[License|license]] * Apple Mail, Gmail and possibly other mail clients corrupt patches when sending them in the body. Using the patchbomb extension always works; see below for details. Because this is a community project and our developers are very busy, patches will sometimes fall through the cracks. If you've gotten '''no response''' to your patch after a few days, feel free to resend it. == Patch descriptions == It's important that you describe your patch. Patch descriptions should be in the following format: {{{ opener: check hardlink count reporting (issue1866) The Linux CIFS kernel driver (even in 2.6.36) suffers from a hardlink count blindness bug (lstat() returning 1 in st_nlink when it is expected to return >1), which causes repository corruption if Mercurial running ... |
| Line 26: | Line 34: |
| Then you can just invoke `hg-crew`. | * lowercase summary line, no trailing period * start with the most useful identifiable command or subsystem (not necessarily the module name!) * summarize the fix, not the problem * add '(issueNNNN)' to refer to a BugTracker issue (and automatically move the issue to testing) * a blank line after the summary * a more complete description of the problem if necessary * all lines less than 80 characters |
| Line 28: | Line 42: |
| Some other useful guidelines: | Patch descriptions should be aimed at helping the reviewer understand the issue you're addressing. Try to answer the following, where appropriate: |
| Line 30: | Line 44: |
| * '''Don't feel discouraged if you're asked to do some changes.''' Even if you feel the requested changes are trivial, the developers could do it themselves or even they are a bit nitpicky, you're the one who can do them more effectively, and this way of working also optimizes the main developers' time. * '''Always resend the entire patch series,''' not just the changed patch, when you had to fix something in a patch series. To quote [[mpm]]: "In general, assume that I've forgotten everything about your earlier patch." * '''Don't hesitate to resend your patches or ask for review''' if no answer comes from the mailing list after a while... people may be busy and your changes may have been overlooked. Contributions are considered very valuable and you can be sure they deserve at least a response. Sometimes this response is just the act of committing your patch to one of the official repos. Scan the emails about new changesets that are sent to the devel list. * '''It may help to send patches to people directly in addition to the developers mailing list.''' Use `hg log` on the appropriate files to see who's been committing a lot to them. That gives you some guidance as to who to send patches to. People are in general more responsive when messages go straight to them instead of via a mailing list. It may also be helpful to have a look at the CategoryHomepage entries to find the relevant people to contact for a specific subsystem. * '''Joining the IRC channel''' (#mercurial on irc.freenode.net) can be an effective way to get ''faster review and valuable initial feedback''. Again, use `hg log` to see whom to chat up. * You could [[Serve|serve]] a repository over HTTP that contains the changes. With it, Matt only needs to [[Pull|pull]] from it if he likes the changes. |
* why we need this patch * how you've implemented it * why the choices you've made are the right ones * why the choices you didn't make are the wrong ones * what all the corner cases are (consider a table!) * what shortcomings exist * what file formats and data structures you've used * what [[CompatibilityRules|compatibility issues]] exist * what's missing, if anything * what it looks like, if relevant ('''include sample output!''') == Coding style and testing == <!> If you send a patch with an underscore in a variable name, we'll know you haven't read this page! * See our [[CodingStyle|coding style]] for what we expect code to look like (yes, we're serious about the underscores) * Use ''`contrib/check-code.py`'' to check for common style errors * Run the [[WritingTests|test suite]] to make sure you haven't broken anything * Add new test cases as needed (preferably by extending existing tests) * Patches should apply cleanly against the tip of the appropriate branch (default or stable) * Do not include '''unrelated changes or cleanups'''! * '''Don't touch the i18n/''' directory for code or doc changes (translations is a [[TranslatingMercurial|separate process]]) == Organizing patches == If you're making a large change, we're probably going to want it broken into a ''series'' of smaller patches. This makes for easier review and tests both for us and for you. This can be tricky at first and you might find tools like [[MQ]] and [[RecordExtension|record]] useful in this process. Each patch in a series should: * implement one clear step in your process * leave the code in a working state (so bisect always works!) * include relevant test changes so they're independently testable * be sent as a separate email <!> '''Do not mix''' formatting changes, organizational changes, or functional changes in the same patch! Things to consider: * put the least controversial pieces first - if you're lucky, they'll get applied right away * the difficulty of reviewing a patch increases rapidly with size - small patches are more likely to get attention * the probability of a patch getting rejected is also a function of its size - smaller patches mean fewer review round-trips * bigger patches have more bugs - smaller patches make it easier to locate regressions == Mailer issues and patchbomb == As mentioned above, patches should be included in the body of emails to make it easy for developers to review and apply your patches. Some mailers make this difficult by mangling the whitespace in patches or similar. If you have trouble sending clean patches or are sending more than a couple patches, you should probably set up the [[PatchbombExtension|patchbomb extensions]] which automates the process. Add something like the following to your ''`.hgrc`'': {{{ [extensions] patchbomb= [email] method = smtp from = Your Name <your@email.address> to = mercurial-devel@selenic.com [smtp] host = your.smtp.server.com }}} (See [[http://www.selenic.com/mercurial/hgrc.5.html#smtp|the hgrc manual]] for more SMTP configuration options). Then run the following to do a dryrun test: {{{ $ hg email --test <change1> <change2> ... }}} Notable options: * '`--flag`' can be used to flag a patch, e.g. "STABLE", "RFC", "v2" * don't use '`--inline`', it's a weird MIME thing and not what we want == Etiquette and advice == * Feel free to resend patches after a few days if you haven't gotten a response * Try to respond quickly to feedback before we forget what your patch is about * Tell us everything you want us to know about a patch, every time * If we ask for fixes, don't send a patch to your patch, send a new fixed patch * Don't overwhelm us - if you can't get all 10 patches reviewed, try the first 5 * Consult '`hg log`' to cc: the most relevant developers for the code you're working on * Consider giving input on other people's patches * Discuss your patches on [[IRC]] to get faster review and valuable initial feedback == See also == * [[BasicCodingStyle|Basic coding style]] * Our [[CompatibilityRules|compatibility rules]] for new features * The [[PatchbombExtension|patchbomb extension]] automates emailing patches * The [[MQ]] extension is handy for managing patch series |
Contributing Changes
How to help us improve Mercurial's code.
![/!\](/moin-static/modernized/img/alert.png "/!\"){kind=small}
This page is intended for (aspiring) developers.
Contents
1. The basics: patches by email
Don't send your patch to the BugTracker - it can't be reviewed there, so it won't go anywhere!
We like to receive changes as patches by email. This allows us to review patches, give feedback, and track which patches need attention.
Patches go to mercurial-devel@selenic.com - no subscription necessary!
Patch emails should have [PATCH] in the subject followed by a summary (not included in the patch)
We prefer patches in the message body so we can review them (no attachments or URLs!)
Patches should be in the '# HG changeset patch' form output by 'hg export' - unified diff with author and full patch description
Sending a patch implies granting permission to use it in our project under an appropriate license
- Apple Mail, Gmail and possibly other mail clients corrupt patches when sending them in the body. Using the patchbomb extension always works; see below for details.
Because this is a community project and our developers are very busy, patches will sometimes fall through the cracks. If you've gotten no response to your patch after a few days, feel free to resend it.
2. Patch descriptions
It's important that you describe your patch. Patch descriptions should be in the following format:
opener: check hardlink count reporting (issue1866) The Linux CIFS kernel driver (even in 2.6.36) suffers from a hardlink count blindness bug (lstat() returning 1 in st_nlink when it is expected to return >1), which causes repository corruption if Mercurial running ...
- lowercase summary line, no trailing period
- start with the most useful identifiable command or subsystem (not necessarily the module name!)
- summarize the fix, not the problem
add '(issueNNNN)' to refer to a BugTracker issue (and automatically move the issue to testing)
- a blank line after the summary
- a more complete description of the problem if necessary
- all lines less than 80 characters
Patch descriptions should be aimed at helping the reviewer understand the issue you're addressing. Try to answer the following, where appropriate:
- why we need this patch
- how you've implemented it
- why the choices you've made are the right ones
- why the choices you didn't make are the wrong ones
- what all the corner cases are (consider a table!)
- what shortcomings exist
- what file formats and data structures you've used
what compatibility issues exist
- what's missing, if anything
what it looks like, if relevant (include sample output!)
3. Coding style and testing
If you send a patch with an underscore in a variable name, we'll know you haven't read this page!
See our coding style for what we expect code to look like (yes, we're serious about the underscores)
Use contrib/check-code.py to check for common style errors
Run the test suite to make sure you haven't broken anything
- Add new test cases as needed (preferably by extending existing tests)
- Patches should apply cleanly against the tip of the appropriate branch (default or stable)
Do not include unrelated changes or cleanups!
Don't touch the i18n/ directory for code or doc changes (translations is a separate process)
4. Organizing patches
If you're making a large change, we're probably going to want it broken into a series of smaller patches. This makes for easier review and tests both for us and for you. This can be tricky at first and you might find tools like MQ and record useful in this process.
Each patch in a series should:
- implement one clear step in your process
- leave the code in a working state (so bisect always works!)
- include relevant test changes so they're independently testable
- be sent as a separate email
Do not mix formatting changes, organizational changes, or functional changes in the same patch!
Things to consider:
- put the least controversial pieces first - if you're lucky, they'll get applied right away
- the difficulty of reviewing a patch increases rapidly with size - small patches are more likely to get attention
- the probability of a patch getting rejected is also a function of its size - smaller patches mean fewer review round-trips
- bigger patches have more bugs - smaller patches make it easier to locate regressions
5. Mailer issues and patchbomb
As mentioned above, patches should be included in the body of emails to make it easy for developers to review and apply your patches. Some mailers make this difficult by mangling the whitespace in patches or similar. If you have trouble sending clean patches or are sending more than a couple patches, you should probably set up the patchbomb extensions which automates the process.
Add something like the following to your .hgrc:
[extensions] patchbomb= [email] method = smtp from = Your Name <your@email.address> to = mercurial-devel@selenic.com [smtp] host = your.smtp.server.com
(See the hgrc manual for more SMTP configuration options).
Then run the following to do a dryrun test:
$ hg email --test <change1> <change2> ...
Notable options:
'--flag' can be used to flag a patch, e.g. "STABLE", "RFC", "v2"
don't use '--inline', it's a weird MIME thing and not what we want
6. Etiquette and advice
- Feel free to resend patches after a few days if you haven't gotten a response
- Try to respond quickly to feedback before we forget what your patch is about
- Tell us everything you want us to know about a patch, every time
- If we ask for fixes, don't send a patch to your patch, send a new fixed patch
- Don't overwhelm us - if you can't get all 10 patches reviewed, try the first 5
Consult 'hg log' to cc: the most relevant developers for the code you're working on
- Consider giving input on other people's patches
Discuss your patches on IRC to get faster review and valuable initial feedback
7. See also
Our compatibility rules for new features
The patchbomb extension automates emailing patches
The MQ extension is handy for managing patch series