Skip to content

commit-graph: fix topo_levels slab propagation regression #2170

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
spkrka wants to merge 2 commits into
base: master
Choose a base branch
from
+30 −1
Open

commit-graph: fix topo_levels slab propagation regression #2170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
100efa2
commit-graph: add trace2 instrumentation for generation DFS
spkrka Jul 9, 2026
679dd2e
commit-graph: propagate topo_levels slab to all chain layers
spkrka Jul 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion commit-graph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1653,6 +1653,7 @@ static void compute_reachable_generation_numbers(
{

@gitgitgadget gitgitgadget Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taylor Blau wrote on the Git mailing list (how to reply to this email):

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

@gitgitgadget gitgitgadget Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):

On Tue, 7 Jul 2026 at 15:47, Taylor Blau <me@ttaylorr.com> wrote:
>
> > 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.

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

@gitgitgadget gitgitgadget Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taylor Blau wrote on the Git mailing list (how to reply to this email):

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.

Thanks,
Taylor

@gitgitgadget gitgitgadget Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

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.

@gitgitgadget gitgitgadget Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taylor Blau wrote on the Git mailing list (how to reply to this email):

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

@gitgitgadget gitgitgadget Bot Jul 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

Taylor Blau <ttaylorr@openai.com> writes:

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

After applying and in viewing "git log -W -p", there is no such
difficulty like the one I described in the message you are
responding to, but it makes it harder on reviewers on the mailing
list, to make a quick pre-review based only on the material that
they can see in the e-mail.

It may be easier to write the commits, but given that we seem to
have more patches sent to the list than reviewers can review, it may
not be a good trade-off.

@gitgitgadget gitgitgadget Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):

On Sat, 11 Jul 2026 at 23:18, Junio C Hamano <gitster@pobox.com> wrote:
>
> Taylor Blau <ttaylorr@openai.com> writes:
>
> >> 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.
>
> After applying and in viewing "git log -W -p", there is no such
> difficulty like the one I described in the message you are
> responding to, but it makes it harder on reviewers on the mailing
> list, to make a quick pre-review based only on the material that
> they can see in the e-mail.
>
> It may be easier to write the commits, but given that we seem to
> have more patches sent to the list than reviewers can review, it may
> not be a good trade-off.

I've been pondering this dilemma for a bit. I agree with Taylor
that atomic commits are valuable and I quite like proving the bug
exists before fixing it. It's not black and white though,
for race conditions or hard to reproduce cases I tend to fold the
test into the fix commit directly instead.

But the review process is also critical and its overhead should be
minimized.

Could tooling help here? The submitter should know which parts
of the patch need more context for review. If they could selectively
expand context before sending, reviewers would see the full picture
in the email without sacrificing having atomic commits.

git apply already handles patches with extra context lines just
fine, so we just need something to assist in producing that extra
context -- either some configurability in git format-patch itself
(like -W, but more fine-grained control over _where_ that gets
applied) or some post-processing tool to expand context in patches
before sending.

Too late for this round, but I might give that a try in the future
if I run into a similar scenario again.

Thanks,
Kristofer

@gitgitgadget gitgitgadget Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

Kristofer Karlsson <krka@spotify.com> writes:

> I've been pondering this dilemma for a bit. I agree with Taylor
> that atomic commits are valuable and I quite like proving the bug
> exists before fixing it.

I do not quite understand.  Even if you fix the code and add a
passing test, the commit remains atomic.  With an artificial
split, you only increase your commit count while making the changes
harder to review.  When grouping a code fix with a newly passing
test:

  * "git show" displays both the implementation changes and the
    test.  You can review both, and if you agree with the behavior
    expected by the test, the change is complete.

  * If the pre-fix behavior is unclear, it is easy to check by
    running:

      $ git show ':!t/' | git apply -R && make test

    This demonstrates exactly how the unfixed code breaks on the
    new test.

> Too late for this round, but I might give that a try in the future
> if I run into a similar scenario again.

The existing tooling already supports this workflow (as demonstrated
by the command above).  Please avoid artificially making the context
larger, as doing so increases the likelihood of merge conflicts with
other changes.

@gitgitgadget gitgitgadget Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):

