commit-graph: fix topo_levels slab propagation regression

When fetch.writeCommitGraph is enabled (or git maintenance runs after fetch), an incremental commit-graph write computes generation numbers for the newly added commits. For commits already in the graph, their topo levels should be read from the existing layers, making the DFS proportional to the number of new commits.

199d452758 (commit-graph: return the prepared commit graph from prepare_commit_graph(), 2025-04-07), part of the ps/commit-graph-via-source series [1], refactored the loop that propagates the topo_levels slab to each layer of the commit-graph chain. The original code used a single variable that advanced through the chain:

while (g) {
    g->topo_levels = &topo_levels;
    g = g->base_graph;
}

The refactored code introduced a separate iteration variable but did not update the loop body to match:

for (struct commit_graph *chain = g; chain; chain = chain->base_graph)

    g->topo_levels = &topo_levels;

This always assigns to the topmost layer instead of the current one. The other loops in the same refactoring all correctly use chain in their bodies:

for (struct commit_graph *chain = g; chain; chain = chain->base_graph)

    ctx.num_commit_graphs_before++;

for (struct commit_graph *chain = g; chain; chain = chain->base_graph)

    ctx.commit_graph_filenames_before[--i] = xstrdup(chain->filename);

With only the topmost layer having topo_levels set, fill_commit_graph_info() cannot store topo levels for commits parsed from lower layers. compute_reachable_generation_numbers() then sees GENERATION_NUMBER_ZERO for those commits and re-walks their entire ancestry.

On a large repo with a 4-layer split commit-graph, the cost of a single incremental commit-graph write drops from 4133ms to 233ms after the fix, which directly impacts every git fetch when commit-graph maintenance is enabled.

[1]
https://lore.kernel.org/git/[email protected]/T/#mb55b5f0e1ccf82d969ac1d8144c56ecf87b833e8

Kristofer Karlsson (2):
commit-graph: add trace2 instrumentation for generation DFS
commit-graph: propagate topo_levels slab to all chain layers

 commit-graph.c                |  7 ++++++-
 t/t5324-split-commit-graph.sh | 24 ++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2170%2Fspkrka%2Fkrka%2Ffix-topo-levels-slab-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2170/spkrka/krka/fix-topo-levels-slab-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2170

gitgitgadget

From: Kristofer Karlsson <[email protected]>

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.

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

---
 commit-graph.c                |  5 +++++
 t/t5324-split-commit-graph.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

index 801471a098..4e39a048c4 100644
@@ -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);
 }
 
 static timestamp_t get_topo_level(struct commit *c, void *data)

index 49a057cc2e..f9c57760f4 100755
@@ -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

From: Kristofer Karlsson <[email protected]>

Fix a regression introduced in 199d452758 (commit-graph: fix "filling in" topological levels, 2025-04-07) where the loop propagating the topo_levels slab to each layer of the commit-graph chain always assigned to g->topo_levels (the topmost layer) instead of chain->topo_levels (the current iteration variable).

This meant only the topmost layer had its topo_levels pointer set. When compute_reachable_generation_numbers() ran for an incremental write, commits parsed from lower layers had their topo levels left at zero in the slab, since fill_commit_graph_info() could not store them without the pointer. The DFS then re-walked the entire commit ancestry instead of stopping at commits with known levels.

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.

---
 commit-graph.c                | 2 +-
 t/t5324-split-commit-graph.sh | 6 +-----
 2 files changed, 2 insertions(+), 6 deletions(-)

index 4e39a048c4..c2a711cceb 100644
@@ -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;

index f9c57760f4..9e5ab7dbd0 100755
@@ -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

Makes sense.

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

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

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.

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

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

"Kristofer Karlsson via GitGitGadget" <[email protected]>

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?

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.

"Kristofer Karlsson via GitGitGadget" <[email protected]>

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

Nice.

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.

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

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

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

When fetch.writeCommitGraph is enabled (or git maintenance runs after fetch), an incremental commit-graph write computes generation numbers for the newly added commits. For commits already in the graph, their topo levels should be read from the existing layers, making the DFS proportional to the number of new commits.

199d452758 (commit-graph: return the prepared commit graph from prepare_commit_graph(), 2025-09-04), part of the ps/commit-graph-via-source series [1], refactored the loop that propagates the topo_levels slab to each layer of the commit-graph chain. The original code used a single variable that advanced through the chain:

while (g) {
    g->topo_levels = &topo_levels;
    g = g->base_graph;
}

The refactored code introduced a separate iteration variable but did not update the loop body to match:

for (struct commit_graph *chain = g; chain; chain = chain->base_graph)

    g->topo_levels = &topo_levels;

This always assigns to the topmost layer instead of the current one. Commits from lower layers appear to have no generation numbers, so the DFS re-walks the entire ancestry.

On a repo with a multi-layer split commit-graph, an incremental commit-graph write triggered by git fetch drops from ~3.5 seconds to ~0.2 seconds after the fix.

[1]
https://lore.kernel.org/git/[email protected]/T/#mb55b5f0e1ccf82d969ac1d8144c56ecf87b833e8

Changes since v1:

* Fixed wrong commit title and date in the reference (Junio, Taylor).
* use test_expect_failure with the correct assertion instead of a # BUG comment (Taylor).
* Simplified commit messages.

