Skip to content

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

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.

Sign up for GitHub

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

Open
spkrka wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c10ff61
t6600: add test for merge-base early exit with clock skew
spkrka Jun 29, 2026
ba3f2bb
commit-reach: guard !FIND_ALL early exit with generation ordering check
spkrka Jun 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions commit-reach.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,14 @@ static int paint_down_to_common(struct repository *r,
{ compare_commits_by_gen_then_commit_date }

@gitgitgadget gitgitgadget Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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>
>
> When paint_down_to_common() falls back to commit-date ordering (for
> v1 commit graphs without corrected commit dates), the !FIND_ALL early
> exit incorrectly fires.  The exit assumes the queue is generation-
> ordered, so the first RESULT commit found must be the shallowest.
> With date ordering this is not guaranteed: a closer merge base with
> a lower committer date (clock skew) may still be in the queue behind
> deeper commits.

Excellent description of a good observation.

> Add a gen_ordered flag that is cleared when the date fallback fires,
> and require it for the early exit.

The solution is simple and straight-forward.

The flag is initialized to true but we drop it when generation order
is not in effect, and the early exit requires the flag to be still
true.

> Update the test from the previous commit to test_expect_success.
>
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---

Let's mark it for 'next'.  Thanks.

>  commit-reach.c        | 10 +++++++---
>  t/t6600-test-reach.sh |  2 +-
>  2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/commit-reach.c b/commit-reach.c
> index 5df471a313..708798a39b 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -108,11 +108,14 @@ static int paint_down_to_common(struct repository *r,
>  		{ compare_commits_by_gen_then_commit_date }
>  	};
>  	int i;
> +	int gen_ordered = 1;
>  	timestamp_t last_gen = GENERATION_NUMBER_INFINITY;
>  	struct commit_list **tail = result;
>  
> -	if (!min_generation && !corrected_commit_dates_enabled(r))
> +	if (!min_generation && !corrected_commit_dates_enabled(r)) {
>  		queue.pq.compare = compare_commits_by_commit_date;
> +		gen_ordered = 0;
> +	}
>  
>  	one->object.flags |= PARENT1;
>  	if (!n) {
> @@ -147,11 +150,12 @@ static int paint_down_to_common(struct repository *r,
>  				commit->object.flags |= RESULT;
>  				tail = commit_list_append(commit, tail);
>  				/*
> -				 * The queue is generation-ordered; no
> -				 * remaining common ancestor can be a
> +				 * When the queue is generation-ordered,
> +				 * no remaining common ancestor can be a
>  				 * descendant of this one.
>  				 */
>  				if (!(mb_flags & MERGE_BASE_FIND_ALL) &&
> +				    gen_ordered &&
>  				    generation < GENERATION_NUMBER_INFINITY)
>  					break;
>  			}
> diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
> index 1090104220..0ff41381ff 100755
> --- a/t/t6600-test-reach.sh
> +++ b/t/t6600-test-reach.sh
> @@ -1003,7 +1003,7 @@ test_expect_success 'merge-base without --all is one of --all results' '
>  	grep -F -f single all
>  '
>  
> -test_expect_failure 'merge-base without --all, clock skew, v1 commit-graph' '
> +test_expect_success 'merge-base without --all, clock skew, v1 commit-graph' '
>  	git rev-parse skew-M2 >expect &&
>  	merge_base_all_modes skew-P1 skew-P2
>  '

};
int i;
int gen_ordered = 1;
timestamp_t last_gen = GENERATION_NUMBER_INFINITY;
struct commit_list **tail = result;

if (!min_generation && !corrected_commit_dates_enabled(r))
if (!min_generation && !corrected_commit_dates_enabled(r)) {
queue.pq.compare = compare_commits_by_commit_date;
gen_ordered = 0;
}

one->object.flags |= PARENT1;
if (!n) {
Expand Down Expand Up @@ -147,11 +150,12 @@ static int paint_down_to_common(struct repository *r,
commit->object.flags |= RESULT;
tail = commit_list_append(commit, tail);
/*
* The queue is generation-ordered; no
* remaining common ancestor can be a
* When the queue is generation-ordered,
* no remaining common ancestor can be a
* descendant of this one.
*/
if (!(mb_flags & MERGE_BASE_FIND_ALL) &&
gen_ordered &&
generation < GENERATION_NUMBER_INFINITY)
break;
}
Expand Down
41 changes: 41 additions & 0 deletions t/t6600-test-reach.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,42 @@ test_expect_success 'setup' '
git tag -a -m "$x-$i" tag-$x-$i commit-$x-$i || return 1
done
done &&
# Build a topology with clock skew to test the !FIND_ALL early
# exit in paint_down_to_common(). M2 is the correct merge base
# of P1 and P2, but its ancestor M1 has a higher committer date
# due to clock skew. With date-only ordering (v1 commit graph
# without corrected commit dates), M1 pops from the queue first,
# gets both paint sides, and the early exit fires before M2 is
# ever visited.
#
# P1 P2 @7000
# | / \
# A B D @6000
# / \ | |
# | M2--+ | @2000 (correct merge base)
# \ | |
# M1--------+ @5000 (clock skew: date > M2)
# |
# root @1000
#
git checkout --orphan skew-orphan &&
skew_tree=$(git mktree </dev/null) &&
skew_commit () {
GIT_COMMITTER_DATE="@$1 +0000" GIT_AUTHOR_DATE="@$1 +0000" \
git commit-tree -m "$2" "$skew_tree" $3 $4 $5 $6
} &&
skew_root=$(skew_commit 1000 root) &&
skew_M1=$(skew_commit 5000 M1 -p "$skew_root") &&
skew_M2=$(skew_commit 2000 M2 -p "$skew_M1") &&
skew_A=$(skew_commit 6000 A -p "$skew_M1" -p "$skew_M2") &&
skew_B=$(skew_commit 6000 B -p "$skew_M2") &&
skew_D=$(skew_commit 6000 D -p "$skew_M1") &&
skew_P1=$(skew_commit 7000 P1 -p "$skew_A") &&
skew_P2=$(skew_commit 7000 P2 -p "$skew_B" -p "$skew_D") &&
git branch -f skew-P1 "$skew_P1" &&
git branch -f skew-P2 "$skew_P2" &&
git tag skew-M2 "$skew_M2" &&

git commit-graph write --reachable &&
mv .git/objects/info/commit-graph commit-graph-full &&
chmod u+w commit-graph-full &&
Expand Down Expand Up @@ -967,4 +1003,9 @@ test_expect_success 'merge-base without --all is one of --all results' '
grep -F -f single all
'

test_expect_success 'merge-base without --all, clock skew, v1 commit-graph' '
git rev-parse skew-M2 >expect &&
merge_base_all_modes skew-P1 skew-P2
'

test_done
Loading