[trivial PATCH 12/22] [otp_mppe.c] eliminate build warnings

[Jeffrey Hutzelman]

--On Tuesday, February 02, 2010 12:01:04 AM +0100 Alan DeKok 
<aland at 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+ at cmu.edu>
   Carnegie Mellon University - Pittsburgh, PA