Re: [PATCH v2 00/33] Yet more preparation for reference backends

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

Michael Haggerty <mhagger@alum.mit.edu> writes:

>> The "demonstrate an existing breakage first" order makes it slightly
>> easier to review and follow a long series, as it forces the reviewer
>> to see the issue first and think about possible avenues to solve it
>> for themselves, before seeing a paticular solution.  For a trivial
>> single-issue fix, it is not necessary (including a fix and a test to
>> protect the fix from future breakage in the same patch is a norm).
>
> I find it useful to add the broken test in a separate patch, because it
> is then easy to cherry pick that patch to other versions of Git to
> discover which ones are also affected by the problem. If the addition of
> the test is combined with the fix, then the patch would more often fail
> to apply to other versions due to conflicts at the locations of the fix,
> and even if it applied, you wouldn't learn whether that version of git
> was broken but the breakage was fixed by the same fix, or whether it
> wasn't broken in the first place and the fix was unnecessary.

Note that I only said "it is not necessary", and did not say "it is
not desirable".  I wouldn't automatically reject a two patch series
with demonstration followed by fix, only because they are not in a
single patch.

Even when I know the maintenance track a particular fix and test
targets at, I'd do the "only try to test to see how it is broken
currently" step manually anyway as part of the initial "acceptance"
check when applying [*1*], so trying the same procedure for older
maintenance tracks is no big burden for me.  Having these as two
separate patches is easier to split them apart, which unfortunately
makes it easier to lose one of them while cherry-picking.

This is of course subjective.


[Footnote]

*1* There is a bit of white-lie here.  Instead of applying only t/
    part, I tend to just do "git am" the whole thing, and then pipe
    "git show" to "git apply -R" to in-work-tree revert only the
    code that fixes it.  But the result I get is the same.

    And the "cherry-picking" would also involve "show only the t/
    part and pipe that to "git apply", which is even simpler than
    actually cherry-picking and creating a commit (I do not have to
    be very careful not to leave such a "test cherry-pick" commit in
    the real history).