threads / patch / 14436

patchBuild in merge is broken

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

## tl;dr

7 messages between Jul 13, 2008 and Jul 14, 2008. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Sverre Hvammen Johansen· Jul 13, 2008, 08:13 UTC · lore

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(-)

Show changes to t/t7600-merge.sh +11 −0

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

Miklos Vajna· Jul 13, 2008, 12:41 UTC · re: Sverre Hvammen Johansen · lore

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

On Sun, Jul 13, 2008 at 08:13:55AM +0000, Sverre Hvammen Johansen <hvammen+git@gmail.com> wrote:

Show 8 quoted lines

> 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. ;-)

Miklos Vajna· Jul 13, 2008, 17:46 UTC · re: Miklos Vajna · lore

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

On Sun, Jul 13, 2008 at 02:41:00PM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:

Show 6 quoted lines

> > 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().

Miklos Vajna· Jul 13, 2008, 18:43 UTC · re: Miklos Vajna · lore

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

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.

Show changes to commit.c +1 −1

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;
 		}

Junio C Hamano· Jul 13, 2008, 19:11 UTC · re: Miklos Vajna · lore

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

Miklos Vajna <vmiklos@frugalware.org> writes:

Show 8 quoted lines

> 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.

Sverre Hvammen Johansen· Jul 14, 2008, 02:15 UTC · re: Miklos Vajna · lore

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

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(-)

Show changes to t/t7600-merge.sh +22 −0

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

Junio C Hamano· Jul 14, 2008, 02:53 UTC · re: Sverre Hvammen Johansen · lore

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

"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.