[PATCHv2 0/2] merge-tree: fix (merge-base a b) b a

[PATCHv2 0/2] merge-tree: fix (merge-base a b) b a

[Will Palmer]

This series notes, then fixes, a regression introduced by
15b4f7a68d8c3c8ee28424415b203f61202d65d1 /
	merge-tree: use ll_merge() not xdl_merge()

I don't know the proper terminology to describe what's being fixed here.
This seems to most-easily be triggered by (for example):
	git merge-tree $(git merge-base HEAD @{u}) HEAD @{u}

In the git repository at the moment, this could be triggered with:
	git merge-tree $(git merge-base origin/next origin/master) \
		origin/next origin/master

Though as I write this, next has only just been merged with master, so
that is not the case. For an example which is less likely to go away,
try:
	git merge-tree c9eaaab4165d8f402930d12899ec097495b599e6 \
		be16ac8cc8ce693c6adf37b80db65d10a41b4eb9 \
		9918285fb10d81af9021dae99c5f4de88ded497c

It's actually very trivial to reproduce this, to the point where I
can't help but wonder how much merge-tree is actually being used. As
I narrowed the test-case more and more, I was surprised by how little
it took to trigger it. The first patch in this series includes some
very basic tests for merge-tree, the last of which demonstrates the
regression.

The second patch implements the trivial fix for it.

This is the second version of the patch. The first version of the patch
included very trivial tests for merge-tree, which were really only
intended to demonstrate the breakage being fixed. This version of the
patch includes proper tests, with real "expected vs actual" testing,
rather than dumb "did it return successfully?" checks.

Will Palmer (2):
  add basic tests for merge-tree
  fix merge-tree where two branches share no changes

 builtin/merge-tree.c  |    3 +-
 t/t4300-merge-tree.sh |  277 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 279 insertions(+), 1 deletions(-)
 create mode 100755 t/t4300-merge-tree.sh

-- 
1.7.1.703.g42c01

[PATCHv2 1/2] add basic tests for merge-tree

[Will Palmer]

merge-tree had no test cases, so here we add some very basic tests for
it, including some known-breakages.

Signed-off-by: Will Palmer <wmpalmer@gmail.com>
---
 t/t4300-merge-tree.sh |  277 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 277 insertions(+), 0 deletions(-)
 create mode 100755 t/t4300-merge-tree.sh

diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
new file mode 100755
index 0000000..b2ae3a1
--- /dev/null
+++ b/t/t4300-merge-tree.sh
@@ -0,0 +1,277 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Will Palmer
+#
+
+test_description='git merge-tree'
+. ./test-lib.sh
+
+test_expect_success setup '
+	test_commit "initial" "initial-file" "initial"
+'
+
+test_expect_'file add A, !B' '
+	cat >expected <<EXPECTED
+added in remote
+  their  100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
+@@ -0,0 +1 @@
++AAA
+EXPECTED
+
+	git reset --hard initial
+	test_commit "add-a-not-b" "ONE" "AAA"
+
+	git merge-tree initial initial add-a-not-b >actual &&
+	test_cmp expected actual
+'
+
+test_expect_failure 'file add !A, B' '
+	cat >expected <<EXPECTED
+added in local
+  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
+EXPECTED
+
+	git reset --hard initial
+	test_commit "add-not-a-b" "ONE" "AAA"
+
+	git merge-tree initial add-not-a-b initial >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'file add A, B (same)' '
+	cat >expected <<EXPECTED
+added in both
+  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
+  their  100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
+EXPECTED
+
+	git reset --hard initial
+	test_commit "add-a-b-same-A" "ONE" "AAA"
+
+	git reset --hard initial
+	test_commit "add-a-b-same-B" "ONE" "AAA"
+
+	git merge-tree initial add-a-b-same-A add-a-b-same-B >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'file add A, B (different)' '
+	cat >expected <<EXPECTED
+added in both
+  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
+  their  100644 ba629238ca89489f2b350e196ca445e09d8bb834 ONE
+@@ -1 +1,5 @@
++<<<<<<< .our
+ AAA
++=======
++BBB
++>>>>>>> .their
+EXPECTED
+
+	git reset --hard initial
+	test_commit "add-a-b-diff-A" "ONE" "AAA"
+
+	git reset --hard initial
+	test_commit "add-a-b-diff-B" "ONE" "BBB"
+
+	git merge-tree initial add-a-b-diff-A add-a-b-diff-B >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'file change A, !B' '
+	cat >expected <<EXPECTED
+EXPECTED
+
+	git reset --hard initial
+	test_commit "change-a-not-b" "initial-file" "BBB"
+
+	git merge-tree initial change-a-not-b initial >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'file change !A, B' '
+	cat >expected <<EXPECTED
+merged
+  result 100644 ba629238ca89489f2b350e196ca445e09d8bb834 initial-file
+  our    100644 e79c5e8f964493290a409888d5413a737e8e5dd5 initial-file
+@@ -1 +1 @@
+-initial
++BBB
+EXPECTED
+	git reset --hard initial
+	test_commit "change-not-a-b" "initial-file" "BBB"
+
+	git merge-tree initial initial change-not-a-b >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'file change A, B (same)' '
+	cat >expected <<EXPECTED
+EXPECTED
+
+	git reset --hard initial
+	test_commit "change-a-b-same-A" "initial-file" "AAA"
+
+	git reset --hard initial
+	test_commit "change-a-b-same-B" "initial-file" "AAA"
+
+	git merge-tree initial change-a-b-same-A change-a-b-same-B >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'file change A, B (different)' '
+	cat >expected <<EXPECTED
+changed in both
+  base   100644 e79c5e8f964493290a409888d5413a737e8e5dd5 initial-file
+  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d initial-file
+  their  100644 ba629238ca89489f2b350e196ca445e09d8bb834 initial-file
+@@ -1 +1,5 @@
++<<<<<<< .our
+ AAA
++=======
++BBB
++>>>>>>> .their
+EXPECTED
+
+	git reset --hard initial
+	test_commit "change-a-b-diff-A" "initial-file" "AAA"
+
+	git reset --hard initial
+	test_commit "change-a-b-diff-B" "initial-file" "BBB"
+
+	git merge-tree initial change-a-b-diff-A change-a-b-diff-B >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'file change A, B (mixed)' '
+	cat >expected <<EXPECTED
+changed in both
+  base   100644 f4f1f998c7776568c4ff38f516d77fef9399b5a7 ONE
+  our    100644 af14c2c3475337c73759d561ef70b59e5c731176 ONE
+  their  100644 372d761493f524d44d59bd24700c3bdf914c973c ONE
+@@ -7,7 +7,11 @@
+ AAA
+ AAA
+ AAA
++<<<<<<< .our
+ BBB
++=======
++CCC
++>>>>>>> .their
+ AAA
+ AAA
+ AAA
+EXPECTED
+
+	git reset --hard initial
+	test_commit "change-a-b-mix-base" "ONE" "
+AAA
+AAA
+AAA
+AAA
+AAA
+AAA
+AAA
+AAA
+AAA
+AAA
+AAA
+AAA
+AAA
+AAA
+AAA"
+
+	test_commit "change-a-b-mix-A" "ONE" \
+		"$(sed -e "1{s/AAA/BBB/;}" -e "10{s/AAA/BBB/;}" <ONE)"
+
+	git reset --hard change-a-b-mix-base
+	test_commit "change-a-b-mix-B" "ONE" \
+		"$(sed -e "1{s/AAA/BBB/;}" -e "10{s/AAA/CCC/;}" <ONE)"
+
+	git merge-tree change-a-b-mix-base change-a-b-mix-A change-a-b-mix-B >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'file remove A, !B' '
+	cat >expected <<EXPECTED
+removed in local
+  base   100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
+  their  100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
+EXPECTED
+
+	git reset --hard initial
+	test_commit "rm-a-not-b-base" "ONE" "AAA"
+
+	git rm ONE
+	git commit -m "rm-a-not-b"
+	git tag "rm-a-not-b"
+
+	git merge-tree rm-a-not-b-base rm-a-not-b rm-a-not-b-base >actual &&
+	test_cmp expected actual
+'
+
+test_expect_failure 'file remove !A, B' '
+	cat >expected <<EXPECTED
+removed in remote
+  base   100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
+  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
+@@ -1 +0,0 @@
+-AAA
+EXPECTED
+
+	git reset --hard initial
+	test_commit "rm-not-a-b-base" "ONE" "AAA"
+
+	git rm ONE
+	git commit -m "rm-not-a-b"
+	git tag "rm-not-a-b"
+
+	git merge-tree rm-a-not-b-base rm-a-not-b-base rm-a-not-b >actual &&
+	test_cmp expected actual
+'
+
+test_expect_failure 'file change A, remove B' '
+	cat >expected <<EXPECTED
+removed in remote
+  base   100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
+  our    100644 ba629238ca89489f2b350e196ca445e09d8bb834 ONE
+@@ -1 +0,0 @@
+-BBB
+EXPECTED
+
+	git reset --hard initial
+	test_commit "change-a-rm-b-base" "ONE" "AAA"
+
+	test_commit "change-a-rm-b-A" "ONE" "BBB"
+
+	git reset --hard change-a-rm-b-base
+	git rm ONE
+	git commit -m "change-a-rm-b-B"
+	git tag "change-a-rm-b-B"
+
+	git merge-tree change-a-rm-b-base change-a-rm-b-A change-a-rm-b-B >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'file remove A, change B' '
+	cat >expected <<EXPECTED
+removed in local
+  base   100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
+  their  100644 ba629238ca89489f2b350e196ca445e09d8bb834 ONE
+EXPECTED
+
+	git reset --hard initial
+	test_commit "rm-a-change-b-base" "ONE" "AAA"
+
+	git rm ONE
+	git commit -m "rm-a-change-b-A"
+	git tag "rm-a-change-b-A"
+
+	git reset --hard rm-a-change-b-base
+	test_commit "rm-a-change-b-B" "ONE" "BBB"
+
+	git merge-tree rm-a-change-b-base rm-a-change-b-A rm-a-change-b-B >actual &&
+	test_cmp expected actual
+'
+
+test_done
-- 
1.7.1.703.g42c01

