Re: [PATCH 1/2] merge-base: fix duplicates and not best ancestors in output

From: Василий Макаров <einmalfel@gmail.com>
Date: Dec 31, 2013, 11:15 UTC
Message-ID: <CABEtfDEVfmrTrsAUqY75NfBPYJpWA-YVouYJDktn5ovkjg_Vtw@mail.gmail.com>
In-Reply-To: <xmqqy532qf23.fsf@gitster.dls.corp.google.com>

Hello, Junio!

Show 8 quoted lines

>> Hi there!
>> First of all: I'm new to mailing-lists, sorry if I'm doing it wrong.
>>
>> I've found a bug in git merge-base, causing it to show not best common
>> ancestors and duplicates under some circumstances (example is given in
>> attached test case).
>
>Attached???

Sorry about this. I expected my first message to be sent back to me by git@vger.kernel.org. As I understand I should have replied to this message with second patch (test). But I did not received first message back, so I just sent second one to git@vger.kernel.org. What am I doing wrong?

> I think we should split that helper function
> handle_octopus().  It does two totally unrelated things

Agree! I have not done this in original patch because I wanted it to be a minimal change.

Show 6 quoted lines

> And assuming that deduping is the right thing to do here, here is a
> follow-up on top of the spliting patch.
>
> Scripts that use "merge-base --octopus" could do the reducing
> themselves, but most of them are expected to want to get the reduced
> results without having to do any work themselves.

Not sure what scripts you are talking about. Man git merge-base says:
"--octopus
           Compute the best common ancestors of all supplied commits"
Without deduping this option doesn't always work, so it is a right
thing to do here.

I've also tested changes you've sent, they are OK.

Happy new year!

2013/12/31 Junio C Hamano <gitster@pobox.com>:

Show 107 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> I do not offhand remember if it was deliberate that we do not dedup
>> the result from the underlying get_octopus_merge_bases() (the most
>> likely reason for not deduping is because the caller is expected to
>> do that if it wants to).
>>
>> Whether it is an improvement to force deduping here or it is an
>> regression to do so, I think we should split that helper function
>> handle_octopus().  It does two totally unrelated things (one is only
>> to list independent heads without showing merge bases, the other is
>> to show one or more merge bases across all the heads given).
>> Perhaps if we split the "independent" codepath introduced by
>> a1e0ad78 (merge-base --independent to print reduced parent list in a
>> merge, 2010-08-17) into its own helper function, like this, it would
>> make it clear what is going on.
>
> And assuming that deduping is the right thing to do here, here is a
> follow-up on top of the spliting patch.
>
> -- >8 --
> Subject: [PATCH] merge-base --octopus: reduce the result from get_octopus_merge_bases()
>
> Scripts that use "merge-base --octopus" could do the reducing
> themselves, but most of them are expected to want to get the reduced
> results without having to do any work themselves.
>
> Tests are taken from a message by Василий Макаров
> <einmalfel@gmail.com>
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  We might want to vet the existing callers of the underlying
>  get_octopus_merge_bases() and find out if _all_ of them are doing
>  anything extra (like deduping) because the machinery can return
>  duplicate results. And if that is the case, then we may want to
>  move the dedupling down the callchain instead of having it here.
>
>  builtin/merge-base.c  |  2 +-
>  t/t6010-merge-base.sh | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/merge-base.c b/builtin/merge-base.c
> index daa96c7..87f4dbc 100644
> --- a/builtin/merge-base.c
> +++ b/builtin/merge-base.c
> @@ -73,7 +73,7 @@ static int handle_octopus(int count, const char **args, int show_all)
>         for (i = count - 1; i >= 0; i--)
>                 commit_list_insert(get_commit_reference(args[i]), &revs);
>
> -       result = get_octopus_merge_bases(revs);
> +       result = reduce_heads(get_octopus_merge_bases(revs));
>
>         if (!result)
>                 return 1;
> diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh
> index f80bba8..abb5728 100755
> --- a/t/t6010-merge-base.sh
> +++ b/t/t6010-merge-base.sh
> @@ -230,4 +230,43 @@ test_expect_success 'criss-cross merge-base for octopus-step' '
>         test_cmp expected.sorted actual.sorted
>  '
>
> +test_expect_success 'merge-base --octopus --all for complex tree' '
> +       # Best common ancestor for JE, JAA and JDD is JC
> +       #             JE
> +       #            / |
> +       #           /  |
> +       #          /   |
> +       #  JAA    /    |
> +       #   |\   /     |
> +       #   | \  | JDD |
> +       #   |  \ |/ |  |
> +       #   |   JC JD  |
> +       #   |    | /|  |
> +       #   |    |/ |  |
> +       #  JA    |  |  |
> +       #   |\  /|  |  |
> +       #   X JB |  X  X
> +       #   \  \ | /   /
> +       #    \__\|/___/
> +       #        J
> +       test_commit J &&
> +       test_commit JB &&
> +       git reset --hard J &&
> +       test_commit JC &&
> +       git reset --hard J &&
> +       test_commit JTEMP1 &&
> +       test_merge JA JB &&
> +       test_merge JAA JC &&
> +       git reset --hard J &&
> +       test_commit JTEMP2 &&
> +       test_merge JD JB &&
> +       test_merge JDD JC &&
> +       git reset --hard J &&
> +       test_commit JTEMP3 &&
> +       test_merge JE JC &&
> +       git rev-parse JC >expected &&
> +       git merge-base --all --octopus JAA JDD JE >actual &&
> +       test_cmp expected actual
> +'
> +
>  test_done
> --
> 1.8.5.2-311-g6427a96
>