On Mon, 13 Jul 2026 at 22:42, Junio C Hamano <gitster@pobox.com> wrote:
>
> I do not quite understand.  Even if you fix the code and add a
> passing test, the commit remains atomic.  With an artificial
> split, you only increase your commit count while making the changes
> harder to review.  When grouping a code fix with a newly passing
> test:
>
>   * "git show" displays both the implementation changes and the
>     test.  You can review both, and if you agree with the behavior
>     expected by the test, the change is complete.
>
>   * If the pre-fix behavior is unclear, it is easy to check by
>     running:
>
>       $ git show ':!t/' | git apply -R && make test

That's quite neat, and it matches the local
development flow if you write the failing test first.

I can see the advantages of grouping the test and bugfix in the
same commit, and I'm happy to follow that convention going forward.

> > Too late for this round, but I might give that a try in the future
> > if I run into a similar scenario again.
>
> The existing tooling already supports this workflow (as demonstrated
> by the command above).  Please avoid artificially making the context
> larger, as doing so increases the likelihood of merge conflicts with
> other changes.

Thanks, that makes sense. It was an interesting thought experiment,
but I'll leave it there.

- Kristofer

@gitgitgadget gitgitgadget Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> 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

Where did "fix filling in" came from?  Are you blaming

    199d452758 (commit-graph: return the prepared commit graph from
    `prepare_commit_graph()`, 2025-09-04)

or something else that happend in April that year?

> incremental commit-graph writes re-walk the entire commit
> ancestry instead of reading topo levels from lower graph
> layers.

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

OK.  I expect that [2/2] would update this exact test to demonstrate
that with code updated in [2/2] the extra walk will no longer happen.

> 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;
>  
>  	for (i = 0; i < info->commits->nr; i++) {
>  		struct commit *c = info->commits->items[i];
> @@ -1671,6 +1672,7 @@ static void compute_reachable_generation_numbers(
>  			int all_parents_computed = 1;
>  			timestamp_t max_gen = 0;
>  
> +			steps++;
>  			for (parent = current->parents; parent; parent = parent->next) {
>  				repo_parse_commit(info->r, parent->item);
>  				gen = info->get_generation(parent->item, info->data);
> @@ -1694,6 +1696,9 @@ static void compute_reachable_generation_numbers(
>  			}
>  		}
>  	}
> +
> +	trace2_data_intmax("commit-graph", info->r,
> +			   "generation-dfs-steps", steps);
>  }

Pretty-much trivial addition of a trace element.

> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
> index 49a057cc2e..f9c57760f4 100755
> --- a/t/t5324-split-commit-graph.sh
> +++ b/t/t5324-split-commit-graph.sh
> @@ -718,6 +718,34 @@ test_expect_success 'write generation data chunk when commit-graph chain is repl
>  	)
>  '
>  
> +test_expect_success 'incremental write reads topo levels from all layers' '
> +	git init topo-from-lower &&
> +	(
> +		cd topo-from-lower &&
> +
> +		for i in $(test_seq 5)
> +		do
> +			test_commit base-$i || return 1
> +		done &&
> +		git commit-graph write --reachable &&
> +
> +		test_commit extra &&
> +		git commit-graph write --reachable --split=no-merge &&
> +
> +		git checkout base-3 &&
> +		test_commit new-branch &&
> +
> +		GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
> +			git commit-graph write --reachable --split=no-merge &&
> +
> +		# 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
> +	)
> +'
> +
>  test_expect_success 'temporary graph layer is discarded upon failure' '
>  	git init layer-discard &&
>  	(

@gitgitgadget gitgitgadget Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):

On Tue, 7 Jul 2026 at 18:56, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > 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
>
> Where did "fix filling in" came from?  Are you blaming
>
>     199d452758 (commit-graph: return the prepared commit graph from
>     `prepare_commit_graph()`, 2025-09-04)
>
> or something else that happend in April that year?

Hm, I actually don't remember that exact text, it must have been
an oversight during editing back and forth and I missed it in
my local review -- the commit oid is correct though, that is
the one I was referring to. I will clean this up and shrink it down.

I did not mean April though, but September 4th. I was using
the ISO 8601 date format out of habit.

> OK.  I expect that [2/2] would update this exact test to demonstrate
> that with code updated in [2/2] the extra walk will no longer happen.