Kristofer Karlsson (2):
commit-graph: add trace2 instrumentation for generation DFS
commit-graph: propagate topo_levels slab to all chain layers

 commit-graph.c                |  7 ++++++-
 t/t5324-split-commit-graph.sh | 24 ++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

base-commit: f85a7e662054a7b0d9070e432508831afa214b47
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2170%2Fspkrka%2Fkrka%2Ffix-topo-levels-slab-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2170/spkrka/krka/fix-topo-levels-slab-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2170

Range-diff vs v1:

1: b865c2bcff ! 1: 100efa22a9 commit-graph: add trace2 instrumentation for generation DFS
@@ Metadata
## Commit message ##
commit-graph: add trace2 instrumentation for generation DFS

- 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.
+ Count the number of steps taken in
+ compute_reachable_generation_numbers() and expose it via
+ trace2 to make it easier to detect performance regressions.

- 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).
+ Add a failing test for such a regression, introduced in
+ 199d452758 (commit-graph: return the prepared commit graph
+ from prepare_commit_graph(), 2025-09-04), where incremental
+ commit-graph writes do not see existing generation numbers
+ from lower graph layers and fall back to walking the full
+ ancestry.

@@ t/t5324-split-commit-graph.sh: test_expect_success 'write generation data chunk
)
'

-+test_expect_success 'incremental write reads topo levels from all layers' ' ++test_expect_failure 'incremental write reads topo levels from all layers' '
+ git init topo-from-lower &&
+ (
+ cd topo-from-lower &&
@@ t/t5324-split-commit-graph.sh: test_expect_success 'write generation data chunk
+ 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
+ )
+'
+
2: f9c1482a76 ! 2: 679dd2e392 commit-graph: propagate topo_levels slab to all chain layers
@@ Metadata
## Commit message ##
commit-graph: propagate topo_levels slab to all chain layers

- Fix a regression introduced in 199d452758 (commit-graph: fix
- "filling in" topological levels, 2025-04-07) where the loop
- propagating the topo_levels slab to each layer of the
- commit-graph chain always assigned to g->topo_levels
- (the topmost layer) instead of chain->topo_levels (the
- current iteration variable).
+ The topo_levels slab is only propagated to the topmost graph
+ layer instead of all layers in the chain. Commits from lower
+ layers appear to have no generation numbers, so the DFS
+ re-walks the entire ancestry.

- This meant only the topmost layer had its topo_levels pointer
- set. When compute_reachable_generation_numbers() ran for an
- incremental write, commits parsed from lower layers had their
- topo levels left at zero in the slab, since
- fill_commit_graph_info() could not store them without the
- pointer. The DFS then re-walked the entire commit ancestry
- instead of stopping at commits with known levels.
-
- 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.
+ Fix by making topo_levels visible to all layers, not just
+ the first one.

@@ commit-graph.c: int write_commit_graph(struct odb_source *source,
ctx.changed_paths = 1;

## t/t5324-split-commit-graph.sh ##
-@@ t/t5324-split-commit-graph.sh: 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
+@@ t/t5324-split-commit-graph.sh: test_expect_success 'write generation data chunk when commit-graph chain is repl
)
'

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

gitgitgadget

From: Kristofer Karlsson <[email protected]>

Count the number of steps taken in compute_reachable_generation_numbers() and expose it via trace2 to make it easier to detect performance regressions.

Add a failing test for such a regression, introduced in 199d452758 (commit-graph: return the prepared commit graph from prepare_commit_graph(), 2025-09-04), where incremental commit-graph writes do not see existing generation numbers from lower graph layers and fall back to walking the full ancestry.

---
 commit-graph.c                |  5 +++++
 t/t5324-split-commit-graph.sh | 24 ++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

index c6d9c5c740..702ba9731b 100644
@@ -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);
 }
 
 static timestamp_t get_topo_level(struct commit *c, void *data)

index 49a057cc2e..b41331e3dd 100755
@@ -718,6 +718,30 @@ test_expect_success 'write generation data chunk when commit-graph chain is repl
 	)
 '
 
+test_expect_failure '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 &&
 	(

gitgitgadget

From: Kristofer Karlsson <[email protected]>

The topo_levels slab is only propagated to the topmost graph layer instead of all layers in the chain. Commits from lower layers appear to have no generation numbers, so the DFS re-walks the entire ancestry.

Fix by making topo_levels visible to all layers, not just the first one.

---
 commit-graph.c                | 2 +-
 t/t5324-split-commit-graph.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index 702ba9731b..a0bca248ac 100644
@@ -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;

index b41331e3dd..9e5ab7dbd0 100755
@@ -718,7 +718,7 @@ test_expect_success 'write generation data chunk when commit-graph chain is repl
 	)
 '
 
-test_expect_failure 'incremental write reads topo levels from all layers' '
+test_expect_success 'incremental write reads topo levels from all layers' '
 	git init topo-from-lower &&
 	(
 		cd topo-from-lower &&

gitgitgadget

Yeah, I think this would be ideal.

Thanks,
Taylor

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.

Agreed.

Thanks,
Taylor

Thanks, this version looks good to me.

Thanks,
Taylor

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

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

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

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.

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.

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.

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

- Kristofer

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.

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