[trivial PATCH 12/22] [otp_mppe.c] eliminate build warnings
jhutz at cmu.edu
Tue Feb 2 01:19:06 CET 2010
--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
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
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
More information about the Freeradius-Devel