--On Tuesday, February 02, 2010 12:01:04 AM +0100 Alan DeKok <aland@deployingradius.com> wrote:
Frank Cusack wrote:
Grouping 22 1-line patches into a single changeset is not large. OTOH submitting them as 22 individual patches belies a naive understanding of source control, and actually makes merging that much more difficult, or at least time consuming.
I also prefer changes to function prototypes to be in the same patch as changes to the functions... otherwise the build breaks.
If you'll allow me to borrow the soapbox for a moment... The guiding principal I've often seen and used is typically expressed as "one change per patch; one patch per change". That is, the fundamental property of interest is not size, but the number of logically separate changes which are part of the same patch. The correct number is always exactly one. Multiple related edits like changing a function's prototype, making the corresponding change to its definition, and updating all of the call sites are really a single change, and should be part of the same patch, and thus the same commit. The same goes for a group of edits spread across the tree that introduce a new feature or fix a bug. If they're part of one logical change, they should be part of one commit. A good rule to use here is to combine things that are syntactically or semantically interdependent, such that applying only part of the change results in a tree that doesn't build or that no longer works correctly. The flip side is that unrelated things should generally not be part of the same patch. Don't fix a bug and add a feature in the same patch. Or fix two bugs, or add two features. That goes triple for keeping cosmetic/stylistic changes such as a reindent separate from warning elimination, and both separate from actual functional changes. Putting unrelated changes in the same patch makes that patch harder to review, makes it harder to accept one change but not the other (which may result in the whole thing being rejected), and makes it harder to track down what happened if one of those changes introduced a bug. "git bisect" is a powerful tool for tracking down when a problem was introduced, but can't split the difference between two changes that were part of the same commit. There is, of course, some grey here. Some projects prefer to get things like reindent or warning elimination in larger chunks, arguing that such changes are related even if not interdependent, and are actually easier to merge and review in large batches. Alan has clearly indicated his preference to be the opposite, and around here, he gets to decide. Similarly, it may often be possible to take a large set of changes to add a complex new feature, and break it down into smaller parts, each building on the last. Such things are often easier to review and test, which is always a good thing. -- Jeffrey T. Hutzelman (N3NHS) <jhutz+@cmu.edu> Carnegie Mellon University - Pittsburgh, PA