[PATCH/Test] Build in merge is broken

[PATCH/Test] Build in merge is broken

[Sverre Hvammen Johansen]

This test case shows breakage of build in merge.  This have been
bisected to 1c7b76be Build in merge.

---
Great that we now are introducing merge in C.  Great job Miklos.
However, it is broken as this patch shows.  This have been
bisected to 1c7b76be Build in merge.

The test case when run will record the parents that were asked for which is
fine.  However, only c2 is recorded as a parent in the commit object.  Both
c1 and c2 should have been recorded.  The merge is otherwise working
correctly.

 t/t7600-merge.sh |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 16f4608..a96a497 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -465,4 +465,15 @@ test_expect_success 'merge log message' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge c1 with c0, c2, c0, and c1' '
+       git reset --hard c1 &&
+       git config branch.master.mergeoptions "" &&
+       test_tick &&
+       git merge c0 c2 c0 c1 &&
+       verify_merge file result.1-5 &&
+       verify_parents $c1 $c2
+'
+
+test_debug 'gitk --all'
+
 test_done
-- 
1.5.5.54.gc6550

Re: [PATCH/Test] Build in merge is broken

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

On Sun, Jul 13, 2008 at 08:13:55AM +0000, Sverre Hvammen Johansen <hvammen+git@gmail.com> wrote:
> Great that we now are introducing merge in C.  Great job Miklos.
> However, it is broken as this patch shows.  This have been
> bisected to 1c7b76be Build in merge.
> 
> The test case when run will record the parents that were asked for which is
> fine.  However, only c2 is recorded as a parent in the commit object.  Both
> c1 and c2 should have been recorded.  The merge is otherwise working
> correctly.

Thanks for the testcase, I'm on it. ;-)

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH/Test] Build in merge is broken

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 709 bytes --]

On Sun, Jul 13, 2008 at 02:41:00PM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:
> > The test case when run will record the parents that were asked for which is
> > fine.  However, only c2 is recorded as a parent in the commit object.  Both
> > c1 and c2 should have been recorded.  The merge is otherwise working
> > correctly.
> 
> Thanks for the testcase, I'm on it. ;-)

So far what I see is that the input for the reduce_heads() function is
(c1, c0, c2, c0, c1). The expected output would be (c1, c2), but the
actual output is c2. So I suspect the bug is not in builtin-merge.c
itself but in reduce_heads().

Hmm..

Adding Junio to Cc, who is the original author of reduce_heads().

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH/Test] Build in merge is broken

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]

On Sun, Jul 13, 2008 at 07:46:59PM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:
> So far what I see is that the input for the reduce_heads() function is
> (c1, c0, c2, c0, c1). The expected output would be (c1, c2), but the
> actual output is c2. So I suspect the bug is not in builtin-merge.c
> itself but in reduce_heads().

This fixes the problem for me. Junio, does the fix looks correct to you
as well?

Thanks.

diff --git a/commit.c b/commit.c
index d20b14e..03e73f3 100644
--- a/commit.c
+++ b/commit.c
@@ -747,7 +747,7 @@ struct commit_list *reduce_heads(struct commit_list *heads)
 
 		num_other = 0;
 		for (q = heads; q; q = q->next) {
-			if (p == q)
+			if (p->item == q->item)
 				continue;
 			other[num_other++] = q->item;
 		}

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH/Test] Build in merge is broken

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> On Sun, Jul 13, 2008 at 07:46:59PM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:
>> So far what I see is that the input for the reduce_heads() function is
>> (c1, c0, c2, c0, c1). The expected output would be (c1, c2), but the
>> actual output is c2. So I suspect the bug is not in builtin-merge.c
>> itself but in reduce_heads().
>
> This fixes the problem for me. Junio, does the fix looks correct to you
> as well?

You are correct, the "item"s are the highlander (i.e. "there can be only
one") objects but commit-list elements that hold pointers to them are not,
so we need to dereference and compare.

Thanks.

Re: [PATCH/Test] Build in merge is broken

[Sverre Hvammen Johansen]

Test cases for build in merge.
---
After applying Miklos's fix there still are some breakages.  I have squashed in
another test case.  c1 is merged with c1 and c2.  Three parents are
recorded in the
merge commit object; c1, c1, and c2.

 t/t7600-merge.sh |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 16f4608..80cfee6 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -465,4 +465,26 @@ test_expect_success 'merge log message' '

 test_debug 'gitk --all'

+test_expect_success 'merge c1 with c0, c2, c0, and c1' '
+       git reset --hard c1 &&
+       git config branch.master.mergeoptions "" &&
+       test_tick &&
+       git merge c0 c2 c0 c1 &&
+       verify_merge file result.1-5 &&
+       verify_parents $c1 $c2
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c1 with c1 and c2' '
+       git reset --hard c1 &&
+       git config branch.master.mergeoptions "" &&
+       test_tick &&
+       git merge c1 c2 &&
+       verify_merge file result.1-5 &&
+       verify_parents $c1 $c2
+'
+
+test_debug 'gitk --all'
+
 test_done
-- 
1.5.5.54.gc6550

-- 
Sverre Hvammen Johansen

Re: [PATCH/Test] Build in merge is broken

[Junio C Hamano]

"Sverre Hvammen Johansen" <hvammen@gmail.com> writes:

> Test cases for build in merge.
> ---

Thanks.

Obviously this is not for application but to help Miklos and others to
help fixing the remaining issues.

Tonight's pu won't have this but that is only because I am currently deep
in today's integration session already.