Re: git bug: moved file with local unstaged changes are lost during merge

[Clemens Buchacher <drizzd@aon.at>]

On Sun, Apr 15, 2012 at 01:15:17AM +0200, Clemens Buchacher wrote:
> --- a/t/t7607-merge-overwrite.sh
> +++ b/t/t7607-merge-overwrite.sh
> @@ -92,6 +92,15 @@ test_expect_success 'will not overwrite removed file with staged changes' '
>  	test_cmp important c1.c
>  '
>  
> +test_expect_failure 'will not overwrite unstaged changes in renamed file' '
> +	git reset --hard c1 &&
> +	git mv c1.c other.c &&
> +	git commit -m rename &&
> +	cp important other.c &&
> +	git merge c1a &&
> +	test_cmp important other.c
> +'

I have looked into this today, and we have a situation that is very
similar to c5ab03f2 (merge-recursive: do not clobber untracked working
tree garbage). The only difference being that the rename was made in our
branch, instead of the other. However, this does make it very difficult.

As you know, we have a fundamental issue in the way we protect untracked
or modified files from changes. Roughly, the procedure is as follows:

1. merge_trees -> threeway_merge

Go through each index entry and try to merge it.  Depending on the
outcome, do one of the following:

 a) If the entry has conflicts, or if it merges cleanly but the result
    is different from HEAD, check that index and work tree are not
    dirty, so that we can checkout the result of the merge without
    overwriting anything. Otherwise abort.

 b) If the entry has no changes, or if it merges cleanly and the result
    is the same as HEAD (or if it does not exist in HEAD), remove the
    CE_UPDATE flag from the entry so that we do not overwrite changes in
    the index or work tree, if any.

2. merge_trees -> process_entry

Find possible rename pairs and try to merge renames. Due to the renames,
entries that were classified as b) above can now become of type a).

If the rename was in the other branch, the entry is new and was
categorized as b). Now we realize it's a modified rename, and it should
have been categorized as a) above.  Untracked files used to be
overridden here. But c5ab03f2 added a last minute safety valve, which
checks that a renamed entry either exists in HEAD, or does not exist in
the work tree. This catches at least the typical case.

If the rename was in our branch, the situation is slightly different.
The entry exists in HEAD, but it is still categorized as b). Due to the
modified rename, we later decide to update the work tree after all.  In
this case, the safety valve above does not help, because the entry was
tracked, but at this point it is too late to tell if the entry was
up-to-date. And so it is overwritten.

One possible fix is to abort in case of trivial merges that would have
been classified as b) above. The reationale is that we have to assume
that the entry could get modified due to a rename later. But that breaks
many test cases where we currently assume that it is ok to have a dirty
work tree during trivial merges. (I did not run all tests.)

Maybe we should disable merging with a dirty work tree altogether, until
we have a solution that is safe to use?

Clemens

-->8--
Subject: [PATCH] trivial merge: always verify that working tree is clean

Even if the index and the merge result match at this stage, if the entry
turns out to be a renamed file, it may still be modified. Make sure that
the work tree is up-to-date, in order to avoid overwriting data.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 t/t1000-read-tree-m-3way.sh |   14 +++++++-------
 t/t1004-read-tree-m-u-wf.sh |    2 +-
 t/t3903-stash.sh            |    2 +-
 t/t7607-merge-overwrite.sh  |    4 ++--
 unpack-trees.c              |    5 +++--
 5 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/t/t1000-read-tree-m-3way.sh b/t/t1000-read-tree-m-3way.sh
index babcdd2..eb03339 100755
--- a/t/t1000-read-tree-m-3way.sh
+++ b/t/t1000-read-tree-m-3way.sh
@@ -222,7 +222,7 @@ test_expect_success \
      git update-index --add NA &&
      read_tree_must_succeed -m $tree_O $tree_A $tree_B"
 
-test_expect_success \
+test_expect_failure \
     '2 - matching B alone is OK in !O && !A && B case.' \
     "rm -f .git/index NA &&
      cp .orig-B/NA NA &&
@@ -238,7 +238,7 @@ test_expect_success \
      read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
      check_result"
 
