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

[spkrka]

Optimize paint_down_to_common() for merge-base queries that hit
large one-sided histories.

When the walk from one side reaches a commit with a very low
generation number that the other side never paints, the walk is
forced to drain most of the graph. A common trigger is a
repository import that grafts a separate history with its own root,
but any merge that introduces a low-generation commit never painted
by the other side has the same effect.

A new merge-base candidate can only be discovered when exclusive
PARENT1 and PARENT2 paint meet. This series teaches
paint_down_to_common() to stop as soon as one side has no exclusive
commits left in the queue; once one side is exhausted, no further
candidates can appear.

origin/HEAD  o   o  PR HEAD
             |   |
   (import)  o   :
            / \ /
           |   o  merge-base
           |   |
           :   :  (~2.5M commits)
           |   |
import root   main root

In the RFC thread [1], Derrick Stolee provided a criss-cross
counterexample that sharpened the halt condition, and Elijah Newren
independently discovered the same optimization and shared an
implementation in PR #2150 [2]. Patch 3 incorporates test
cases from Elijah's branch.

This series implements the optimization only after the walk enters
the finite-generation region, where generation ordering guarantees
that paint on visited commits is final.

Patch 2 adds a test_trace2_data_singular helper to
test-lib-functions.sh that reports expected/actual values on
assertion failure instead of a silent grep exit. This was
invaluable during development for iterating on step counts
across the series, and should be valuable for repairing tests
after future algorithmic changes. Happy to drop it if it is
considered unnecessary infrastructure.

The final patch removes the commit-date ordering fallback
introduced by 091f4cf (commit: don't use generation numbers
if not needed, 2018-08-30). With side-exhaustion in place,
the fallback is no longer needed for performance, and removing it
ensures the queue is always generation-ordered regardless of graph
version, so every termination condition can rely on a single
ordering invariant. This patch can be dropped if
the scope is too broad for this series.

Benchmarks

Trace2 step counts are deterministic (measured via
trace2_data_intmax added in patch 5). Wall-clock times are
best-of-11 runs.

2.6M-commit monorepo with commit-graph:

                                      steps              wall-clock
merge-base --all  (across import)  2143438 ->      3     3.67s ->    5ms
merge-base --all  (1000 apart)     2692915 ->   1035     4.41s ->    7ms
merge-base --all  (5000 apart)     2692915 ->   6401     4.45s ->   13ms
merge-base --all  (HEAD vs import) 2698872 ->  45960     4.50s ->   79ms
merge-tree        (across import)  2143438 ->      3     4.42s ->   11ms

git.git (88k commits, commit-graph):

                                      steps              wall-clock
merge-base --all v2.0.0 v2.55.0-rc1 72264 ->  44589      110ms ->   68ms
merge-base --all HEAD HEAD~1000      9891 ->   3828       18ms ->   10ms
merge-base --all HEAD HEAD~10000    72303 ->  41487      101ms ->   50ms

This series is based on next (depends on kk/commit-reach-find-all-fix
and kk/commit-reach-optim) but is expected to merge cleanly once
kk/commit-reach-find-all-fix graduates to master.

[1] https://lore.kernel.org/git/CAL71e4Ps-2_0+uuZu43N9pFnXBemoAohPs_eyRJf8taXHJPAXQ@mail.gmail.com/T/#u
[2] #2150

Changes since v5:

• Rebased on next, which now contains kk/commit-reach-find-all-fix.
  The gen_ordered guard from that topic is carried through patches
  7-9 via state.gen_ordered, then removed in patch 10 along with
  the date-ordering fallback.

• Minor documentation and test comment improvements.

Changes since v4:

• New patch 2/10: added test_trace2_data_singular helper to
  test-lib-functions.sh. Shows expected/actual values on
  assertion failure instead of a silent grep failure. Makes
  iterating on step counts much easier.

