On Thu, Nov 14, 2024, at 14:40, Taylor Blau wrote: > On Wed, Nov 13, 2024 at 07:25:04PM -0500, Jeff King 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). I recently made a parallel branch for a topic where the parallel branch had `test_expect_failure` for each commit (i.e. the commits had only `t/` changes). That ended up catching a bug I introduced when I tried to simplify the test: the test was OK on my topic branch but didn’t fail (`test_expect_failure`) on the parallel branch. I use a worktree for `master` so at least I didn’t have to build specifically for that branch. -- Kristoffer Haugsbakk