[PATCHv2 2/2] fix merge-tree where two branches share no changes

[Will Palmer]

Here we fix a regression which was introduced by
15b4f7a68d8c3c8ee28424415b203f61202d65d1 /
	merge-tree: use ll_merge() not xdl_merge()

Which caused merge-tree to segfault in particular combinations of
merging files which existed in one branch, but not in the other or in
the merge-base. This was caused by referencing entry->path at a time
when entry was known to be possibly-NULL.

To correct the problem, we save the path of the entry we came in with,
as the path should be the same among all the stages no matter which
sides are involved in the merge.

Signed-off-by: Will Palmer <wmpalmer@gmail.com>
---
 builtin/merge-tree.c  |    3 ++-
 t/t4300-merge-tree.sh |    6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index fc00d79..9b25ddc 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -60,6 +60,7 @@ static void *result(struct merge_list *entry, unsigned long *size)
 {
 	enum object_type type;
 	struct blob *base, *our, *their;
+	const char *path = entry->path;
 
 	if (!entry->stage)
 		return read_sha1_file(entry->blob->object.sha1, &type, size);
@@ -76,7 +77,7 @@ static void *result(struct merge_list *entry, unsigned long *size)
 	their = NULL;
 	if (entry)
 		their = entry->blob;
-	return merge_file(entry->path, base, our, their, size);
+	return merge_file(path, base, our, their, size);
 }
 
 static void *origin(struct merge_list *entry, unsigned long *size)
diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
index b2ae3a1..5feff91 100755
--- a/t/t4300-merge-tree.sh
+++ b/t/t4300-merge-tree.sh
@@ -25,7 +25,7 @@ EXPECTED
 	test_cmp expected actual
 '
 
