threads / bug / 19273

Re: [BUG] fatal error during merge

Subject: Re: [BUG] fatal error during merge

## tl;dr

5 messages between May 10, 2009 and May 11, 2009.

replies: 4people: 3as markdown or json

Alex Riesen· May 10, 2009, 16:33 UTC · lore

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:

Show 27 quoted lines

> 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

Johannes Schindelin· May 10, 2009, 23:10 UTC · re: Alex Riesen · lore

Hi,

On Sun, 10 May 2009, Alex Riesen wrote:

Show 20 quoted lines

> 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

Alex Riesen· May 11, 2009, 07:39 UTC · re: Johannes Schindelin · lore

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

Anders Melchiorsen· May 11, 2009, 07:37 UTC · re: Alex Riesen · lore

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.

Johannes Schindelin· May 11, 2009, 08:34 UTC · re: Anders Melchiorsen · lore

Hi,

On Mon, 11 May 2009, Anders Melchiorsen wrote:

Show 16 quoted lines

> 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