• New patch 6/10: added clock-skew topologies (se-, se2-)
  that expose side-exhaustion bugs when the commit-date ordering
  fallback fires with a v1 commit graph. All topologies use a
  shared skew_commit helper. Includes step count assertions for
  edge-case tests from patch 3.

• Folded the nonstale_queue dedup wrapper removal (previously
  separate patch 6/8) into the paint_state introduction in
  patch 7/10.

• New patch 10/10: remove the commit-date ordering fallback in
  paint_down_to_common(). The fallback (091cf18e) was a
  performance optimization for v1 commit graphs, but it breaks
  the generation ordering invariant that both the side-exhaustion
  and single-result optimizations depend on. With
  side-exhaustion in place, the fallback is no longer needed.
  If kept, this supersedes the separate
  "commit-reach: fix !FIND_ALL early exit with v1 commit graph"
  topic.

Changes since v3:

• Fixed BUG assertion that was accidentally made unconditional
  in v3: restored the min_generation guard so it only fires
  when generation-based ordering is active.

• Moved generation cutoff and single-result termination
  conditions into the documentation in patch 1, since they
  describe existing behavior.

• Renamed paint_state counter fields for clarity: p1_count ->
  parent1_count, p2_count -> parent2_count, pending_merge_bases
  -> mb_candidate_count. Changed counter types from int to
  size_t. (Suggested by Rene Scharfe.)

Changes since v2:

• New patch 9/10 (was 8/8): moved the min_generation termination
  check and the last_gen monotonicity assertion into
  paint_queue_get(), consolidating halt conditions.
  commit_graph_generation() is now called once per dequeued
  commit and shared across all checks.

• Moved all halt conditions inside paint_queue_get() with the
  "pop first" form: pop, check, then decrement counters. This
  keeps the optimization commit's diff minimal (just inserting
  the new checks between pop and decrement).

• Shortened the doc comment on paint_queue_get() to describe
  what it does rather than how. Inline comments on each
  return NULL explain the specific halt condition.

• Replaced the manual commit-graph setup in the step-count test
  with run_all_modes, which now sets GIT_TRACE2_EVENT per mode
  and produces trace-mode-{none,full,half,no-gdat}.txt files.

• Added a test_paint_down_steps helper for concise 4-mode step
  assertions with diagnostic output on mismatch (prints
  "expected X, got Y" instead of a silent grep failure).

• Added step-count assertions to the single-walk edge-case
  tests: in_merge_bases_many:self, pending-stale,
  infinity-both-sides, mixed-finite-infinity.

• Included step counts alongside wall-clock times in the
  benchmark tables.

Changes since v1:

• Reordered patches: documentation first (describing the existing
  algorithm), tests before code changes, so they demonstrate
  passing with old logic first.

• Dropped the ahead_behind decoupling patch. paint_state is now
  a NEW struct alongside nonstale_queue instead of replacing it.
  ahead_behind() is completely untouched.

• Removed nonstale_queue_put_dedup() and
  nonstale_queue_get_dedup() (dead code after the conversion) in
  a separate commit.

• Renamed: struct paint_queue -> paint_state, field pq -> queue,
  paint_count_add/remove -> paint_count_update (single function
  with signed delta parameter).

• Split the old paint_count_transition (which handled both old
  and new flags in one call) into separate remove/add calls with
  a signed delta. This eliminates the need for the case 0
  handler (which tracked "not in the queue") and allows an
  exhaustive switch on (PARENT1 | PARENT2 | STALE) that
  documents all valid flag combinations, with BUG() in default.

• Added trace2_data_intmax() instrumentation to report the number
  of commits visited per paint walk (separate commit), with
  step-count assertions in tests for deterministic regression
  detection.

cc: Derrick Stolee stolee@gmail.com
cc: Elijah Newren newren@gmail.com
cc: Kristofer Karlsson krka@spotify.com
cc: René Scharfe l.s.r@web.de
cc: SZEDER Gábor szeder.dev@gmail.com

