-
Notifications
You must be signed in to change notification settings - Fork 189
commit-reach: terminate merge-base walk when one side is exhausted #2149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
18cf52c
ae68032
596c3ee
f3715c0
4127964
776d1f3
fbd80fd
be4b8f9
36beaf2
b71fdd6
cd8be5f
e09b7e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,6 +129,7 @@ TECH_DOCS += technical/long-running-process-protocol | |
| TECH_DOCS += technical/multi-pack-index | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Kristofer Karlsson <krka@spotify.com>
>
> Add a technical document describing the paint_down_to_common()
> algorithm used for merge-base computation, covering the paint
> walk, generation number regions, and termination conditions.
>
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> Documentation/Makefile | 1 +
> Documentation/technical/meson.build | 1 +
> .../technical/paint-down-to-common.adoc | 114 ++++++++++++++++++
> commit-reach.c | 6 +-
> 4 files changed, 121 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/technical/paint-down-to-common.adoc
Great write-up that very clearly and concisely explains what goes on
inside the merge-base computation. Thanks for a pleasant read. |
||
| TECH_DOCS += technical/packfile-uri | ||
| TECH_DOCS += technical/pack-heuristics | ||
| TECH_DOCS += technical/paint-down-to-common | ||
| TECH_DOCS += technical/parallel-checkout | ||
| TECH_DOCS += technical/partial-clone | ||
| TECH_DOCS += technical/platform-support | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| Merge-Base Computation and paint_down_to_common() | ||
| ================================================== | ||
|
|
||
| The function `paint_down_to_common()` in `commit-reach.c` computes merge | ||
| bases by walking the commit graph backwards from two sets of tips and | ||
| finding where their ancestry meets. | ||
|
|
||
| Use cases | ||
| --------- | ||
|
|
||
| Computing merge bases is used in two different ways: | ||
|
|
||
| 1. *Finding all merge bases* (`merge-base --all`, `merge-tree`, | ||
| `merge`, `rebase`). A merge base is a common ancestor that is | ||
| not itself an ancestor of another common ancestor. | ||
|
|
||
| 2. *Ancestry checks* (`in_merge_bases`, used by `merge-base | ||
| --is-ancestor`, `branch -d`, `fetch`). These ask: "is commit A | ||
| an ancestor of commit B?" If a common ancestor equals one of the | ||
| inputs, that input is necessarily the only merge base -- no other | ||
| common ancestor can be both as recent and not an ancestor of it. | ||
|
|
||
| Both use cases share the same algorithm and implementation. | ||
|
|
||
| Algorithm | ||
| --------- | ||
|
|
||
| Given a commit `one` and a set of commits `twos[]`, the walk paints | ||
| commits with two colors: | ||
|
|
||
| - PARENT1: reachable from `one` | ||
| - PARENT2: reachable from any commit in `twos[]` | ||
|
|
||
| The walk uses a priority queue ordered by generation number | ||
| (highest first), breaking ties by commit date. Each step dequeues | ||
| the highest-priority commit (this is when we say a commit is | ||
| "visited") and propagates its paint flags to its parents, enqueuing | ||
| them if they gained new flags. When a commit receives both PARENT1 | ||
| and PARENT2, it is a merge-base candidate. A candidate gains the | ||
| STALE flag so its ancestors propagate staleness -- any deeper common | ||
| ancestor is necessarily redundant. | ||
|
|
||
| [[generation-regions]] | ||
| INFINITY and finite generation regions | ||
| -------------------------------------- | ||
|
|
||
| The commit-graph stores a generation number for each commit. Commits | ||
| not in the commit-graph have generation `GENERATION_NUMBER_INFINITY`. The | ||
| graph is closed under reachability: if a commit is in the graph, all | ||
| its ancestors are too. This partitions the commit graph into two regions: | ||
|
|
||
| .... | ||
| +---------------------------------------+ | ||
| | INFINITY region | | ||
| | generation = INFINITY | | ||
| | queue order: heuristic (commit date) | | ||
| +---------------------------------------+ | ||
| | | ||
| v | ||
| +---------------------------------------+ | ||
| | Finite region | | ||
| | generation = finite | | ||
| | queue order: topological | | ||
| +---------------------------------------+ | ||
| .... | ||
|
|
||
| When the commit-graph is enabled, the INFINITY region is typically | ||
| very small -- it only contains commits added since the last | ||
| commit-graph refresh. | ||
|
|
||
| All reachable INFINITY-generation commits are visited before any | ||
| finite-generation commit, because INFINITY is larger than any finite | ||
| value. Once the walk crosses into the finite region, it stays there. | ||
|
|
||
| In the finite region, generation ordering guarantees topological | ||
| traversal: children are always visited before their parents. This | ||
| means that paint on already-visited commits is final -- no future | ||
| traversal step can add paint to them. | ||
|
|
||
| In the INFINITY region, 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. | ||
|
|
||
| Paint flags are only added, never removed. Since each flag can be set | ||
| at most once per commit, the number of times a commit can be | ||
| re-enqueued is bounded by the number of flag transitions. | ||
|
|
||
| Termination | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 6/24/2026 8:14 AM, Kristofer Karlsson via GitGitGadget wrote:
> Termination
> -----------
>
> -The walk uses a `nonstale_queue` wrapper around `prio_queue` that
> -tracks `max_nonstale`: the lowest-priority non-stale commit enqueued
> -so far. Once that commit is dequeued, every remaining entry is known
> -to be STALE and the loop terminates. Specifically, the main loop
> +The walk tracks the number of commits of each type in the queue
> +(PARENT1-only, PARENT2-only, pending merge-base). The main loop
> ends when one of the following conditions holds:
>
> 1. The queue is empty.
> - 2. `max_nonstale` has been dequeued, meaning the queue only contains
> - STALE entries.
> + 2. The queue contains only stale entries.
I'm grateful to see these changes happening to the doc in real-
time. I know it was extra work, but I'm grateful right now.
Hopefully future historians will also benefit from this effort.
> +static void paint_count_update(struct paint_state *state,
> + unsigned flags, int delta)
> +{
> + switch (flags & (PARENT1 | PARENT2 | STALE)) {
> + case PARENT1:
> + state->p1_count += delta;
> + break;
> +
> + case PARENT2:
> + state->p2_count += delta;
> + break;
> +
> + case PARENT1 | PARENT2:
> + state->pending_merge_bases += delta;
> + break;
> +
> + case PARENT1 | PARENT2 | STALE:
> + break;
> +
> + default:
> + BUG("unexpected paint state");
> + }
> +}
I like the use of 'delta' to allow reuse of this switch.
> +
> +static void paint_queue_put(struct paint_state *state,
> + struct commit *c, unsigned add_flags)
> +{
> + unsigned old_flags = c->object.flags;
> + c->object.flags |= add_flags;
> +
> + if (old_flags & ENQUEUED) {
> + paint_count_update(state, old_flags, -1);
> + paint_count_update(state, c->object.flags, 1);
> + } else {
> + c->object.flags |= ENQUEUED;
> + prio_queue_put(&state->queue, c);
> + paint_count_update(state, c->object.flags, 1);
> + }
> +}
ok: if we are already in the queue then we have old flags and
may need to subtract their values because they were counted
already. Otherwise, we need to queue it for the first time and
only add the values. Makes sense.
> +
> +static struct commit *paint_queue_get(struct paint_state *state)
> +{
Since we are going to make this a more complete termination
condition, we may want to make that very explicit with a doc-
comment. Something along the lines of "dequeue a commit when
possible, but also signal termination of the walk when we
conclude that no more merge bases will be discovered due to
internal state."
> @@ -187,12 +253,11 @@ static int paint_down_to_common(struct repository *r,
> return error(_("could not parse commit %s"),
> oid_to_hex(&p->object.oid));
> }
> - p->object.flags |= flags;
> - nonstale_queue_put_dedup(&queue, p);
> + paint_queue_put(&state, p, flags);
I like how this simplifies the flag-assignment logic somewhat.
You mentioned in your cover letter how the min_generation value
can add extra termination conditions. It may be a good idea to
insert min_generation into the paint_queue struct and make it a
termination condition for paint_queue_get(). If you consider this
direction, then I'd make it a separate patch on top of this one
_before_ adding the one-sided change. The extra tests that cover
the exact number of walked commits can help to guarantee the same
behavior, assuming that some of those tests check a non-zero
min_generation input. (It may be good to add such trace tests in
an earlier patch to help confidence in this case.)
Thanks,
-StoleeThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): On Wed, 24 Jun 2026 at 15:54, Derrick Stolee <stolee@gmail.com> wrote:
>
> I'm grateful to see these changes happening to the doc in real-
> time. I know it was extra work, but I'm grateful right now.
>
> Hopefully future historians will also benefit from this effort.
It was honestly not bad at all, and I agree it felt quite nice to
see how the doc naturally changed along with the implementation.
> > +static struct commit *paint_queue_get(struct paint_state *state)
> > +{
>
> Since we are going to make this a more complete termination
> condition, we may want to make that very explicit with a doc-
> comment. Something along the lines of "dequeue a commit when
> possible, but also signal termination of the walk when we
> conclude that no more merge bases will be discovered due to
> internal state."
Yes, I'll make sure to clean that part up more, maybe also
rename the function to be more descriptive.
> You mentioned in your cover letter how the min_generation value
> can add extra termination conditions. It may be a good idea to
> insert min_generation into the paint_queue struct and make it a
> termination condition for paint_queue_get(). If you consider this
> direction, then I'd make it a separate patch on top of this one
> _before_ adding the one-sided change. The extra tests that cover
> the exact number of walked commits can help to guarantee the same
> behavior, assuming that some of those tests check a non-zero
> min_generation input. (It may be good to add such trace tests in
> an earlier patch to help confidence in this case.)
I think I might wait with this - the patch series already feels
quite big, and I think it has a natural progression and finish now.
But I can definitely commit to following up later -- it would be a
smaller series that is easier to reason about, likely a single commit.
Thanks,
KristoferThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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é
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| ----------- | ||
|
|
||
| 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: | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 6/24/2026 8:14 AM, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
>
> Add an early termination check to paint_down_to_common() using the
> per-side counters introduced earlier. Once the walk enters the
> finite-generation region, terminate early when one side's exclusive
> count drops to zero -- no new merge-base can form without both paint
> sides meeting.
Having this as the last patch is truly a nice climax moment for the
patch series!
> @@ -94,6 +94,9 @@ ends when one of the following conditions holds:
>
> 1. The queue is empty.
> 2. The queue contains only stale entries.
> + 3. Side exhaustion: no pure PARENT1 or pure PARENT2 commits
> + remain in the queue, no pending merge-base candidates exist,
> + and the walk has entered the finite-generation region.
...> +Side-exhaustion condition
> +~~~~~~~~~~~~~~~~~~~~~~~~~
> +A new merge-base requires commits from both sides to meet. When one
> +side's exclusive counter reaches zero and there are no pending
> +merge-base candidates, no future traversal step can produce a new
> +candidate.
> +
> +This optimization only activates in the finite-generation region
> +where topological ordering holds. In that region, children are
> +always visited before parents, so paint flags are final at visit
> +time and an exhausted side cannot reappear. In the INFINITY region,
> +commit-date ordering can violate this guarantee, so the check is
> +skipped.
> +
And these doc updates inline make me happy.
> Related documentation
> ---------------------
>
> diff --git a/commit-reach.c b/commit-reach.c
> index e0d9874f99..f79d0b64d6 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -133,17 +133,30 @@ static void paint_queue_put(struct paint_state *state,
>
> static struct commit *paint_queue_get(struct paint_state *state)
> {
> - struct commit *commit;
> + struct commit *commit = prio_queue_get(&state->queue);
>
> - if (!state->p1_count && !state->p2_count &&
> - !state->pending_merge_bases)
> + if (!commit)
> return NULL;
I see how the previous implementation has a termination condition
before calling prio_queue_get(), which is technically more
efficient. It does make this initial diff a bit more complicated
because we are moving the prio_queue_get() line.
If the introduction of the method in patch 5/7 looked like this:
+static struct commit *paint_queue_get(struct paint_state *state)
+{
+ struct commit *commit = prio_queue_get(&state->queue);
+
+ if (!commit)
+ return NULL;
+
+ if (!state->p1_count && !state->p2_count &&
+ !state->pending_merge_bases)
+ return NULL;
+
+ commit->object.flags &= ~ENQUEUED;
+ paint_count_update(state, commit->object.flags, -1);
+ return commit;
+}
Then this diff would look cleaner.
(This is the nittiest of nitpicks so feel free to ignore if this
doesn't bother you at all.)
> - commit = prio_queue_get(&state->queue);
> - if (commit) {
> - commit->object.flags &= ~ENQUEUED;
> - paint_count_update(state, commit->object.flags, -1);
> + commit->object.flags &= ~ENQUEUED;
> +
> + if (!state->pending_merge_bases) {
> + if (!state->p1_count && !state->p2_count)
> + return NULL;
> + /*
> + * Side exhaustion: a new merge-base can only form
> + * when both PARENT1-only and PARENT2-only commits
> + * remain in the queue. In the finite-generation
> + * region the queue is ordered topologically, so
> + * no future step can add paint to visited commits
> + * and an exhausted side cannot reappear.
> + */
> + if ((!state->p1_count || !state->p2_count) &&
> + commit_graph_generation(commit) < GENERATION_NUMBER_INFINITY)
> + return NULL;
> }
> +
> + paint_count_update(state, commit->object.flags, -1);
> return commit;
> }
I like how the crux of this implementation is entirely within
paint_queue_get() now.
> diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
> index c1109fb42f..03175befb3 100755
> --- a/t/t6600-test-reach.sh
> +++ b/t/t6600-test-reach.sh
> @@ -332,12 +332,12 @@ test_expect_success 'merge-base --all commit-walk steps' '
> cp commit-graph-full .git/objects/info/commit-graph &&
> GIT_TRACE2_EVENT="$(pwd)/trace-full.txt" \
> git merge-base --all commit-9-9 commit-9-1 >actual &&
> - test_trace2_data paint_down_to_common steps 80 <trace-full.txt &&
> + test_trace2_data paint_down_to_common steps 9 <trace-full.txt &&
>
> cp commit-graph-half .git/objects/info/commit-graph &&
> GIT_TRACE2_EVENT="$(pwd)/trace-half.txt" \
> git merge-base --all commit-9-9 commit-9-1 >actual &&
> - test_trace2_data paint_down_to_common steps 81 <trace-half.txt
> + test_trace2_data paint_down_to_common steps 57 <trace-half.txt
> '
I love to see these steps change. If you take my suggestion to
update more tests with these checks, then this diff will get bigger
(but in a deserved way).
Also, when I suggested that 'test_all_modes' creates the trace
files on our behalf, I forgot to mention that this specific test
that you added in patch 4/7 simplifies by running the merge-base
check under 'test_all_modes' and then checking the trace2 data
on the three well-known files afterwards.
Thanks,
-Stolee
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): On Wed, 24 Jun 2026 at 16:02, Derrick Stolee <stolee@gmail.com> wrote:
>
> I see how the previous implementation has a termination condition
> before calling prio_queue_get(), which is technically more
> efficient. It does make this initial diff a bit more complicated
> because we are moving the prio_queue_get() line.
I was thinking the efficiency here does not matter in practice -
prio_queue_get() only returns NULL once, and all other times
where we keep looping we do need the value.
I agree it does get a bit complex though.
> If the introduction of the method in patch 5/7 looked like this:
>
> +static struct commit *paint_queue_get(struct paint_state *state)
> +{
> + struct commit *commit = prio_queue_get(&state->queue);
> +
> + if (!commit)
> + return NULL;
> +
> + if (!state->p1_count && !state->p2_count &&
> + !state->pending_merge_bases)
> + return NULL;
> +
> + commit->object.flags &= ~ENQUEUED;
> + paint_count_update(state, commit->object.flags, -1);
> + return commit;
> +}
>
> Then this diff would look cleaner.
>
> (This is the nittiest of nitpicks so feel free to ignore if this
> doesn't bother you at all.)
That's a good point. It doesn't technically bother me,
but it would be cleaner. The refactor commit would effectively
be looking into the future and prepare for it. I can change it for
the next version - my only thinking was that the current refactor
patch matched my original idea for how to best handle
the halt condition, but that did indeed change after this discussion.
> > - test_trace2_data paint_down_to_common steps 81 <trace-half.txt
> > + test_trace2_data paint_down_to_common steps 57 <trace-half.txt
> > '
> I love to see these steps change. If you take my suggestion to
> update more tests with these checks, then this diff will get bigger
> (but in a deserved way).
I will try to add them to some (but not all) tests since it's more
closely related to performance than correctness and I want to
avoid making too many tests overly fragile.
> Also, when I suggested that 'test_all_modes' creates the trace
> files on our behalf, I forgot to mention that this specific test
> that you added in patch 4/7 simplifies by running the merge-base
> check under 'test_all_modes' and then checking the trace2 data
> on the three well-known files afterwards.
That's a nice bonus, I will try to see if I can manage to utilize it.
Thanks,
KristoferThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 6/24/2026 10:47 AM, Kristofer Karlsson wrote:
> On Wed, 24 Jun 2026 at 16:02, Derrick Stolee <stolee@gmail.com> wrote:
>>> - test_trace2_data paint_down_to_common steps 81 <trace-half.txt
>>> + test_trace2_data paint_down_to_common steps 57 <trace-half.txt
>>> '
>> I love to see these steps change. If you take my suggestion to
>> update more tests with these checks, then this diff will get bigger
>> (but in a deserved way).
>
> I will try to add them to some (but not all) tests since it's more
> closely related to performance than correctness and I want to
> avoid making too many tests overly fragile.
In this case, I think it's more about protecting all of our special-
cased termination conditions. The rigidity means that it is hard to
accidentally change the behavior. It does have the downside that
more tests need to change if there is an intentional change, but it
also gives the same _evidence_ that the change has the intended
impact.
We are definitely leaning into personal preferences, though. There
is no hard rule one way or another.
Thanks,
-Stolee
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): On Fri, 26 Jun 2026 at 15:08, Kristofer Karlsson via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Kristofer Karlsson <krka@spotify.com>
>
> - if (min_generation && generation > last_gen)
> + if (generation > last_gen)
I have to note that I accidentally pushed this version before noticing
that it now fails for a subset of commit-graph modes.
Apologies for that - I will rework the logic here later
to preserve the behavior better.
I think (and hope) the rest of the patch series is in good shape though
and addressed the previous feedback, so any partial new review
feedback would still be appreciated.
Thanks,
KristoferThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 6/26/2026 10:29 AM, Kristofer Karlsson wrote:
> On Fri, 26 Jun 2026 at 15:08, Kristofer Karlsson via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Kristofer Karlsson <krka@spotify.com>
>>
>> - if (min_generation && generation > last_gen)
>> + if (generation > last_gen)
>
> I have to note that I accidentally pushed this version before noticing
> that it now fails for a subset of commit-graph modes.
> Apologies for that - I will rework the logic here later
> to preserve the behavior better.
And do we catch this with a test case? I'm hoping that you discovered
this error through the test suite, even if you submitted the series a
little early.
> I think (and hope) the rest of the patch series is in good shape though
> and addressed the previous feedback, so any partial new review
> feedback would still be appreciated.
Thanks for calling this out, as now I can avoid trying to understand
this change during my review.
Thanks,
-Stolee
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): On Fri, 26 Jun 2026 at 16:32, Derrick Stolee <stolee@gmail.com> wrote:
>
> > I have to note that I accidentally pushed this version before noticing
> > that it now fails for a subset of commit-graph modes.
> > Apologies for that - I will rework the logic here later
> > to preserve the behavior better.
>
> And do we catch this with a test case? I'm hoping that you discovered
> this error through the test suite, even if you submitted the series a
> little early.
(I missed replying to this message initially, sorry)
Yes exactly - it was caught by t6600 but somehow I missed running it before
submitting it (so I noticed it in the GGG CI instead)
So the existing tests are good, I only wish I could be equally reliable as them.
Thanks,
KristoferThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 6/26/2026 9:08 AM, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
> @@ -140,9 +144,16 @@ static struct commit *paint_queue_get(struct paint_state *state)
>
> commit->object.flags &= ~ENQUEUED;
>
> - if (!state->p1_count && !state->p2_count &&
> - !state->pending_merge_bases)
> - return NULL;
> + if (!state->pending_merge_bases) {
> + /* only stale entries remain */
> + if (!state->p1_count && !state->p2_count)
> + return NULL;
> +
> + /* one side is exhausted */
> + if ((!state->p1_count || !state->p2_count) &&
> + commit_graph_generation(commit) < GENERATION_NUMBER_INFINITY)
> + return NULL;
> + }
This continues to look correct.
> paint_count_update(state, commit->object.flags, -1);
> return commit;
> @@ -188,7 +199,7 @@ static int paint_down_to_common(struct repository *r,
> timestamp_t generation = commit_graph_generation(commit);
> steps++;
>
> - if (min_generation && generation > last_gen)
> + if (generation > last_gen)
> BUG("bad generation skip %"PRItime" > %"PRItime" at %s",
> generation, last_gen,
> oid_to_hex(&commit->object.oid));
You mention in your own reply that this is broken. This also looks
like a stray change for this patch, so perhaps your end state is
correct despite this patch causing failures. Will inspect soon.
> - test_paint_down_steps 45 2 25 3
> + test_paint_down_steps 45 1 25 1
...> - test_paint_down_steps 81 80 81 81
> + test_paint_down_steps 81 9 57 10
These diffs are satisfying.
Thanks,
-Stolee
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): On Fri, 26 Jun 2026 at 16:35, Derrick Stolee <stolee@gmail.com> wrote:
>
> > - if (min_generation && generation > last_gen)
> > + if (generation > last_gen)
> > BUG("bad generation skip %"PRItime" > %"PRItime" at %s",
> > generation, last_gen,
> > oid_to_hex(&commit->object.oid));
>
> You mention in your own reply that this is broken. This also looks
> like a stray change for this patch, so perhaps your end state is
> correct despite this patch causing failures. Will inspect soon.
I did not intend it to be a stray change, but rather a natural followup
to the idea that we could fold all of the halt conditions into the same
place. I am happy to either revert that part for v4 (to keep the change
simpler, but not fully unified) or fix it properly - I think it should be easy
since this was just human error, not a sign of a fundamentally tricky
problem.
> > - test_paint_down_steps 45 2 25 3
> > + test_paint_down_steps 45 1 25 1
> ...> - test_paint_down_steps 81 80 81 81
> > + test_paint_down_steps 81 9 57 10
> These diffs are satisfying.
Agreed! It was nice to introduce the steps counter to the
test suite, showing that the patch reached its intended goal
which is clearer than just having benchmarks in the messages.
Thanks again,
Kristofer |
||
| 1. The queue is empty. | ||
| 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 | ||
| been found, and the walk has entered the finite-generation | ||
| region. | ||
| 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. | ||
|
|
||
| Stale entry condition | ||
| ~~~~~~~~~~~~~~~~~~~~~ | ||
| Once all queued entries are stale, no new merge-base candidates can | ||
| be discovered -- that requires at least one non-stale commit from | ||
| each side meeting. Continuing the walk could still invalidate | ||
| existing candidates by proving one is an ancestor of another, but | ||
| `remove_redundant()` handles that as a post-processing step, so it | ||
| is safe to exit early. | ||
|
|
||
| Side-exhaustion condition | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| A new merge-base requires commits from both sides to meet. When one | ||
| side's exclusive counter reaches zero and there are no pending | ||
| merge-base candidates, no future traversal step can produce a new | ||
| candidate. | ||
|
|
||
| This optimization only activates in the finite-generation region | ||
| where topological ordering holds. In that region, children are | ||
| always visited before parents, so paint flags are final at visit | ||
| time and an exhausted side cannot reappear. In the INFINITY region, | ||
| commit-date ordering can violate this guarantee, so the check is | ||
| skipped. | ||
|
|
||
| Generation cutoff | ||
| ~~~~~~~~~~~~~~~~~ | ||
| Some callers (notably `remove_redundant()`) supply a `min_generation` | ||
| threshold -- the minimum generation of the input commits. No merge | ||
| base can have a generation below this threshold, so the walk | ||
| terminates as soon as it dequeues such a commit. | ||
|
|
||
| Single result | ||
| ~~~~~~~~~~~~~ | ||
| When only one merge base is needed, the walk is in the | ||
| finite-generation region, and the queue uses generation ordering, | ||
| the first candidate found is necessarily the highest-generation | ||
| common ancestor. No remaining commit in the queue can be a | ||
| descendant of this candidate (generation ordering guarantees | ||
| children are visited first), so it cannot be redundant and the walk | ||
| can stop immediately. | ||
|
|
||
| Related documentation | ||
| --------------------- | ||
|
|
||
| - `Documentation/technical/commit-graph.adoc` -- generation numbers | ||
| and the reachability closure property. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Derrick Stolee wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):