{"thread":{"id":"65941","subject":"[PATCH 0/2] commit-graph: fix topo_levels slab propagation regression","startedAt":"2026-07-07T09:59:46Z","lastAt":"2026-07-14T03:31:37Z","messageCount":28,"participants":["Kristofer Karlsson via GitGitGadget","Taylor Blau","Kristofer Karlsson","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"547318","messageId":"pull.2170.git.1783418384.gitgitgadget@gmail.com","threadId":"65941","inReplyTo":null,"subject":"[PATCH 0/2] commit-graph: fix topo_levels slab propagation regression","fromName":"Kristofer Karlsson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-07-07T09:59:41Z","receivedAt":"2026-07-07T09:59:46Z","isPatch":true,"body":"When fetch.writeCommitGraph is enabled (or git maintenance runs after\nfetch), an incremental commit-graph write computes generation numbers for\nthe newly added commits. For commits already in the graph, their topo levels\nshould be read from the existing layers, making the DFS proportional to the\nnumber of new commits.\n\n199d452758 (commit-graph: return the prepared commit graph from\nprepare_commit_graph(), 2025-04-07), part of the ps/commit-graph-via-source\nseries [1], refactored the loop that propagates the topo_levels slab to each\nlayer of the commit-graph chain. The original code used a single variable\nthat advanced through the chain:\n\nwhile (g) {\n g->topo_levels = &topo_levels;\n g = g->base_graph;\n}\n\n\nThe refactored code introduced a separate iteration variable but did not\nupdate the loop body to match:\n\nfor (struct commit_graph *chain = g; chain; chain = chain->base_graph)\n g->topo_levels = &topo_levels;\n\n\nThis always assigns to the topmost layer instead of the current one. The\nother loops in the same refactoring all correctly use chain in their bodies:\n\nfor (struct commit_graph *chain = g; chain; chain = chain->base_graph)\n ctx.num_commit_graphs_before++;\n\nfor (struct commit_graph *chain = g; chain; chain = chain->base_graph)\n ctx.commit_graph_filenames_before[--i] = xstrdup(chain->filename);\n\n\nWith only the topmost layer having topo_levels set, fill_commit_graph_info()\ncannot store topo levels for commits parsed from lower layers.\ncompute_reachable_generation_numbers() then sees GENERATION_NUMBER_ZERO for\nthose commits and re-walks their entire ancestry.\n\nOn a large repo with a 4-layer split commit-graph, the cost of a single\nincremental commit-graph write drops from 4133ms to 233ms after the fix,\nwhich directly impacts every git fetch when commit-graph maintenance is\nenabled.\n\n[1]\nhttps://lore.kernel.org/git/aMNTELw0Wk8jWoPc@nand.local/T/#mb55b5f0e1ccf82d969ac1d8144c56ecf87b833e8\n\nKristofer Karlsson (2):\n commit-graph: add trace2 instrumentation for generation DFS\n commit-graph: propagate topo_levels slab to all chain layers\n\n commit-graph.c | 7 ++++++-\n t/t5324-split-commit-graph.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\n\nbase-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2170%2Fspkrka%2Fkrka%2Ffix-topo-levels-slab-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2170/spkrka/krka/fix-topo-levels-slab-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2170\n-- \ngitgitgadget\n"},{"id":"547319","messageId":"b865c2bcff53a32637aac426dd2c6ef4a4c27077.1783418384.git.gitgitgadget@gmail.com","threadId":"65941","inReplyTo":"pull.2170.git.1783418384.gitgitgadget@gmail.com","subject":"[PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS","fromName":"Kristofer Karlsson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-07-07T09:59:42Z","receivedAt":"2026-07-07T09:59:47Z","isPatch":true,"body":"From: Kristofer Karlsson \n\nAdd a step counter and trace2_data_intmax call to\ncompute_reachable_generation_numbers() to make the cost of\nthe generation number DFS observable. This exposes a\nregression introduced in 199d452758 (commit-graph: fix\n\"filling in\" topological levels, 2025-04-07) where\nincremental commit-graph writes re-walk the entire commit\nancestry instead of reading topo levels from lower graph\nlayers.\n\nAdd a test that demonstrates the problem: with a two-layer\nsplit commit-graph, writing a new incremental layer for a\ncommit whose parent is in the base layer walks all the way\ndown to the root (7 steps for 5 base commits) instead of\nreading the existing topo level and stopping immediately\n(1 step).\n\nSigned-off-by: Kristofer Karlsson \n---\n commit-graph.c | 5 +++++\n t/t5324-split-commit-graph.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 801471a098..4e39a048c4 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1653,6 +1653,7 @@ static void compute_reachable_generation_numbers(\n {\n \tint i;\n \tstruct commit_list *list = NULL;\n+\tintmax_t steps = 0;\n \n \tfor (i = 0; i < info->commits->nr; i++) {\n \t\tstruct commit *c = info->commits->items[i];\n@@ -1671,6 +1672,7 @@ static void compute_reachable_generation_numbers(\n \t\t\tint all_parents_computed = 1;\n \t\t\ttimestamp_t max_gen = 0;\n \n+\t\t\tsteps++;\n \t\t\tfor (parent = current->parents; parent; parent = parent->next) {\n \t\t\t\trepo_parse_commit(info->r, parent->item);\n \t\t\t\tgen = info->get_generation(parent->item, info->data);\n@@ -1694,6 +1696,9 @@ static void compute_reachable_generation_numbers(\n \t\t\t}\n \t\t}\n \t}\n+\n+\ttrace2_data_intmax(\"commit-graph\", info->r,\n+\t\t\t \"generation-dfs-steps\", steps);\n }\n \n static timestamp_t get_topo_level(struct commit *c, void *data)\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 49a057cc2e..f9c57760f4 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -718,6 +718,34 @@ test_expect_success 'write generation data chunk when commit-graph chain is repl\n \t)\n '\n \n+test_expect_success 'incremental write reads topo levels from all layers' '\n+\tgit init topo-from-lower &&\n+\t(\n+\t\tcd topo-from-lower &&\n+\n+\t\tfor i in $(test_seq 5)\n+\t\tdo\n+\t\t\ttest_commit base-$i || return 1\n+\t\tdone &&\n+\t\tgit commit-graph write --reachable &&\n+\n+\t\ttest_commit extra &&\n+\t\tgit commit-graph write --reachable --split=no-merge &&\n+\n+\t\tgit checkout base-3 &&\n+\t\ttest_commit new-branch &&\n+\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" \\\n+\t\t\tgit commit-graph write --reachable --split=no-merge &&\n+\n+\t\t# BUG: topo levels from lower graph layers are not\n+\t\t# propagated, so the DFS re-walks from base-3 down to\n+\t\t# the root (7 steps) instead of reading topo levels\n+\t\t# from the existing graph (1 step).\n+\t\ttest_trace2_data commit-graph generation-dfs-steps 7 \n\nFix a regression introduced in 199d452758 (commit-graph: fix\n\"filling in\" topological levels, 2025-04-07) where the loop\npropagating the topo_levels slab to each layer of the\ncommit-graph chain always assigned to `g->topo_levels`\n(the topmost layer) instead of `chain->topo_levels` (the\ncurrent iteration variable).\n\nThis meant only the topmost layer had its topo_levels pointer\nset. When compute_reachable_generation_numbers() ran for an\nincremental write, commits parsed from lower layers had their\ntopo levels left at zero in the slab, since\nfill_commit_graph_info() could not store them without the\npointer. The DFS then re-walked the entire commit ancestry\ninstead of stopping at commits with known levels.\n\nOn a repository with 2.78M commits and a multi-layer split\ncommit-graph, this caused a single incremental commit-graph\nwrite to spend ~3.7 seconds in the generation DFS instead of\nmicroseconds.\n\nSigned-off-by: Kristofer Karlsson \n---\n commit-graph.c | 2 +-\n t/t5324-split-commit-graph.sh | 6 +-----\n 2 files changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 4e39a048c4..c2a711cceb 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2610,7 +2610,7 @@ int write_commit_graph(struct odb_source *source,\n \n \tg = prepare_commit_graph(ctx.r);\n \tfor (struct commit_graph *chain = g; chain; chain = chain->base_graph)\n-\t\tg->topo_levels = &topo_levels;\n+\t\tchain->topo_levels = &topo_levels;\n \n \tif (flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS)\n \t\tctx.changed_paths = 1;\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex f9c57760f4..9e5ab7dbd0 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -738,11 +738,7 @@ test_expect_success 'incremental write reads topo levels from all layers' '\n \t\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" \\\n \t\t\tgit commit-graph write --reachable --split=no-merge &&\n \n-\t\t# BUG: topo levels from lower graph layers are not\n-\t\t# propagated, so the DFS re-walks from base-3 down to\n-\t\t# the root (7 steps) instead of reading topo levels\n-\t\t# from the existing graph (1 step).\n-\t\ttest_trace2_data commit-graph generation-dfs-steps 7 From: Kristofer Karlsson \n>\n> Add a step counter and trace2_data_intmax call to\n> compute_reachable_generation_numbers() to make the cost of\n> the generation number DFS observable. This exposes a\n> regression introduced in 199d452758 (commit-graph: fix\n> \"filling in\" topological levels, 2025-04-07) where\n> incremental commit-graph writes re-walk the entire commit\n> ancestry instead of reading topo levels from lower graph\n> layers.\n\nMakes sense.\n\n> Add a test that demonstrates the problem: with a two-layer\n> split commit-graph, writing a new incremental layer for a\n> commit whose parent is in the base layer walks all the way\n> down to the root (7 steps for 5 base commits) instead of\n> reading the existing topo level and stopping immediately\n> (1 step).\n\nThis paragraph only describes verbatim what is already included in the\npatch. I think we could easily do without it, but I do not feel so\nstrongly about it.\n\n> Signed-off-by: Kristofer Karlsson \n> ---\n> commit-graph.c | 5 +++++\n> t/t5324-split-commit-graph.sh | 28 ++++++++++++++++++++++++++++\n> 2 files changed, 33 insertions(+)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 801471a098..4e39a048c4 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1653,6 +1653,7 @@ static void compute_reachable_generation_numbers(\n> {\n> \tint i;\n> \tstruct commit_list *list = NULL;\n> +\tintmax_t steps = 0;\n\nAny reason that this should be signed? Obviously in practice, I don't\nthink we're going to wrap around with a greater-than-INT_MAX number of\ncommits here, but perhaps we would at the very least prefer uintmax_t.\n\nI guess trace2 only has a data_intmax() function, so perhaps the point\nis moot. Regardless, it seems that we would want to have a convenience\nwrapper to be able to print out unsigned integer values which are\notherwise un-representable as signed integers.\n\nThat is outside the scope of your patch, though, so what you have\nbelow here is fine in my opinion.\n\n> +\t\t# BUG: topo levels from lower graph layers are not\n> +\t\t# propagated, so the DFS re-walks from base-3 down to\n> +\t\t# the root (7 steps) instead of reading topo levels\n> +\t\t# from the existing graph (1 step).\n> +\t\ttest_trace2_data commit-graph generation-dfs-steps 7 diff --git a/commit-graph.c b/commit-graph.c\n> index 4e39a048c4..c2a711cceb 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -2610,7 +2610,7 @@ int write_commit_graph(struct odb_source *source,\n>\n> \tg = prepare_commit_graph(ctx.r);\n> \tfor (struct commit_graph *chain = g; chain; chain = chain->base_graph)\n> -\t\tg->topo_levels = &topo_levels;\n> +\t\tchain->topo_levels = &topo_levels;\n>\n> \tif (flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS)\n> \t\tctx.changed_paths = 1;\n\nLooks obviously good.\n\nI think that there is a more permanent fix, though, which would have not\nallowed this bug to evade both its author, and reviewer (me). I *think*\nthat we may clear up some scoping issues if we removed g->topo_levels\nentirely, and instead stored it in the write_commit_graph_ctx struct.\n\nI haven't thought through the implications of doing so completely, so\nit's entirely possible that this idea is bunk for some other reason. But\nit was the first thing that came to mind, and so feels worth exploring\nto see if it might have prevented something like this from ever\nhappening in the first place.\n\nThanks,\nTaylor\n"},{"id":"547328","messageId":"CAL71e4P4kM3a80DkyfWXXrCt2o+KKem8iFDM6qgmodRy1aiqLg@mail.gmail.com","threadId":"65941","inReplyTo":"ak0D44nhSH/98WYD@nand.local","subject":"Re: [PATCH 2/2] commit-graph: propagate topo_levels slab to all chain layers","fromName":"Kristofer Karlsson","fromEmail":"krka@spotify.com","sentAt":"2026-07-07T14:02:52Z","receivedAt":"2026-07-07T14:03:05Z","isPatch":true,"body":"On Tue, 7 Jul 2026 at 15:49, Taylor Blau wrote:\n>\n> > g = prepare_commit_graph(ctx.r);\n> > for (struct commit_graph *chain = g; chain; chain = chain->base_graph)\n> > - g->topo_levels = &topo_levels;\n> > + chain->topo_levels = &topo_levels;\n> >\n>\n> Looks obviously good.\n>\n> I think that there is a more permanent fix, though, which would have not\n> allowed this bug to evade both its author, and reviewer (me). I *think*\n> that we may clear up some scoping issues if we removed g->topo_levels\n> entirely, and instead stored it in the write_commit_graph_ctx struct.\n\nI think that sounds feasible, but it would be a larger change.\nI wanted to keep this fix minimal and restore\nthe code to match the pre-regression state. I can maybe look\ninto a refactoring as followup (or help review someone elses\nrefactoring?), though I would also be happy just to get that\nextra 4 seconds back on every fetch for now :)\n\nThanks,\nKristofer\n"},{"id":"547329","messageId":"CAL71e4PuD9D8LRbP3mfxxeMrM+1q--3sCp6oJs=hezdasZUPMw@mail.gmail.com","threadId":"65941","inReplyTo":"ak0DUx5Y/5y1OINz@nand.local","subject":"Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS","fromName":"Kristofer Karlsson","fromEmail":"krka@spotify.com","sentAt":"2026-07-07T14:08:36Z","receivedAt":"2026-07-07T14:08:48Z","isPatch":true,"body":"On Tue, 7 Jul 2026 at 15:47, Taylor Blau wrote:\n>\n> > Add a test that demonstrates the problem: with a two-layer\n> > split commit-graph, writing a new incremental layer for a\n> > commit whose parent is in the base layer walks all the way\n> > down to the root (7 steps for 5 base commits) instead of\n> > reading the existing topo level and stopping immediately\n> > (1 step).\n>\n> This paragraph only describes verbatim what is already included in the\n> patch. I think we could easily do without it, but I do not feel so\n> strongly about it.\n\nI also don't feel strongly about it, I could remove it entirely.\n\n> > + intmax_t steps = 0;\n>\n> Any reason that this should be signed? Obviously in practice, I don't\n> think we're going to wrap around with a greater-than-INT_MAX number of\n> commits here, but perhaps we would at the very least prefer uintmax_t.\n>\n> I guess trace2 only has a data_intmax() function, so perhaps the point\n> is moot. Regardless, it seems that we would want to have a convenience\n> wrapper to be able to print out unsigned integer values which are\n> otherwise un-representable as signed integers.\n\nYes, my only rationale here was to match the type that\ntrace2_data_intmax expects - and as you say, it's very\nunlikely that we'll need to use all bits anyway, and since\nthis is only used for testing and debugging, and overflows\nwould be noticed that way and would not affect general\ncorrectness.\n\n> Instead of writing \"# BUG ...\" and then an incorrect assertion, I\n> would suggest that you write the assertion you expect:\n>\n> test_trace2_data commit-graph generation-dfs-steps 1 \n> , but mark the test as \"test_expect_failure\".\n\nI started with this actually and then changed my mind in order\nto demonstrate exactly how the counter changed, not just that it\nchanged from failure to success. But I'd be happy to change this\ntoo if needed - it would effectively reduce the second commit to\njust the bugfix line and switching from test_expect_failure\nto test_expect_success.\n\nThanks,\nKristofer\n"},{"id":"547336","messageId":"CAL71e4OuU1+KHd0TrcxDX2dyoWEJXmi86m8u+E7vtxhcSF6M1Q@mail.gmail.com","threadId":"65941","inReplyTo":"ak0D44nhSH/98WYD@nand.local","subject":"Re: [PATCH 2/2] commit-graph: propagate topo_levels slab to all chain layers","fromName":"Kristofer Karlsson","fromEmail":"krka@spotify.com","sentAt":"2026-07-07T14:57:13Z","receivedAt":"2026-07-07T15:04:02Z","isPatch":true,"body":"On Tue, 7 Jul 2026 at 15:49, Taylor Blau wrote:\n>\n> I think that there is a more permanent fix, though, which would have not\n> allowed this bug to evade both its author, and reviewer (me). I *think*\n> that we may clear up some scoping issues if we removed g->topo_levels\n> entirely, and instead stored it in the write_commit_graph_ctx struct.\n>\n> I haven't thought through the implications of doing so completely, so\n> it's entirely possible that this idea is bunk for some other reason. But\n> it was the first thing that came to mind, and so feels worth exploring\n> to see if it might have prevented something like this from ever\n> happening in the first place.\n>\n\nI looked into the structural change you suggested and I think\nit's doable, though not quite as simple as just moving\nit into ctx (since fill_commit_graph_info() doesn't have ctx).\n\nI found three approaches:\n\n(a) Thread topo_levels through the call chain. This would\naffect:\n- fill_commit_graph_info()\n- fill_commit_in_graph()\n- parse_commit_in_graph_one()\n- parse_commit_in_graph()\n- load_commit_graph_info()\n- lookup_commit_in_graph().\n\nThis is the most direct approach, but it touches many functions\nand some callers would need to pass in NULL which makes it a bit\nnoisy.\n\n(b) Move topo_levels to struct object_database. Since\nfill_commit_graph_info() can already reach the odb via\ng->odb_source->odb, no signature changes are needed.\nThe write side becomes a single assignment:\n\n ctx.r->objects->topo_levels = &topo_levels;\n\nand cleanup becomes:\n\n ctx.r->objects->topo_levels = NULL;\n\nNo chain walk needed and the diff is fairly small.\nI am not sure about the semantics of it though -- should the odb\nhave a reference to topo_levels?\n\n(c) Introduce a struct for the chain as a whole, separating it from\nthe per-layer struct commit_graph. Right now struct commit_graph\nrepresents a single layer but also serves as the chain head, so\nchain-wide state like topo_levels gets duplicated on every layer\n(only logically -- the actual overhead is still small).\nA dedicated chain struct could own topo_levels and the linked list\nof layers. IMO this is the cleanest model but a larger refactoring.\n\nI have a prototype of (b) that compiles and passes the test suite.\n\nFor now though, I think the minimal bugfix is the right thing to do.\n\nThanks,\nKristofer\n"},{"id":"547365","messageId":"xmqq1pde7n8h.fsf@gitster.g","threadId":"65941","inReplyTo":"b865c2bcff53a32637aac426dd2c6ef4a4c27077.1783418384.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-07T16:55:58Z","receivedAt":"2026-07-07T16:56:01Z","isPatch":true,"body":"\"Kristofer Karlsson via GitGitGadget\" \nwrites:\n\n> From: Kristofer Karlsson \n>\n> Add a step counter and trace2_data_intmax call to\n> compute_reachable_generation_numbers() to make the cost of\n> the generation number DFS observable. This exposes a\n> regression introduced in 199d452758 (commit-graph: fix\n> \"filling in\" topological levels, 2025-04-07) where\n\nWhere did \"fix filling in\" came from? Are you blaming\n\n 199d452758 (commit-graph: return the prepared commit graph from\n `prepare_commit_graph()`, 2025-09-04)\n\nor something else that happend in April that year?\n\n> incremental commit-graph writes re-walk the entire commit\n> ancestry instead of reading topo levels from lower graph\n> layers.\n\n> Add a test that demonstrates the problem: with a two-layer\n> split commit-graph, writing a new incremental layer for a\n> commit whose parent is in the base layer walks all the way\n> down to the root (7 steps for 5 base commits) instead of\n> reading the existing topo level and stopping immediately\n> (1 step).\n\nOK. I expect that [2/2] would update this exact test to demonstrate\nthat with code updated in [2/2] the extra walk will no longer happen.\n\n> Signed-off-by: Kristofer Karlsson \n> ---\n> commit-graph.c | 5 +++++\n> t/t5324-split-commit-graph.sh | 28 ++++++++++++++++++++++++++++\n> 2 files changed, 33 insertions(+)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 801471a098..4e39a048c4 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1653,6 +1653,7 @@ static void compute_reachable_generation_numbers(\n> {\n> \tint i;\n> \tstruct commit_list *list = NULL;\n> +\tintmax_t steps = 0;\n> \n> \tfor (i = 0; i < info->commits->nr; i++) {\n> \t\tstruct commit *c = info->commits->items[i];\n> @@ -1671,6 +1672,7 @@ static void compute_reachable_generation_numbers(\n> \t\t\tint all_parents_computed = 1;\n> \t\t\ttimestamp_t max_gen = 0;\n> \n> +\t\t\tsteps++;\n> \t\t\tfor (parent = current->parents; parent; parent = parent->next) {\n> \t\t\t\trepo_parse_commit(info->r, parent->item);\n> \t\t\t\tgen = info->get_generation(parent->item, info->data);\n> @@ -1694,6 +1696,9 @@ static void compute_reachable_generation_numbers(\n> \t\t\t}\n> \t\t}\n> \t}\n> +\n> +\ttrace2_data_intmax(\"commit-graph\", info->r,\n> +\t\t\t \"generation-dfs-steps\", steps);\n> }\n\nPretty-much trivial addition of a trace element.\n\n> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\n> index 49a057cc2e..f9c57760f4 100755\n> --- a/t/t5324-split-commit-graph.sh\n> +++ b/t/t5324-split-commit-graph.sh\n> @@ -718,6 +718,34 @@ test_expect_success 'write generation data chunk when commit-graph chain is repl\n> \t)\n> '\n> \n> +test_expect_success 'incremental write reads topo levels from all layers' '\n> +\tgit init topo-from-lower &&\n> +\t(\n> +\t\tcd topo-from-lower &&\n> +\n> +\t\tfor i in $(test_seq 5)\n> +\t\tdo\n> +\t\t\ttest_commit base-$i || return 1\n> +\t\tdone &&\n> +\t\tgit commit-graph write --reachable &&\n> +\n> +\t\ttest_commit extra &&\n> +\t\tgit commit-graph write --reachable --split=no-merge &&\n> +\n> +\t\tgit checkout base-3 &&\n> +\t\ttest_commit new-branch &&\n> +\n> +\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" \\\n> +\t\t\tgit commit-graph write --reachable --split=no-merge &&\n> +\n> +\t\t# BUG: topo levels from lower graph layers are not\n> +\t\t# propagated, so the DFS re-walks from base-3 down to\n> +\t\t# the root (7 steps) instead of reading topo levels\n> +\t\t# from the existing graph (1 step).\n> +\t\ttest_trace2_data commit-graph generation-dfs-steps 7 +\t)\n> +'\n> +\n> test_expect_success 'temporary graph layer is discarded upon failure' '\n> \tgit init layer-discard &&\n> \t(\n"},{"id":"547366","messageId":"xmqqo6gi68go.fsf@gitster.g","threadId":"65941","inReplyTo":"f9c1482a76493520b948a2e918de7a5481fa1043.1783418384.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] commit-graph: propagate topo_levels slab to all chain layers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-07T17:00:23Z","receivedAt":"2026-07-07T17:00:25Z","isPatch":true,"body":"\"Kristofer Karlsson via GitGitGadget\" \nwrites:\n\n> From: Kristofer Karlsson \n>\n> Fix a regression introduced in 199d452758 (commit-graph: fix\n> \"filling in\" topological levels, 2025-04-07) where the loop\n\nI guess the same comment from [1/2] applies. We might be chasing\nghosts here. Is that elusive commit a total hallucination?\n\n> On a repository with 2.78M commits and a multi-layer split\n> commit-graph, this caused a single incremental commit-graph\n> write to spend ~3.7 seconds in the generation DFS instead of\n> microseconds.\n\nNice.\n\n> Signed-off-by: Kristofer Karlsson \n> ---\n> commit-graph.c | 2 +-\n> t/t5324-split-commit-graph.sh | 6 +-----\n> 2 files changed, 2 insertions(+), 6 deletions(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 4e39a048c4..c2a711cceb 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -2610,7 +2610,7 @@ int write_commit_graph(struct odb_source *source,\n> \n> \tg = prepare_commit_graph(ctx.r);\n> \tfor (struct commit_graph *chain = g; chain; chain = chain->base_graph)\n> -\t\tg->topo_levels = &topo_levels;\n> +\t\tchain->topo_levels = &topo_levels;\n> \n> \tif (flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS)\n> \t\tctx.changed_paths = 1;\n> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\n> index f9c57760f4..9e5ab7dbd0 100755\n> --- a/t/t5324-split-commit-graph.sh\n> +++ b/t/t5324-split-commit-graph.sh\n> @@ -738,11 +738,7 @@ test_expect_success 'incremental write reads topo levels from all layers' '\n> \t\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" \\\n> \t\t\tgit commit-graph write --reachable --split=no-merge &&\n> \n> -\t\t# BUG: topo levels from lower graph layers are not\n> -\t\t# propagated, so the DFS re-walks from base-3 down to\n> -\t\t# the root (7 steps) instead of reading topo levels\n> -\t\t# from the existing graph (1 step).\n> -\t\ttest_trace2_data commit-graph generation-dfs-steps 7 +\t\ttest_trace2_data commit-graph generation-dfs-steps 1 \t)\n> '\n"},{"id":"547372","messageId":"CAL71e4N82nxjXbLD2Lre58r6O8CBQaGx9ddtB4K5k8-6BFV7Yg@mail.gmail.com","threadId":"65941","inReplyTo":"xmqq1pde7n8h.fsf@gitster.g","subject":"Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS","fromName":"Kristofer Karlsson","fromEmail":"krka@spotify.com","sentAt":"2026-07-07T17:39:37Z","receivedAt":"2026-07-07T17:39:49Z","isPatch":true,"body":"On Tue, 7 Jul 2026 at 18:56, Junio C Hamano wrote:\n> >\n> > Add a step counter and trace2_data_intmax call to\n> > compute_reachable_generation_numbers() to make the cost of\n> > the generation number DFS observable. This exposes a\n> > regression introduced in 199d452758 (commit-graph: fix\n> > \"filling in\" topological levels, 2025-04-07) where\n>\n> Where did \"fix filling in\" came from? Are you blaming\n>\n> 199d452758 (commit-graph: return the prepared commit graph from\n> `prepare_commit_graph()`, 2025-09-04)\n>\n> or something else that happend in April that year?\n\nHm, I actually don't remember that exact text, it must have been\nan oversight during editing back and forth and I missed it in\nmy local review -- the commit oid is correct though, that is\nthe one I was referring to. I will clean this up and shrink it down.\n\nI did not mean April though, but September 4th. I was using\nthe ISO 8601 date format out of habit.\n\n> OK. I expect that [2/2] would update this exact test to demonstrate\n> that with code updated in [2/2] the extra walk will no longer happen.\n\nYes, I first considered doing this as a single commit, but\nI figured it would be easier to reason about the fix if the\nproblem was identified before-hand.\n\nThanks,\nKristofer\n"},{"id":"547373","messageId":"CAL71e4NXPAitqQtCnwLCyXvigD5KjOCSj5em+3v4WSUaYQKHRg@mail.gmail.com","threadId":"65941","inReplyTo":"xmqqo6gi68go.fsf@gitster.g","subject":"Re: [PATCH 2/2] commit-graph: propagate topo_levels slab to all chain layers","fromName":"Kristofer Karlsson","fromEmail":"krka@spotify.com","sentAt":"2026-07-07T17:42:52Z","receivedAt":"2026-07-07T17:43:07Z","isPatch":true,"body":"On Tue, 7 Jul 2026 at 19:00, Junio C Hamano wrote:\n> >\n> > Fix a regression introduced in 199d452758 (commit-graph: fix\n> > \"filling in\" topological levels, 2025-04-07) where the loop\n>\n> I guess the same comment from [1/2] applies. We might be chasing\n> ghosts here. Is that elusive commit a total hallucination?\n\nOops! The commit exists but the date there is indeed wrong.\nWill fix (or just remove it, I am starting to regret trying to make\nthe commit reference too detailed in the first place).\n\nThanks,\nKristofer\n"},{"id":"547397","messageId":"xmqqjyr61rsq.fsf@gitster.g","threadId":"65941","inReplyTo":"CAL71e4NXPAitqQtCnwLCyXvigD5KjOCSj5em+3v4WSUaYQKHRg@mail.gmail.com","subject":"Re: [PATCH 2/2] commit-graph: propagate topo_levels slab to all chain layers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-07T20:13:57Z","receivedAt":"2026-07-07T20:14:00Z","isPatch":true,"body":"Kristofer Karlsson writes:\n\n> On Tue, 7 Jul 2026 at 19:00, Junio C Hamano wrote:\n>> >\n>> > Fix a regression introduced in 199d452758 (commit-graph: fix\n>> > \"filling in\" topological levels, 2025-04-07) where the loop\n>>\n>> I guess the same comment from [1/2] applies. We might be chasing\n>> ghosts here. Is that elusive commit a total hallucination?\n>\n> Oops! The commit exists but the date there is indeed wrong.\n> Will fix (or just remove it, I am starting to regret trying to make\n> the commit reference too detailed in the first place).\n\nHeh, \"git show -s --pretty=reference\" would give the right amount of\ninformation without giving leeway to users to decide what level of\ndetail they want ;-)\n\nThanks. Will mark the topic as \"Expecting a reroll.\".\n\n"},{"id":"547606","messageId":"ak-ljlV33GLigFf6@pks.im","threadId":"65941","inReplyTo":"f9c1482a76493520b948a2e918de7a5481fa1043.1783418384.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] commit-graph: propagate topo_levels slab to all chain layers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-07-09T13:43:42Z","receivedAt":"2026-07-09T13:43:56Z","isPatch":true,"body":"On Tue, Jul 07, 2026 at 09:59:43AM +0000, Kristofer Karlsson via GitGitGadget wrote:\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 4e39a048c4..c2a711cceb 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -2610,7 +2610,7 @@ int write_commit_graph(struct odb_source *source,\n> \n> \tg = prepare_commit_graph(ctx.r);\n> \tfor (struct commit_graph *chain = g; chain; chain = chain->base_graph)\n> -\t\tg->topo_levels = &topo_levels;\n> +\t\tchain->topo_levels = &topo_levels;\n> \n> \tif (flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS)\n> \t\tctx.changed_paths = 1;\n\nOops, that's an embarrassing bug indeed. Thanks for finding and fixing\nit!\n\n> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\n> index f9c57760f4..9e5ab7dbd0 100755\n> --- a/t/t5324-split-commit-graph.sh\n> +++ b/t/t5324-split-commit-graph.sh\n> @@ -738,11 +738,7 @@ test_expect_success 'incremental write reads topo levels from all layers' '\n> \t\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" \\\n> \t\t\tgit commit-graph write --reachable --split=no-merge &&\n> \n> -\t\t# BUG: topo levels from lower graph layers are not\n> -\t\t# propagated, so the DFS re-walks from base-3 down to\n> -\t\t# the root (7 steps) instead of reading topo levels\n> -\t\t# from the existing graph (1 step).\n> -\t\ttest_trace2_data commit-graph generation-dfs-steps 7 +\t\ttest_trace2_data commit-graph generation-dfs-steps 1 \t)\n> '\n\nMakes sense.\n\nPatrick\n"},{"id":"547612","messageId":"pull.2170.v2.git.1783609382.gitgitgadget@gmail.com","threadId":"65941","inReplyTo":"pull.2170.git.1783418384.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] commit-graph: fix topo_levels slab propagation regression","fromName":"Kristofer Karlsson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-07-09T15:02:59Z","receivedAt":"2026-07-09T15:03:05Z","isPatch":true,"body":"When fetch.writeCommitGraph is enabled (or git maintenance runs after\nfetch), an incremental commit-graph write computes generation numbers for\nthe newly added commits. For commits already in the graph, their topo levels\nshould be read from the existing layers, making the DFS proportional to the\nnumber of new commits.\n\n199d452758 (commit-graph: return the prepared commit graph from\nprepare_commit_graph(), 2025-09-04), part of the ps/commit-graph-via-source\nseries [1], refactored the loop that propagates the topo_levels slab to each\nlayer of the commit-graph chain. The original code used a single variable\nthat advanced through the chain:\n\nwhile (g) {\n g->topo_levels = &topo_levels;\n g = g->base_graph;\n}\n\n\nThe refactored code introduced a separate iteration variable but did not\nupdate the loop body to match:\n\nfor (struct commit_graph *chain = g; chain; chain = chain->base_graph)\n g->topo_levels = &topo_levels;\n\n\nThis always assigns to the topmost layer instead of the current one. Commits\nfrom lower layers appear to have no generation numbers, so the DFS re-walks\nthe entire ancestry.\n\nOn a repo with a multi-layer split commit-graph, an incremental commit-graph\nwrite triggered by git fetch drops from ~3.5 seconds to ~0.2 seconds after\nthe fix.\n\n[1]\nhttps://lore.kernel.org/git/aMNTELw0Wk8jWoPc@nand.local/T/#mb55b5f0e1ccf82d969ac1d8144c56ecf87b833e8\n\nChanges since v1:\n\n * Fixed wrong commit title and date in the reference (Junio, Taylor).\n * use test_expect_failure with the correct assertion instead of a # BUG\n comment (Taylor).\n * Simplified commit messages.\n\nKristofer Karlsson (2):\n commit-graph: add trace2 instrumentation for generation DFS\n commit-graph: propagate topo_levels slab to all chain layers\n\n commit-graph.c | 7 ++++++-\n t/t5324-split-commit-graph.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\n\nbase-commit: f85a7e662054a7b0d9070e432508831afa214b47\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2170%2Fspkrka%2Fkrka%2Ffix-topo-levels-slab-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2170/spkrka/krka/fix-topo-levels-slab-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/2170\n\nRange-diff vs v1:\n\n 1: b865c2bcff ! 1: 100efa22a9 commit-graph: add trace2 instrumentation for generation DFS\n @@ Metadata\n ## Commit message ##\n commit-graph: add trace2 instrumentation for generation DFS\n \n - Add a step counter and trace2_data_intmax call to\n - compute_reachable_generation_numbers() to make the cost of\n - the generation number DFS observable. This exposes a\n - regression introduced in 199d452758 (commit-graph: fix\n - \"filling in\" topological levels, 2025-04-07) where\n - incremental commit-graph writes re-walk the entire commit\n - ancestry instead of reading topo levels from lower graph\n - layers.\n + Count the number of steps taken in\n + compute_reachable_generation_numbers() and expose it via\n + trace2 to make it easier to detect performance regressions.\n \n - Add a test that demonstrates the problem: with a two-layer\n - split commit-graph, writing a new incremental layer for a\n - commit whose parent is in the base layer walks all the way\n - down to the root (7 steps for 5 base commits) instead of\n - reading the existing topo level and stopping immediately\n - (1 step).\n + Add a failing test for such a regression, introduced in\n + 199d452758 (commit-graph: return the prepared commit graph\n + from `prepare_commit_graph()`, 2025-09-04), where incremental\n + commit-graph writes do not see existing generation numbers\n + from lower graph layers and fall back to walking the full\n + ancestry.\n \n Signed-off-by: Kristofer Karlsson \n \n @@ t/t5324-split-commit-graph.sh: test_expect_success 'write generation data chunk\n \t)\n '\n \n -+test_expect_success 'incremental write reads topo levels from all layers' '\n ++test_expect_failure 'incremental write reads topo levels from all layers' '\n +\tgit init topo-from-lower &&\n +\t(\n +\t\tcd topo-from-lower &&\n @@ t/t5324-split-commit-graph.sh: test_expect_success 'write generation data chunk\n +\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" \\\n +\t\t\tgit commit-graph write --reachable --split=no-merge &&\n +\n -+\t\t# BUG: topo levels from lower graph layers are not\n -+\t\t# propagated, so the DFS re-walks from base-3 down to\n -+\t\t# the root (7 steps) instead of reading topo levels\n -+\t\t# from the existing graph (1 step).\n -+\t\ttest_trace2_data commit-graph generation-dfs-steps 7 topo_levels`\n - (the topmost layer) instead of `chain->topo_levels` (the\n - current iteration variable).\n + The topo_levels slab is only propagated to the topmost graph\n + layer instead of all layers in the chain. Commits from lower\n + layers appear to have no generation numbers, so the DFS\n + re-walks the entire ancestry.\n \n - This meant only the topmost layer had its topo_levels pointer\n - set. When compute_reachable_generation_numbers() ran for an\n - incremental write, commits parsed from lower layers had their\n - topo levels left at zero in the slab, since\n - fill_commit_graph_info() could not store them without the\n - pointer. The DFS then re-walked the entire commit ancestry\n - instead of stopping at commits with known levels.\n -\n - On a repository with 2.78M commits and a multi-layer split\n - commit-graph, this caused a single incremental commit-graph\n - write to spend ~3.7 seconds in the generation DFS instead of\n - microseconds.\n + Fix by making topo_levels visible to all layers, not just\n + the first one.\n \n Signed-off-by: Kristofer Karlsson \n \n @@ commit-graph.c: int write_commit_graph(struct odb_source *source,\n \t\tctx.changed_paths = 1;\n \n ## t/t5324-split-commit-graph.sh ##\n -@@ t/t5324-split-commit-graph.sh: test_expect_success 'incremental write reads topo levels from all layers' '\n - \t\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" \\\n - \t\t\tgit commit-graph write --reachable --split=no-merge &&\n - \n --\t\t# BUG: topo levels from lower graph layers are not\n --\t\t# propagated, so the DFS re-walks from base-3 down to\n --\t\t# the root (7 steps) instead of reading topo levels\n --\t\t# from the existing graph (1 step).\n --\t\ttest_trace2_data commit-graph generation-dfs-steps 7 \n\nCount the number of steps taken in\ncompute_reachable_generation_numbers() and expose it via\ntrace2 to make it easier to detect performance regressions.\n\nAdd a failing test for such a regression, introduced in\n199d452758 (commit-graph: return the prepared commit graph\nfrom `prepare_commit_graph()`, 2025-09-04), where incremental\ncommit-graph writes do not see existing generation numbers\nfrom lower graph layers and fall back to walking the full\nancestry.\n\nSigned-off-by: Kristofer Karlsson \n---\n commit-graph.c | 5 +++++\n t/t5324-split-commit-graph.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 29 insertions(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex c6d9c5c740..702ba9731b 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1653,6 +1653,7 @@ static void compute_reachable_generation_numbers(\n {\n \tint i;\n \tstruct commit_list *list = NULL;\n+\tintmax_t steps = 0;\n \n \tfor (i = 0; i < info->commits->nr; i++) {\n \t\tstruct commit *c = info->commits->items[i];\n@@ -1671,6 +1672,7 @@ static void compute_reachable_generation_numbers(\n \t\t\tint all_parents_computed = 1;\n \t\t\ttimestamp_t max_gen = 0;\n \n+\t\t\tsteps++;\n \t\t\tfor (parent = current->parents; parent; parent = parent->next) {\n \t\t\t\trepo_parse_commit(info->r, parent->item);\n \t\t\t\tgen = info->get_generation(parent->item, info->data);\n@@ -1694,6 +1696,9 @@ static void compute_reachable_generation_numbers(\n \t\t\t}\n \t\t}\n \t}\n+\n+\ttrace2_data_intmax(\"commit-graph\", info->r,\n+\t\t\t \"generation-dfs-steps\", steps);\n }\n \n static timestamp_t get_topo_level(struct commit *c, void *data)\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 49a057cc2e..b41331e3dd 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -718,6 +718,30 @@ test_expect_success 'write generation data chunk when commit-graph chain is repl\n \t)\n '\n \n+test_expect_failure 'incremental write reads topo levels from all layers' '\n+\tgit init topo-from-lower &&\n+\t(\n+\t\tcd topo-from-lower &&\n+\n+\t\tfor i in $(test_seq 5)\n+\t\tdo\n+\t\t\ttest_commit base-$i || return 1\n+\t\tdone &&\n+\t\tgit commit-graph write --reachable &&\n+\n+\t\ttest_commit extra &&\n+\t\tgit commit-graph write --reachable --split=no-merge &&\n+\n+\t\tgit checkout base-3 &&\n+\t\ttest_commit new-branch &&\n+\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" \\\n+\t\t\tgit commit-graph write --reachable --split=no-merge &&\n+\n+\t\ttest_trace2_data commit-graph generation-dfs-steps 1 \n\nThe topo_levels slab is only propagated to the topmost graph\nlayer instead of all layers in the chain. Commits from lower\nlayers appear to have no generation numbers, so the DFS\nre-walks the entire ancestry.\n\nFix by making topo_levels visible to all layers, not just\nthe first one.\n\nSigned-off-by: Kristofer Karlsson \n---\n commit-graph.c | 2 +-\n t/t5324-split-commit-graph.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 702ba9731b..a0bca248ac 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2610,7 +2610,7 @@ int write_commit_graph(struct odb_source *source,\n \n \tg = prepare_commit_graph(ctx.r);\n \tfor (struct commit_graph *chain = g; chain; chain = chain->base_graph)\n-\t\tg->topo_levels = &topo_levels;\n+\t\tchain->topo_levels = &topo_levels;\n \n \tif (flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS)\n \t\tctx.changed_paths = 1;\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex b41331e3dd..9e5ab7dbd0 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -718,7 +718,7 @@ test_expect_success 'write generation data chunk when commit-graph chain is repl\n \t)\n '\n \n-test_expect_failure 'incremental write reads topo levels from all layers' '\n+test_expect_success 'incremental write reads topo levels from all layers' '\n \tgit init topo-from-lower &&\n \t(\n \t\tcd topo-from-lower &&\n-- \ngitgitgadget\n"},{"id":"547798","messageId":"alFthqGQjsowvpEz@com-79390","threadId":"65941","inReplyTo":"CAL71e4PuD9D8LRbP3mfxxeMrM+1q--3sCp6oJs=hezdasZUPMw@mail.gmail.com","subject":"Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-07-10T22:09:10Z","receivedAt":"2026-07-10T22:09:15Z","isPatch":true,"body":"On Tue, Jul 07, 2026 at 04:08:36PM +0200, Kristofer Karlsson wrote:\n> > Instead of writing \"# BUG ...\" and then an incorrect assertion, I\n> > would suggest that you write the assertion you expect:\n> >\n> > test_trace2_data commit-graph generation-dfs-steps 1 >\n> > , but mark the test as \"test_expect_failure\".\n>\n> I started with this actually and then changed my mind in order\n> to demonstrate exactly how the counter changed, not just that it\n> changed from failure to success. But I'd be happy to change this\n> too if needed - it would effectively reduce the second commit to\n> just the bugfix line and switching from test_expect_failure\n> to test_expect_success.\n\nYeah, I think this would be ideal.\n\nThanks,\nTaylor\n"},{"id":"547799","messageId":"alFuxPQQcFxseAzh@com-79390","threadId":"65941","inReplyTo":"CAL71e4OuU1+KHd0TrcxDX2dyoWEJXmi86m8u+E7vtxhcSF6M1Q@mail.gmail.com","subject":"Re: [PATCH 2/2] commit-graph: propagate topo_levels slab to all chain layers","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-07-10T22:14:28Z","receivedAt":"2026-07-10T22:14:33Z","isPatch":true,"body":"On Tue, Jul 07, 2026 at 04:57:13PM +0200, Kristofer Karlsson wrote:\n> (b) Move topo_levels to struct object_database. Since\n> fill_commit_graph_info() can already reach the odb via\n> g->odb_source->odb, no signature changes are needed.\n> The write side becomes a single assignment:\n>\n> ctx.r->objects->topo_levels = &topo_levels;\n>\n> and cleanup becomes:\n>\n> ctx.r->objects->topo_levels = NULL;\n>\n> No chain walk needed and the diff is fairly small.\n> I am not sure about the semantics of it though -- should the odb\n> have a reference to topo_levels?\n\nThis seems to be the most promising approach, though I'd be curious what\nPatrick's thoughts are. The commit-slab API is really a property of the\nobject database, but we treat these as a global as I do not recall them\nyet being touched by the ODB refactoring effort.\n\n> [...]\n>\n> I have a prototype of (b) that compiles and passes the test suite.\n>\n> For now though, I think the minimal bugfix is the right thing to do.\n\nAgreed.\n\nThanks,\nTaylor\n"},{"id":"547800","messageId":"alFu8gZURKhYr1VE@com-79390","threadId":"65941","inReplyTo":"pull.2170.v2.git.1783609382.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] commit-graph: fix topo_levels slab propagation regression","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-07-10T22:15:14Z","receivedAt":"2026-07-10T22:15:20Z","isPatch":true,"body":"On Thu, Jul 09, 2026 at 03:02:59PM +0000, Kristofer Karlsson via GitGitGadget wrote:\n> Changes since v1:\n>\n> * Fixed wrong commit title and date in the reference (Junio, Taylor).\n> * use test_expect_failure with the correct assertion instead of a # BUG\n> comment (Taylor).\n> * Simplified commit messages.\n>\n> Kristofer Karlsson (2):\n> commit-graph: add trace2 instrumentation for generation DFS\n> commit-graph: propagate topo_levels slab to all chain layers\n>\n> commit-graph.c | 7 ++++++-\n> t/t5324-split-commit-graph.sh | 24 ++++++++++++++++++++++++\n> 2 files changed, 30 insertions(+), 1 deletion(-)\n\nThanks, this version looks good to me.\n\nThanks,\nTaylor\n"},{"id":"547804","messageId":"xmqqik6mbhtw.fsf@gitster.g","threadId":"65941","inReplyTo":"alFthqGQjsowvpEz@com-79390","subject":"Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-10T22:28:11Z","receivedAt":"2026-07-10T22:28:13Z","isPatch":true,"body":"Taylor Blau writes:\n\n> On Tue, Jul 07, 2026 at 04:08:36PM +0200, Kristofer Karlsson wrote:\n>> > Instead of writing \"# BUG ...\" and then an incorrect assertion, I\n>> > would suggest that you write the assertion you expect:\n>> >\n>> > test_trace2_data commit-graph generation-dfs-steps 1 > >\n>> > , but mark the test as \"test_expect_failure\".\n>>\n>> I started with this actually and then changed my mind in order\n>> to demonstrate exactly how the counter changed, not just that it\n>> changed from failure to success. But I'd be happy to change this\n>> too if needed - it would effectively reduce the second commit to\n>> just the bugfix line and switching from test_expect_failure\n>> to test_expect_success.\n>\n> Yeah, I think this would be ideal.\n\nIf the test involved is longer than 3 lines, I would recommend\nagainst it, as \"git show\" of such a patch will show the full code\nchange to implement a different behaviour plus \"_failure\" changing\nto \"_success\" in the test, with the body of the test hidden outside\nthe context, which makes it hard to guess what the behaviour change\nis really about.\n\n"},{"id":"547809","messageId":"alF4rYSTxpQUC38K@com-79390","threadId":"65941","inReplyTo":"xmqqik6mbhtw.fsf@gitster.g","subject":"Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-07-10T22:56:45Z","receivedAt":"2026-07-10T22:56:50Z","isPatch":true,"body":"On Fri, Jul 10, 2026 at 03:28:11PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > On Tue, Jul 07, 2026 at 04:08:36PM +0200, Kristofer Karlsson wrote:\n> >> > Instead of writing \"# BUG ...\" and then an incorrect assertion, I\n> >> > would suggest that you write the assertion you expect:\n> >> >\n> >> > test_trace2_data commit-graph generation-dfs-steps 1 >> >\n> >> > , but mark the test as \"test_expect_failure\".\n> >>\n> >> I started with this actually and then changed my mind in order\n> >> to demonstrate exactly how the counter changed, not just that it\n> >> changed from failure to success. But I'd be happy to change this\n> >> too if needed - it would effectively reduce the second commit to\n> >> just the bugfix line and switching from test_expect_failure\n> >> to test_expect_success.\n> >\n> > Yeah, I think this would be ideal.\n>\n> If the test involved is longer than 3 lines, I would recommend\n> against it, as \"git show\" of such a patch will show the full code\n> change to implement a different behaviour plus \"_failure\" changing\n> to \"_success\" in the test, with the body of the test hidden outside\n> the context, which makes it hard to guess what the behaviour change\n> is really about.\n\nHmm, I am not sure that I agree. Or, at the very least, that is now how\nI have written series in the past where I want to demonstrate and then\nsubsequently fix an existing bug.\n\nWhen either the test setup or the bugfix is trivial, I think having it\nin the same commit is just fine. But I think there are two good reasons\nfor splitting it out if the test or bug is complex:\n\n - If the test is complex, but the complexity is not directly related to\n the bugfix, having to explain both in the same commit message can be\n awkward, and makes it harder for a reviewer to reason about either\n component of the patch.\n\n - If the bugfix is complex, having the failing test in a separate\n commit demonstrates that the bug existed before, but is definitively\n fixed in the following commit, as both would be expected to 'make\n test' cleanly.\n\nI am happy to change my style if you feel strongly. It would be nice to\ndocument this in CodingGuidelines (or SubmittingPatches?) if it is not\nalready.\n\nThanks,\nTaylor\n"},{"id":"547866","messageId":"xmqqech99qe3.fsf@gitster.g","threadId":"65941","inReplyTo":"alF4rYSTxpQUC38K@com-79390","subject":"Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-11T21:18:28Z","receivedAt":"2026-07-11T21:18:31Z","isPatch":true,"body":"Taylor Blau writes:\n\n>> If the test involved is longer than 3 lines, I would recommend\n>> against it, as \"git show\" of such a patch will show the full code\n>> change to implement a different behaviour plus \"_failure\" changing\n>> to \"_success\" in the test, with the body of the test hidden outside\n>> the context, which makes it hard to guess what the behaviour change\n>> is really about.\n>\n> Hmm, I am not sure that I agree. Or, at the very least, that is now how\n> I have written series in the past where I want to demonstrate and then\n> subsequently fix an existing bug.\n\nAfter applying and in viewing \"git log -W -p\", there is no such\ndifficulty like the one I described in the message you are\nresponding to, but it makes it harder on reviewers on the mailing\nlist, to make a quick pre-review based only on the material that\nthey can see in the e-mail.\n\nIt may be easier to write the commits, but given that we seem to\nhave more patches sent to the list than reviewers can review, it may\nnot be a good trade-off.\n"},{"id":"547959","messageId":"alSCv5I94qjbSucQ@pks.im","threadId":"65941","inReplyTo":"alFuxPQQcFxseAzh@com-79390","subject":"Re: [PATCH 2/2] commit-graph: propagate topo_levels slab to all chain layers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-07-13T06:16:31Z","receivedAt":"2026-07-13T06:16:38Z","isPatch":true,"body":"On Fri, Jul 10, 2026 at 03:14:28PM -0700, Taylor Blau wrote:\n> On Tue, Jul 07, 2026 at 04:57:13PM +0200, Kristofer Karlsson wrote:\n> > (b) Move topo_levels to struct object_database. Since\n> > fill_commit_graph_info() can already reach the odb via\n> > g->odb_source->odb, no signature changes are needed.\n> > The write side becomes a single assignment:\n> >\n> > ctx.r->objects->topo_levels = &topo_levels;\n> >\n> > and cleanup becomes:\n> >\n> > ctx.r->objects->topo_levels = NULL;\n> >\n> > No chain walk needed and the diff is fairly small.\n> > I am not sure about the semantics of it though -- should the odb\n> > have a reference to topo_levels?\n> \n> This seems to be the most promising approach, though I'd be curious what\n> Patrick's thoughts are. The commit-slab API is really a property of the\n> object database, but we treat these as a global as I do not recall them\n> yet being touched by the ODB refactoring effort.\n\nI was investigating several times whether we can remove them from global\nscope and move them into the object database indeed. The answer is that\nit's somewhat complicated because we reuse the slab for multiple\ndifferent things, and detangling that has proven to be a bit of a mess.\n\nThe other question here is whether commit graphs really are a property\nof the object database itself, or whether they are rather a property of\na given backend. Sure, we can only have a single commit graph at any\npoint in time, so they feel like they are at the object database level.\nBut is the current implementation of a commit graph really the best for\nall potential backends out there?\n\nIf you take for example a distributed backend to store objects, then you\nprobably don't want to have a single local commit graph that is stored\nin \".git/objects/info\". Furthermore, the current format may not even be\nthe best one to store the cached information, either.\n\nSo ultimately, I can see one of two approaches:\n \n - Either we make the commit graph itself pluggable as a standalone\n mechanism, too.\n\n - Or we treat it as a property of the object backend.\n\nI haven't fully made up my mind yet. But I guess detangling the current\nmess that we have with the commit graphs would help regardless of which\ndirection we eventually go into.\n\nThanks!\n\nPatrick\n"},{"id":"548035","messageId":"CAL71e4M8-KtnkC5qQP2iuhON=ROoOTVZfbZB8UhJ-+3KgEP9=g@mail.gmail.com","threadId":"65941","inReplyTo":"xmqqech99qe3.fsf@gitster.g","subject":"Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS","fromName":"Kristofer Karlsson","fromEmail":"krka@spotify.com","sentAt":"2026-07-13T19:55:51Z","receivedAt":"2026-07-13T19:56:04Z","isPatch":true,"body":"On Sat, 11 Jul 2026 at 23:18, Junio C Hamano wrote:\n>\n> Taylor Blau writes:\n>\n> >> If the test involved is longer than 3 lines, I would recommend\n> >> against it, as \"git show\" of such a patch will show the full code\n> >> change to implement a different behaviour plus \"_failure\" changing\n> >> to \"_success\" in the test, with the body of the test hidden outside\n> >> the context, which makes it hard to guess what the behaviour change\n> >> is really about.\n> >\n> > Hmm, I am not sure that I agree. Or, at the very least, that is now how\n> > I have written series in the past where I want to demonstrate and then\n> > subsequently fix an existing bug.\n>\n> After applying and in viewing \"git log -W -p\", there is no such\n> difficulty like the one I described in the message you are\n> responding to, but it makes it harder on reviewers on the mailing\n> list, to make a quick pre-review based only on the material that\n> they can see in the e-mail.\n>\n> It may be easier to write the commits, but given that we seem to\n> have more patches sent to the list than reviewers can review, it may\n> not be a good trade-off.\n\nI've been pondering this dilemma for a bit. I agree with Taylor\nthat atomic commits are valuable and I quite like proving the bug\nexists before fixing it. It's not black and white though,\nfor race conditions or hard to reproduce cases I tend to fold the\ntest into the fix commit directly instead.\n\nBut the review process is also critical and its overhead should be\nminimized.\n\nCould tooling help here? The submitter should know which parts\nof the patch need more context for review. If they could selectively\nexpand context before sending, reviewers would see the full picture\nin the email without sacrificing having atomic commits.\n\ngit apply already handles patches with extra context lines just\nfine, so we just need something to assist in producing that extra\ncontext -- either some configurability in git format-patch itself\n(like -W, but more fine-grained control over _where_ that gets\napplied) or some post-processing tool to expand context in patches\nbefore sending.\n\nToo late for this round, but I might give that a try in the future\nif I run into a similar scenario again.\n\nThanks,\nKristofer\n"},{"id":"548039","messageId":"xmqqldbewriu.fsf@gitster.g","threadId":"65941","inReplyTo":"CAL71e4M8-KtnkC5qQP2iuhON=ROoOTVZfbZB8UhJ-+3KgEP9=g@mail.gmail.com","subject":"Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-13T20:42:17Z","receivedAt":"2026-07-13T20:42:19Z","isPatch":true,"body":"Kristofer Karlsson writes:\n\n> I've been pondering this dilemma for a bit. I agree with Taylor\n> that atomic commits are valuable and I quite like proving the bug\n> exists before fixing it.\n\nI do not quite understand. Even if you fix the code and add a\npassing test, the commit remains atomic. With an artificial\nsplit, you only increase your commit count while making the changes\nharder to review. When grouping a code fix with a newly passing\ntest:\n\n * \"git show\" displays both the implementation changes and the\n test. You can review both, and if you agree with the behavior\n expected by the test, the change is complete.\n\n * If the pre-fix behavior is unclear, it is easy to check by\n running:\n\n $ git show ':!t/' | git apply -R && make test\n\n This demonstrates exactly how the unfixed code breaks on the\n new test.\n\n> Too late for this round, but I might give that a try in the future\n> if I run into a similar scenario again.\n\nThe existing tooling already supports this workflow (as demonstrated\nby the command above). Please avoid artificially making the context\nlarger, as doing so increases the likelihood of merge conflicts with\nother changes.\n"},{"id":"548043","messageId":"CAL71e4MOz1PqAAdGCnKsdkWkOs+HN_Q1d4mpZc_g1Mi2+2czgg@mail.gmail.com","threadId":"65941","inReplyTo":"xmqqldbewriu.fsf@gitster.g","subject":"Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS","fromName":"Kristofer Karlsson","fromEmail":"krka@spotify.com","sentAt":"2026-07-13T22:17:08Z","receivedAt":"2026-07-13T22:17:20Z","isPatch":true,"body":"On Mon, 13 Jul 2026 at 22:42, Junio C Hamano wrote:\n>\n> I do not quite understand. Even if you fix the code and add a\n> passing test, the commit remains atomic. With an artificial\n> split, you only increase your commit count while making the changes\n> harder to review. When grouping a code fix with a newly passing\n> test:\n>\n> * \"git show\" displays both the implementation changes and the\n> test. You can review both, and if you agree with the behavior\n> expected by the test, the change is complete.\n>\n> * If the pre-fix behavior is unclear, it is easy to check by\n> running:\n>\n> $ git show ':!t/' | git apply -R && make test\n\nThat's quite neat, and it matches the local\ndevelopment flow if you write the failing test first.\n\nI can see the advantages of grouping the test and bugfix in the\nsame commit, and I'm happy to follow that convention going forward.\n\n> > Too late for this round, but I might give that a try in the future\n> > if I run into a similar scenario again.\n>\n> The existing tooling already supports this workflow (as demonstrated\n> by the command above). Please avoid artificially making the context\n> larger, as doing so increases the likelihood of merge conflicts with\n> other changes.\n\nThanks, that makes sense. It was an interesting thought experiment,\nbut I'll leave it there.\n\n- Kristofer\n"},{"id":"548067","messageId":"alWtk5eQqS9JTzDr@com-79390","threadId":"65941","inReplyTo":"alSCv5I94qjbSucQ@pks.im","subject":"Re: [PATCH 2/2] commit-graph: propagate topo_levels slab to all chain layers","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-07-14T03:31:31Z","receivedAt":"2026-07-14T03:31:37Z","isPatch":true,"body":"On Mon, Jul 13, 2026 at 08:16:31AM +0200, Patrick Steinhardt wrote:\n> On Fri, Jul 10, 2026 at 03:14:28PM -0700, Taylor Blau wrote:\n> > On Tue, Jul 07, 2026 at 04:57:13PM +0200, Kristofer Karlsson wrote:\n> > > (b) Move topo_levels to struct object_database. Since\n> > > fill_commit_graph_info() can already reach the odb via\n> > > g->odb_source->odb, no signature changes are needed.\n> > > The write side becomes a single assignment:\n> > >\n> > > ctx.r->objects->topo_levels = &topo_levels;\n> > >\n> > > and cleanup becomes:\n> > >\n> > > ctx.r->objects->topo_levels = NULL;\n> > >\n> > > No chain walk needed and the diff is fairly small.\n> > > I am not sure about the semantics of it though -- should the odb\n> > > have a reference to topo_levels?\n> >\n> > This seems to be the most promising approach, though I'd be curious what\n> > Patrick's thoughts are. The commit-slab API is really a property of the\n> > object database, but we treat these as a global as I do not recall them\n> > yet being touched by the ODB refactoring effort.\n>\n> I was investigating several times whether we can remove them from global\n> scope and move them into the object database indeed. The answer is that\n> it's somewhat complicated because we reuse the slab for multiple\n> different things, and detangling that has proven to be a bit of a mess.\n\nIt's an interesting question, and I think worth discussing, though note\nthat I would also like to ensure that we resolve this in the short-term\nto prevent any future regression while the pluggable ODB refactor\ncontinues on.\n\n> The other question here is whether commit graphs really are a property\n> of the object database itself, or whether they are rather a property of\n> a given backend. Sure, we can only have a single commit graph at any\n> point in time, so they feel like they are at the object database level.\n> But is the current implementation of a commit graph really the best for\n> all potential backends out there?\n>\n> If you take for example a distributed backend to store objects, then you\n> probably don't want to have a single local commit graph that is stored\n> in \".git/objects/info\". Furthermore, the current format may not even be\n> the best one to store the cached information, either.\n\nI think I agree here in part, though I think there is some subtlety that\nis specific to commit-graphs.\n\nIf I understand your argument correctly, I think that I am on-board with\nit if you substitute \"commit-graph\" with \"MIDX\" or \"reachability\nbitmaps\", as those are optimizations over a specific representation of\nthe object store.\n\nThe commit-graph is somewhat of an oddity in that regard. While it is\npartially an optimization in the representation format, it is also a\ndata-structure which is useful independent of the underlying storage. On\nthe former, I absolutely agree with what you're saying: having a\nrow-oriented layout to optimize commit traversals may not be necessary\nin a different implementation of the object store which has efficient\nenough access to the commit objects so as to make the row-oriented\nlayout unnecessary.\n\nHowever, it is a useful question to ask \"what is the generation number\nof this commit?\" independently of whether we store the commit objects\nthemselves in the existing ODB, in a generic blob storage system, or\nsomething else entirely.\n\nThanks,\nTaylor\n"}]}