Note:

This page is primarily intended for developers of Mercurial.

One Change Per Patch

Why and how to make one change per patch.

Contents

  1. What does "one change per patch" mean?
  2. Why so strict?
    1. Reviewability
    2. Selectability
    3. Bisectability and regression testing
  3. Guidelines
    1. Tests
  4. Some examples
    1. Breaking a complex change into a number of steps
    2. Multiple simultaneous changes causing problems
  5. This discipline is hard!
  6. See also

1. What does "one change per patch" mean?

Mercurial's development process insists on "one change per patch" and code that doesn't follow this rule routinely gets rejected. This page tries to explain what "a change" is and why this rule exists.

One possible definition is: "a change is the smallest reasonable design increment." The ideal patch is "trivial and obviously correct". But it may be easier to define in terms of what is not a single change. Here are some examples:

Quite often, fixing a bug or adding a feature will itself involve multiple logical steps, for instance first factoring out a helper function, then rewriting part of that helper function.

In general, if your description of your change uses 'and' or includes a bullet list of changes... you've probably got more than one change. On the other hand, if you're doing some large but highly repetitive change (eg "change all uses of repo.foo to repo.bar"), then that's an acceptable single change.

2. Why so strict?

The short answer is: it's good engineering practice.

The traditional software development approach is "code, test, code, test, code, test, commit", where the final commit contains a large number of different changes that achieve some larger overall result. This is really unfortunate from an engineering perspective because it's not possible to isolate the history of the internal steps from each other. In the jargon of the scientific method, your change is an experiment on multiple independent variables, and just like such an experiment, it's hard to draw conclusions when the experiment fails.

Mercurial's strategy is instead "code, test, commit, code, test, commit", with each step being a clearly-delineated step towards our goal. This has various benefits:

2.1. Reviewability

Simple changes are much easier to review, because the reviewer doesn't need to independently correlate each line of change with a different piece of the commit description, or assign it to an implicit goal like "tidy things up as we go". A simple change is often "obviously correct" and can be reviewed with very little thought. Five simple changes rolled together are often impenetrable and often take much more than five times as long to properly review.

2.2. Selectability

If the nth step of a series of changes doesn't pass review, the reviewer's time reading patches 1 through n-1 hasn't been wasted and won't have to be repeated on the next iteration. Those patches can be accepted, and the process has now made forward progress.

2.3. Bisectability and regression testing

If some part of a large commit introduces a problem that gets discovered later, it is much harder to isolate it from other changes. By contrast, with single-change commits, systematic search via bisect can rapidly pinpoint problems.

3. Guidelines

The following changes should never be combined:

3.1. Tests

In general, tests should pass before and after every change.

Thus, if a change breaks existing tests, those tests should be updated together with a test.

If a change fixes a bug, a test for that bug should be introduced either after or together with the corresponding change. Do not introduce failing or incorrectly-passing tests before a change.

4. Some examples

4.1. Breaking a complex change into a number of steps

These changes were all introduced in a single session to optimize performance of a single code path:

As it turns out, the first patch introduced a performance regression in another workload that was trivially located by an end-user via bisect. Had these patches been combined, it would have been very difficult to locate.

4.2. Multiple simultaneous changes causing problems

5. This discipline is hard!

Yes, but it becomes easier with practice and good tools. Unfortunately, core Mercurial (and SCMs in general) aren't particularly well-suited to the iterative refinement of changes implied here. Several tools that are helpful here are:

Also, we have an ongoing effort to integrate intelligent mutable history into our core:

6. See also

CategoryDeveloper

OneChangePerPatch (last edited 2012-10-23 06:54:38 by SimonHeimberg)