On Thu, Nov 14, 2024 at 08:40:13AM -0500, Taylor Blau wrote: > > 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
