commit-reach: terminate merge-base walk when one side is exhausted #2149

gitgitgadget · 2026-07-06T12:03:21Z

Derrick Stolee wrote on the Git mailing list (how to reply to this email):

On 6/20/2026 6:36 AM, Kristofer Karlsson via GitGitGadget wrote: > From: Kristofer Karlsson <krka@spotify.com> > > Add a technical document describing the paint_down_to_common() > algorithm used for merge-base computation. I like the idea of documenting this so it's easier to understand. There is risk of drift from the actual implementation. You may want to add a comment to the method in commit-reach.c to indicate that any change should be reflected in this document. > +Termination > +----------- > + > +Termination happens when we can prove that no extra progress is > +possible. We are done with the main loop when one of the following > +conditions holds: > + > + 1. The queue is empty. > + 2. The queue only contains STALE entries. > + 3. Side-exhaustion: the walk has reached the finite region and one > + of the sides is fully exhausted. It could be an interesting exercise, but potentially wasteful, to add this document as a Patch 1, but reflecting the old algorithm and then to update the document at the same time as you update the code. The changes in your patch 2 would impact this doc in terms of the data being tracked by the paint_queue data structure instead of the nonstale_queue structure (though those details are not currently handled in the current version). The change to the termination condition would come along with patch 3. Thanks, -Stolee

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

On Mon, 22 Jun 2026 at 20:21, Derrick Stolee <stolee@gmail.com> wrote: > > I like the idea of documenting this so it's easier to understand. Yes I was myself thinking that I can prove it to myself now that it works, and anyone else could also prove it to themselves, but having it explicit here is even better. I found the other documents (i.e. commit-graph) to be a good source of inspiration here. > There is risk of drift from the actual implementation. You may want > to add a comment to the method in commit-reach.c to indicate that > any change should be reflected in this document. Good idea, will add that. > > +Termination > > +----------- > > + > > +Termination happens when we can prove that no extra progress is > > +possible. We are done with the main loop when one of the following > > +conditions holds: > > + > > + 1. The queue is empty. > > + 2. The queue only contains STALE entries. > > + 3. Side-exhaustion: the walk has reached the finite region and one > > + of the sides is fully exhausted. > It could be an interesting exercise, but potentially wasteful, to > add this document as a Patch 1, but reflecting the old algorithm > and then to update the document at the same time as you update the > code. I did consider that initially but I was worried it would be considered noisy. I am quite happy to rework it in a way that first explains the status quo. That would make the document diff more interesting. Agreed that should become the first patch, and the patch that changes the algorithm should include the documentation change. > The changes in your patch 2 would impact this doc in terms of the > data being tracked by the paint_queue data structure instead of the > nonstale_queue structure (though those details are not currently > handled in the current version). The change to the termination > condition would come along with patch 3. Agreed, I would need to rephrase from tracking non-stale to tracking counts of p1 and p2 (and pending merge bases) commits, but I think that would be a small tweak and well worth doing. Thanks, Kristofer

gitgitgadget · 2026-07-06T12:33:35Z

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

"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Kristofer Karlsson <krka@spotify.com> > > Add a technical document describing the paint_down_to_common() > algorithm used for merge-base computation, covering the paint > walk, generation number regions, and termination conditions. > > Signed-off-by: Kristofer Karlsson <krka@spotify.com> > --- > Documentation/Makefile | 1 + > Documentation/technical/meson.build | 1 + > .../technical/paint-down-to-common.adoc | 114 ++++++++++++++++++ > commit-reach.c | 6 +- > 4 files changed, 121 insertions(+), 1 deletion(-) > create mode 100644 Documentation/technical/paint-down-to-common.adoc Great write-up that very clearly and concisely explains what goes on inside the merge-base computation. Thanks for a pleasant read.

gitgitgadget · 2026-07-26T07:26:34Z

Elijah Newren wrote on the Git mailing list (how to reply to this email):

On Sat, Jul 11, 2026 at 6:27 AM Kristofer Karlsson via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Kristofer Karlsson <krka@spotify.com> > > Add a technical document describing the paint_down_to_common() > algorithm used for merge-base computation, covering the paint > walk, generation number regions, and termination conditions. Thanks, this is really nice. > +In the finite region, generation ordering guarantees topological > +traversal: children are always visited before their parents. This > +means that paint on already-visited commits is final -- no future > +traversal step can add paint to them. This is the critical invariant. I think there's a small hole here, however. For a v1 commit-graph, generation numbers saturate at GENERATION_NUMBER_V1_MAX; from Documentation/technical/commit-graph.adoc: """ We use the macro GENERATION_NUMBER_V1_MAX = 0x3FFFFFFF for commits whose topological levels (generation number v1) are computed to be at least this value. We limit at this value since it is the largest value that can be stored in the commit-graph file using the 30 bits available to topological levels. This presents another case where a commit can have generation number equal to that of a parent. """ > +In the INFINITY region, commit-date ordering can violate this: a > +parent with a later date can be visited before a child with an earlier > +date. Paint flags are therefore NOT final at visit time, and a > +commit visited with only one side's paint may later gain the other. Perhaps we could lump GENERATION_NUMBER_V1_MAX the same as INFINITY for this algorithm, since GENERATION_NUMBER_V1_MAX can also violate the ordering we want? > +Generation cutoff > +~~~~~~~~~~~~~~~~~ > +Some callers (notably `remove_redundant()`) supply a `min_generation` > +threshold -- the minimum generation of the input commits. No merge > +base can have a generation below this threshold, so the walk > +terminates as soon as it dequeues such a commit. ? I'm not sure I'm following the wording here. Typically a merge-base is a common ancestor of the inputs, and ancestors have a strictly lower generation than their descendants, and there's no limit to how far back we might need to read to find a merge base. I think what makes the min_generation cutoff safe is that callers passing a nonzero min_generation (remove_redundant() and repo_in_merge_bases_many()) don't need those deeper merge bases at all: they only need to determine reachability among the input commits, all of which sit at or above min_generation. Is there a risk that with the current wording of this paragraph that future callers might be tempted to pass a nonzero min_generation and still expect a complete MERGE_BASE_FIND_ALL result?

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

On Sun, 26 Jul 2026 at 08:59, Elijah Newren <newren@gmail.com> wrote: > > > Perhaps we could lump GENERATION_NUMBER_V1_MAX the same as INFINITY > for this algorithm, since GENERATION_NUMBER_V1_MAX can also violate > the ordering we want? Oh, that's a nice catch. I had completely missed this V1_MAX saturation case! I spent some time thinking about this edge-case and fortunately I think it's very hard to trigger. You would need to construct a graph with 1B generations, which is a very big git graph indeed. That said, regardless of how hard it is to trigger the code should be correct. I agree that we could lump GENERATION_NUMBER_V1_MAX together with GENERATION_NUMBER_INFINITY since they are both topologically unordered values and naturally must be topologically above any lower values. I considered two approaches. Both are correct as far as I can tell and both only matter for very large v1 graphs, so in practice either should be fine. Option A: remap V1_MAX to INFINITY at load time in fill_commit_graph_info(). On the read side: uint32_t level = get_be32(...) >> 2; if (level >= GENERATION_NUMBER_V1_MAX) graph_data->generation = GENERATION_NUMBER_INFINITY; else graph_data->generation = level; This is a fairly small change and everything downstream just works. The write path already uses the separate topo_levels slab so it would keep writing V1_MAX for backward compatibility. The downside is that it conflates two distinct concepts: "not in the commit-graph at all" and "in the graph but ordering has saturated." This specifically affects the bloom filter checks in blame.c, revision.c and last-modified.c where we check for gen == INFINITY to mean specifically "not in the graph" because a commit not in the graph cannot have a bloom filter. With the remap, V1_MAX commits would unnecessarily skip bloom filters. Not a correctness bug, but a performance regression for those commits. Perhaps not a very important case though -- if you have that many commits and notice performance issues you should probably upgrade to v2 anyway. That said, even if it technically works today, maybe new code in the future would have stronger dependencies on the semantics so it feels fragile. Option B: introduce commit_graph_generation_topo_ceiling(r) that returns the generation value where topological ordering is no longer guaranteed -- V1_MAX for v1 graphs, INFINITY for v2 or no graph. Then the early exit gates use: if (generation < state.topo_ceiling) /* in the topologically ordered region */ This keeps INFINITY meaning "not in the graph" and V1_MAX meaning "saturated but present." Bloom filter checks continue to work the same as before. It does introduce a new concept that callers of the ordering gates need to be aware of, but the concept maps directly to the underlying graph format difference. I went with option B in my local v7 draft since it felt like a less intrusive change, though it would be nice to hide the v1/v2 differences more from the rest of the code. Let me know if you have a preference or see issues with either approach. > I think what makes the min_generation cutoff safe is that callers > passing a nonzero min_generation (remove_redundant() and > repo_in_merge_bases_many()) don't need those deeper merge bases at > all: they only need to determine reachability among the input commits, > all of which sit at or above min_generation. > > Is there a risk that with the current wording of this paragraph that > future callers might be tempted to pass a nonzero min_generation and > still expect a complete MERGE_BASE_FIND_ALL result? You are right, the wording is misleading. I can update it to something like this instead (will polish it more, just a draft): Note: A non-zero min_generation floor means that you are not guaranteed to find any merge-base, it is purely useful for determining the ancestry relation between the input commits. If it is set, the walk can terminate as soon as we have passed the bottom commit, because we then know that there is no direct ancestry. Thanks, I really appreciate the careful review and spotting this edge case! Kristofer

