Re: [PATCH] commit-reach: early exit paint_down_to_common for single merge-base

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

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

> diff --git a/commit-reach.c b/commit-reach.c
> index d3a9b3ed6f..c9d2d594de 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -55,14 +55,16 @@ static int paint_down_to_common(struct repository *r,
>  				struct commit **twos,
>  				timestamp_t min_generation,
>  				int ignore_missing_commits,
> +				int find_all,
>  				struct commit_list **result)
>  {
>  	struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
>  	int i;
> +	int has_gens = min_generation || corrected_commit_dates_enabled(r);

Giving a name that identifies what the commonly-used logical
expression means is a very good idea, but don't some caller pass
min_generation==infinity (i.e., not zero) when there is no
generation available?  E.g., remove_redundant_no_gen() passes
the result of calling commit_graph_generation(), and without commit
graph, we would get infinity here, right?

Given that the second user of this variable is guarded by !find_all,
the variable being true even when we shouldn't be using generation
numbers may not matter for the purpose of the early break, but then
it means that the patch squanders the perfect opportunity to clarify
what the variable means, which is not very lovely.

>  	timestamp_t last_gen = GENERATION_NUMBER_INFINITY;
>  	struct commit_list **tail = result;
>  
> -	if (!min_generation && !corrected_commit_dates_enabled(r))
> +	if (!has_gens)
>  		queue.compare = compare_commits_by_commit_date;
>  
>  	one->object.flags |= PARENT1;
> @@ -97,6 +99,11 @@ static int paint_down_to_common(struct repository *r,
>  			if (!(commit->object.flags & RESULT)) {
>  				commit->object.flags |= RESULT;
>  				tail = commit_list_append(commit, tail);
> +				/* Generation-ordered queue: no later
> +				 * commit can dominate this one. */
> +				if (!find_all && has_gens &&
> +				    generation < GENERATION_NUMBER_INFINITY)
> +					break;

Three comments

 * See Documentation/CodingGuidelines for our preferred style for
   multi-line comments.

 * I do not think we often use "dominate" to describe relationship
   between commits, and I am not sure what you want the word to mean
   in this context.  Can you clarify?

 * The code is getting way too deeply indented.  Perhaps using a
   helper function and


                        if (!(commit->object.flags & RESULT)) {
                                if (mark_result(r, &tail, commit,
						find_all, min_generation))
                                        break;
                        }

   move the logic to mark the newly discovered commit as one of the
   possible result, and to tell the loop to terminate, to it, along
   with the comment on the meaning of its return value, may make the
   code easier to follow?

> +/* To be used only when object flags after this call no longer matter.
> + * When find_all is false and generation numbers are available, returns
> + * after finding the first merge-base, skipping the STALE drain. */

Ditto on the style.