-test_expect_success \
+test_expect_failure \
     '3 - matching A alone is OK in !O && A && !B case.' \
     "rm -f .git/index AN &&
      cp .orig-A/AN AN &&
@@ -289,7 +289,7 @@ test_expect_success \
      read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
      check_result"
 
-test_expect_success \
+test_expect_failure \
     '5 - must match in !O && A && B && A==B case.' \
     "rm -f .git/index LL &&
      cp .orig-A/LL LL &&
@@ -417,7 +417,7 @@ test_expect_success \
      read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
      check_result"
 
-test_expect_success \
+test_expect_failure \
     '12 - must match A in O && A && B && O!=A && A==B case' \
     "rm -f .git/index SS &&
      cp .orig-A/SS SS &&
@@ -443,7 +443,7 @@ test_expect_success \
      read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
      check_result"
 
-test_expect_success \
+test_expect_failure \
     '13 - must match A in O && A && B && O!=A && O==B case' \
     "rm -f .git/index MN &&
      cp .orig-A/MN MN &&
@@ -460,7 +460,7 @@ test_expect_success \
      read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
      check_result"
 
-test_expect_success \
+test_expect_failure \
     '14 - may match B in O && A && B && O==A && O!=B case' \
     "rm -f .git/index NM &&
      cp .orig-B/NM NM &&
@@ -495,7 +495,7 @@ test_expect_success \
      read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
      check_result"
 
-test_expect_success \
+test_expect_failure \
     '15 - must match A in O && A && B && O==A && O==B case' \
     "rm -f .git/index NN &&
      cp .orig-A/NN NN &&
diff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh
index b3ae7d5..5002af2 100755
--- a/t/t1004-read-tree-m-u-wf.sh
+++ b/t/t1004-read-tree-m-u-wf.sh
@@ -130,7 +130,7 @@ test_expect_success '3-way not overwriting local changes (setup)' '
 
 '
 
-test_expect_success '3-way not overwriting local changes (our side)' '
+test_expect_failure '3-way not overwriting local changes (our side)' '
 
 	# At this point, file1 from side-a should be kept as side-b
 	# did not touch it.
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 3addb80..5f54c30 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -43,7 +43,7 @@ test_expect_success 'applying bogus stash does nothing' '
 	test_cmp expect file
 '
 
-test_expect_success 'apply does not need clean working directory' '
+test_expect_failure 'apply does not need clean working directory' '
 	echo 4 >other-file &&
 	git add other-file &&
 	echo 5 >other-file &&
diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh
index 6547eb8..72f84b8 100755
--- a/t/t7607-merge-overwrite.sh
+++ b/t/t7607-merge-overwrite.sh
@@ -92,12 +92,12 @@ test_expect_success 'will not overwrite removed file with staged changes' '
 	test_cmp important c1.c
 '
 
-test_expect_failure 'will not overwrite unstaged changes in renamed file' '
+test_expect_success 'will not overwrite unstaged changes in renamed file' '
 	git reset --hard c1 &&
 	git mv c1.c other.c &&
 	git commit -m rename &&
 	cp important other.c &&
-	git merge c1a &&
+	test_must_fail git merge c1a &&
 	test_cmp important other.c
 '
 
diff --git a/unpack-trees.c b/unpack-trees.c
index 36523da..dcaef43 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1480,6 +1480,9 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 			return -1;
 		invalidate_ce_path(merge, o);
 	} else if (!(old->ce_flags & CE_CONFLICTED)) {
+		if (verify_uptodate(old, o))
+			return -1;
+
 		/*
 		 * See if we can re-use the old CE directly?
 		 * That way we get the uptodate stat info.
@@ -1491,8 +1494,6 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 			copy_cache_entry(merge, old);
 			update = 0;
 		} else {
-			if (verify_uptodate(old, o))
-				return -1;
 			/* Migrate old flags over */
 			update |= old->ce_flags & (CE_SKIP_WORKTREE | CE_NEW_SKIP_WORKTREE);
 			invalidate_ce_path(old, o);
-- 
1.7.10