#pragma section-numbers 2 <> = Contributing Changes = How to help us improve Mercurial's code. <> == Submission checklist == Please double-check your patch meets the following guidelines (explained below) before submitting for review: 1. first line of commit message is of the form "'''topic: uncapitalized, no trailing period'''" 1. bugs that are resolved are mentioned in summary in the form "'''(issueNNNN)'''" (no space) 1. patch [[OneChangePerPatch|does just one thing]] (if you need a bullet list, split your patch) 1. patch contains no other gratuitous changes: * whitespace changes * code movement * typo fixes * string changes 1. patch leaves Mercurial in a working state (doesn't depend on future patches) 1. test suite has been updated and [[#Coding_style_and_testing|runs cleanly]] (use the [[Heptapod|Heptapod]] CI to make sure) 1. code passes check-code and matches [[CodingStyle|coding style]] 1. relevant help text is updated (see `mercurial/help/`) If emailing: 1. patches are sent to the mercurial-devel mailing list (not the [[Bugs|BTS]]) and are properly formatted (see below) 1. appropriate branch is indicated in email subject (eg hg email --flag stable) 1. version is indicated in email subject if this is an amended patch, (eg hg email --flag v2 if it has been amended once, or v3, v4, etc.) == Getting started == Start by cloning a copy of Mercurial's main development repo: {{{ hg clone https://foss.heptapod.net/mercurial/mercurial-devel }}} You'll also want to read the following pages: * MercurialApi * CodingStyle * CompatibilityRules * TimeBasedReleasePlan * OneChangePerPatch Other pages you may find important: * LockingDesign * EncodingStrategy == Organizing patches == {X} If your first submission is not 'minimal', you will probably be sent back here. Save yourself time and start small! If you're making a large change, we're probably going to want it broken into a ''series'' of smaller patches (see OneChangePerPatch). 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 [[HisteditExtension|histedit]] and [[EvolveExtension|the Evolve extension]] 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 * if emailing: be sent as a separate email '''Do not mix''' formatting changes, organizational changes, or multiple 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 === Editing history === You will almost certainly find it necessary to do some form of history editing to generate clean commits, especially after you get your first review feedback. There are multiple tools of varying degrees of power available for this purpose: * [[Cmd:commit|commit --amend]] - modify a head commit's description and contents * [[RebaseExtension|rebase]] - move your commits from one part of history to another, combine commits * [[HisteditExtension|histedit]] - interactively rearrange and modify your commits * [[EvolveExtension|evolve]] - our vision for how history editing should work in the future (experimental) * [[MqExtension|mq]] - manage a stack of changes as patches == 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 ( {{{ python contrib/check-code.py --blame `/usr/bin/hg manifest` }}} ) * Add new [[WritingTests#Be_careful_with_new_test_scripts.21|test cases as needed]] * [[WritingTests#Running_the_test_suite|Run the test suite]] to make sure you haven't broken anything * Patches should apply cleanly against the tip of the appropriate branch (default or stable) * '''Don't touch the i18n/''' directory for code or doc changes (translations is a [[TranslatingMercurial|separate process]]) == Example patch == Here is an example of a Mercurial patch with proper summary, description, coding style, and testing. {{{ changeset: 21111:dda11e799529 branch: stable user: FUJIWARA Katsunori date: Tue Mar 25 19:34:17 2014 +0900 files: mercurial/hg.py tests/test-clone.t description: hg: use "os.path.join()" to join path components which may be empty (issue4203) Changset 2d0ab571b822 rewriting "hg.copystore()" with vfs uses 'dstbase + "/lock"' instead of "os.path.join()", because target files given from "store.copyfiles()" already uses "/" as path separator But in the repository using revlog format 0, "dstbase" becomes empty ("data" directory is located under ".hg" directly), and 'dstbase + "/lock"' is treated as "/lock": in almost all cases, write access to "/lock" causes "permission denied". This patch uses "os.path.join()" to join path components which may be empty in "hg.copystore()". diff -r c57c9cece645 -r dda11e799529 mercurial/hg.py --- a/mercurial/hg.py Mon Mar 24 21:27:40 2014 -0400 +++ b/mercurial/hg.py Tue Mar 25 19:34:17 2014 +0900 @@ -213,8 +213,10 @@ dstvfs.mkdir(dstbase) if srcvfs.exists(f): if f.endswith('data'): + # 'dstbase' may be empty (e.g. revlog format 0) + lockfile = os.path.join(dstbase, "lock") # lock to avoid premature writing to the target - destlock = lock.lock(dstvfs, dstbase + "/lock") + destlock = lock.lock(dstvfs, lockfile) hardlink, n = util.copyfiles(srcvfs.join(f), dstvfs.join(f), hardlink) num += n diff -r c57c9cece645 -r dda11e799529 tests/test-clone.t --- a/tests/test-clone.t Mon Mar 24 21:27:40 2014 -0400 +++ b/tests/test-clone.t Tue Mar 25 19:34:17 2014 +0900 @@ -621,3 +621,17 @@ #endif $ cd .. + +Test clone from the repository in (emulated) revlog format 0 (issue4203): + + $ mkdir issue4203 + $ mkdir -p src/.hg + $ echo foo > src/foo + $ hg -R src add src/foo + $ hg -R src commit -m '#0' + $ hg -R src log -q + 0:e1bab28bca43 + $ hg clone -U -q src dst + $ hg -R dst log -q + 0:e1bab28bca43 + $ cd .. }}} == Changing C code == Changes involving the C code should be done so that the tests will pass across changesets even without recompiling. This allows "bisect" to be run without needing to run "make" each time, for example. A recommended strategy for changing behavior in the C code is to add a new interface to the C code when changing behavior (do not change old interfaces). Then in the Python code, check for the existence of the new interface. If you change tests that rely on changes to C extensions, make sure you also run the tests in "pure" Python mode (and vice versa). For example: {{{ $ python run-tests.py --pure }}} == 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 topic keyword (eg command name, subsystem), (the keywords mostly exist to sort our end-user release notes). * summarize the fix, not the problem * add '(issueNNNN)' if it fixes an issue in the BugTracker (and automatically move the issue to testing) * use '(BC)' to flag backward compatibility changes, use '(API)' to flag major internal API changes. * 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 use the form "When I tried to do X, I got result Y, but the result should be Z". This is better than "X does not work" which assumes a common understanding of what it means for X to work and leaves the reader to intuit what Y and Z might have been. 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 [[CompatibilityRules|compatibility issues]] exist * what's missing, if anything * what it looks like, if relevant ('''include sample output!''') == Sending patches == /!\ Don't send your patch to the BugTracker - it can't be [[ManagingBugs#Dealing_with_patches_on_the_BTS|reviewed there]], so it won't go anywhere! /!\ Sending a patch implies granting permission to use it in our project under an appropriate [[License|license]] There are multiple options for sending patches to Mercurial. In order of preference: 1. Heptapod. See [[Heptapod]] for usage instructions. 1. email (see below) If you have already multiple series waiting for review, sending any more patches is not advised. Please, please, please read the [[#Flow_control|Flow control]] section. 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. Find the patch submission on the [[MailingLists|mercurial-devel@mercurial-scm.org]] and reply to it, kindly requesting someone look at it. If it was submitted to Heptapod, you can leave a comment on Heptapod. === Emailing patches === The (heavily) recommended way of sending changes is through [[Heptapod]]. Email is kept as an option as a low-friction tool for small changes for drive-by contributors. * Patches go to [[MailingLists|mercurial-devel@mercurial-scm.org]] - '''no subscription necessary!''' (we manually whitelist all legitimate posters) * Patch emails should have '''[PATCH]''' in the subject followed by a summary (not included in the patch). New versions of the patch should be flagged with V2, V3 etc. so it becomes e.g. '''[PATCH V2]'''. See --flag option to hg email. * 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 (see also the PatchbombExtension) * Apple Mail, Gmail and possibly other mail clients corrupt patches when sending them in the body, even when sending as plain text. Use the patchbomb extension, instead. The best way to achieve well-formed patches is to use [[PatchbombExtension|patchbomb extension]] which automates the process. Add something like the following to your ''`.hgrc`'': {{{ [extensions] patchbomb= [patchbomb] confirm=True intro=never [email] to = mercurial-devel@mercurial-scm.org from = Ada Lovelace method = smtp [smtp] host = smtp.gmail.com port = 587 tls = starttls username = adalovelace@gmail.com }}} Run `hg help config.smtp` for more SMTP configuration options and `hg help email` and `hg help -e patchbomb` for more info on the `email` command and the extension that provides it. Then run the following to do a dryrun test: {{{ $ hg email --test ... }}} 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 * '`--in-reply-to`' can be used to specify a message-id to preserve threading (helpful for followup patches). * Please '''do not''' send a 0 of X summary message. Those will be deleted in the inbox of code reviewers, and never be read. It's totally fine to discuss the history and purpose of a patch in a patch description. Future archaeologists will thank you. == Flow control == Please try not to swamp the list with patches. We have more contributors than reviewers and we'd like to be sure everyone's patches get looked at. Other advice: * Avoid sending more than 10 patches at once. If you have a big stack of patches for review, get them reviewed small group (around 6 patches) by small group. If your stack is very big you probably want to tell people on the list about it to figure out a way to get it reviewed. It's not uncommon for mechanical cleanups to get special attention from a dedicated reviewer. * Avoid having too many series in review at the same time. If you already have multiple series in review (even unrelated) sending more unrelated series is not going to help. Apply feedback you received on existing series to close them. Or do some review yourself to help people reviewing your patches. (see ReviewProcess) == Etiquette and advice == * 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 * 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 ---- CategoryDeveloper