Re: [PATCH v1 2/3] sub-process: refactor the filter process code into a reusable module

[Junio C Hamano <gitster@pobox.com>]

Ben Peart <Ben.Peart@microsoft.com> writes:

> How about I squash the last two patches together so that its more
> apparent that it's just a refactoring of existing code with the before
> and after in a single patch?

I do not think making a pair of patches, each already does too much,
into one patch would make things easier to chew.  It would make it
even harder to understand.

I offhand do not know how many patches the ideal split of this
series should be, but I know it shouldn't be one.

More likely that it should be N+1, and I know the last one should be
"Now all data structures and variables have been renamed, all helper
functions have been renamed and generalized for reuse while the
original code these helper functions came from have been updated to
call them, we can move them from convert.c to sub-process.[ch];
let's create these two files".  This last step will be moving lines
from an old file to two new files, almost without any modification
(of course, "#ifndef SUB_PROCESS_H", "#include sub-process.h", etc.
would be new lines so this will not be strictnly pure movement, but
all readers of this message are intelligent enough to understand
what I mean).

What would the other N patches before the last step should contain,
then?

For example, your name2process_cmp() is just a renamed version of
the original.  Some of its callers may also just straight rename.
These "only renaming, doing nothing else" changes can go into a
single large patch and as long as you mark as such, the review
burden can be lessened.  It would be a "boring mechanical" part of
the whole thing.

On the other hand, your subprocess_start() shares quite a lot with
the original start_multi_file_filter() but the latter does some more
than the former (because the latter is more specific to the need of
convert.c).  

A patch series must be structured to make it easier to review such
changes, because they are the likely places where mistakes happen
(both in implementation and in the helper API design).  To get from
the original start_multi_file_filter() to a new version that calls
subprocess_start(), such a patch would _MOVE_ bunch of lines from
the old function to the new function, the new function may acquire
its own unique new lines, the old function would lose these moved
lines but instead call the new function, etc.  You can call it as "I
refactored subprocess_start() out of start_multi_file_filter() and
updated the latter to call the former." and that would be a single
logical unit of the change.  To make patches easier to understand,
these logical unit would want to be reasonably small.

And for something of sub-process.[ch]'s size, I suspect that it
would have more than 1 such logical unit to be independently
refactored, so in total, I would suspect the series would become

    1 (boring mechanical part) +
    2 or more (refactoring) +
    1 (final movement)

i.e. 4 or more patches?