Re: [PATCH v4 2/2] 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:

> From: Kristofer Karlsson <krka@spotify.com>
>
> Commits not in the commit-graph get GENERATION_NUMBER_INFINITY and
> sort to the top of the priority queue.  After those, commits with
> finite generation numbers are popped in non-increasing order.
> When MERGE_BASE_FIND_ALL is not set the first doubly-painted commit
> with a finite generation is therefore a best merge-base: no commit
> still in the queue can be a descendant of it.  Skip the expensive
> STALE drain in this case.
>
> Add MERGE_BASE_FIND_ALL to the merge_base_flags enum.  Callers that
> need every merge-base (repo_get_merge_bases_many, repo_get_merge_bases,
> repo_in_merge_bases_many, remove_redundant_no_gen) pass the flag to
> preserve existing behavior.  git merge-base (without --all) passes 0,
> triggering the early exit.
>
> On a 2.2M-commit merge-heavy monorepo with commit-graph:
>
>   HEAD vs ~500:   5,229ms -> 24ms
>   HEAD vs ~1000:  4,214ms -> 39ms
>   HEAD vs ~5000:  3,799ms -> 46ms
>   HEAD vs ~10000: 3,827ms -> 61ms
>
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
>  builtin/merge-base.c  |  3 ++-
>  commit-reach.c        | 19 +++++++++++++++----
>  commit-reach.h        |  7 ++++++-
>  t/t6600-test-reach.sh | 40 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 63 insertions(+), 6 deletions(-)

Very nicely done and well described.

> diff --git a/builtin/merge-base.c b/builtin/merge-base.c
> index 9b50b4660e..a87011c6cd 100644
> --- a/builtin/merge-base.c
> +++ b/builtin/merge-base.c
> @@ -11,11 +11,12 @@
>  
>  static int show_merge_base(struct commit **rev, size_t rev_nr, int show_all)
>  {
> +	enum merge_base_flags flags = show_all ? MERGE_BASE_FIND_ALL : 0;

Curious that only this variable, among 6 that this two-patch series
introduces for the type, is called "flags" while all others are
called "mb_flags".  No need to change it; the comment is mostly to
show I did read the two patches with reasonable attention to the
detail ;-).

Will queue.  Thanks.