-test_expect_failure 'file add !A, B' '
+test_expect_success 'file add !A, B' '
 	cat >expected <<EXPECTED
 added in local
   our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
@@ -210,7 +210,7 @@ EXPECTED
 	test_cmp expected actual
 '
 
-test_expect_failure 'file remove !A, B' '
+test_expect_success 'file remove !A, B' '
 	cat >expected <<EXPECTED
 removed in remote
   base   100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
@@ -230,7 +230,7 @@ EXPECTED
 	test_cmp expected actual
 '
 
-test_expect_failure 'file change A, remove B' '
+test_expect_success 'file change A, remove B' '
 	cat >expected <<EXPECTED
 removed in remote
   base   100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
-- 
1.7.1.703.g42c01

Re: [PATCHv2 1/2] add basic tests for merge-tree

[Junio C Hamano]

Will Palmer <wmpalmer@gmail.com> writes:

> merge-tree had no test cases, so here we add some very basic tests for
> it, including some known-breakages.
>
> Signed-off-by: Will Palmer <wmpalmer@gmail.com>
> ---
>  t/t4300-merge-tree.sh |  277 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 277 insertions(+), 0 deletions(-)
>  create mode 100755 t/t4300-merge-tree.sh

This does not sound like part of "the diff commands", unlike apply/diff/log.
Perhaps somewhere in t3xxx "other basic commands" series?

> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
> new file mode 100755
> index 0000000..b2ae3a1
> --- /dev/null
> +++ b/t/t4300-merge-tree.sh
> @@ -0,0 +1,277 @@
> ...
> +test_expect_'file add A, !B' '
> +	cat >expected <<EXPECTED
> +added in remote
> +  their  100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
> +@@ -0,0 +1 @@
> ++AAA
> +EXPECTED

Since you do not use any variable substitution in the here text, please
make that clear by quoting EXPECTED like this:

	cat >expected <<\EXPECTED
... your here text goes here ...
EXPECTED

to help the readers, who otherwise have to scan the here text to find if
there is any interesting substitution going on.

> +	git reset --hard initial
> +	test_commit "add-a-not-b" "ONE" "AAA"

Please fix missing && cascades.

Thanks.

Re: [PATCHv2 1/2] add basic tests for merge-tree

[Will Palmer]

On Tue, 2010-07-13 at 16:31 -0700, Junio C Hamano wrote:
> Will Palmer <wmpalmer@gmail.com> writes:
> 
> > merge-tree had no test cases, so here we add some very basic tests for
> > it, including some known-breakages.
> >
> > Signed-off-by: Will Palmer <wmpalmer@gmail.com>
> > ---
> >  t/t4300-merge-tree.sh |  277 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 277 insertions(+), 0 deletions(-)
> >  create mode 100755 t/t4300-merge-tree.sh
> 
> This does not sound like part of "the diff commands", unlike apply/diff/log.
> Perhaps somewhere in t3xxx "other basic commands" series?

I was a bit iffy on this one. Part of this was, admittedly, because
t3[0-9] were all taken, and I'm not sure about what number should be
used after that (perhaps t/README needs to be updated? It only mentions
three digits, but lists the template as tNNNN)

However, merge-tree has a function similar to diff (it shows differences
between trees) and apply (it creates the "result" object when there are
no conflicts), and its purpose seems to be more of a "shower" than a
"do-er" (contrasted with something like git-merge, which actually
creates a commit object, for example), so I thought that diff-commands
would be a good classification.

If you can tell me what number it should be, I'll change it.


-- 
-- Will

Re: [PATCHv2 1/2] add basic tests for merge-tree

[Junio C Hamano]

Will Palmer <wmpalmer@gmail.com> writes:

> However, merge-tree has a function similar to diff (it shows differences
> between trees) and apply (it creates the "result" object when there are
> no conflicts), and its purpose seems to be more of a "shower" than a
> "do-er" (contrasted with something like git-merge, which actually
> creates a commit object, for example), so I thought that diff-commands
> would be a good classification.

Ok, that makes sense (I haven't heard anybody use merge-tree in real life,
though).  Let's keep the numbering as-is.