commit-reach: fix !FIND_ALL early exit with v1 commit graph

[spkrka]

Fixes a bug introduced by 93e5b16 (commit-reach: early exit
paint_down_to_common for single merge-base, 2025-04-10) where
git merge-base can return the wrong result.

The bug requires all of the following to trigger:

1. A v1 commit graph (topological levels only, no corrected commit
   dates). Generation v2 with corrected commit dates has been the
   default since 2021, so only repos that have not rewritten their
   commit graph in over four years would be affected.
2. git merge-base without --all (the common case, but --all
   is unaffected because it disables the early exit).
3. A topology with clock skew: the correct merge base has a lower
   committer date than one of its ancestors that is also a common
   ancestor. With date ordering, the deeper ancestor pops first
   and the early exit fires before the correct result is found.

This two-patch series:

1. Adds a test demonstrating the bug (clock-skew topology where the
   correct merge base has a lower date than its ancestor)
2. Fixes it by tracking whether the queue is generation-ordered and
   gating the early exit on that flag

cc: Junio C Hamano gitster@pobox.com
cc: Derrick Stolee stolee@gmail.com
cc: Kristofer Karlsson krka@spotify.com

[spkrka]

/submit

[gitgitgadget]

Submitted as pull.2162.git.1782739162.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2162/spkrka/find-all-fix-v1

To fetch this version to local tag pr-2162/spkrka/find-all-fix-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2162/spkrka/find-all-fix-v1

[gitgitgadget]

This branch is now known as kk/commit-reach-find-all-fix.

[gitgitgadget]

This patch series was integrated into seen via git@a560473.

[gitgitgadget]

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

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

> Fixes a bug introduced by 93e5b1680e (commit-reach: early exit
> paint_down_to_common for single merge-base, 2025-04-10) where git merge-base
> can return the wrong result.
>
> The bug requires all of the following to trigger:
>
>  1. A v1 commit graph (topological levels only, no corrected commit dates).
>     Generation v2 with corrected commit dates has been the default since
>     2021, so only repos that have not rewritten their commit graph in over
>     four years would be affected.
>  2. git merge-base without --all (the common case, but --all is unaffected
>     because it disables the early exit).
>  3. A topology with clock skew: the correct merge base has a lower committer
>     date than one of its ancestors that is also a common ancestor. With date
>     ordering, the deeper ancestor pops first and the early exit fires before
>     the correct result is found.
>
> This two-patch series:
>
>  1. Adds a test demonstrating the bug (clock-skew topology where the correct
>     merge base has a lower date than its ancestor)
>  2. Fixes it by tracking whether the queue is generation-ordered and gating
>     the early exit on that flag

Where should this new "gen_ordered" flag go in the world with
kk/merge-base-exhaustion topic merged in?  Does it also belong
to the paint_state struct or can it be on-stack independent variable
to the function?

[gitgitgadget]

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

On Mon, 29 Jun 2026 at 19:50, Junio C Hamano <gitster@pobox.com> wrote:
>
> Where should this new "gen_ordered" flag go in the world with
> kk/merge-base-exhaustion topic merged in?  Does it also belong
> to the paint_state struct or can it be on-stack independent variable
> to the function?

I don't think kk/merge-base-exhaustion is ready to be merged as-is
and you are right that these two topics would conflict.
I will need to reroll v5 and exactly how/when I do that
depends on what we do here.

I wanted to quickly share this small patch as a bugfix
since the related code is already merged. I think the bug
itself might be a very unlikely edge case but I can't really be
sure.

My preference would be to merge this as-is, and then I can rework
v5 of kk/merge-base-exhaustion on top of it later - it will
add some delay but I don't want to rush it since it's a non-trivial
change.

The other option is to drop this topic if the risk is deemed low
enough, and then I will rework v5 to either apply a similar
gen_ordered flag or eliminate that different ordering entirely -
I see that as a good long-term goal that simplifies the code while
retaining or improving the performance.

I realize the timing is terrible, I wish I had spotted this a week
ago instead of right at the 2.55 finalization period.

Thanks,
Kristofer

[gitgitgadget]

User Kristofer Karlsson <krka@spotify.com> has been added to the cc: list.

[gitgitgadget]

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

Kristofer Karlsson <krka@spotify.com> writes:

> My preference would be to merge this as-is, and then I can rework
> v5 of kk/merge-base-exhaustion on top of it later - it will
> add some delay but I don't want to rush it since it's a non-trivial
> change.

That sounds very sensible.  Let's do that.

I've taken a look at both of these two patches and found the
solution quite sensible, but I'd prefer a second set of eyes
to confirm.

Thanks.

[gitgitgadget]

There was a status update in the "New Topics" section about the branch kk/commit-reach-find-all-fix on the Git mailing list:

The early-exit optimization in paint_down_to_common() has been gated
on the queue being generation-ordered. This fixes a bug where git
merge-base (without --all) could return incorrect results on
repositories with v1 commit graphs and clock skew.

Comments?
cf. <xmqqa4sdw55v.fsf@gitster.g>
source: <pull.2162.git.1782739162.gitgitgadget@gmail.com>