Re: [PATCH 2/2] commit-reach: guard !FIND_ALL early exit with generation ordering check

[Junio C Hamano <gitster@pobox.com>]

"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
>  '