Re: [BUG] fatal error during merge

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:

quoted

On Thu, Nov 13, 2008 at 06:06:52PM +0100, Anders Melchiorsen wrote:

quoted

SZEDER Gábor wrote:

quoted

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

Hi,

On Sun, 10 May 2009, Alex Riesen wrote:

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

On Sun, 10 May 2009 18:33:36 +0200, Alex Riesen [off-list ref] 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.

2009/5/11 Johannes Schindelin [off-list ref]:

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

Hi,

On Mon, 11 May 2009, Anders Melchiorsen wrote:

On Sun, 10 May 2009 18:33:36 +0200, Alex Riesen [off-list ref] wrote:

quoted

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

quoted

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