[BUG] fatal error during merge

[BUG] fatal error during merge

[Anders Melchiorsen]

It took a while to make a minimal test case for this, but I think that all
the below steps are required. It ends up with this message:

after/one: unmerged (e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)
fatal: git write-tree failed to write a tree

and the file that was moved has disappeared from the working tree.

I have tested the script with Git 1.6.0.2, but the real scenario that made
this appear seems to also fail with master and next from git.git.

Cheers,
Anders.


Test script:

mkdir am-merge-fail
cd am-merge-fail
git init

mkdir before
touch before/one after
git add -A
git commit -minitial

rm -f after
git mv before after
git commit -mmove

git checkout -b parallel HEAD~
touch another
git add -A
git commit -mparallel

git merge master

Re: [BUG] fatal error during merge

[Samuel Tardieu]

>>>>> "Anders" == Anders Melchiorsen <mail@cup.kalibalik.dk> writes:

Anders> I have tested the script with Git 1.6.0.2, but the real
Anders> scenario that made this appear seems to also fail with master
Anders> and next from git.git.

I confirm that your test case also fails with the current "next".

  Sam
-- 
Samuel Tardieu -- sam@rfc1149.net -- http://www.rfc1149.net/

Re: [BUG] fatal error during merge

[SZEDER Gábor]

Hi,

On Thu, Nov 13, 2008 at 02:23:19PM +0100, Samuel Tardieu wrote:
> >>>>> "Anders" == Anders Melchiorsen <mail@cup.kalibalik.dk> writes:
> 
> Anders> I have tested the script with Git 1.6.0.2, but the real
> Anders> scenario that made this appear seems to also fail with master
> Anders> and next from git.git.
> 
> I confirm that your test case also fails with the current "next".

Yeah, and it can be bisected to commit 621ff675 (rev-parse: fix
meaning of rev~ vs rev~0, 2008-03-14), which is from Linus, so put him
on Cc.


Best,
Gábor

Re: [BUG] fatal error during merge

[Anders Melchiorsen]

SZEDER Gábor wrote:
>
> On Thu, Nov 13, 2008 at 02:23:19PM +0100, Samuel Tardieu wrote:
>> >>>>> "Anders" == Anders Melchiorsen <mail@cup.kalibalik.dk> writes:
>>
>> Anders> I have tested the script with Git 1.6.0.2, but the real
>> Anders> scenario that made this appear seems to also fail with master
>> Anders> and next from git.git.
>>
>> I confirm that your test case also fails with the current "next".
>
> Yeah, and it can be bisected to commit 621ff675 (rev-parse: fix
> meaning of rev~ vs rev~0, 2008-03-14), which is from Linus, so put him
> on Cc.

I guess that is due to using HEAD~ in the test script? If you are
bisecting, here is a version that does not use the HEAD~ notation:


mkdir am-merge-fail
cd am-merge-fail
git init

mkdir before
touch before/one after
git add before after
git commit -minitial

git branch parallel

rm -f after
git mv before after
git commit -mmove

git checkout parallel
touch other
git add other
git commit -mparallel

git merge master

Re: [BUG] fatal error during merge

[Samuel Tardieu]

* SZEDER Gábor <szeder@ira.uka.de> [2008-11-13 15:03:23 +0100]

| On Thu, Nov 13, 2008 at 02:23:19PM +0100, Samuel Tardieu wrote:
| > >>>>> "Anders" == Anders Melchiorsen <mail@cup.kalibalik.dk> writes:
| > 
| > Anders> I have tested the script with Git 1.6.0.2, but the real
| > Anders> scenario that made this appear seems to also fail with master
| > Anders> and next from git.git.
| > 
| > I confirm that your test case also fails with the current "next".
| 
| Yeah, and it can be bisected to commit 621ff675 (rev-parse: fix
| meaning of rev~ vs rev~0, 2008-03-14), which is from Linus, so put him
| on Cc.