[spkrka]

/preview

[gitgitgadget]

Preview email sent as pull.2149.git.1781946989.gitgitgadget@gmail.com

[spkrka]

/submit

[gitgitgadget]

Submitted as pull.2149.git.1781951820.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2149/spkrka/side-exhaust-pr-v1

To fetch this version to local tag pr-2149/spkrka/side-exhaust-pr-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2149/spkrka/side-exhaust-pr-v1

[gitgitgadget]

This patch series was integrated into seen via git@418052d.

[spkrka]

/submit

[gitgitgadget]

Submitted as pull.2149.v2.git.1782303254.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2149/spkrka/side-exhaust-pr-v2

To fetch this version to local tag pr-2149/spkrka/side-exhaust-pr-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2149/spkrka/side-exhaust-pr-v2

[gitgitgadget]

This branch is now known as kk/merge-base-exhaustion.

[gitgitgadget]

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é

[gitgitgadget]

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]

User René Scharfe <l.s.r@web.de> has been added to the cc: list.

[gitgitgadget]

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

On 6/28/26 8:25 AM, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
...> @@ -138,11 +140,23 @@ static void paint_queue_put(struct paint_state *state,
>   static struct commit *paint_queue_get(struct paint_state *state)
>   {
>   	struct commit *commit = prio_queue_get(&state->queue);
> +	timestamp_t generation;
>   >   	if (!commit)
>   		return NULL;
>   >   	commit->object.flags &= ~ENQUEUED;
> +	generation = commit_graph_generation(commit);
> +
> +	if (state->min_generation && generation > state->last_gen)
> +		BUG("bad generation skip %"PRItime" > %"PRItime" at %s",
> +		    generation, state->last_gen,
> +		    oid_to_hex(&commit->object.oid));
> +	state->last_gen = generation;
> +
> +	/* generation cutoff */
> +	if (generation < state->min_generation)
> +		return NULL;

...

> -		if (min_generation && generation > last_gen)
> -			BUG("bad generation skip %"PRItime" > %"PRItime" at %s",
> -			    generation, last_gen,
> -			    oid_to_hex(&commit->object.oid));
> -		last_gen = generation;
> -
> -		if (generation < min_generation)
> -			break;

I'm just stopping in to say that this looks like a clean code move
in this version, without mutating this chunk in the previous patch.

LGTM.
-Stolee

[gitgitgadget]

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

On 6/28/26 8:25 AM, Kristofer Karlsson via GitGitGadget wrote:
> commit-reach: terminate merge-base walk when one paint side is exhausted
> > Optimize paint_down_to_common() for merge-base queries that hit large
> one-sided histories.
> > When the walk from one side reaches a commit with a very low generation
> number that the other side never paints, the walk is forced to drain most of
> the graph. A common trigger is a repository import that grafts a separate
> history with its own root, but any merge that introduces a low-generation
> commit never painted by the other side has the same effect.

> Changes since v3:
> >   * Fixed BUG assertion that was accidentally made unconditional in v3:
>     restored the min_generation guard so it only fires when generation-based
>     ordering is active.
> >   * Moved generation cutoff and single-result termination conditions into the
>     documentation in patch 1/8, since they describe existing behavior.
> >   * Renamed paint_state counter fields for clarity: p1_count ->
>     parent1_count, p2_count -> parent2_count, pending_merge_bases ->
>     mb_candidate_count. Changed counter types from int to size_t. (Suggested
>     by Rene Scharfe.)

I reviewed the v3 discussion, the range-diff, and reread patch 8. I think
that this version is good to go.

Thanks for your hard work!
-Stolee

[gitgitgadget]

SZEDER Gábor wrote on the Git mailing list (how to reply to this email):

On Sun, Jun 28, 2026 at 12:25:44PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
> 
> nonstale_queue_put_dedup() and nonstale_queue_get_dedup() became
> unused after the previous commit. The core nonstale_queue functions
> remain in use by ahead_behind().

Please squash this patch into the previous one.  Since the last
callers of these static functions went away in that commit, it can't
be built with DEVELOPER=1:

  commit-reach.c:91:23: warning: ‘nonstale_queue_get_dedup’ defined but not used [-Wunused-function]
     91 | static struct commit *nonstale_queue_get_dedup(struct nonstale_queue *queue)
        |                       ^~~~~~~~~~~~~~~~~~~~~~~~
  commit-reach.c:82:13: warning: ‘nonstale_queue_put_dedup’ defined but not used [-Wunused-function]
     82 | static void nonstale_queue_put_dedup(struct nonstale_queue *queue,
        |             ^~~~~~~~~~~~~~~~~~~~~~~~

> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
>  commit-reach.c | 18 ------------------
>  1 file changed, 18 deletions(-)
> 
> diff --git a/commit-reach.c b/commit-reach.c
> index 9ae306f60c..176ffd68d0 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -79,24 +79,6 @@ 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)
> -{
> -	if (c->object.flags & ENQUEUED)
> -		return;
> -	c->object.flags |= ENQUEUED;
> -	nonstale_queue_put(queue, c);
> -}
> -
> -static struct commit *nonstale_queue_get_dedup(struct nonstale_queue *queue)
> -{
> -	struct commit *commit = nonstale_queue_get(queue);
> -
> -	if (commit)
> -		commit->object.flags &= ~ENQUEUED;
> -	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,
> -- 
> gitgitgadget
> 

[gitgitgadget]

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

On Mon, 29 Jun 2026 at 07:25, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Sun, Jun 28, 2026 at 12:25:44PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> > From: Kristofer Karlsson <krka@spotify.com>
> >
> > nonstale_queue_put_dedup() and nonstale_queue_get_dedup() became
> > unused after the previous commit. The core nonstale_queue functions
> > remain in use by ahead_behind().
>
> Please squash this patch into the previous one.  Since the last
> callers of these static functions went away in that commit, it can't
> be built with DEVELOPER=1:
>
>   commit-reach.c:91:23: warning: ‘nonstale_queue_get_dedup’ defined but not used [-Wunused-function]
>      91 | static struct commit *nonstale_queue_get_dedup(struct nonstale_queue *queue)
>         |                       ^~~~~~~~~~~~~~~~~~~~~~~~
>   commit-reach.c:82:13: warning: ‘nonstale_queue_put_dedup’ defined but not used [-Wunused-function]
>      82 | static void nonstale_queue_put_dedup(struct nonstale_queue *queue,
>         |             ^~~~~~~~~~~~~~~~~~~~~~~~
>

Thanks, will squash for v5! It's unfortunate that this means the commit itself
becomes less clean, but I don't have any other good solution
-- and having each commit compile cleanly is more important.

- Kristofer

[gitgitgadget]

User SZEDER Gábor <szeder.dev@gmail.com> has been added to the cc: list.

[gitgitgadget]

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

On Sun, 28 Jun 2026 at 17:16, Derrick Stolee <stolee@gmail.com> wrote:
>
> I reviewed the v3 discussion, the range-diff, and reread patch 8. I think
> that this version is good to go.

Thanks for all your reviews and feedback. However, I found one more
problem that needs to be resolved before this is good to go.

paint_down_to_common() has this fallback:

    if (!min_generation && !corrected_commit_dates_enabled(r))
        queue.pq.compare = compare_commits_by_commit_date;

When this fires, the queue uses commit-date ordering instead of
generation ordering. The side-exhaustion optimization and my older
patch for !FIND_ALL early exit both check for reaching the finite
generation, but with date ordering, that check is wrong --
a commit can have a finite topo level (it is in a v1 commit graph)
while the queue is not ordered by generation. This unfortunately
means there is a regression for the !FIND_ALL optimization that
I should fix before 2.55 is final. I will send a small patch for
that separately: add tests that demonstrate the problem, and disable
the !FIND_ALL early exit when generation ordering is not active.

I traced the history of this fallback. The queue was switched from
date ordering to generation ordering in 3afc679b (2018-05). Then in
091f4cf3 (2018-08) you added the date fallback after finding that v1
topo levels caused "git merge-base v4.8 v4.9" on the Linux kernel 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. Later, 8d00d7c3 (2021-01) tightened the fallback to
only fire without corrected commit dates, since v2 does not have the
regression.

The problem that 091f4cf3 addresses looks closely related to what
side-exhaustion solves: the walk goes deep into a subgraph where
only one paint side has presence. With side-exhaustion, the walk
terminates as soon as one paint side is exhausted from the queue,
so the deep walk never happens regardless of queue ordering.

I benchmarked "git merge-base --all v4.8 v4.9" on the Linux kernel
(the same case from 091f4cf3) with three configurations:

                    master (--all)    side-exhaust (--all, gen ordering)
  no graph:           3212 ms            3268 ms
  v1 graph:            188 ms              17 ms
  v2 graph:            227 ms              17 ms

With side-exhaustion, the v1 case no longer shows a regression
compared to the date fallback -- if anything, it is slightly faster
since the walk terminates earlier. This suggests that the workaround
from 091f4cf3 may no longer be needed when side-exhaustion is
present.

It is also worth noting that commitGraph.generationVersion has
defaulted to 2 since 2021, so the v1 fallback path is rarely
exercised in practice. Any commit-graph rewrite produces v2 data,
and only repos that have not rewritten their commit graph in over
four years would still have v1-only data.

If that reasoning holds, the fix for v5 would be to remove the date
fallback entirely, always using compare_commits_by_gen_then_commit_date.
This would:

 1. Fix the bug (finite generation always means generation-ordered
    queue).
 2. Remove corrected_commit_dates_enabled() which has no other
    callers.

The alternative would be to keep the fallback and disable the
optimizations that depend on ordering (via a flag like
paint_state.gen_ordered).

Do you see any cases I might be missing where removing the fallback
could cause problems?

Thanks,
Kristofer

[gitgitgadget]

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

On 6/29/2026 8:11 AM, Kristofer Karlsson wrote:
> On Sun, 28 Jun 2026 at 17:16, Derrick Stolee <stolee@gmail.com> wrote:
>>
>> I reviewed the v3 discussion, the range-diff, and reread patch 8. I think
>> that this version is good to go.
> 
> Thanks for all your reviews and feedback. However, I found one more
> problem that needs to be resolved before this is good to go.
> 
> paint_down_to_common() has this fallback:
> 
>     if (!min_generation && !corrected_commit_dates_enabled(r))
>         queue.pq.compare = compare_commits_by_commit_date;
...> I traced the history of this fallback. 
...> The problem that 091f4cf3 addresses looks closely related to what
> side-exhaustion solves: the walk goes deep into a subgraph where
> only one paint side has presence. With side-exhaustion, the walk
> terminates as soon as one paint side is exhausted from the queue,
> so the deep walk never happens regardless of queue ordering.
> 
> I benchmarked "git merge-base --all v4.8 v4.9" on the Linux kernel
> (the same case from 091f4cf3) with three configurations:
> 
>                     master (--all)    side-exhaust (--all, gen ordering)
>   no graph:           3212 ms            3268 ms
>   v1 graph:            188 ms              17 ms
>   v2 graph:            227 ms              17 ms
> 
> With side-exhaustion, the v1 case no longer shows a regression
> compared to the date fallback -- if anything, it is slightly faster
> since the walk terminates earlier. This suggests that the workaround
> from 091f4cf3 may no longer be needed when side-exhaustion is
> present.

Thanks for digging into the history of this fallback and catching it
during review!

> If that reasoning holds, the fix for v5 would be to remove the date
> fallback entirely, always using compare_commits_by_gen_then_commit_date.
> This would:
> 
>  1. Fix the bug (finite generation always means generation-ordered
>     queue).
>  2. Remove corrected_commit_dates_enabled() which has no other
>     callers.

I agree with your reasoning, data-backed discovery, and the course of
action to fix this. I'm happy that you're able to close the loop on
this long-standing performance issue even with v1 generation numbers.
> Do you see any cases I might be missing where removing the fallback
> could cause problems?
I don't see any other concerns here. You're right that if we were to
have a different mode that changes the priority-queue ordering, then
the side-exhaustion optimization cannot be trusted, but you will
remove this possibility.

It _may_ be worth mentioning this with a comment when initializing
the queue order for the paint_queue, because the use of the queue
requires topological ordering.

Thanks,
-Stolee

[gitgitgadget]

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

On Mon, 29 Jun 2026 at 14:40, Derrick Stolee <stolee@gmail.com> wrote:
>
> I agree with your reasoning, data-backed discovery, and the course of
> action to fix this. I'm happy that you're able to close the loop on
> this long-standing performance issue even with v1 generation numbers.

Sounds good, then I can continue with the approach of removing some code
(even though it will likely be a net addition in the end).

> > Do you see any cases I might be missing where removing the fallback
> > could cause problems?
> I don't see any other concerns here. You're right that if we were to
> have a different mode that changes the priority-queue ordering, then
> the side-exhaustion optimization cannot be trusted, but you will
> remove this possibility.
>
> It _may_ be worth mentioning this with a comment when initializing
> the queue order for the paint_queue, because the use of the queue
> requires topological ordering.

Yes my plan is to rewrite v5 in a few ways:
- update original documentation to note that infinite -> finite
   generation does not always hold
- add a test (or more than one) for this problem
- don't introduce the bug at any point
- add a commit to replace the disabled optimization with
   removal of the commit-date based ordering (+ doc update)

Thanks for helping with this,
Kristofer

[gitgitgadget]

There was a status update in the "Cooking" section about the branch kk/merge-base-exhaustion on the Git mailing list:

The merge-base computation has been optimized by stopping the walk
early when one side's exclusive commits in the queue are exhausted,
yielding significant speedups for queries with one-sided histories.

Expecting a reroll.
cf. <48bfdb11-2624-4aa6-8fbd-d3f894c33bcc@gmail.com>
cf. <CAL71e4N92t8170UBW3rMA6B-rEUeOm-R_HSioB957mUKOpwRyQ@mail.gmail.com>
source: <pull.2149.v4.git.1782649547.gitgitgadget@gmail.com>

[gitgitgadget]

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

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

> Changes since v4:
>
>  * New patch 2/10: added test_trace2_data_singular helper to
>    test-lib-functions.sh. Shows expected/actual values on assertion failure
>    instead of a silent grep failure. Makes iterating on step counts much
>    easier.
>
>  * New patch 6/10: added clock-skew topologies (se-, se2-) that expose
>    side-exhaustion bugs when the commit-date ordering fallback fires with a
>    v1 commit graph. All topologies use a shared skew_commit helper. Includes
>    step count assertions for edge-case tests from patch 3.
>
>  * Folded the nonstale_queue dedup wrapper removal (previously separate
>    patch 6/8) into the paint_state introduction in patch 7/10.
>
>  * New patch 10/10: remove the commit-date ordering fallback in
>    paint_down_to_common(). The fallback (091cf18e) was a performance
>    optimization for v1 commit graphs, but it breaks the generation ordering
>    invariant that both the side-exhaustion and single-result optimizations
>    depend on. With side-exhaustion in place, the fallback is no longer
>    needed. If kept, this supersedes the separate "commit-reach: fix
>    !FIND_ALL early exit with v1 commit graph" topic.

I thought that the plan in

https://lore.kernel.org/git/CAL71e4P4GbYYv1LdarAbeodm06q841wj4gdGpn0QYADQjOB5gw@mail.gmail.com/

was to make this v5 on top of kk/commit-reach-find-all-fix topic.

I tried to prepare a merge of kk/commit-reach-find-all-fix into
v2.55.0 and then used "git am -3" to apply these patches on top,
but there were conflicts, and after resolving 7/10, t6600 stops
passing.

Perhaps it is best to ask you rebase these patches on top of a merge
of kk/commit-reach-find-all-fix into v2.55.0?

[gitgitgadget]

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

On Wed, 1 Jul 2026 at 22:06, Junio C Hamano <gitster@pobox.com> wrote:
>
> I thought that the plan in
>
> https://lore.kernel.org/git/CAL71e4P4GbYYv1LdarAbeodm06q841wj4gdGpn0QYADQjOB5gw@mail.gmail.com/
>
> was to make this v5 on top of kk/commit-reach-find-all-fix topic.
>
> I tried to prepare a merge of kk/commit-reach-find-all-fix into
> v2.55.0 and then used "git am -3" to apply these patches on top,
> but there were conflicts, and after resolving 7/10, t6600 stops
> passing.
>
> Perhaps it is best to ask you rebase these patches on top of a merge
> of kk/commit-reach-find-all-fix into v2.55.0?

You are right, I am sorry about that -- I will wait for
kk/commit-reach-find-all-fix to land and then fix up a proper v6.

In the meantime, there are still some aspects of this v5 that would
benefit from some discussion and feedback -- specifically the new
test diagnostic helper (patch 2) and the commit-date ordering
fallback removal (patch 10). Both are new in this version and could
be seen as optional.

Thanks,
Kristofer

[gitgitgadget]

There was a status update in the "Cooking" section about the branch kk/merge-base-exhaustion on the Git mailing list:

The merge-base computation has been optimized by stopping the walk
early when one side's exclusive commits in the queue are exhausted,
yielding significant speedups for queries with one-sided histories.

Needs review.
passes t6600 standalone, breaks when merged to 'seen'.
source: <pull.2149.v5.git.1782923832.gitgitgadget@gmail.com>

[gitgitgadget]

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

Kristofer Karlsson <krka@spotify.com> writes:

> In the meantime, there are still some aspects of this v5 that would
> benefit from some discussion and feedback -- specifically the new
> test diagnostic helper (patch 2) and the commit-date ordering
> fallback removal (patch 10). Both are new in this version and could
> be seen as optional.

Sure, review comment on this iterations are welcome, of course, but
I'll punt on integrating it in 'seen'.

Thanks.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch kk/merge-base-exhaustion on the Git mailing list:

The merge-base computation has been optimized by stopping the walk
early when one side's exclusive commits in the queue are exhausted,
yielding significant speedups for queries with one-sided histories.

Expecting a reroll.
cf. <CAL71e4PgcZDK-gJziJa_yjEqX9TE+PFMwZn0xbjAUzuUDDDBYA@mail.gmail.com>
source: <pull.2149.v5.git.1782923832.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch kk/merge-base-exhaustion on the Git mailing list:

The merge-base computation has been optimized by stopping the walk
early when one side's exclusive commits in the queue are exhausted,
yielding significant speedups for queries with one-sided histories.

Expecting a reroll.
cf. <CAL71e4PgcZDK-gJziJa_yjEqX9TE+PFMwZn0xbjAUzuUDDDBYA@mail.gmail.com>
source: <pull.2149.v5.git.1782923832.gitgitgadget@gmail.com>

[spkrka]

/submit

[gitgitgadget]

Submitted as pull.2149.v6.git.1783776466.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2149/spkrka/side-exhaust-pr-v6

To fetch this version to local tag pr-2149/spkrka/side-exhaust-pr-v6:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2149/spkrka/side-exhaust-pr-v6