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

[Taylor Blau <me@ttaylorr.com>]

On Tue, Jul 07, 2026 at 09:59:42AM +0000, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
>
> Add a step counter and trace2_data_intmax call to
> compute_reachable_generation_numbers() to make the cost of
> the generation number DFS observable.  This exposes a
> regression introduced in 199d452758 (commit-graph: fix
> "filling in" topological levels, 2025-04-07) where
> incremental commit-graph writes re-walk the entire commit
> ancestry instead of reading topo levels from lower graph
> layers.

Makes sense.

> 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.

> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
>  commit-graph.c                |  5 +++++
>  t/t5324-split-commit-graph.sh | 28 ++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 801471a098..4e39a048c4 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -1653,6 +1653,7 @@ static void compute_reachable_generation_numbers(
>  {
>  	int i;
>  	struct commit_list *list = NULL;
> +	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.

That is outside the scope of your patch, though, so what you have
below here is fine in my opinion.

> +		# BUG: topo levels from lower graph layers are not
> +		# propagated, so the DFS re-walks from base-3 down to
> +		# the root (7 steps) instead of reading topo levels
> +		# from the existing graph (1 step).
> +		test_trace2_data commit-graph generation-dfs-steps 7 <trace.txt

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".

Thanks,
Taylor