I think your pinpointed a change of behaviour in "HEAD~", which is
probably not the problem at hand. To find the real bug, you should
update the test script so that it uses "HEAD~1" instead of "HEAD~".

Re: [BUG] fatal error during merge

[SZEDER Gábor]

Hi,

On Thu, Nov 13, 2008 at 03:26:33PM +0100, Samuel Tardieu wrote:
> * SZEDER Gábor <szeder@ira.uka.de> [2008-11-13 15:03:23 +0100]
> | Yeah, and it can be bisected to commit 621ff675 (rev-parse: fix
> | meaning of rev~ vs rev~0, 2008-03-14), which is from Linus, so put him
> | on Cc.
> 
> I think your pinpointed a change of behaviour in "HEAD~", which is
> probably not the problem at hand.  To find the real bug, you should
> update the test script so that it uses "HEAD~1" instead of "HEAD~".

It doesn't matter.  The test script errors out at the merge, and not
at the checkout.  Furthermore, it doesn't matter, whether HEAD~,
HEAD~, or HEAD^ is checked out, the results are the same.

But yes, it's possible that not the bisected commit is the culprit,
but something in or behind merge relies on the old undocumented and
illogical behaviour.

Gábor

Re: [BUG] fatal error during merge

[Anders Melchiorsen]

SZEDER Gábor wrote:

> It doesn't matter.  The test script errors out at the merge, and not
> at the checkout.  Furthermore, it doesn't matter, whether HEAD~,
> HEAD~, or HEAD^ is checked out, the results are the same.

Just to be sure, I tried reverting the commit that you bisected -- and my
test case still fails.


Anders.

Re: [BUG] fatal error during merge

[SZEDER Gábor]

On Thu, Nov 13, 2008 at 06:06:52PM +0100, Anders Melchiorsen wrote:
> SZEDER Gábor wrote:
> > It doesn't matter.  The test script errors out at the merge, and not
> > at the checkout.  Furthermore, it doesn't matter, whether HEAD~,
> > HEAD~, or HEAD^ is checked out, the results are the same.
> 
> Just to be sure, I tried reverting the commit that you bisected -- and my
> test case still fails.

Well, oddly enough, your second test case behaves somewhat differently
than the first one, at least as far as bisect is concerned.  Bisect
nails down the second test case to 0d5e6c97 (Ignore merged status of
the file-level merge, 2007-04-26; put Alex on Cc).  Reverting this
commit on master makes both of your test cases pass.

Gábor

Re: [BUG] fatal error during merge

[Anders Melchiorsen]

SZEDER Gábor wrote:

> Well, oddly enough, your second test case behaves somewhat differently
> than the first one, at least as far as bisect is concerned.  Bisect
> nails down the second test case to 0d5e6c97 (Ignore merged status of
> the file-level merge, 2007-04-26; put Alex on Cc).  Reverting this
> commit on master makes both of your test cases pass.

Hmm. With that one reverted, git prints out this error instead:

error: failed to create path 'after/one': perhaps a D/F conflict?

and still has the moved file disapper in the working tree. The merge is
successful, though, and the moved file is in the committed tree.


Anders.

Re: [BUG] fatal error during merge

[Alex Riesen]

SZEDER Gábor, Thu, Nov 13, 2008 19:09:31 +0100:
> On Thu, Nov 13, 2008 at 06:06:52PM +0100, Anders Melchiorsen wrote:
> > SZEDER Gábor wrote:
> > > It doesn't matter.  The test script errors out at the merge, and not
> > > at the checkout.  Furthermore, it doesn't matter, whether HEAD~,
> > > HEAD~, or HEAD^ is checked out, the results are the same.
> > 
> > Just to be sure, I tried reverting the commit that you bisected -- and my
> > test case still fails.
> 
> Well, oddly enough, your second test case behaves somewhat differently
> than the first one, at least as far as bisect is concerned.  Bisect
> nails down the second test case to 0d5e6c97 (Ignore merged status of
> the file-level merge, 2007-04-26; put Alex on Cc).  Reverting this
> commit on master makes both of your test cases pass.

