threads / bug / 16301

[BUG] fatal error during merge

Subject: [BUG] fatal error during merge

## tl;dr

14 messages between Nov 13, 2008 and Nov 14, 2008.

replies: 13people: 5as markdown or json

Anders Melchiorsen· Nov 13, 2008, 09:22 UTC · lore

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
Samuel Tardieu· Nov 13, 2008, 13:23 UTC · re: Anders Melchiorsen · lore

Re: [BUG] fatal error during merge

>>>>> "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/
SZEDER Gábor· Nov 13, 2008, 14:03 UTC · re: Samuel Tardieu · lore

Re: [BUG] fatal error during merge

Hi,
On Thu, Nov 13, 2008 at 02:23:19PM +0100, Samuel Tardieu wrote:
Show 7 quoted lines
> >>>>> "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

Anders Melchiorsen· Nov 13, 2008, 14:25 UTC · re: SZEDER Gábor · lore

Re: [BUG] fatal error during merge

SZEDER Gábor wrote:
Show 13 quoted lines
>
> 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
Samuel Tardieu· Nov 13, 2008, 14:26 UTC · re: SZEDER Gábor · lore

Re: [BUG] fatal error during merge

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

SZEDER Gábor· Nov 13, 2008, 14:53 UTC · re: Samuel Tardieu · lore

Re: [BUG] fatal error during merge

Hi,
On Thu, Nov 13, 2008 at 03:26:33PM +0100, Samuel Tardieu wrote:
Show 8 quoted lines
> * 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
Anders Melchiorsen· Nov 13, 2008, 17:06 UTC · re: SZEDER Gábor · lore

Re: [BUG] fatal error during merge

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.
SZEDER Gábor· Nov 13, 2008, 18:09 UTC · re: Anders Melchiorsen · lore

Re: [BUG] fatal error during merge

On Thu, Nov 13, 2008 at 06:06:52PM +0100, Anders Melchiorsen wrote:
Show 7 quoted lines
> 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
Anders Melchiorsen· Nov 13, 2008, 22:25 UTC · re: SZEDER Gábor · lore

Re: [BUG] fatal error during merge

SZEDER Gábor wrote:
Show 5 quoted lines
> 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.
Alex Riesen· Nov 13, 2008, 23:09 UTC · re: SZEDER Gábor · lore

Re: [BUG] fatal error during merge

SZEDER Gábor, Thu, Nov 13, 2008 19:09:31 +0100:
Show 14 quoted lines
> 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)
Anders Melchiorsen· Nov 13, 2008, 23:34 UTC · re: Alex Riesen · lore

Re: [BUG] fatal error during merge

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
Junio C Hamano· Nov 14, 2008, 00:29 UTC · re: Anders Melchiorsen · lore

Re: [BUG] fatal error during merge

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

Alex Riesen· Nov 14, 2008, 07:16 UTC · re: Junio C Hamano · lore

Re: [BUG] fatal error during merge

Junio C Hamano, Fri, Nov 14, 2008 01:29:34 +0100:
Show 7 quoted lines
> "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...
Anders Melchiorsen· Nov 14, 2008, 09:16 UTC · re: Alex Riesen · lore

Re: [BUG] fatal error during merge

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.

← back to recent threads