Yes, I first considered doing this as a single commit, but
I figured it would be easier to reason about the fix if the
problem was identified before-hand.

Thanks,
Kristofer

int i;
struct commit_list *list = NULL;
intmax_t steps = 0;

for (i = 0; i < info->commits->nr; i++) {
struct commit *c = info->commits->items[i];
Expand All @@ -1671,6 +1672,7 @@ static void compute_reachable_generation_numbers(
int all_parents_computed = 1;
timestamp_t max_gen = 0;

steps++;
for (parent = current->parents; parent; parent = parent->next) {
repo_parse_commit(info->r, parent->item);
gen = info->get_generation(parent->item, info->data);
Expand All @@ -1694,6 +1696,9 @@ static void compute_reachable_generation_numbers(
}
}
}

trace2_data_intmax("commit-graph", info->r,
"generation-dfs-steps", steps);
}

static timestamp_t get_topo_level(struct commit *c, void *data)
Expand Down Expand Up @@ -2605,7 +2610,7 @@ int write_commit_graph(struct odb_source *source,

@gitgitgadget gitgitgadget Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taylor Blau wrote on the Git mailing list (how to reply to this email):

On Tue, Jul 07, 2026 at 09:59:43AM +0000, Kristofer Karlsson via GitGitGadget wrote:
> diff --git a/commit-graph.c b/commit-graph.c
> index 4e39a048c4..c2a711cceb 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -2610,7 +2610,7 @@ int write_commit_graph(struct odb_source *source,
>
>  	g = prepare_commit_graph(ctx.r);
>  	for (struct commit_graph *chain = g; chain; chain = chain->base_graph)
> -		g->topo_levels = &topo_levels;
> +		chain->topo_levels = &topo_levels;
>
>  	if (flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS)
>  		ctx.changed_paths = 1;

Looks obviously good.

I think that there is a more permanent fix, though, which would have not
allowed this bug to evade both its author, and reviewer (me). I *think*
that we may clear up some scoping issues if we removed g->topo_levels
entirely, and instead stored it in the write_commit_graph_ctx struct.

I haven't thought through the implications of doing so completely, so
it's entirely possible that this idea is bunk for some other reason. But
it was the first thing that came to mind, and so feels worth exploring
to see if it might have prevented something like this from ever
happening in the first place.

Thanks,
Taylor

@gitgitgadget gitgitgadget Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):

On Tue, 7 Jul 2026 at 15:49, Taylor Blau <me@ttaylorr.com> wrote:
>
> >       g = prepare_commit_graph(ctx.r);
> >       for (struct commit_graph *chain = g; chain; chain = chain->base_graph)
> > -             g->topo_levels = &topo_levels;
> > +             chain->topo_levels = &topo_levels;
> >
>
> Looks obviously good.
>
> I think that there is a more permanent fix, though, which would have not
> allowed this bug to evade both its author, and reviewer (me). I *think*
> that we may clear up some scoping issues if we removed g->topo_levels
> entirely, and instead stored it in the write_commit_graph_ctx struct.

I think that sounds feasible, but it would be a larger change.
I wanted to keep this fix minimal and restore
the code to match the pre-regression state. I can maybe look
into a refactoring as followup (or help review someone elses
refactoring?), though I would also be happy just to get that
extra 4 seconds back on every fetch for now :)

Thanks,
Kristofer

@gitgitgadget gitgitgadget Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):

On Tue, 7 Jul 2026 at 15:49, Taylor Blau <me@ttaylorr.com> wrote:
>
> I think that there is a more permanent fix, though, which would have not
> allowed this bug to evade both its author, and reviewer (me). I *think*
> that we may clear up some scoping issues if we removed g->topo_levels
> entirely, and instead stored it in the write_commit_graph_ctx struct.
>
> I haven't thought through the implications of doing so completely, so
> it's entirely possible that this idea is bunk for some other reason. But
> it was the first thing that came to mind, and so feels worth exploring
> to see if it might have prevented something like this from ever
> happening in the first place.
>

