Re: [PATCH 2/2] pack-objects: only perform verbatim reuse on the preferred pack

From: Jeff King <peff@peff.net>
Date: Nov 15, 2024, 09:57 UTC
Message-ID: <20241115095746.GD1749331@coredump.intra.peff.net>
In-Reply-To: <ZzX9vT4GVqCyfFUE@nand.local>

On Thu, Nov 14, 2024 at 08:40:13AM -0500, Taylor Blau wrote:

Show 22 quoted lines

> > I do think the explanation in the message of the first commit would be a
> > lot simpler if it were simply combined into this patch. With them split
> > you effectively have to explain the problem twice. I don't feel that
> > strongly about changing it, though.
> 
> I always seem to go back and forth on that. I feel somewhat strongly
> that for complicated regression fixes that we should demonstrate the
> existing failure mode in a separate commit with a test_expect_failure.
> That forces the author to ensure they really understand the bug and can
> produce a minimal (or close to it) reproduction.
> 
> It also makes it easier to demonstrate that the fix actually does what
> it says, instead of assuming that the test fails without the fix applied
> (and passes with it applied).
> 
> That does force the author to potentially explain the bug twice. In my
> experience, I tend to keep the explanation in the first patch relatively
> brief, hinting at details that will appear in the subsequent patch
> instead of explaining them in full detail.
> 
> So I dunno. It's a tradeoff for sure, but I think having an explicit
> point in the log that demonstrates the existing bug is valuable.

I do think it's important to make sure your tests fail in the way you expect. I can't count the number of times I have _thought_ I had a solution and then after seeing that the test didn't fail without my patch, had to go back and study it more. :)

But I generally do it with:

  # build the old version
  git checkout HEAD^
  make

  # now go back to the new and test _without_ building
  git checkout -
  cd t && ./t1234-whatever.sh

(or more likely with a "break" in "git rebase -i" for a longer series).

That is, I'm running a mis-matched test/build combo. I guess in one sense that is horrible and I'm a bad person. It is easy to make a mistake and test the wrong thing. But it does make the resulting patches easier to read/review, IMHO.

  As an aside, I think this kind of mismatch gets awkward with the
  proposed meson out-of-tree build stuff. Running the tests through
  meson would presumably trigger a build. And running without means
  pointing to the built git binaries manually.

-Peff