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

On Tue, 7 Jul 2026 at 15:47, Taylor Blau [off-list ref] wrote:

quoted

Add a test that demonstrates the problem: with a two-layer
split commit-graph, writing a new incremental layer for a
commit whose parent is in the base layer walks all the way
down to the root (7 steps for 5 base commits) instead of
reading the existing topo level and stopping immediately
(1 step).

This paragraph only describes verbatim what is already included in the
patch. I think we could easily do without it, but I do not feel so
strongly about it.

I also don't feel strongly about it, I could remove it entirely.

quoted

+     intmax_t steps = 0;

Any reason that this should be signed? Obviously in practice, I don't
think we're going to wrap around with a greater-than-INT_MAX number of
commits here, but perhaps we would at the very least prefer uintmax_t.

I guess trace2 only has a data_intmax() function, so perhaps the point
is moot. Regardless, it seems that we would want to have a convenience
wrapper to be able to print out unsigned integer values which are
otherwise un-representable as signed integers.

Yes, my only rationale here was to match the type that
trace2_data_intmax expects - and as you say, it's very
unlikely that we'll need to use all bits anyway, and since
this is only used for testing and debugging, and overflows
would be noticed that way and would not affect general
correctness.

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.

Thanks,
Kristofer