I looked into the structural change you suggested and I think
it's doable, though not quite as simple as just moving
it into ctx (since fill_commit_graph_info() doesn't have ctx).

I found three approaches:

(a) Thread topo_levels through the call chain. This would
affect:
- fill_commit_graph_info()
- fill_commit_in_graph()
- parse_commit_in_graph_one()
- parse_commit_in_graph()
- load_commit_graph_info()
- lookup_commit_in_graph().

This is the most direct approach, but it touches many functions
and some callers would need to pass in NULL which makes it a bit
noisy.

(b) Move topo_levels to struct object_database. Since
fill_commit_graph_info() can already reach the odb via
g->odb_source->odb, no signature changes are needed.
The write side becomes a single assignment:

    ctx.r->objects->topo_levels = &topo_levels;

and cleanup becomes:

    ctx.r->objects->topo_levels = NULL;

No chain walk needed and the diff is fairly small.
I am not sure about the semantics of it though -- should the odb
have a reference to topo_levels?

(c) Introduce a struct for the chain as a whole, separating it from
the per-layer struct commit_graph. Right now struct commit_graph
represents a single layer but also serves as the chain head, so
chain-wide state like topo_levels gets duplicated on every layer
(only logically -- the actual overhead is still small).
A dedicated chain struct could own topo_levels and the linked list
of layers. IMO this is the cleanest model but a larger refactoring.

I have a prototype of (b) that compiles and passes the test suite.

For now though, I think the minimal bugfix is the right thing to do.

Thanks,
Kristofer

@gitgitgadget gitgitgadget Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taylor Blau wrote on the Git mailing list (how to reply to this email):

On Tue, Jul 07, 2026 at 04:57:13PM +0200, Kristofer Karlsson wrote:
> (b) Move topo_levels to struct object_database. Since
> fill_commit_graph_info() can already reach the odb via
> g->odb_source->odb, no signature changes are needed.
> The write side becomes a single assignment:
>
>     ctx.r->objects->topo_levels = &topo_levels;
>
> and cleanup becomes:
>
>     ctx.r->objects->topo_levels = NULL;
>
> No chain walk needed and the diff is fairly small.
> I am not sure about the semantics of it though -- should the odb
> have a reference to topo_levels?

This seems to be the most promising approach, though I'd be curious what
Patrick's thoughts are. The commit-slab API is really a property of the
object database, but we treat these as a global as I do not recall them
yet being touched by the ODB refactoring effort.

> [...]
>
> I have a prototype of (b) that compiles and passes the test suite.
>
> For now though, I think the minimal bugfix is the right thing to do.

Agreed.

Thanks,
Taylor

@gitgitgadget gitgitgadget Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Fri, Jul 10, 2026 at 03:14:28PM -0700, Taylor Blau wrote:
> On Tue, Jul 07, 2026 at 04:57:13PM +0200, Kristofer Karlsson wrote:
> > (b) Move topo_levels to struct object_database. Since
> > fill_commit_graph_info() can already reach the odb via
> > g->odb_source->odb, no signature changes are needed.
> > The write side becomes a single assignment:
> >
> >     ctx.r->objects->topo_levels = &topo_levels;
> >
> > and cleanup becomes:
> >
> >     ctx.r->objects->topo_levels = NULL;
> >
> > No chain walk needed and the diff is fairly small.
> > I am not sure about the semantics of it though -- should the odb
> > have a reference to topo_levels?
> 
> This seems to be the most promising approach, though I'd be curious what
> Patrick's thoughts are. The commit-slab API is really a property of the
> object database, but we treat these as a global as I do not recall them
> yet being touched by the ODB refactoring effort.

I was investigating several times whether we can remove them from global
scope and move them into the object database indeed. The answer is that
it's somewhat complicated because we reuse the slab for multiple
different things, and detangling that has proven to be a bit of a mess.

The other question here is whether commit graphs really are a property
of the object database itself, or whether they are rather a property of
a given backend. Sure, we can only have a single commit graph at any
point in time, so they feel like they are at the object database level.
But is the current implementation of a commit graph really the best for
all potential backends out there?

If you take for example a distributed backend to store objects, then you
probably don't want to have a single local commit graph that is stored
in ".git/objects/info". Furthermore, the current format may not even be
the best one to store the cached information, either.

So ultimately, I can see one of two approaches:
 
  - Either we make the commit graph itself pluggable as a standalone
    mechanism, too.

  - Or we treat it as a property of the object backend.

I haven't fully made up my mind yet. But I guess detangling the current
mess that we have with the commit graphs would help regardless of which
direction we eventually go into.

Thanks!

Patrick

@gitgitgadget gitgitgadget Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taylor Blau wrote on the Git mailing list (how to reply to this email):

On Mon, Jul 13, 2026 at 08:16:31AM +0200, Patrick Steinhardt wrote:
> On Fri, Jul 10, 2026 at 03:14:28PM -0700, Taylor Blau wrote:
> > On Tue, Jul 07, 2026 at 04:57:13PM +0200, Kristofer Karlsson wrote:
> > > (b) Move topo_levels to struct object_database. Since
> > > fill_commit_graph_info() can already reach the odb via
> > > g->odb_source->odb, no signature changes are needed.
> > > The write side becomes a single assignment:
> > >
> > >     ctx.r->objects->topo_levels = &topo_levels;
> > >
> > > and cleanup becomes:
> > >
> > >     ctx.r->objects->topo_levels = NULL;
> > >
> > > No chain walk needed and the diff is fairly small.
> > > I am not sure about the semantics of it though -- should the odb
> > > have a reference to topo_levels?
> >
> > This seems to be the most promising approach, though I'd be curious what
> > Patrick's thoughts are. The commit-slab API is really a property of the
> > object database, but we treat these as a global as I do not recall them
> > yet being touched by the ODB refactoring effort.
>
> I was investigating several times whether we can remove them from global
> scope and move them into the object database indeed. The answer is that
> it's somewhat complicated because we reuse the slab for multiple
> different things, and detangling that has proven to be a bit of a mess.

It's an interesting question, and I think worth discussing, though note
that I would also like to ensure that we resolve this in the short-term
to prevent any future regression while the pluggable ODB refactor
continues on.

> The other question here is whether commit graphs really are a property
> of the object database itself, or whether they are rather a property of
> a given backend. Sure, we can only have a single commit graph at any
> point in time, so they feel like they are at the object database level.
> But is the current implementation of a commit graph really the best for
> all potential backends out there?
>
> If you take for example a distributed backend to store objects, then you
> probably don't want to have a single local commit graph that is stored
> in ".git/objects/info". Furthermore, the current format may not even be
> the best one to store the cached information, either.

I think I agree here in part, though I think there is some subtlety that
is specific to commit-graphs.

If I understand your argument correctly, I think that I am on-board with
it if you substitute "commit-graph" with "MIDX" or "reachability
bitmaps", as those are optimizations over a specific representation of
the object store.

The commit-graph is somewhat of an oddity in that regard. While it is
partially an optimization in the representation format, it is also a
data-structure which is useful independent of the underlying storage. On
the former, I absolutely agree with what you're saying: having a
row-oriented layout to optimize commit traversals may not be necessary
in a different implementation of the object store which has efficient
enough access to the commit objects so as to make the row-oriented
layout unnecessary.

However, it is a useful question to ask "what is the generation number
of this commit?" independently of whether we store the commit objects
themselves in the existing ODB, in a generic blob storage system, or
something else entirely.

Thanks,
Taylor

@gitgitgadget gitgitgadget Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Kristofer Karlsson <krka@spotify.com>
>
> Fix a regression introduced in 199d452758 (commit-graph: fix
> "filling in" topological levels, 2025-04-07) where the loop

I guess the same comment from [1/2] applies.  We might be chasing
ghosts here.  Is that elusive commit a total hallucination?

> On a repository with 2.78M commits and a multi-layer split
> commit-graph, this caused a single incremental commit-graph
> write to spend ~3.7 seconds in the generation DFS instead of
> microseconds.

Nice.

> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
>  commit-graph.c                | 2 +-
>  t/t5324-split-commit-graph.sh | 6 +-----
>  2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 4e39a048c4..c2a711cceb 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -2610,7 +2610,7 @@ int write_commit_graph(struct odb_source *source,
>  
>  	g = prepare_commit_graph(ctx.r);
>  	for (struct commit_graph *chain = g; chain; chain = chain->base_graph)
> -		g->topo_levels = &topo_levels;
> +		chain->topo_levels = &topo_levels;
>  
>  	if (flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS)
>  		ctx.changed_paths = 1;
> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
> index f9c57760f4..9e5ab7dbd0 100755
> --- a/t/t5324-split-commit-graph.sh
> +++ b/t/t5324-split-commit-graph.sh
> @@ -738,11 +738,7 @@ test_expect_success 'incremental write reads topo levels from all layers' '
>  		GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
>  			git commit-graph write --reachable --split=no-merge &&
>  
> -		# 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
> +		test_trace2_data commit-graph generation-dfs-steps 1 <trace.txt
>  	)
>  '

@gitgitgadget gitgitgadget Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):