gitgitgadget · 2026-08-07T03:18:40Z

Elijah Newren wrote on the Git mailing list (how to reply to this email):

On Thu, Aug 6, 2026 at 4:04 AM Kristofer Karlsson via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Kristofer Karlsson <krka@spotify.com> > > Add a technical document describing the paint_down_to_common() > algorithm used for merge-base computation, covering the paint > walk, generation number regions, and termination conditions. This is a great doc providing an overview of how everything works. > +With v1 commit-graphs (topological levels, no GDAT chunk), > +generation numbers saturate at `GENERATION_NUMBER_V1_MAX`. > +Saturated commits share the same generation value despite > +different topological depths, which breaks ordering guarantees > +in the same way as INFINITY. The early exit gates compare > +against `GENERATION_NUMBER_V1_MAX` for v1 graphs and > +`GENERATION_NUMBER_INFINITY` for v2 graphs, so that saturated > +commits are treated as unordered. Perfect, thanks for addressing this since the previous round. > +Generation cutoff > +~~~~~~~~~~~~~~~~~ > +Some callers (notably `remove_redundant()`) supply a `min_generation` > +threshold equal to the minimum generation of the input commits. > +These callers only need to determine reachability among the inputs, > +not find deep merge bases, so the walk can safely terminate when it > +dequeues a commit below this threshold. This reads much better; thanks. The rest looks the same as the last round that I already reviewed and looks good.

gitgitgadget · 2026-08-07T03:22:17Z

Elijah Newren wrote on the Git mailing list (how to reply to this email):

