Hello, Junio!
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.
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 gits...@pobox.com:
Junio C Hamano gits...@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 Василий Макаров
einmal...@gmail.com
Signed-off-by: Junio C Hamano gits...@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
