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

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

[Joe Angell]

What steps will reproduce the problem?
git init
echo "initial checkin" >> readme
git add readme
git commit -m "inital checkin"
git branch b1
git checkout b1
echo "b1" >> readme
git add readme
git commit -m "b1 readme"
git checkout master
git mv readme readme_master
git ci -m "moved readme"
echo "master" >> readme_master
git merge b1

What is the expected output? What do you see instead?
I expect to have git prevent the merge due to local changes to the
file.  Instead it overwrites the file (erasing the local modification
"master") and you end up with:
cat readme_master
initial readme
b1

What version of the product are you using? On what operating system?
Reproduced on 1.7.9.6 and from the git-core repo 1.7.10.128.g7945c.
This is on ubuntu 10.04.

Please provide any additional information below.

This problem only seems to occur after you check in the move, then
make local modifications, then do the merge.

-- 
---------------
Joe Angell
cell: (720) 260-2190

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

[Joe Angell]

Is this the right place to post bug reports?



On Wed, Apr 11, 2012 at 12:20 PM, Joe Angell <joe.d.angell@gmail.com> wrote:
> What steps will reproduce the problem?
> git init
> echo "initial checkin" >> readme
> git add readme
> git commit -m "inital checkin"
> git branch b1
> git checkout b1
> echo "b1" >> readme
> git add readme
> git commit -m "b1 readme"
> git checkout master
> git mv readme readme_master
> git ci -m "moved readme"
> echo "master" >> readme_master
> git merge b1
>
> What is the expected output? What do you see instead?
> I expect to have git prevent the merge due to local changes to the
> file.  Instead it overwrites the file (erasing the local modification
> "master") and you end up with:
> cat readme_master
> initial readme
> b1
>
> What version of the product are you using? On what operating system?
> Reproduced on 1.7.9.6 and from the git-core repo 1.7.10.128.g7945c.
> This is on ubuntu 10.04.
>
> Please provide any additional information below.
>
> This problem only seems to occur after you check in the move, then
> make local modifications, then do the merge.
>
> --
> ---------------
> Joe Angell
> cell: (720) 260-2190



-- 
---------------
Joe Angell
cell: (720) 260-2190

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

[Jeff King]

On Thu, Apr 12, 2012 at 10:13:04AM -0600, Joe Angell wrote:

> Is this the right place to post bug reports?

It is. Thanks for including a concise test case with your bug report.

Unfortunately, the merge-recursive code is a mess, and has several known
buggy corner cases with renames. Elijah (cc'd) spent a lot of time
trying to sort these out a while ago, but there still some known
failures. t6042 and t6036 detect some of them. But I thought we managed
to clean up all of the overwriting bugs.

Original bug report is below.

-Peff

> On Wed, Apr 11, 2012 at 12:20 PM, Joe Angell <joe.d.angell@gmail.com> wrote:
> > What steps will reproduce the problem?
> > git init
> > echo "initial checkin" >> readme
> > git add readme
> > git commit -m "inital checkin"
> > git branch b1
> > git checkout b1
> > echo "b1" >> readme
> > git add readme
> > git commit -m "b1 readme"
> > git checkout master
> > git mv readme readme_master
> > git ci -m "moved readme"
> > echo "master" >> readme_master
> > git merge b1
> >
> > What is the expected output? What do you see instead?
> > I expect to have git prevent the merge due to local changes to the
> > file.  Instead it overwrites the file (erasing the local modification
> > "master") and you end up with:
> > cat readme_master
> > initial readme
> > b1
> >
> > What version of the product are you using? On what operating system?
> > Reproduced on 1.7.9.6 and from the git-core repo 1.7.10.128.g7945c.
> > This is on ubuntu 10.04.
> >
> > Please provide any additional information below.
> >
> > This problem only seems to occur after you check in the move, then
> > make local modifications, then do the merge.
> >
> > --
> > ---------------
> > Joe Angell
> > cell: (720) 260-2190

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

[Clemens Buchacher]

On Fri, Apr 13, 2012 at 02:49:41AM -0400, Jeff King wrote:
> 
> t6042 and t6036 detect some of them. But I thought we managed to clean
> up all of the overwriting bugs.

I could not find an existing test for this.

-->o--
Subject: [PATCH] merge overwrites unstaged changes in renamed file


Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 t/t7607-merge-overwrite.sh |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh
index aa74184..6547eb8 100755
--- 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
+'
+
 test_expect_success 'will not overwrite untracked subtree' '
 	git reset --hard c0 &&
 	rm -rf sub &&
-- 
1.7.9.6

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

[Clemens Buchacher]

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

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

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> 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).

Yes, unfortunately the rename-processing part of merge-recursive is known
to be broken wrt things like this.  If you want to punt,

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

this is not the way to go, as "resolve" is perfectly safe.  It would make
more sense to disable the rename-processing part of recursive by default,
until it gets cleaned up further.