On Tue, 7 Jul 2026 at 19:00, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Fix a regression introduced in 199d452758 (commit-graph: fix
> > "filling in" topological levels, 2025-04-07) where the loop
>
> I guess the same comment from [1/2] applies.  We might be chasing
> ghosts here.  Is that elusive commit a total hallucination?

Oops! The commit exists but the date there is indeed wrong.
Will fix (or just remove it, I am starting to regret trying to make
the commit reference too detailed in the first place).

Thanks,
Kristofer

@gitgitgadget gitgitgadget Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

Kristofer Karlsson <krka@spotify.com> writes:

> On Tue, 7 Jul 2026 at 19:00, Junio C Hamano <gitster@pobox.com> wrote:
>> >
>> > Fix a regression introduced in 199d452758 (commit-graph: fix
>> > "filling in" topological levels, 2025-04-07) where the loop
>>
>> I guess the same comment from [1/2] applies.  We might be chasing
>> ghosts here.  Is that elusive commit a total hallucination?
>
> Oops! The commit exists but the date there is indeed wrong.
> Will fix (or just remove it, I am starting to regret trying to make
> the commit reference too detailed in the first place).

Heh, "git show -s --pretty=reference" would give the right amount of
information without giving leeway to users to decide what level of
detail they want ;-)

Thanks.  Will mark the topic as "Expecting a reroll.".