On Thu, Aug 6, 2026 at 4:00 AM Kristofer Karlsson via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Kristofer Karlsson <krka@spotify.com> > > Remove the fallback that switched paint_down_to_common() from > generation ordering to commit-date ordering when the commit-graph > lacks corrected commit dates (v1 graph with topo levels only). > > The fallback was added in 091f4cf3 (commit: don't use generation > numbers if not needed, 2018-08-30) to avoid a performance > regression on the Linux kernel repo where v1 topo levels caused > "git merge-base v4.8 v4.9" to walk 636k commits instead of 167k. > A side branch with a low topo level stayed in the queue behind a > long chain, preventing early STALE propagation. > > Side-exhaustion (added in the previous commits) solves this > differently by terminating the walk as soon as one paint side > empties from the queue, preventing the deep walk regardless of > queue ordering. Nice! > Benchmarks of "git merge-base --all v4.8 v4.9" > on the Linux kernel repo show that side-exhaustion reduces the > step count far below what the date-ordering fallback achieved: > > steps time > no graph, baseline: 167,413 3.25 s > v1 graph, baseline: 167,413 0.25 s > v2 graph, baseline: 167,441 0.29 s > v1 graph, this series: 5,725 0.02 s > v2 graph, this series: 3,887 0.01 s Even better! > With generation ordering always active, the existing min_generation > check in paint_queue_get() correctly identifies when the walk has > reached the finite generation region. The date ordering fallback > broke this invariant: a commit could have a finite topo level > while the queue was date-ordered, causing the early exit to fire > before all merge bases were found. > > For v1 commit-graphs where generation numbers saturate at > GENERATION_NUMBER_V1_MAX, introduce a topological ceiling that > the early exit gates compare against instead of > GENERATION_NUMBER_INFINITY. This ensures saturated commits are > treated as unordered, preventing premature termination when > generation values are unreliable. Should the work associated with this paragraph come earlier so 8/10 doesn't have its weird split? > > Signed-off-by: Kristofer Karlsson <krka@spotify.com> > --- > .../technical/paint-down-to-common.adoc | 51 +++---------------- > commit-reach.c | 23 +++++---- > t/t6600-test-reach.sh | 23 ++++----- > 3 files changed, 27 insertions(+), 70 deletions(-) > > diff --git a/Documentation/technical/paint-down-to-common.adoc b/Documentation/technical/paint-down-to-common.adoc > index 7c93f7e676..bdd5ffb5c3 100644 > --- a/Documentation/technical/paint-down-to-common.adoc > +++ b/Documentation/technical/paint-down-to-common.adoc > @@ -44,10 +44,6 @@ ancestor is necessarily redundant. > INFINITY and finite generation regions > -------------------------------------- > > -The properties in this section assume generation-number ordering (the > -default comparator). They do NOT hold when the date-ordering fallback > -is active -- see <<date-ordering-fallback>>. > - > The commit-graph stores a generation number for each commit. > Commits not in the commit-graph have generation > `GENERATION_NUMBER_INFINITY`. The graph is closed under > @@ -91,10 +87,12 @@ traversal: children are always visited before their parents. This > means that paint on already-visited commits is final -- no future > traversal step can add paint to them. > > -In the INFINITY region, commit-date ordering can violate this: a > -parent with a later date can be visited before a child with an earlier > -date. Paint flags are therefore NOT final at visit time, and a > -commit visited with only one side's paint may later gain the other. > +In the INFINITY region, all commits share the same generation > +value, so the queue breaks ties by commit date. This can violate > +topological ordering: a parent with a later date can be visited > +before a child with an earlier date. Paint flags are therefore > +NOT final at visit time, and a commit visited with only one > +side's paint may later gain the other. Similar issues exist in the GENERATION_NUMBER_V1_MAX region, right? > Paint flags are only added, never removed. Since each flag can be set > at most once per commit, the number of times a commit can be > @@ -159,43 +157,6 @@ descendant of this candidate (generation ordering guarantees > children are visited first), so it cannot be redundant and the walk > can stop immediately. > > -This optimization is NOT safe when the date-ordering fallback is > -active, because commit-date order can visit a deeper ancestor > -before a shallower one -- see <<date-ordering-fallback>>. > - > -[[date-ordering-fallback]] > -Date-ordering fallback > ----------------------- > - > -When the commit-graph has generation numbers v1 and no > -generation floor is specified, topological ordering > -(via generation numbers) is disabled. Topological levels are > -correct but unbalanced -- ordering by such generation numbers > -can sometimes cause the walk to detour too far before finding > -merge bases. Commit-date ordering typically reaches them in > -fewer steps -- see this change for more details: > - > - 091f4cf3 (commit: don't use generation numbers if not needed, > - 2018-08-30) > - > -With generation number v2 (corrected commit dates) we have the best > -of both worlds and do not need this fallback. > - > -For v1, `paint_down_to_common()` falls back to pure commit-date > -ordering via `compare_commits_by_commit_date`. Because commit > -dates are not monotonic (clock skew, rebases, etc.), the queue > -may visit commits out of topological order. > - > -This disables the optimizations that depend on generation ordering: > - > - - *Single result*: the first merge-base candidate found may not > - be the shallowest, because a deeper ancestor with a higher > - commit date can be dequeued first. > - > - - *Side exhaustion*: one paint side can appear to drain from the > - queue while commits from that side are still waiting with lower > - dates, causing premature termination. > - Nice seeing all the date-ordering stuff get ripped out. > Related documentation > --------------------- > > diff --git a/commit-reach.c b/commit-reach.c > index b50b0e4e47..85bda146e6 100644 > --- a/commit-reach.c > +++ b/commit-reach.c > @@ -89,9 +89,9 @@ struct paint_state { > size_t parent1_count; > size_t parent2_count; > size_t mb_candidate_count; > - int gen_ordered; > timestamp_t min_generation; > timestamp_t last_gen; > + timestamp_t topo_ceiling; > }; > > static void paint_count_update(struct paint_state *state, > @@ -166,8 +166,7 @@ static struct commit *paint_queue_get(struct paint_state *state) > > /* one side is exhausted */ > if ((!state->parent1_count || !state->parent2_count) && > - state->gen_ordered && > - generation < GENERATION_NUMBER_INFINITY) > + generation < state->topo_ceiling) > return NULL; > } Good, together with the setting of state->topo_ceiling, this fixes the GENERATION_NUMBER_V1_MAX issue. > > @@ -187,9 +186,13 @@ static int paint_down_to_common(struct repository *r, > enum merge_base_flags mb_flags, > struct commit_list **result) > { > + /* > + * Generation ordering is required for the side-exhaustion and > + * single-result early exits, which rely on topological traversal > + * order (children visited before parents) in the finite region. > + */ > struct paint_state state = { > - .queue = { compare_commits_by_gen_then_commit_date }, > - .gen_ordered = 1, > + .queue = { compare_commits_by_gen_then_commit_date } > }; > struct commit *commit; > int i; > @@ -198,10 +201,9 @@ static int paint_down_to_common(struct repository *r, > > state.min_generation = min_generation; > state.last_gen = GENERATION_NUMBER_INFINITY; > - if (!min_generation && !corrected_commit_dates_enabled(r)) { > - state.queue.compare = compare_commits_by_commit_date; > - state.gen_ordered = 0; > - } > + state.topo_ceiling = corrected_commit_dates_enabled(r) > + ? GENERATION_NUMBER_INFINITY > + : GENERATION_NUMBER_V1_MAX; > > one->object.flags |= PARENT1; > if (!n) { > @@ -229,8 +231,7 @@ static int paint_down_to_common(struct repository *r, > * descendant of this one. > */ > if (!(mb_flags & MERGE_BASE_FIND_ALL) && > - state.gen_ordered && > - state.last_gen < GENERATION_NUMBER_INFINITY) > + state.last_gen < state.topo_ceiling) > break; > } > /* Mark parents of a found merge stale */ > diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh > index 6bf17cb7b6..445449a458 100755 > --- a/t/t6600-test-reach.sh > +++ b/t/t6600-test-reach.sh > @@ -381,7 +381,7 @@ test_expect_success 'get_merge_bases_many:infinity-both-sides' ' > git rev-parse pi-B > } >expect && > test_all_modes get_merge_bases_many && > - test_paint_down_steps 5 4 5 5 > + test_paint_down_steps 5 4 5 4 > ' > > test_expect_success 'setup mixed finite/INFINITY topology' ' > @@ -414,31 +414,26 @@ test_expect_success 'merge-base --all commit-walk steps' ' > >input && > git rev-parse commit-9-1 >expect && > run_all_modes git merge-base --all commit-9-9 commit-9-1 && > - test_paint_down_steps 81 9 57 81 > + test_paint_down_steps 81 9 57 37 > ' > > test_expect_success 'merge-base --all with clock skew (side-exhaustion)' ' > - # Verify correct merge base under clock skew. se-D (the > - # merge base) has a higher date than its child se-C. > - # Generation ordering ensures se-C is visited before se-D, > - # so P1 paint propagates correctly and se-D is found. > + # Verify that the merge base is computed correctly even > + # when commits have non-monotonic commit dates. > >input && > git rev-parse se-D >expect && > run_all_modes git merge-base --all se-A se-B && > - test_paint_down_steps 6 4 6 6 > + test_paint_down_steps 6 4 6 4 > ' > > test_expect_success 'merge-base --all with clock skew and redundant ancestor (side-exhaustion)' ' > - # Verify correct merge base when clock skew could cause a > - # too-deep result. MB1 is the correct merge base; MB2 is > - # its ancestor. A reaches MB2 via E (high date) and MB1 > - # via C (low date). Generation ordering ensures C is > - # visited before side-exhaustion fires, so MB1 is found > - # and remove_redundant correctly discards MB2. > + # Verify that the correct merge base is found even when > + # non-monotonic commit dates could cause a redundant > + # ancestor to be visited first. > >input && > git rev-parse se2-MB1 >expect && > run_all_modes git merge-base --all se2-A se2-B && > - test_paint_down_steps 8 6 8 8 > + test_paint_down_steps 8 6 8 6 > ' > > test_expect_success 'reduce_heads' ' > -- > gitgitgadget The code and tests look good, my main issue is that the documentation and code are not consistent at patch 08/10, so we need some way of correcting that. I don't know whether that means splitting the code differently in patches 8 & 10, or splitting the documentation differently or something else. Thoughts? Anyway, nicely done overall, this is nearly ready to merge; it just needs a few small touch-ups.

gitgitgadget · 2026-07-06T12:23:51Z

Derrick Stolee wrote on the Git mailing list (how to reply to this email):

On 6/24/2026 8:14 AM, Kristofer Karlsson via GitGitGadget wrote: > Termination > ----------- > > -The walk uses a `nonstale_queue` wrapper around `prio_queue` that > -tracks `max_nonstale`: the lowest-priority non-stale commit enqueued > -so far. Once that commit is dequeued, every remaining entry is known > -to be STALE and the loop terminates. Specifically, the main loop > +The walk tracks the number of commits of each type in the queue > +(PARENT1-only, PARENT2-only, pending merge-base). The main loop > ends when one of the following conditions holds: > > 1. The queue is empty. > - 2. `max_nonstale` has been dequeued, meaning the queue only contains > - STALE entries. > + 2. The queue contains only stale entries. I'm grateful to see these changes happening to the doc in real- time. I know it was extra work, but I'm grateful right now. Hopefully future historians will also benefit from this effort. > +static void paint_count_update(struct paint_state *state, > + unsigned flags, int delta) > +{ > + switch (flags & (PARENT1 | PARENT2 | STALE)) { > + case PARENT1: > + state->p1_count += delta; > + break; > + > + case PARENT2: > + state->p2_count += delta; > + break; > + > + case PARENT1 | PARENT2: > + state->pending_merge_bases += delta; > + break; > + > + case PARENT1 | PARENT2 | STALE: > + break; > + > + default: > + BUG("unexpected paint state"); > + } > +} I like the use of 'delta' to allow reuse of this switch. > + > +static void paint_queue_put(struct paint_state *state, > + struct commit *c, unsigned add_flags) > +{ > + unsigned old_flags = c->object.flags; > + c->object.flags |= add_flags; > + > + if (old_flags & ENQUEUED) { > + paint_count_update(state, old_flags, -1); > + paint_count_update(state, c->object.flags, 1); > + } else { > + c->object.flags |= ENQUEUED; > + prio_queue_put(&state->queue, c); > + paint_count_update(state, c->object.flags, 1); > + } > +} ok: if we are already in the queue then we have old flags and may need to subtract their values because they were counted already. Otherwise, we need to queue it for the first time and only add the values. Makes sense. > + > +static struct commit *paint_queue_get(struct paint_state *state) > +{ Since we are going to make this a more complete termination condition, we may want to make that very explicit with a doc- comment. Something along the lines of "dequeue a commit when possible, but also signal termination of the walk when we conclude that no more merge bases will be discovered due to internal state." > @@ -187,12 +253,11 @@ static int paint_down_to_common(struct repository *r, > return error(_("could not parse commit %s"), > oid_to_hex(&p->object.oid)); > } > - p->object.flags |= flags; > - nonstale_queue_put_dedup(&queue, p); > + paint_queue_put(&state, p, flags); I like how this simplifies the flag-assignment logic somewhat. You mentioned in your cover letter how the min_generation value can add extra termination conditions. It may be a good idea to insert min_generation into the paint_queue struct and make it a termination condition for paint_queue_get(). If you consider this direction, then I'd make it a separate patch on top of this one _before_ adding the one-sided change. The extra tests that cover the exact number of walked commits can help to guarantee the same behavior, assuming that some of those tests check a non-zero min_generation input. (It may be good to add such trace tests in an earlier patch to help confidence in this case.) Thanks, -Stolee

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

On Wed, 24 Jun 2026 at 15:54, Derrick Stolee <stolee@gmail.com> wrote: > > I'm grateful to see these changes happening to the doc in real- > time. I know it was extra work, but I'm grateful right now. > > Hopefully future historians will also benefit from this effort. It was honestly not bad at all, and I agree it felt quite nice to see how the doc naturally changed along with the implementation. > > +static struct commit *paint_queue_get(struct paint_state *state) > > +{ > > Since we are going to make this a more complete termination > condition, we may want to make that very explicit with a doc- > comment. Something along the lines of "dequeue a commit when > possible, but also signal termination of the walk when we > conclude that no more merge bases will be discovered due to > internal state." Yes, I'll make sure to clean that part up more, maybe also rename the function to be more descriptive. > You mentioned in your cover letter how the min_generation value > can add extra termination conditions. It may be a good idea to > insert min_generation into the paint_queue struct and make it a > termination condition for paint_queue_get(). If you consider this > direction, then I'd make it a separate patch on top of this one > _before_ adding the one-sided change. The extra tests that cover > the exact number of walked commits can help to guarantee the same > behavior, assuming that some of those tests check a non-zero > min_generation input. (It may be good to add such trace tests in > an earlier patch to help confidence in this case.) I think I might wait with this - the patch series already feels quite big, and I think it has a natural progression and finish now. But I can definitely commit to following up later -- it would be a smaller series that is easier to reason about, likely a single commit. Thanks, Kristofer

gitgitgadget · 2026-07-06T12:42:27Z

René Scharfe wrote on the Git mailing list (how to reply to this email):

On 6/26/26 3:08 PM, Kristofer Karlsson via GitGitGadget wrote: > > diff --git a/commit-reach.c b/commit-reach.c > index f6a438550b..0f29b143bd 100644 > --- a/commit-reach.c > +++ b/commit-reach.c > @@ -97,6 +97,75 @@ static struct commit *nonstale_queue_get_dedup(struct nonstale_queue *queue) > return commit; > } > > +/* > + * Priority queue with per-side commit counters for paint_down_to_common(). > + * Each non-stale queued commit occupies exactly one bucket: PARENT1-only, > + * PARENT2-only, or both (a pending merge-base candidate). > + */ > +struct paint_state { > + struct prio_queue queue; > + int p1_count; > + int p2_count; > + int pending_merge_bases; > +}; Can they become negative? Wouldn't size_t be a more natural fit, matching nr from struct prio_queue? And some bikeshedding: Why abbreviate? parent1_count and parent2_count would be slightly easier to read and associate with PARENT1 and PARENT2. And pending_merge_bases is a counter as well. Why not call it like that, pending_merge_base_count? Well, that's pretty long. both_count? That's quite generic and nondescript. Call the other counters parents1 and parents2? Nah. Or parent1s and parent2s? Not sure why this inconsistency bothers me to begin with. René

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

On Fri, 26 Jun 2026 at 23:13, René Scharfe <l.s.r@web.de> wrote: > > > +struct paint_state { > > + struct prio_queue queue; > > + int p1_count; > > + int p2_count; > > + int pending_merge_bases; > > +}; > Can they become negative? Wouldn't size_t be a more natural fit, > matching nr from struct prio_queue? Negative would be a clear indication of a bug though that's not checked right now anyway. And since it's not checked we might as well use size_t instead - and it would technically be more correct though I struggle to imagine a case where the number of active elements in the frontier exceeds 2^31 or whatever a signed int would give. I am happy to change to size_t. > And some bikeshedding: > > Why abbreviate? parent1_count and parent2_count would be slightly > easier to read and associate with PARENT1 and PARENT2. > > And pending_merge_bases is a counter as well. Why not call it > like that, pending_merge_base_count? Well, that's pretty long. > both_count? That's quite generic and nondescript. Call the other > counters parents1 and parents2? Nah. Or parent1s and parent2s? > Not sure why this inconsistency bothers me to begin with. Fair point, I was thinking that the surrounding context is so small that the naming almost doesn't matter - the terms don't escape paint_down_to_common. I am happy to change to something like: parent1_count, parent2_count, mb_candidate_count to make it more consistent. It seems the mb_ prefix is already used for merge bases in some files - best example is perhaps builtin/diff.c I see in the codebase that we are using multiple styles, perhaps depending on specific context. - nr_ prefix: nr_objects, nr_paths_watching - num_ prefix: num_commits, num_hashes, num_workers - _count suffix: entry_count, max_count, skip_count so I think _count suffix is a good choice at least - it matches other usages where we typically just increment or decrement. Thanks, Kristofer

gitgitgadget · 2026-08-07T03:20:38Z

Elijah Newren wrote on the Git mailing list (how to reply to this email):

On Thu, Aug 6, 2026 at 4:05 AM Kristofer Karlsson via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Kristofer Karlsson <krka@spotify.com> > > Add a paint_state struct for use by paint_down_to_common() that > wraps a prio_queue with per-side commit counters. Each non-stale > queued commit occupies exactly one counter bucket based on its > paint flags: PARENT1-only, PARENT2-only, or both sides (a pending > merge-base candidate). > > The counters are maintained by paint_count_update() which adjusts > the appropriate bucket by a signed delta. An exhaustive switch on > the paint+stale bits documents all valid flag combinations in one > place. > > Convert paint_down_to_common() to use paint_state. The loop now > drains the queue via paint_queue_get() which returns NULL when all > counters reach zero, replacing the old pointer-based termination > (max_nonstale). Ooh, I like this setup for what comes later; it sets the stage perfectly for the key insight behind the optimization. Very nice. > This is equivalent behavior -- both conditions > detect that no non-stale entries remain. > > paint_queue_get() uses a "pop first" form: it dequeues a commit, > then checks the counters. This means the loop exits one iteration > earlier than the old code in some topologies (the popped stale > commit is never processed), so a few step counts drop by one. > > The existing nonstale_queue is left in place for ahead_behind(), > though nonstale_queue_put_dedup() and nonstale_queue_get_dedup() > became unused and are removed. became -> become > > Signed-off-by: Kristofer Karlsson <krka@spotify.com> > --- > .../technical/paint-down-to-common.adoc | 9 +- > commit-reach.c | 103 +++++++++++++----- > t/t6600-test-reach.sh | 6 +- > 3 files changed, 82 insertions(+), 36 deletions(-) > > diff --git a/Documentation/technical/paint-down-to-common.adoc b/Documentation/technical/paint-down-to-common.adoc > index cea0cc2f91..37fa6f93c1 100644 > --- a/Documentation/technical/paint-down-to-common.adoc > +++ b/Documentation/technical/paint-down-to-common.adoc > @@ -103,15 +103,12 @@ re-enqueued is bounded by the number of flag transitions. > Termination > ----------- > > -The walk uses a `nonstale_queue` wrapper around `prio_queue` that > -tracks `max_nonstale`: the lowest-priority non-stale commit enqueued > -so far. Once that commit is dequeued, every remaining entry is known > -to be STALE and the loop terminates. Specifically, the main loop > +The walk tracks the number of commits of each type in the queue > +(PARENT1-only, PARENT2-only, pending merge-base). The main loop > ends when one of the following conditions holds: > > 1. The queue is empty. > - 2. `max_nonstale` has been dequeued, meaning the queue only contains > - STALE entries. > + 2. The queue contains only stale entries. > 3. Generation cutoff: the dequeued commit's generation is below > a caller-supplied `min_generation` threshold. > 4. Single result: the caller only needs one merge base, one has > diff --git a/commit-reach.c b/commit-reach.c > index d59e76a2e2..a62b5e4624 100644 > --- a/commit-reach.c > +++ b/commit-reach.c > @@ -79,21 +79,73 @@ static void clear_nonstale_queue(struct nonstale_queue *queue) > queue->max_nonstale = NULL; > } > > -static void nonstale_queue_put_dedup(struct nonstale_queue *queue, > - struct commit *c) > +/* > + * Priority queue with per-side commit counters for paint_down_to_common(). > + * Each non-stale queued commit occupies exactly one bucket: PARENT1-only, > + * PARENT2-only, or both (a pending merge-base candidate). > + */ > +struct paint_state { > + struct prio_queue queue; > + size_t parent1_count; > + size_t parent2_count; > + size_t mb_candidate_count; > + int gen_ordered; > +}; > + > +static void paint_count_update(struct paint_state *state, > + unsigned flags, int delta) > { > - if (c->object.flags & ENQUEUED) > - return; > - c->object.flags |= ENQUEUED; > - nonstale_queue_put(queue, c); > + switch (flags & (PARENT1 | PARENT2 | STALE)) { > + case PARENT1: > + state->parent1_count += delta; > + break; > + > + case PARENT2: > + state->parent2_count += delta; > + break; > + > + case PARENT1 | PARENT2: > + state->mb_candidate_count += delta; > + break; > + > + case PARENT1 | PARENT2 | STALE: > + break; > + > + default: > + BUG("unexpected paint state"); So, if anyone tries to refactor and adds a nonsense flag combination, e.g. PARENT1 | STALE, this will trip. Good. > + } > +} > + > +static void paint_queue_put(struct paint_state *state, > + struct commit *c, unsigned add_flags) > +{ > + unsigned old_flags = c->object.flags; > + c->object.flags |= add_flags; > + > + if (old_flags & ENQUEUED) { > + paint_count_update(state, old_flags, -1); > + paint_count_update(state, c->object.flags, 1); If this object was already in the queue, remove the old counters for it (e.g. PARENT1), and add the new union counters for it (e.g. PARENT1 | PARENT2). Good. > + } else { > + c->object.flags |= ENQUEUED; > + prio_queue_put(&state->queue, c); > + paint_count_update(state, c->object.flags, 1); ...and if it wasn't, put it in the queue and add the counters for it. Also good. > + } > } > > -static struct commit *nonstale_queue_get_dedup(struct nonstale_queue *queue) > +static struct commit *paint_queue_get(struct paint_state *state) > { > - struct commit *commit = nonstale_queue_get(queue); > + struct commit *commit = prio_queue_get(&state->queue); > + > + if (!commit) > + return NULL; > + > + commit->object.flags &= ~ENQUEUED; > + > + if (!state->parent1_count && !state->parent2_count && > + !state->mb_candidate_count) > + return NULL; > > - if (commit) > - commit->object.flags &= ~ENQUEUED; > + paint_count_update(state, commit->object.flags, -1); > return commit; > } So: pop, clear, check the counters, and _then_ decrement the counters. This means the zero-counter-check still include the just-popped commit. If the decrement were before the check, we'd actually just barely miss the merge-base most the time, so this order is important. > > @@ -109,18 +161,19 @@ static int paint_down_to_common(struct repository *r, > enum merge_base_flags mb_flags, > struct commit_list **result) > { > - struct nonstale_queue queue = { > - { compare_commits_by_gen_then_commit_date } > + struct paint_state state = { > + .queue = { compare_commits_by_gen_then_commit_date }, > + .gen_ordered = 1, > }; > + struct commit *commit; > int i; > - int gen_ordered = 1; > int steps = 0; > timestamp_t last_gen = GENERATION_NUMBER_INFINITY; > struct commit_list **tail = result; > > if (!min_generation && !corrected_commit_dates_enabled(r)) { > - queue.pq.compare = compare_commits_by_commit_date; > - gen_ordered = 0; > + state.queue.compare = compare_commits_by_commit_date; > + state.gen_ordered = 0; > } > > one->object.flags |= PARENT1; > @@ -128,15 +181,12 @@ static int paint_down_to_common(struct repository *r, > commit_list_append(one, result); > return 0; > } > - nonstale_queue_put_dedup(&queue, one); > + paint_queue_put(&state, one, 0); > > - for (i = 0; i < n; i++) { > - twos[i]->object.flags |= PARENT2; > - nonstale_queue_put_dedup(&queue, twos[i]); > - } > + for (i = 0; i < n; i++) > + paint_queue_put(&state, twos[i], PARENT2); > > - while (queue.max_nonstale) { > - struct commit *commit = nonstale_queue_get_dedup(&queue); > + while ((commit = paint_queue_get(&state))) { > struct commit_list *parents; > int flags; > timestamp_t generation = commit_graph_generation(commit); > @@ -162,7 +212,7 @@ static int paint_down_to_common(struct repository *r, > * descendant of this one. > */ > if (!(mb_flags & MERGE_BASE_FIND_ALL) && > - gen_ordered && > + state.gen_ordered && > generation < GENERATION_NUMBER_INFINITY) > break; > } > @@ -176,7 +226,7 @@ static int paint_down_to_common(struct repository *r, > if ((p->object.flags & flags) == flags) > continue; > if (repo_parse_commit(r, p)) { > - clear_nonstale_queue(&queue); > + clear_prio_queue(&state.queue); > commit_list_free(*result); > *result = NULL; > /* > @@ -191,12 +241,11 @@ static int paint_down_to_common(struct repository *r, > return error(_("could not parse commit %s"), > oid_to_hex(&p->object.oid)); > } > - p->object.flags |= flags; > - nonstale_queue_put_dedup(&queue, p); > + paint_queue_put(&state, p, flags); > } > } > > - clear_nonstale_queue(&queue); > + clear_prio_queue(&state.queue); > trace2_data_intmax("paint_down_to_common", r, > "steps", steps); > commit_list_sort_by_date(result); Looks like the straightforward translation in paint_down_to_common() from the old algorithm to the new adjustment; nice that a few spots actually become a little shorter. > diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh > index 55aa220bb3..f9895f5fd7 100755 > --- a/t/t6600-test-reach.sh > +++ b/t/t6600-test-reach.sh > @@ -366,7 +366,7 @@ test_expect_success 'get_merge_bases_many:pending-stale' ' > git rev-parse ps-B > } >expect && > test_all_modes get_merge_bases_many && > - test_paint_down_steps 6 6 6 6 > + test_paint_down_steps 5 5 5 5 > ' > > test_expect_success 'get_merge_bases_many:infinity-both-sides' ' > @@ -381,7 +381,7 @@ test_expect_success 'get_merge_bases_many:infinity-both-sides' ' > git rev-parse pi-B > } >expect && > test_all_modes get_merge_bases_many && > - test_paint_down_steps 5 5 5 5 > + test_paint_down_steps 5 4 5 5 > ' > > test_expect_success 'setup mixed finite/INFINITY topology' ' > @@ -438,7 +438,7 @@ test_expect_success 'merge-base --all with clock skew and redundant ancestor (si > >input && > git rev-parse se2-MB1 >expect && > run_all_modes git merge-base --all se2-A se2-B && > - test_paint_down_steps 8 7 8 8 > + test_paint_down_steps 8 6 8 8 > ' > > test_expect_success 'reduce_heads' ' > -- > gitgitgadget Looks good.

gitgitgadget · 2026-07-06T12:25:54Z

Derrick Stolee wrote on the Git mailing list (how to reply to this email):

On 6/24/2026 8:14 AM, Kristofer Karlsson via GitGitGadget wrote: > From: Kristofer Karlsson <krka@spotify.com> > > Add an early termination check to paint_down_to_common() using the > per-side counters introduced earlier. Once the walk enters the > finite-generation region, terminate early when one side's exclusive > count drops to zero -- no new merge-base can form without both paint > sides meeting. Having this as the last patch is truly a nice climax moment for the patch series! > @@ -94,6 +94,9 @@ ends when one of the following conditions holds: > > 1. The queue is empty. > 2. The queue contains only stale entries. > + 3. Side exhaustion: no pure PARENT1 or pure PARENT2 commits > + remain in the queue, no pending merge-base candidates exist, > + and the walk has entered the finite-generation region. ...> +Side-exhaustion condition > +~~~~~~~~~~~~~~~~~~~~~~~~~ > +A new merge-base requires commits from both sides to meet. When one > +side's exclusive counter reaches zero and there are no pending > +merge-base candidates, no future traversal step can produce a new > +candidate. > + > +This optimization only activates in the finite-generation region > +where topological ordering holds. In that region, children are > +always visited before parents, so paint flags are final at visit > +time and an exhausted side cannot reappear. In the INFINITY region, > +commit-date ordering can violate this guarantee, so the check is > +skipped. > + And these doc updates inline make me happy. > Related documentation > --------------------- > > diff --git a/commit-reach.c b/commit-reach.c > index e0d9874f99..f79d0b64d6 100644 > --- a/commit-reach.c > +++ b/commit-reach.c > @@ -133,17 +133,30 @@ static void paint_queue_put(struct paint_state *state, > > static struct commit *paint_queue_get(struct paint_state *state) > { > - struct commit *commit; > + struct commit *commit = prio_queue_get(&state->queue); > > - if (!state->p1_count && !state->p2_count && > - !state->pending_merge_bases) > + if (!commit) > return NULL; I see how the previous implementation has a termination condition before calling prio_queue_get(), which is technically more efficient. It does make this initial diff a bit more complicated because we are moving the prio_queue_get() line. If the introduction of the method in patch 5/7 looked like this: +static struct commit *paint_queue_get(struct paint_state *state) +{ + struct commit *commit = prio_queue_get(&state->queue); + + if (!commit) + return NULL; + + if (!state->p1_count && !state->p2_count && + !state->pending_merge_bases) + return NULL; + + commit->object.flags &= ~ENQUEUED; + paint_count_update(state, commit->object.flags, -1); + return commit; +} Then this diff would look cleaner. (This is the nittiest of nitpicks so feel free to ignore if this doesn't bother you at all.) > - commit = prio_queue_get(&state->queue); > - if (commit) { > - commit->object.flags &= ~ENQUEUED; > - paint_count_update(state, commit->object.flags, -1); > + commit->object.flags &= ~ENQUEUED; > + > + if (!state->pending_merge_bases) { > + if (!state->p1_count && !state->p2_count) > + return NULL; > + /* > + * Side exhaustion: a new merge-base can only form > + * when both PARENT1-only and PARENT2-only commits > + * remain in the queue. In the finite-generation > + * region the queue is ordered topologically, so > + * no future step can add paint to visited commits > + * and an exhausted side cannot reappear. > + */ > + if ((!state->p1_count || !state->p2_count) && > + commit_graph_generation(commit) < GENERATION_NUMBER_INFINITY) > + return NULL; > } > + > + paint_count_update(state, commit->object.flags, -1); > return commit; > } I like how the crux of this implementation is entirely within paint_queue_get() now. > diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh > index c1109fb42f..03175befb3 100755 > --- a/t/t6600-test-reach.sh > +++ b/t/t6600-test-reach.sh > @@ -332,12 +332,12 @@ test_expect_success 'merge-base --all commit-walk steps' ' > cp commit-graph-full .git/objects/info/commit-graph && > GIT_TRACE2_EVENT="$(pwd)/trace-full.txt" \ > git merge-base --all commit-9-9 commit-9-1 >actual && > - test_trace2_data paint_down_to_common steps 80 <trace-full.txt && > + test_trace2_data paint_down_to_common steps 9 <trace-full.txt && > > cp commit-graph-half .git/objects/info/commit-graph && > GIT_TRACE2_EVENT="$(pwd)/trace-half.txt" \ > git merge-base --all commit-9-9 commit-9-1 >actual && > - test_trace2_data paint_down_to_common steps 81 <trace-half.txt > + test_trace2_data paint_down_to_common steps 57 <trace-half.txt > ' I love to see these steps change. If you take my suggestion to update more tests with these checks, then this diff will get bigger (but in a deserved way). Also, when I suggested that 'test_all_modes' creates the trace files on our behalf, I forgot to mention that this specific test that you added in patch 4/7 simplifies by running the merge-base check under 'test_all_modes' and then checking the trace2 data on the three well-known files afterwards. Thanks, -Stolee

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

On Wed, 24 Jun 2026 at 16:02, Derrick Stolee <stolee@gmail.com> wrote: > > I see how the previous implementation has a termination condition > before calling prio_queue_get(), which is technically more > efficient. It does make this initial diff a bit more complicated > because we are moving the prio_queue_get() line. I was thinking the efficiency here does not matter in practice - prio_queue_get() only returns NULL once, and all other times where we keep looping we do need the value. I agree it does get a bit complex though. > If the introduction of the method in patch 5/7 looked like this: > > +static struct commit *paint_queue_get(struct paint_state *state) > +{ > + struct commit *commit = prio_queue_get(&state->queue); > + > + if (!commit) > + return NULL; > + > + if (!state->p1_count && !state->p2_count && > + !state->pending_merge_bases) > + return NULL; > + > + commit->object.flags &= ~ENQUEUED; > + paint_count_update(state, commit->object.flags, -1); > + return commit; > +} > > Then this diff would look cleaner. > > (This is the nittiest of nitpicks so feel free to ignore if this > doesn't bother you at all.) That's a good point. It doesn't technically bother me, but it would be cleaner. The refactor commit would effectively be looking into the future and prepare for it. I can change it for the next version - my only thinking was that the current refactor patch matched my original idea for how to best handle the halt condition, but that did indeed change after this discussion. > > - test_trace2_data paint_down_to_common steps 81 <trace-half.txt > > + test_trace2_data paint_down_to_common steps 57 <trace-half.txt > > ' > I love to see these steps change. If you take my suggestion to > update more tests with these checks, then this diff will get bigger > (but in a deserved way). I will try to add them to some (but not all) tests since it's more closely related to performance than correctness and I want to avoid making too many tests overly fragile. > Also, when I suggested that 'test_all_modes' creates the trace > files on our behalf, I forgot to mention that this specific test > that you added in patch 4/7 simplifies by running the merge-base > check under 'test_all_modes' and then checking the trace2 data > on the three well-known files afterwards. That's a nice bonus, I will try to see if I can manage to utilize it. Thanks, Kristofer

Derrick Stolee wrote on the Git mailing list (how to reply to this email):

On 6/24/2026 10:47 AM, Kristofer Karlsson wrote: > On Wed, 24 Jun 2026 at 16:02, Derrick Stolee <stolee@gmail.com> wrote: >>> - test_trace2_data paint_down_to_common steps 81 <trace-half.txt >>> + test_trace2_data paint_down_to_common steps 57 <trace-half.txt >>> ' >> I love to see these steps change. If you take my suggestion to >> update more tests with these checks, then this diff will get bigger >> (but in a deserved way). > > I will try to add them to some (but not all) tests since it's more > closely related to performance than correctness and I want to > avoid making too many tests overly fragile. In this case, I think it's more about protecting all of our special- cased termination conditions. The rigidity means that it is hard to accidentally change the behavior. It does have the downside that more tests need to change if there is an intentional change, but it also gives the same _evidence_ that the change has the intended impact. We are definitely leaning into personal preferences, though. There is no hard rule one way or another. Thanks, -Stolee

gitgitgadget · 2026-07-06T12:39:09Z

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

On Fri, 26 Jun 2026 at 15:08, Kristofer Karlsson via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Kristofer Karlsson <krka@spotify.com> > > - if (min_generation && generation > last_gen) > + if (generation > last_gen) I have to note that I accidentally pushed this version before noticing that it now fails for a subset of commit-graph modes. Apologies for that - I will rework the logic here later to preserve the behavior better. I think (and hope) the rest of the patch series is in good shape though and addressed the previous feedback, so any partial new review feedback would still be appreciated. Thanks, Kristofer

Derrick Stolee wrote on the Git mailing list (how to reply to this email):

On 6/26/2026 10:29 AM, Kristofer Karlsson wrote: > On Fri, 26 Jun 2026 at 15:08, Kristofer Karlsson via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> >> From: Kristofer Karlsson <krka@spotify.com> >> >> - if (min_generation && generation > last_gen) >> + if (generation > last_gen) > > I have to note that I accidentally pushed this version before noticing > that it now fails for a subset of commit-graph modes. > Apologies for that - I will rework the logic here later > to preserve the behavior better. And do we catch this with a test case? I'm hoping that you discovered this error through the test suite, even if you submitted the series a little early. > I think (and hope) the rest of the patch series is in good shape though > and addressed the previous feedback, so any partial new review > feedback would still be appreciated. Thanks for calling this out, as now I can avoid trying to understand this change during my review. Thanks, -Stolee

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

On Fri, 26 Jun 2026 at 16:32, Derrick Stolee <stolee@gmail.com> wrote: > > > I have to note that I accidentally pushed this version before noticing > > that it now fails for a subset of commit-graph modes. > > Apologies for that - I will rework the logic here later > > to preserve the behavior better. > > And do we catch this with a test case? I'm hoping that you discovered > this error through the test suite, even if you submitted the series a > little early. (I missed replying to this message initially, sorry) Yes exactly - it was caught by t6600 but somehow I missed running it before submitting it (so I noticed it in the GGG CI instead) So the existing tests are good, I only wish I could be equally reliable as them. Thanks, Kristofer

gitgitgadget · 2026-07-06T12:39:41Z

Derrick Stolee wrote on the Git mailing list (how to reply to this email):

On 6/26/2026 9:08 AM, Kristofer Karlsson via GitGitGadget wrote: > From: Kristofer Karlsson <krka@spotify.com> > @@ -140,9 +144,16 @@ static struct commit *paint_queue_get(struct paint_state *state) > > commit->object.flags &= ~ENQUEUED; > > - if (!state->p1_count && !state->p2_count && > - !state->pending_merge_bases) > - return NULL; > + if (!state->pending_merge_bases) { > + /* only stale entries remain */ > + if (!state->p1_count && !state->p2_count) > + return NULL; > + > + /* one side is exhausted */ > + if ((!state->p1_count || !state->p2_count) && > + commit_graph_generation(commit) < GENERATION_NUMBER_INFINITY) > + return NULL; > + } This continues to look correct. > paint_count_update(state, commit->object.flags, -1); > return commit; > @@ -188,7 +199,7 @@ static int paint_down_to_common(struct repository *r, > timestamp_t generation = commit_graph_generation(commit); > steps++; > > - if (min_generation && generation > last_gen) > + if (generation > last_gen) > BUG("bad generation skip %"PRItime" > %"PRItime" at %s", > generation, last_gen, > oid_to_hex(&commit->object.oid)); You mention in your own reply that this is broken. This also looks like a stray change for this patch, so perhaps your end state is correct despite this patch causing failures. Will inspect soon. > - test_paint_down_steps 45 2 25 3 > + test_paint_down_steps 45 1 25 1 ...> - test_paint_down_steps 81 80 81 81 > + test_paint_down_steps 81 9 57 10 These diffs are satisfying. Thanks, -Stolee

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

On Fri, 26 Jun 2026 at 16:35, Derrick Stolee <stolee@gmail.com> wrote: > > > - if (min_generation && generation > last_gen) > > + if (generation > last_gen) > > BUG("bad generation skip %"PRItime" > %"PRItime" at %s", > > generation, last_gen, > > oid_to_hex(&commit->object.oid)); > > You mention in your own reply that this is broken. This also looks > like a stray change for this patch, so perhaps your end state is > correct despite this patch causing failures. Will inspect soon. I did not intend it to be a stray change, but rather a natural followup to the idea that we could fold all of the halt conditions into the same place. I am happy to either revert that part for v4 (to keep the change simpler, but not fully unified) or fix it properly - I think it should be easy since this was just human error, not a sign of a fundamentally tricky problem. > > - test_paint_down_steps 45 2 25 3 > > + test_paint_down_steps 45 1 25 1 > ...> - test_paint_down_steps 81 80 81 81 > > + test_paint_down_steps 81 9 57 10 > These diffs are satisfying. Agreed! It was nice to introduce the steps counter to the test suite, showing that the patch reached its intended goal which is clearer than just having benchmarks in the messages. Thanks again, Kristofer

gitgitgadget · 2026-08-07T03:21:12Z

Elijah Newren wrote on the Git mailing list (how to reply to this email):

On Thu, Aug 6, 2026 at 4:05 AM Kristofer Karlsson via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Kristofer Karlsson <krka@spotify.com> > > Add an early termination check to paint_down_to_common() using the > per-side counters introduced earlier. Once the walk enters the > finite-generation region, terminate early when one side's exclusive > count drops to zero -- no new merge-base can form without both paint > sides meeting. ...this is the insight behind this optimization, which the previous patch set up so nicely. > The check also waits for pending_merge_bases to reach zero, ensuring > all merge-base candidates have been dequeued and recorded before > exiting. > > The INFINITY gate ensures correctness: commits without a commit-graph > entry have GENERATION_NUMBER_INFINITY and are ordered by commit date, > which is not topologically reliable. The optimization only fires > once the walk enters the finite-generation region where ordering > guarantees hold. What about GENERATION_NUMBER_V1_MAX ? > > Step counts measured with trace2 on git.git with commit-graph: > > merge-base --all v2.0.0 v2.55.0-rc1: > before: 72264 steps after: 44589 steps > > merge-base --all v2.55.0-rc1 v2.55.0-rc1~5: > before: 110 steps after: 7 steps > > Helped-by: Derrick Stolee <stolee@gmail.com> > Helped-by: Elijah Newren <newren@gmail.com> > Signed-off-by: Kristofer Karlsson <krka@spotify.com> > --- > .../technical/paint-down-to-common.adoc | 23 ++++++++++++++++++- > commit-reach.c | 18 ++++++++++++--- > t/t6600-test-reach.sh | 4 ++-- > 3 files changed, 39 insertions(+), 6 deletions(-) > > diff --git a/Documentation/technical/paint-down-to-common.adoc b/Documentation/technical/paint-down-to-common.adoc > index 37fa6f93c1..7c93f7e676 100644 > --- a/Documentation/technical/paint-down-to-common.adoc > +++ b/Documentation/technical/paint-down-to-common.adoc [...] > + 5. Side exhaustion: no pure PARENT1 or pure PARENT2 commits > + remain in the queue, no pending merge-base candidates exist, > + and the walk has entered the finite-generation region. "finite" or "small enough" ? > +Side-exhaustion condition > +~~~~~~~~~~~~~~~~~~~~~~~~~ > +A new merge-base requires commits from both sides to meet. When one > +side's exclusive counter reaches zero and there are no pending > +merge-base candidates, no future traversal step can produce a new > +candidate. > + > +This optimization only activates in the finite-generation region "finite-generation region" -> "reliably-ordered region" , or something like that? > +where topological ordering holds. In that region, children are > +always visited before parents, so paint flags are final at visit > +time and an exhausted side cannot reappear. In the INFINITY region, > +commit-date ordering can violate this guarantee, so the check is > +skipped. "In the INFINITY region" -> "outside the reliably-ordered region" ? > Related documentation > --------------------- > > diff --git a/commit-reach.c b/commit-reach.c > index a62b5e4624..e03505b535 100644 > --- a/commit-reach.c > +++ b/commit-reach.c > @@ -132,6 +132,10 @@ static void paint_queue_put(struct paint_state *state, > } > } > > +/* > + * Dequeue the next commit for the paint walk, or return NULL when > + * no more merge bases can be discovered. > + */ > static struct commit *paint_queue_get(struct paint_state *state) > { > struct commit *commit = prio_queue_get(&state->queue); > @@ -141,9 +145,17 @@ static struct commit *paint_queue_get(struct paint_state *state) > > commit->object.flags &= ~ENQUEUED; > > - if (!state->parent1_count && !state->parent2_count && > - !state->mb_candidate_count) > - return NULL; > + if (!state->mb_candidate_count) { > + /* only stale entries remain */ > + if (!state->parent1_count && !state->parent2_count) > + return NULL; > + > + /* one side is exhausted */ > + if ((!state->parent1_count || !state->parent2_count) && > + state->gen_ordered && > + commit_graph_generation(commit) < GENERATION_NUMBER_INFINITY) At this point in the series, Documentation/technical/paint-down-to-common.adoc does point out the GENERATION_NUMBER_V1_MAX issue in one of the paragraphs; it's kind of glossed over in other later paragraphs (as I highlighted above), but there's a clear incongruence at this point in the series. I'm guessing you're going to fix that up in the next two patches, but the splitting feels a bit off. > + return NULL; > + } > > paint_count_update(state, commit->object.flags, -1); > return commit; > diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh > index f9895f5fd7..6bf17cb7b6 100755 > --- a/t/t6600-test-reach.sh > +++ b/t/t6600-test-reach.sh > @@ -297,7 +297,7 @@ test_expect_success 'in_merge_bases_many:self' ' > EOF > echo "in_merge_bases_many(A,X):1" >expect && > test_all_modes in_merge_bases_many && > - test_paint_down_steps 45 2 25 3 > + test_paint_down_steps 45 1 25 1 > ' > > test_expect_success 'is_descendant_of:hit' ' > @@ -414,7 +414,7 @@ test_expect_success 'merge-base --all commit-walk steps' ' > >input && > git rev-parse commit-9-1 >expect && > run_all_modes git merge-base --all commit-9-9 commit-9-1 && > - test_paint_down_steps 81 80 81 81 > + test_paint_down_steps 81 9 57 81 > ' > > test_expect_success 'merge-base --all with clock skew (side-exhaustion)' ' > -- > gitgitgadget Other than the GENERATION_NUMBER_V1_MAX stuff, this commit looks good. There may be a way to reword things to allow the current split, but I'll keep reading to the next patches.

-Original file line number
+Diff line change
@@ Expand Up / @@ -129,6 +129,7 @@ TECH_DOCS += technical/long-running-process-protocol @@
     TECH_DOCS += technical/multi-pack-index
     TECH_DOCS += technical/packfile-uri
     TECH_DOCS += technical/pack-heuristics
+    TECH_DOCS += technical/paint-down-to-common
     TECH_DOCS += technical/parallel-checkout
     TECH_DOCS += technical/partial-clone
     TECH_DOCS += technical/platform-support
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -18,6 +18,7 @@ articles = [ @@
       'multi-pack-index.adoc',
       'packfile-uri.adoc',
       'pack-heuristics.adoc',
+      'paint-down-to-common.adoc',
       'parallel-checkout.adoc',
       'partial-clone.adoc',
       'platform-support.adoc',
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -0,0 +1,164 @@
+    Merge-Base Computation and paint_down_to_common()
+    ==================================================
+    The function `paint_down_to_common()` in `commit-reach.c` computes merge
+    bases by walking the commit graph backwards from two sets of tips and
+    finding where their ancestry meets.
+    Use cases
+    ---------
+    Computing merge bases is used in two different ways:
+. *Finding all merge bases* (`merge-base --all`, `merge-tree`,
+        `merge`, `rebase`). A merge base is a common ancestor that is
+        not itself an ancestor of another common ancestor.
+. *Ancestry checks* (`in_merge_bases`, used by `merge-base
+        --is-ancestor`, `branch -d`, `fetch`). These ask: "is commit A
+        an ancestor of commit B?" If a common ancestor equals one of the
+        inputs, that input is necessarily the only merge base -- no other
+        common ancestor can be both as recent and not an ancestor of it.
+    Both use cases share the same algorithm and implementation.
+    Algorithm
+    ---------
+    Given a commit `one` and a set of commits `twos[]`, the walk paints
+    commits with two colors:
+      - PARENT1: reachable from `one`
+      - PARENT2: reachable from any commit in `twos[]`
+    The walk uses a priority queue ordered by generation number
+    (highest first), breaking ties by commit date. Each step dequeues
+    the highest-priority commit (this is when we say a commit is
+    "visited") and propagates its paint flags to its parents, enqueuing
+    them if they gained new flags. When a commit receives both PARENT1
+    and PARENT2, it is a merge-base candidate. A candidate gains the
+    STALE flag so its ancestors propagate staleness -- any deeper common
+    ancestor is necessarily redundant.
+    [[generation-regions]]
+    INFINITY and finite generation regions
+    --------------------------------------
+    The commit-graph stores a generation number for each commit.
+    Commits not in the commit-graph have generation
+    `GENERATION_NUMBER_INFINITY`. The graph is closed under
+    reachability: if a commit is in the graph, all its ancestors are
+    too. This partitions the commit graph into two regions:
+    ....
+        +---------------------------------------+
+        |          INFINITY region              |
+        |  generation = INFINITY                |
+        |  queue order: heuristic (commit date) |
+        +---------------------------------------+
+                        |
+                        v
+        +---------------------------------------+
+        |          Finite region                |
+        |  generation = finite                  |
+        |  queue order: topological             |
+        +---------------------------------------+
+    ....
+    When the commit-graph is enabled, the INFINITY region is typically
+    very small -- it only contains commits added since the last
+    commit-graph refresh.
+    With v1 commit-graphs (topological levels, no GDAT chunk),
+    generation numbers saturate at `GENERATION_NUMBER_V1_MAX`.
+    Saturated commits share the same generation value despite
+    different topological depths, which breaks ordering guarantees
+    in the same way as INFINITY. The early exit gates compare
+    against `GENERATION_NUMBER_V1_MAX` for v1 graphs and
+    `GENERATION_NUMBER_INFINITY` for v2 graphs, so that saturated
+    commits are treated as unordered.
+    All reachable INFINITY-generation commits are visited before any
+    finite-generation commit, because INFINITY is larger than any finite
+    value. Once the walk crosses into the finite region, it stays there.
+    In the finite region, generation ordering guarantees topological
+    traversal: children are always visited before their parents. This
+    means that paint on already-visited commits is final -- no future
+    traversal step can add paint to them.
+    In the INFINITY region, all commits share the same generation
+    value, so the queue breaks ties by commit date. This can violate
+    topological ordering: a parent with a later date can be visited
+    before a child with an earlier date. Paint flags are therefore
+    NOT final at visit time, and a commit visited with only one
+    side's paint may later gain the other.
+    Paint flags are only added, never removed. Since each flag can be set
+    at most once per commit, the number of times a commit can be
+    re-enqueued is bounded by the number of flag transitions.
+    Termination
+    -----------
+    The walk tracks the number of commits of each type in the queue
+    (PARENT1-only, PARENT2-only, pending merge-base). The main loop
+    ends when one of the following conditions holds:
+. The queue is empty.
+. The queue contains only stale entries.
+. Generation cutoff: the dequeued commit's generation is below
+         a caller-supplied `min_generation` threshold.
+. Single result: the caller only needs one merge base, one has
+         been found, and the walk has entered the finite-generation
+         region.
+. Side exhaustion: no pure PARENT1 or pure PARENT2 commits
+         remain in the queue, no pending merge-base candidates exist,
+         and the walk has entered the finite-generation region.
+    Stale entry condition
+    ~~~~~~~~~~~~~~~~~~~~~
+    Once all queued entries are stale, no new merge-base candidates can
+    be discovered -- that requires at least one non-stale commit from
+    each side meeting. Continuing the walk could still invalidate
+    existing candidates by proving one is an ancestor of another, but
+    `remove_redundant()` handles that as a post-processing step, so it
+    is safe to exit early.
+    Side-exhaustion condition
+    ~~~~~~~~~~~~~~~~~~~~~~~~~
+    A new merge-base requires commits from both sides to meet. When one
+    side's exclusive counter reaches zero and there are no pending
+    merge-base candidates, no future traversal step can produce a new
+    candidate.
+    This optimization only activates in the finite-generation region
+    where topological ordering holds. In that region, children are
+    always visited before parents, so paint flags are final at visit
+    time and an exhausted side cannot reappear. In the INFINITY region,
+    commit-date ordering can violate this guarantee, so the check is
+    skipped.
+    Generation cutoff
+    ~~~~~~~~~~~~~~~~~
+    Some callers (notably `remove_redundant()`) supply a `min_generation`
+    threshold equal to the minimum generation of the input commits.
+    These callers only need to determine reachability among the inputs,
+    not find deep merge bases, so the walk can safely terminate when it
+    dequeues a commit below this threshold.
+    Single result
+    ~~~~~~~~~~~~~
+    When only one merge base is needed, the walk is in the
+    finite-generation region, and the queue uses generation ordering,
+    the first candidate found is necessarily the highest-generation
+    common ancestor. No remaining commit in the queue can be a
+    descendant of this candidate (generation ordering guarantees
+    children are visited first), so it cannot be redundant and the walk
+    can stop immediately.
+    Related documentation
+    ---------------------
+      - `Documentation/technical/commit-graph.adoc` -- generation numbers
+        and the reachability closure property.

Uh oh!

commit-reach: terminate merge-base walk when one side is exhausted #2149

Are you sure you want to change the base?

Uh oh!

commit-reach: terminate merge-base walk when one side is exhausted #2149

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

gitgitgadget Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!