Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS

[Taylor Blau <ttaylorr@openai.com>]

On Fri, Jul 10, 2026 at 03:28:11PM -0700, Junio C Hamano wrote:
> Taylor Blau <ttaylorr@openai.com> writes:
>
> > On Tue, Jul 07, 2026 at 04:08:36PM +0200, Kristofer Karlsson wrote:
> >> > Instead of writing "# BUG ..." and then an incorrect assertion, I
> >> > would suggest that you write the assertion you expect:
> >> >
> >> >     test_trace2_data commit-graph generation-dfs-steps 1 <trace.txt
> >> >
> >> > , but mark the test as "test_expect_failure".
> >>
> >> I started with this actually and then changed my mind in order
> >> to demonstrate exactly how the counter changed, not just that it
> >> changed from failure to success. But I'd be happy to change this
> >> too if needed - it would effectively reduce the second commit to
> >> just the bugfix line and switching from test_expect_failure
> >> to test_expect_success.
> >
> > Yeah, I think this would be ideal.
>
> If the test involved is longer than 3 lines, I would recommend
> against it, as "git show" of such a patch will show the full code
> change to implement a different behaviour plus "_failure" changing
> to "_success" in the test, with the body of the test hidden outside
> the context, which makes it hard to guess what the behaviour change
> is really about.

Hmm, I am not sure that I agree. Or, at the very least, that is now how
I have written series in the past where I want to demonstrate and then
subsequently fix an existing bug.

When either the test setup or the bugfix is trivial, I think having it
in the same commit is just fine. But I think there are two good reasons
for splitting it out if the test or bug is complex:

 - If the test is complex, but the complexity is not directly related to
   the bugfix, having to explain both in the same commit message can be
   awkward, and makes it harder for a reviewer to reason about either
   component of the patch.

 - If the bugfix is complex, having the failing test in a separate
   commit demonstrates that the bug existed before, but is definitively
   fixed in the following commit, as both would be expected to 'make
   test' cleanly.

I am happy to change my style if you feel strongly. It would be nice to
document this in CodingGuidelines (or SubmittingPatches?) if it is not
already.

Thanks,
Taylor