@gitgitgadget gitgitgadget Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Tue, Jul 07, 2026 at 09:59:43AM +0000, Kristofer Karlsson via GitGitGadget wrote:
> diff --git a/commit-graph.c b/commit-graph.c
> index 4e39a048c4..c2a711cceb 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -2610,7 +2610,7 @@ int write_commit_graph(struct odb_source *source,
>  
>  	g = prepare_commit_graph(ctx.r);
>  	for (struct commit_graph *chain = g; chain; chain = chain->base_graph)
> -		g->topo_levels = &topo_levels;
> +		chain->topo_levels = &topo_levels;
>  
>  	if (flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS)
>  		ctx.changed_paths = 1;

Oops, that's an embarrassing bug indeed. Thanks for finding and fixing
it!

> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
> index f9c57760f4..9e5ab7dbd0 100755
> --- a/t/t5324-split-commit-graph.sh
> +++ b/t/t5324-split-commit-graph.sh
> @@ -738,11 +738,7 @@ test_expect_success 'incremental write reads topo levels from all layers' '
>  		GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
>  			git commit-graph write --reachable --split=no-merge &&
>  
> -		# 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
> +		test_trace2_data commit-graph generation-dfs-steps 1 <trace.txt
>  	)
>  '

Makes sense.

Patrick

g = prepare_commit_graph(ctx.r);
for (struct commit_graph *chain = g; chain; chain = chain->base_graph)
g->topo_levels = &topo_levels;
chain->topo_levels = &topo_levels;

if (flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS)
ctx.changed_paths = 1;
Expand Down
24 changes: 24 additions & 0 deletions t/t5324-split-commit-graph.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,30 @@ test_expect_success 'write generation data chunk when commit-graph chain is repl
)
'

test_expect_success 'incremental write reads topo levels from all layers' '
git init topo-from-lower &&
(
cd topo-from-lower &&

for i in $(test_seq 5)
do
test_commit base-$i || return 1
done &&
git commit-graph write --reachable &&

test_commit extra &&
git commit-graph write --reachable --split=no-merge &&

git checkout base-3 &&
test_commit new-branch &&

GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
git commit-graph write --reachable --split=no-merge &&

test_trace2_data commit-graph generation-dfs-steps 1 <trace.txt
)
'

test_expect_success 'temporary graph layer is discarded upon failure' '
git init layer-discard &&
(
Expand Down
Loading