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

From: Jeff King <peff@peff.net>
Date: May 12, 2016, 07:55 UTC
Message-ID: <20160512075555.GC10922@sigill.intra.peff.net>
In-Reply-To: <xmqq37pqubup.fsf@gitster.mtv.corp.google.com>

On Mon, May 09, 2016 at 03:04:46PM -0700, Junio C Hamano wrote:

Show 14 quoted lines

> 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.

As a patch-writer, I often find that splitting the tests from the fix means that you end up having to explain it twice, and often missing some of the context. IOW, it's often hard to explain why a test is checking the right thing without basically explaining the fix. And explaining the fix when it's not part of that patch gets awkward.

So I don't think split tests/fixes are wrong, but I'd urge people to look at how their commit messages turn out. Sometimes the split makes things _easier_ to explain, too.

> *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.

My trick for checking the before/after of a patch is:

  1. Compile git without the patch.

  2. Apply the patch, then run the test (via ./t1234-*, which does not
     want to re-build git), confirm that it fails.

  3. Re-compile and re-run the test, confirming that it passes.

That also works well with "rebase -i" where you stop at the patch before to compile.

I like it because it's simple and doesn't affect git's view (so you can't accidentally commit the in-work-tree revert, for example). But since there's nothing telling you what state the compiled git is in, it can be easy to get confused.

-Peff