Well, the case is a bit unfair: all files have the same SHA-1!

Whatever, the code pointed by the commit you bisected does look like a
problem: it does not update the index after refusing to rewrite the
worktree file (because its SHA-1 matches the SHA-1 of the data it
would be rewritten with. So updating the file would be a no-op, just
wasted effort). Instead of reverting the commit, I suggest the
attached patch. It is a long time ago since I looked at the code
(and it is a mess, which I'm feeling a bit ashamed of), so another
lot of reviewing eyeglasses is definitely in order.

From c395f4234ca5492206923e1821a316a777c651cd Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Thu, 13 Nov 2008 23:55:04 +0100
Subject: [PATCH] Update index after refusing to rewrite unchanged files

Specifically, which were not changed during recursive merge.
Otherwise the path can stay marked as unresolved in the index,
causing the merge to fail.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>

diff --git a/merge-recursive.c b/merge-recursive.c
index 7472d3e..28f9e12 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -944,14 +944,15 @@ static int process_renames(struct merge_options *o,
 
 				if (mfi.clean &&
 				    sha_eq(mfi.sha, ren1->pair->two->sha1) &&
-				    mfi.mode == ren1->pair->two->mode)
+				    mfi.mode == ren1->pair->two->mode) {
 					/*
 					 * This messaged is part of
 					 * t6022 test. If you change
 					 * it update the test too.
 					 */
 					output(o, 3, "Skipped %s (merged same as existing)", ren1_dst);
-				else {
+					add_cacheinfo(mfi.mode, mfi.sha, ren1_dst, 0, 0, ADD_CACHE_OK_TO_ADD);
+				} else {
 					if (mfi.merge || !mfi.clean)
 						output(o, 1, "Renaming %s => %s", ren1_src, ren1_dst);
 					if (mfi.merge)

Re: [BUG] fatal error during merge

[Anders Melchiorsen]

Alex Riesen wrote:

> Well, the case is a bit unfair: all files have the same SHA-1!

Here is an updated test, where all files have different SHA-1. It still
fails.

Your patch got rid of the errors and it commits the merged tree. But the
working tree is not updated correctly, so the moved file disappears.


Cheers,
Anders.


mkdir am-merge-fail
cd am-merge-fail
git init

mkdir before
yes | head -n100 >before/100
touch after
git add before/100 after
git commit -minitial
git branch parallel

git rm before/100 after
mkdir after
yes | head -n99 >after/99
git add after/99
git commit -mmove

git checkout parallel
echo "Just to cause a non-ff" >other
git add other
git commit -mparallel

git merge master

Re: [BUG] fatal error during merge

[Junio C Hamano]

"Anders Melchiorsen" <mail@cup.kalibalik.dk> writes:

> Your patch got rid of the errors and it commits the merged tree. But the
> working tree is not updated correctly, so the moved file disappears.

Isn't this a long known breakage of D/F conflict check logic in
merge-recursive backend?

Re: [BUG] fatal error during merge

[Alex Riesen]

Junio C Hamano, Fri, Nov 14, 2008 01:29:34 +0100:
> "Anders Melchiorsen" <mail@cup.kalibalik.dk> writes:
> 
> > Your patch got rid of the errors and it commits the merged tree. But the
> > working tree is not updated correctly, so the moved file disappears.
> 
> Isn't this a long known breakage of D/F conflict check logic in
> merge-recursive backend?

Could be, but there is no conflicting entries in the merged branches.
All file names are different and the D/F transition happens completely
on one branch.

And the patch isn't enough: the file Anders noticed missing is in the
index (correctly) but somehow removed from worktree.  Which is maybe
because it _still_ was a rename: 99% similarity. Well, just
"yes | tail -nNN" usually producess very similar data :)

Still broken, of course...

Re: [BUG] fatal error during merge

[Anders Melchiorsen]

Alex Riesen wrote:

> And the patch isn't enough: the file Anders noticed missing is in the
> index (correctly) but somehow removed from worktree.  Which is maybe
> because it _still_ was a rename: 99% similarity. Well, just
> "yes | tail -nNN" usually producess very similar data :)

Maybe reducing the example all the way down to "touch" lost the implicit
information that, indeed, it has to be a rename for this to fail.


Anders.

Re: [BUG] fatal error during merge

[Alex Riesen]

I still have the patch below (rebased) in my tree.
Was the problem fixed somehow differently?

Alex Riesen, Fri, Nov 14, 2008 00:09:32 +0100:
> SZEDER Gábor, Thu, Nov 13, 2008 19:09:31 +0100:
> > On Thu, Nov 13, 2008 at 06:06:52PM +0100, Anders Melchiorsen wrote:
> > > SZEDER Gábor wrote:
> > > > It doesn't matter.  The test script errors out at the merge, and not
> > > > at the checkout.  Furthermore, it doesn't matter, whether HEAD~,
> > > > HEAD~, or HEAD^ is checked out, the results are the same.
> > > 
> > > Just to be sure, I tried reverting the commit that you bisected -- and my
> > > test case still fails.
> > 
> > Well, oddly enough, your second test case behaves somewhat differently
> > than the first one, at least as far as bisect is concerned.  Bisect
> > nails down the second test case to 0d5e6c97 (Ignore merged status of
> > the file-level merge, 2007-04-26; put Alex on Cc).  Reverting this
> > commit on master makes both of your test cases pass.
> 
> Well, the case is a bit unfair: all files have the same SHA-1!
> 
> Whatever, the code pointed by the commit you bisected does look like a
> problem: it does not update the index after refusing to rewrite the
> worktree file (because its SHA-1 matches the SHA-1 of the data it
> would be rewritten with. So updating the file would be a no-op, just
> wasted effort). Instead of reverting the commit, I suggest the
> attached patch. It is a long time ago since I looked at the code
> (and it is a mess, which I'm feeling a bit ashamed of), so another
> lot of reviewing eyeglasses is definitely in order.
> 

From f8eb1a64251b3d4ce080c5aaa7240b209a1b5257 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Thu, 13 Nov 2008 23:55:04 +0100
Subject: [PATCH] Update index after refusing to rewrite files unchanged during merge

Otherwise the path can stay marked as unresolved in the index,
causing the merge to fail.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 merge-recursive.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index a3721ef..d5c43d1 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -980,14 +980,15 @@ static int process_renames(struct merge_options *o,
 
 				if (mfi.clean &&
 				    sha_eq(mfi.sha, ren1->pair->two->sha1) &&
-				    mfi.mode == ren1->pair->two->mode)
+				    mfi.mode == ren1->pair->two->mode) {
 					/*
 					 * This messaged is part of
 					 * t6022 test. If you change
 					 * it update the test too.
 					 */
 					output(o, 3, "Skipped %s (merged same as existing)", ren1_dst);
-				else {
+					add_cacheinfo(mfi.mode, mfi.sha, ren1_dst, 0, 0, ADD_CACHE_OK_TO_ADD);
+				} else {
 					if (mfi.merge || !mfi.clean)
 						output(o, 1, "Renaming %s => %s", ren1_src, ren1_dst);
 					if (mfi.merge)
-- 
1.6.3.28.ga852b

Re: [BUG] fatal error during merge

[Johannes Schindelin]

Hi,

On Sun, 10 May 2009, Alex Riesen wrote:

> diff --git a/merge-recursive.c b/merge-recursive.c
> index a3721ef..d5c43d1 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -980,14 +980,15 @@ static int process_renames(struct merge_options *o,
>  
>  				if (mfi.clean &&
>  				    sha_eq(mfi.sha, ren1->pair->two->sha1) &&
> -				    mfi.mode == ren1->pair->two->mode)
> +				    mfi.mode == ren1->pair->two->mode) {
>  					/*
>  					 * This messaged is part of
>  					 * t6022 test. If you change
>  					 * it update the test too.
>  					 */
>  					output(o, 3, "Skipped %s (merged same as existing)", ren1_dst);
> -				else {
> +					add_cacheinfo(mfi.mode, mfi.sha, ren1_dst, 0, 0, ADD_CACHE_OK_TO_ADD);
> +				} else {
>  					if (mfi.merge || !mfi.clean)

If I read the message right, the file revision is supposed not to be 
changed from HEAD.  Is unpack_trees() invalidating the "cleanness" of that 
file?  (I would really love to have a better idea what's going on than 
what I get from both the commit message and the patch before giving my 
ACK.)

Ciao,
Dscho

Re: [BUG] fatal error during merge

[Anders Melchiorsen]

On Sun, 10 May 2009 18:33:36 +0200, Alex Riesen <raa.lkml@gmail.com> wrote:

> I still have the patch below (rebased) in my tree.
> Was the problem fixed somehow differently?

> Subject: [PATCH] Update index after refusing to rewrite files unchanged
> during merge

I tested recently, and it does not appear to be fixed yet.

However, your patch was not enough to fix my test case completely,
so I am unsure whether it makes sense to apply it as a partial fix.

The test is here:

   http://article.gmane.org/gmane.comp.version-control.git/116999


Anders.

Re: [BUG] fatal error during merge

[Alex Riesen]

2009/5/11 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>
> If I read the message right, the file revision is supposed not to be
> changed from HEAD.  Is unpack_trees() invalidating the "cleanness" of that
> file?

I think it is the D/F (or F/D?) conflict. A file in one branch is renamed into
a directory. The script in the original post still works (err... fails).

>  (I would really love to have a better idea what's going on than
> what I get from both the commit message and the patch before giving my
> ACK.)

Ah, scrap that. The patch is no good, and does not fix the original problem
at all. In fact, it makes it even worse - hides the problem by removing conflict
information from the index and _deletes_ the problematic file.
That's why it wasn't included - the brokenness was noticed.
Pity that then I run out of time, too.

The script to reproduce (note GIT_EXEC_PATH!):

#!/bin/sh

rm -rf merge-rename-fail
mkdir merge-rename-fail || exit
cd merge-rename-fail || exit
export GIT_MERGE_VERBOSITY=5
export GIT_EXEC_PATH=$HOME/projects/git
$GIT_EXEC_PATH/git init
mkdir before
echo FILE >before/one
echo FILE >after
$GIT_EXEC_PATH/git add .
$GIT_EXEC_PATH/git commit -mfirst

rm -f after
$GIT_EXEC_PATH/git mv before after
$GIT_EXEC_PATH/git commit -mmove

$GIT_EXEC_PATH/git checkout -b para HEAD^
echo COMPLETELY ANOTHER FILE >another
$GIT_EXEC_PATH/git add .
$GIT_EXEC_PATH/git commit -mpara

echo '***
*** MERGE ***
***'
echo export GIT_EXEC_PATH=$GIT_EXEC_PATH
echo $GIT_EXEC_PATH/git merge master

Re: [BUG] fatal error during merge

[Johannes Schindelin]

Hi,

On Mon, 11 May 2009, Anders Melchiorsen wrote:

> On Sun, 10 May 2009 18:33:36 +0200, Alex Riesen <raa.lkml@gmail.com> wrote:
> 
> > I still have the patch below (rebased) in my tree.
> > Was the problem fixed somehow differently?
> 
> > Subject: [PATCH] Update index after refusing to rewrite files unchanged
> > during merge
> 
> I tested recently, and it does not appear to be fixed yet.
> 
> However, your patch was not enough to fix my test case completely,
> so I am unsure whether it makes sense to apply it as a partial fix.
> 
> The test is here:
> 
>    http://article.gmane.org/gmane.comp.version-control.git/116999

Maybe you can turn this into a patch adding a test (with 
test_expect_failure to mark it as a bug)?  This would make debugging a lot 
easier, as a non-installed Git could be tested.

Ciao,
Dscho