Re: [PATCH 1/2] merge-base: fix duplicates and not best ancestors in output
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
>
Василий Макаров