commit-reach: terminate merge-base walk when one side is exhausted - #2149
Open
spkrka wants to merge 10 commits into
Open
commit-reach: terminate merge-base walk when one side is exhausted#2149spkrka wants to merge 10 commits into
spkrka wants to merge 10 commits into
Conversation
spkrka
force-pushed
the
side-exhaust-pr
branch
10 times, most recently
from
June 20, 2026 08:55
7d5b1bb to
3e1315e
Compare
spkrka
changed the title
Author
|
/preview |
|
Preview email sent as pull.2149.git.1781946989.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2149.git.1781951820.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
spkrka
marked this pull request as ready for review
June 22, 2026 11:32
|
This patch series was integrated into seen via git@418052d. |
Author
|
/submit |
|
Submitted as pull.2149.v2.git.1782303254.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This branch is now known as |
spkrka
force-pushed
the
side-exhaust-pr
branch
3 times, most recently
from
June 26, 2026 12:54
f574f35 to
4b9f192
Compare
|
There was a status update in the "Cooking" section about the branch 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. Waiting for review. cf. <CAL71e4PwoJ4fxKBNuf3HB3Po92WRaV4yDBUDcuEYiggiDD=+Ew@mail.gmail.com> cf. <CABPp-BGATrNJyT7trzUzAMB_v-1ssVe_SRqp+281X5GzU=2eow@mail.gmail.com> source: <pull.2149.v6.git.1783776466.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch 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. <CABPp-BGATrNJyT7trzUzAMB_v-1ssVe_SRqp+281X5GzU=2eow@mail.gmail.com> cf. <CAL71e4Mve9EbTkuWoGdtNTJJC5oj_W9enfSceMvH0FFm4T8ALA@mail.gmail.com> cf. <CAL71e4Mc5b8rqD_x=0XPvrF9NtNw6Y_twrdwJAF5vE3sWtkzOA@mail.gmail.com> source: <pull.2149.v6.git.1783776466.gitgitgadget@gmail.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>
test_trace2_data is a bare grep that silently exits on failure.
Add a more informative variant that verifies the event appears
exactly once and reports what went wrong: key not found, multiple
entries, or value mismatch. Diagnostics go to FD 4 like test_grep.
Before (value mismatch):
$ test_trace2_data status count/changed 999 <trace2.txt
$ echo $?
1
(no output)
After:
$ test_trace2_data_singular status count/changed 999 <trace2.txt
error: trace2 data 'status/count/changed'
expected: 999
actual: 0
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
Add test cases to t6600-test-reach.sh that exercise edge cases in the side-exhaustion optimization for paint_down_to_common(): - in_merge_bases_many:self: commit is both A and one of the X inputs - get_merge_bases_many:duplicate-twos: duplicate entries in X list - get_merge_bases_many:pending-stale: STALE transition on an already-painted commit (ps-* diamond topology) - get_merge_bases_many:infinity-both-sides: both tips outside the commit-graph with non-monotonic dates (pi-* topology) Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
Add t6099 to test the case where multiple merge-base candidates exist and one is an ancestor of another. This exercises the side-exhaustion optimization in paint_down_to_common together with the remove_redundant safety net in get_merge_bases_many_0. Add a mixed finite/INFINITY test to t6600 where one tip is outside the commit-graph (INFINITY generation) and the other is inside. This exercises the region transition: the walk starts in the INFINITY region where side-exhaustion is disabled, then crosses into the finite region where it can fire. Signed-off-by: Kristofer Karlsson <krka@spotify.com>
Add a step counter and trace2_data_intmax() call so that the number of commits visited during the paint walk is observable via GIT_TRACE2_EVENT. This provides a way to measure the impact of future optimizations without relying on wall-clock benchmarks alone. Signed-off-by: Kristofer Karlsson <krka@spotify.com>
Add topologies and tests exercising paint_down_to_common() under clock skew, where commit-date ordering (v1 commit-graph without corrected commit dates) violates the topological invariant that children are dequeued before parents: - se-*: side-exhaustion fires too early when one paint side fully drains from the queue while a low-date ancestor on the other side is still queued - se2-*: side-exhaustion returns a too-deep merge base because the correct (closer) base never receives both paint sides Also add step counts to the edge-case tests from the previous commit, a mixed finite/INFINITY generation topology exercising the transition from INFINITY-generation commits to graph-backed commits, and step counts for the grid-based merge-base test. Signed-off-by: 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). 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. Signed-off-by: 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.
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.
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>
Consolidate the min_generation termination condition into paint_queue_get(), alongside the existing stale-entry and side-exhaustion checks. Move last_gen into struct paint_state so that commit_graph_generation() is called exactly once per dequeued commit and the result is shared across all termination checks and the monotonicity BUG assertion. Signed-off-by: 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 091f4cf (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. 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 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. Signed-off-by: Kristofer Karlsson <krka@spotify.com>
Author
|
/submit |
|
Submitted as pull.2149.v7.git.1786013982.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series is no longer integrated into seen. |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -795,6 +795,7 @@ integration_tests = [ | |||
| 't6041-bisect-submodule.sh', | |||
There was a problem hiding this comment.
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 t6099 to test the case where multiple merge-base candidates exist
> and one is an ancestor of another. This exercises the side-exhaustion
> optimization in paint_down_to_common together with the
> remove_redundant safety net in get_merge_bases_many_0.
>
> Add a mixed finite/INFINITY test to t6600 where one tip is outside
> the commit-graph (INFINITY generation) and the other is inside.
> This exercises the region transition: the walk starts in the
> INFINITY region where side-exhaustion is disabled, then crosses
> into the finite region where it can fire.
>
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> t/meson.build | 1 +
> t/t6099-merge-base-side-exhaustion.sh | 82 +++++++++++++++++++++++++++
> 2 files changed, 83 insertions(+)
> create mode 100755 t/t6099-merge-base-side-exhaustion.sh
The log message and diffstat contradict each other. The addition to
't6600' happens a bit later at step 6/10, which presumably introduces
this finite/infinite distinction, does it not?There was a problem hiding this comment.
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):
On Thu, 6 Aug 2026 at 18:11, Junio C Hamano <gitster@pobox.com> wrote:
>
> The log message and diffstat contradict each other. The addition to
> 't6600' happens a bit later at step 6/10, which presumably introduces
> this finite/infinite distinction, does it not?
Oops, you're right, that was well spotted. I am not quite
sure how I overlooked that. Will fix for v8,
Looking back at the history, the commit message was correct
at v4 but when the test commits were split/reorganized for v5 I
failed to update the commit message to reflect that.
Thanks,
KristoferThere was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
Kristofer Karlsson <krka@spotify.com> writes:
> On Thu, 6 Aug 2026 at 18:11, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> The log message and diffstat contradict each other. The addition to
>> 't6600' happens a bit later at step 6/10, which presumably introduces
>> this finite/infinite distinction, does it not?
>
> Oops, you're right, that was well spotted. I am not quite
> sure how I overlooked that. Will fix for v8,
>
> Looking back at the history, the commit message was correct
> at v4 but when the test commits were split/reorganized for v5 I
> failed to update the commit message to reflect that.
Heh, sorry for nitpicking. Maybe others can give more serious
reviews on the topic. This gives us an important optimization.
Thanks.
| @@ -129,6 +129,7 @@ TECH_DOCS += technical/long-running-process-protocol | |||
| TECH_DOCS += technical/multi-pack-index | |||
There was a problem hiding this comment.
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.
| @@ -1996,6 +1996,41 @@ test_trace2_data () { | |||
| grep -e '"category":"'"$1"'","key":"'"$2"'","value":"'"$3"'"' | |||
There was a problem hiding this comment.
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>
>
> test_trace2_data is a bare grep that silently exits on failure.
> Add a more informative variant that verifies the event appears
> exactly once and reports what went wrong: key not found, multiple
> entries, or value mismatch. Diagnostics go to FD 4 like test_grep.
>
> Before (value mismatch):
>
> $ test_trace2_data status count/changed 999 <trace2.txt
> $ echo $?
> 1
> (no output)
>
> After:
>
> $ test_trace2_data_singular status count/changed 999 <trace2.txt
> error: trace2 data 'status/count/changed'
> expected: 999
> actual: 0
Nice.
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> t/test-lib-functions.sh | 35 +++++++++++++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 809c662124..8c6d327b03 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1996,6 +1996,41 @@ test_trace2_data () {
> grep -e '"category":"'"$1"'","key":"'"$2"'","value":"'"$3"'"'
> }
>
> +# Check that the given trace2 data event has the expected value and
> +# appears exactly once. Produces a diagnostic on failure.
> +#
> +# test_trace2_data_singular <category> <key> <value> [<label>]
> +test_trace2_data_singular () {
> + local category="$1" key="$2" expect_val="$3"
> + local label_suffix="${4:+ [$4]}"
> + local kv_pattern='"category":"'"$category"'","key":"'"$key"'","value":"\([^"]*\)"'
> + local actual
> +
> + actual=$(sed -n "s|.*${kv_pattern}.*|\1|p") &&
> +
> + if test -z "$actual"
> + then
> + echo >&4 "error: trace2 data '$category/$key'$label_suffix not found"
> + return 1
> + fi &&
> +
> + case "$actual" in
> + *"$LF"*)
Ah, you've got Rene's suggestion from v6 included as well; nice.
> + echo >&4 "error: trace2 data '$category/$key'$label_suffix has multiple entries, expected 1"
> + printf '%s\n' "$actual" | sed 's/^/ actual: /' >&4
> + return 1
> + ;;
> + esac &&
> +
> + if test "$actual" != "$expect_val"
> + then
> + echo >&4 "error: trace2 data '$category/$key'$label_suffix"
> + echo >&4 " expected: $expect_val"
> + echo >&4 " actual: $actual"
> + return 1
> + fi
> +}
> +
Function appears to match the comment above it and the commit message.
It looks like a nice usability addition.
| @@ -85,6 +85,61 @@ test_expect_success 'setup' ' | |||
| git branch -f skew-P2 "$skew_P2" && | |||
There was a problem hiding this comment.
Elijah Newren wrote on the Git mailing list (how to reply to this email):
On Thu, Aug 6, 2026 at 3:59 AM Elijah Newren via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Elijah Newren <newren@gmail.com>
>
> Add test cases to t6600-test-reach.sh that exercise edge cases in the
> side-exhaustion optimization for paint_down_to_common():
>
> - in_merge_bases_many:self: commit is both A and one of the X inputs
> - get_merge_bases_many:duplicate-twos: duplicate entries in X list
> - get_merge_bases_many:pending-stale: STALE transition on an
> already-painted commit (ps-* diamond topology)
> - get_merge_bases_many:infinity-both-sides: both tips outside the
> commit-graph with non-monotonic dates (pi-* topology)
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
As the author of these tests, and as my Signed-off-by attests, I can
confirm with the full weight of my authority that these tests are
good.
However, I would be remiss not to note the perfidious destruction of
my two spaces after each period, cruelly collapsed down to a mere one.
Have you no decency, sir?
(Kidding, of course -- I mostly point it out so the next reviewer can
appreciate just how little else changed from the original.)
| @@ -795,6 +795,7 @@ integration_tests = [ | |||
| 't6041-bisect-submodule.sh', | |||
There was a problem hiding this comment.
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 t6099 to test the case where multiple merge-base candidates exist
> and one is an ancestor of another. This exercises the side-exhaustion
> optimization in paint_down_to_common together with the
> remove_redundant safety net in get_merge_bases_many_0.
>
> Add a mixed finite/INFINITY test to t6600 where one tip is outside
> the commit-graph (INFINITY generation) and the other is inside.
> This exercises the region transition: the walk starts in the
> INFINITY region where side-exhaustion is disabled, then crosses
> into the finite region where it can fire.
Junio already commented on the second paragraph not following your
earlier split.
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> t/meson.build | 1 +
> t/t6099-merge-base-side-exhaustion.sh | 82 +++++++++++++++++++++++++++
> 2 files changed, 83 insertions(+)
> create mode 100755 t/t6099-merge-base-side-exhaustion.sh
>
> diff --git a/t/meson.build b/t/meson.build
> index a25f37d2f5..655c94f860 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -795,6 +795,7 @@ integration_tests = [
> 't6041-bisect-submodule.sh',
> 't6050-replace.sh',
> 't6060-merge-index.sh',
> + 't6099-merge-base-side-exhaustion.sh',
> 't6100-rev-list-in-order.sh',
> 't6101-rev-parse-parents.sh',
> 't6102-rev-list-unexpected-objects.sh',
> diff --git a/t/t6099-merge-base-side-exhaustion.sh b/t/t6099-merge-base-side-exhaustion.sh
> new file mode 100755
> index 0000000000..4f1e0d50ef
> --- /dev/null
> +++ b/t/t6099-merge-base-side-exhaustion.sh
> @@ -0,0 +1,82 @@
> +#!/bin/sh
> +
> +test_description='merge-base with ancestor among merge-base candidates
> +
> +Test that merge-base --all correctly handles cases where
> +multiple merge-base candidates exist and one is an ancestor
> +of another. The side-exhaustion optimization in
> +paint_down_to_common may exit before STALE propagation
> +removes the ancestor, but remove_redundant catches it.
> +
> +Graph shape (parents are below children):
> +
> + A ----------- X
> + |\ /|
> + | B---------/ |
> + | | |
> + e2 \ f2
> + | | |
> + e1 d1 f1
> + \ | /
> + \ | /
> + \| /
> + C
> +
> +A and X are the two tips.
> +B and C are both reachable from A and X.
> +B reaches C through d1.
> +Only B should appear in merge-base --all output.
Was this graph created in an editor using a variable width font? In a
fixed width font, it makes one assume that C is not an ancestor of X,
but instead that C and f1 will likely eventually converge on common
history. One might need to know what your original variable width
font was in order to see it right. The description below if very
helpful, but could we replace the graph with:
A ----- X
|\ /|
| B---/ |
| \ |
e2 \ f2
| | |
e1 d1 f1
\ | /
\ | /
\|/
C
?
> +'
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> +TEST_PASSES_SANITIZE_LEAK=true
> +. ./test-lib.sh
> +
> +test_expect_success 'setup ancestor merge-base candidate' '
> + test_commit C &&
> +
> + git checkout -b d-chain HEAD &&
> + test_commit d1 &&
> + test_commit B &&
> +
> + git checkout -b e-path C &&
> + test_commit e1 &&
> + test_commit e2 &&
> +
> + git checkout -b f-path C &&
> + test_commit f1 &&
> + test_commit f2 &&
> +
> + git checkout -b branch-A e-path &&
> + test_merge A B &&
> +
> + git checkout -b branch-X f-path &&
> + test_merge X B &&
> +
> + git commit-graph write --reachable
> +'
> +
> +test_expect_success 'merge-base --all excludes ancestor candidate' '
> + git rev-parse B >expected &&
> + git merge-base --all A X >actual &&
> + test_cmp expected actual
> +'
> +
> +test_expect_success 'merge-base (single) finds shallowest' '
> + git rev-parse B >expected &&
> + git merge-base A X >actual &&
> + test_cmp expected actual
> +'
> +
> +# Without commit-graph: generation numbers are INFINITY,
> +# side-exhaustion optimization does not fire.
This comment made me think the test would run with trace2 regions
printing in order to verify which codepath it went through. That
might make it a stronger test (if coupled with trace2 regions above to
ensure the above code is testing something different than what below
does), but certainly not worth a re-roll.
> +test_expect_success 'merge-base --all without commit-graph' '
> + rm -f .git/objects/info/commit-graph &&
> + git rev-parse B >expected &&
> + git merge-base --all A X >actual &&
> + test_cmp expected actual
> +'
> +
> +test_done
I like the tests added here; look good to me.
| GIT_AUTHOR_DATE="@500 +0000" GIT_COMMITTER_DATE="@500 +0000" \ | ||
| git commit-tree -p pi-D -p pi-P -m pi-X pi-D^{tree} >pi-X-oid && | ||
| pi_x="$(cat pi-X-oid)" && | ||
| git branch -f pi-X-br "$pi_x" && |
There was a problem hiding this comment.
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 topologies and tests exercising paint_down_to_common() under
> clock skew, where commit-date ordering (v1 commit-graph without
> corrected commit dates) violates the topological invariant that
> children are dequeued before parents:
I love the care and attention being put in here to test all the edge
and corner cases.
>
> - se-*: side-exhaustion fires too early when one paint side fully
> drains from the queue while a low-date ancestor on the other
> side is still queued
>
> - se2-*: side-exhaustion returns a too-deep merge base because
> the correct (closer) base never receives both paint sides
>
> Also add step counts to the edge-case tests from the previous
> commit, a mixed finite/INFINITY generation topology exercising
> the transition from INFINITY-generation commits to graph-backed
> commits, and step counts for the grid-based merge-base test.
Another nice addition.
>
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> t/t6600-test-reach.sh | 98 ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 96 insertions(+), 2 deletions(-)
>
> diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
> index 45aa26cd44..55aa220bb3 100755
> --- a/t/t6600-test-reach.sh
> +++ b/t/t6600-test-reach.sh
> @@ -140,6 +140,48 @@ test_expect_success 'setup' '
> git branch -f pi-X-br "$pi_x" &&
> git tag pi-X "$pi_x" &&
>
> + # Clock-skew topology for side-exhaustion testing.
> + # D is the correct merge base but has a higher committer date
> + # than C (its child). With date ordering, D would be dequeued
> + # before C, causing side-exhaustion to fire too early.
> + # Generation ordering prevents this by visiting children
> + # before parents regardless of dates.
> + #
> + # se-A (date 7000) --> se-C (date 3000) --> se-D (date 5000) --> se-root (date 4000)
> + # se-B (date 6000) --> se-D
> + #
> + se_root=$(skew_commit 4000 se-root) &&
> + se_D=$(skew_commit 5000 se-D -p "$se_root") &&
> + se_C=$(skew_commit 3000 se-C -p "$se_D") &&
> + se_A=$(skew_commit 7000 se-A -p "$se_C") &&
> + se_B=$(skew_commit 6000 se-B -p "$se_D") &&
> + git branch -f se-A "$se_A" &&
> + git branch -f se-B "$se_B" &&
> + git tag se-D "$se_D" &&
> +
> + # Clock-skew topology with redundant ancestor for
> + # side-exhaustion testing. MB1 is the correct merge base;
> + # MB2 is its parent. A reaches MB2 via E (high date) and
> + # MB1 via C (low date). B reaches MB1 via D. With date
> + # ordering, side-exhaustion would fire before C is dequeued,
> + # missing MB1. Generation ordering ensures both are found.
> + #
> + # se2-A (date 8000) --> se2-C (date 2000) --> se2-MB1 (date 5000) --> se2-MB2 (date 4000) --> se2-root (date 1000)
> + # se2-A --> se2-E (date 6500) --> se2-MB2
> + # se2-B (date 7000) --> se2-D (date 6000) --> se2-MB1
> + #
> + se2_root=$(skew_commit 1000 se2-root) &&
> + se2_MB2=$(skew_commit 4000 se2-MB2 -p "$se2_root") &&
> + se2_MB1=$(skew_commit 5000 se2-MB1 -p "$se2_MB2") &&
> + se2_C=$(skew_commit 2000 se2-C -p "$se2_MB1") &&
> + se2_D=$(skew_commit 6000 se2-D -p "$se2_MB1") &&
> + se2_E=$(skew_commit 6500 se2-E -p "$se2_MB2") &&
> + se2_A=$(skew_commit 8000 se2-A -p "$se2_C" -p "$se2_E") &&
> + se2_B=$(skew_commit 7000 se2-B -p "$se2_D") &&
> + git branch -f se2-A "$se2_A" &&
> + git branch -f se2-B "$se2_B" &&
> + git tag se2-MB1 "$se2_MB1" &&
> +
> git commit-graph write --reachable &&
> mv .git/objects/info/commit-graph commit-graph-full &&
> chmod u+w commit-graph-full &&
> @@ -323,7 +365,8 @@ test_expect_success 'get_merge_bases_many:pending-stale' '
> echo "get_merge_bases_many(A,X):" &&
> git rev-parse ps-B
> } >expect &&
> - test_all_modes get_merge_bases_many
> + test_all_modes get_merge_bases_many &&
> + test_paint_down_steps 6 6 6 6
> '
>
> test_expect_success 'get_merge_bases_many:infinity-both-sides' '
> @@ -337,7 +380,34 @@ test_expect_success 'get_merge_bases_many:infinity-both-sides' '
> echo "get_merge_bases_many(A,X):" &&
> git rev-parse pi-B
> } >expect &&
> - test_all_modes get_merge_bases_many
> + test_all_modes get_merge_bases_many &&
> + test_paint_down_steps 5 5 5 5
> +'
> +
> +test_expect_success 'setup mixed finite/INFINITY topology' '
> + # Create a commit outside all saved commit-graph files so it always
> + # has INFINITY generation, while its parent (ps-X) is in the graph
> + # with a finite generation. Use the ps-* orphan topology so we do
> + # not pollute the grid-based rev-list tests.
> + git checkout ps-X &&
> + test_env GIT_TEST_COMMIT_GRAPH= test_commit pm-INF
> +'
> +
> +test_expect_success 'get_merge_bases_many:mixed-finite-infinity' '
> + # One tip (pm-INF) is outside the commit-graph with INFINITY
> + # generation; the other (ps-B) is in the graph with finite
> + # generation. The walk starts in the INFINITY region and crosses
> + # into the finite region where side-exhaustion can fire.
> + cat >input <<-\EOF &&
> + A:pm-INF
> + X:ps-B
> + EOF
> + {
> + echo "get_merge_bases_many(A,X):" &&
> + git rev-parse ps-X
> + } >expect &&
> + test_all_modes get_merge_bases_many &&
> + test_paint_down_steps 3 3 3 3
> '
>
> test_expect_success 'merge-base --all commit-walk steps' '
> @@ -347,6 +417,30 @@ test_expect_success 'merge-base --all commit-walk steps' '
> test_paint_down_steps 81 80 81 81
> '
>
> +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.
> + >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_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.
> + >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_expect_success 'reduce_heads' ',
> cat >input <<-\EOF &&
> X:commit-1-10
> --
> gitgitgadget
Tests look like they match the commit message, and they look good to me.
| at most once per commit, the number of times a commit can be | ||
| re-enqueued is bounded by the number of flag transitions. | ||
|
|
||
| Termination |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
| @@ -11,6 +11,7 @@ | |||
| #include "tag.h" | |||
There was a problem hiding this comment.
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 step counter and trace2_data_intmax() call so that the number
> of commits visited during the paint walk is observable via
> GIT_TRACE2_EVENT. This provides a way to measure the impact of
> future optimizations without relying on wall-clock benchmarks alone.
Ooh, I like it.
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> commit-reach.c | 5 +++++
> t/t6600-test-reach.sh | 44 ++++++++++++++++++++++++++++++-------------
> 2 files changed, 36 insertions(+), 13 deletions(-)
>
> diff --git a/commit-reach.c b/commit-reach.c
> index 8541264136..d59e76a2e2 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -11,6 +11,7 @@
> #include "tag.h"
> #include "commit-reach.h"
> #include "ewah/ewok.h"
> +#include "trace2.h"
>
> /* Remember to update object flag allocation in object.h */
> #define PARENT1 (1u<<16)
> @@ -113,6 +114,7 @@ static int paint_down_to_common(struct repository *r,
> };
> int i;
> int gen_ordered = 1;
> + int steps = 0;
> timestamp_t last_gen = GENERATION_NUMBER_INFINITY;
> struct commit_list **tail = result;
>
> @@ -138,6 +140,7 @@ static int paint_down_to_common(struct repository *r,
> struct commit_list *parents;
> int flags;
> timestamp_t generation = commit_graph_generation(commit);
> + steps++;
>
> if (min_generation && generation > last_gen)
> BUG("bad generation skip %"PRItime" > %"PRItime" at %s",
> @@ -194,6 +197,8 @@ static int paint_down_to_common(struct repository *r,
> }
>
> clear_nonstale_queue(&queue);
> + trace2_data_intmax("paint_down_to_common", r,
> + "steps", steps);
> commit_list_sort_by_date(result);
> return 0;
> }
> diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
> index 698b831a6e..45aa26cd44 100755
> --- a/t/t6600-test-reach.sh
> +++ b/t/t6600-test-reach.sh
> @@ -153,24 +153,34 @@ test_expect_success 'setup' '
> '
>
> run_all_modes () {
> - test_when_finished rm -rf .git/objects/info/commit-graph &&
> - "$@" <input >actual &&
> - test_cmp expect actual &&
> - cp commit-graph-full .git/objects/info/commit-graph &&
> - "$@" <input >actual &&
> - test_cmp expect actual &&
> - cp commit-graph-half .git/objects/info/commit-graph &&
> - "$@" <input >actual &&
> - test_cmp expect actual &&
> - cp commit-graph-no-gdat .git/objects/info/commit-graph &&
> - "$@" <input >actual &&
> - test_cmp expect actual
> + graph=.git/objects/info/commit-graph &&
> + test_when_finished rm -rf "$graph" "${graph}s" &&
> + rm -f trace-mode-*.txt &&
> +
> + for mode in none full half no-gdat
> + do
> + rm -rf "$graph" "${graph}s" &&
> + cp "commit-graph-${mode}" "$graph" 2>/dev/null ||
> + true &&
> + GIT_TRACE2_EVENT="$(pwd)/trace-mode-${mode}.txt" \
> + "$@" <input >actual &&
> + test_cmp expect actual || return 1
> + done
> }
>
> test_all_modes () {
> run_all_modes test-tool reach "$@"
> }
>
> +test_paint_down_steps () {
> + for mode in none full half no-gdat
> + do
> + test_trace2_data_singular paint_down_to_common steps "$1" \
> + "mode=$mode" <"trace-mode-${mode}.txt" || return 1
> + shift
> + done
> +}
> +
> test_expect_success 'ref_newer:miss' '
> cat >input <<-\EOF &&
> A:commit-5-7
> @@ -244,7 +254,8 @@ test_expect_success 'in_merge_bases_many:self' '
> X:commit-6-8
> EOF
> echo "in_merge_bases_many(A,X):1" >expect &&
> - test_all_modes in_merge_bases_many
> + test_all_modes in_merge_bases_many &&
> + test_paint_down_steps 45 2 25 3
> '
Whoa, what? <Digs around for a while.> So, this is really confusing
at first to a reviewer; it makes me think you are testing that you've
already written the optimization and that some forms of commit-graphs
provide a speedup from your work that doesn't land until later in the
series. It might help if you point out either in the commit message
or a comment here that this code is just relying on pre-existing
optimization where a min_generation is passed and --all is not passed.
(In contrast to below where --all is passed, so it has to dig deeper
with or without the commit graph).
>
> test_expect_success 'is_descendant_of:hit' '
> @@ -329,6 +340,13 @@ test_expect_success 'get_merge_bases_many:infinity-both-sides' '
> test_all_modes get_merge_bases_many
> '
>
> +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_expect_success 'reduce_heads' '
> cat >input <<-\EOF &&
> X:commit-1-10
> --
> gitgitgadget
Other than the double take above, looks good.
| ancestor is necessarily redundant. | ||
|
|
||
| [[generation-regions]] | ||
| INFINITY and finite generation regions |
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
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:
git.git (88k commits, commit-graph):
This series is based on master.
[1] https://lore.kernel.org/git/CAL71e4Ps-2_0+uuZu43N9pFnXBemoAohPs_eyRJf8taXHJPAXQ@mail.gmail.com/T/#u
[2] #2150
Changes since v6:
Now based on master; all prerequisite topics have graduated.
Added a topological ceiling concept for v1 commit-graph
support. When the commit-graph uses v1 topological levels
(no GDAT chunk), generation numbers saturate at V1_MAX,
breaking ordering guarantees in the same way as INFINITY.
Patch 10 introduces a topo_ceiling (V1_MAX for v1, INFINITY
for v2) that the side-exhaustion and single-result gates
compare against, so saturated commits are treated as
unordered.
Used $LF variable instead of a literal newline in the
test_trace2_data_singular helper (patch 2), matching the
existing pattern in test-lib. (Suggested by Rene Scharfe.)
Improved the min_generation / generation cutoff documentation
to explain why callers can safely terminate early, rather than
just stating the threshold rule.
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