{"thread":{"id":"30215","subject":"git bug: moved file with local unstaged changes are lost during merge","startedAt":"2012-04-11T18:20:34Z","lastAt":"2012-04-30T00:16:18Z","messageCount":6,"participants":["Joe Angell","Jeff King","Clemens Buchacher","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"189008","messageId":"CAAZnV3Einocd4TJ0iqcxPJNsr44j3RwhczS9OhyURX0faGWgBQ@mail.gmail.com","threadId":"30215","inReplyTo":null,"subject":"git bug: moved file with local unstaged changes are lost during merge","fromName":"Joe Angell","fromEmail":"joe.d.angell@gmail.com","sentAt":"2012-04-11T18:20:34Z","receivedAt":"2012-04-11T18:20:34Z","isPatch":false,"sender":{"key":"joe.d.angell@gmail.com","avatar":null},"body":"What steps will reproduce the problem?\ngit init\necho \"initial checkin\" >> readme\ngit add readme\ngit commit -m \"inital checkin\"\ngit branch b1\ngit checkout b1\necho \"b1\" >> readme\ngit add readme\ngit commit -m \"b1 readme\"\ngit checkout master\ngit mv readme readme_master\ngit ci -m \"moved readme\"\necho \"master\" >> readme_master\ngit merge b1\n\nWhat is the expected output? What do you see instead?\nI expect to have git prevent the merge due to local changes to the\nfile.  Instead it overwrites the file (erasing the local modification\n\"master\") and you end up with:\ncat readme_master\ninitial readme\nb1\n\nWhat version of the product are you using? On what operating system?\nReproduced on 1.7.9.6 and from the git-core repo 1.7.10.128.g7945c.\nThis is on ubuntu 10.04.\n\nPlease provide any additional information below.\n\nThis problem only seems to occur after you check in the move, then\nmake local modifications, then do the merge.\n\n-- \n---------------\nJoe Angell\ncell: (720) 260-2190\n"},{"id":"189117","messageId":"CAAZnV3EwZ6kminW7D3ssn_Rtj2SsMHd++VCx7w14K5rQKba4_g@mail.gmail.com","threadId":"30215","inReplyTo":"CAAZnV3Einocd4TJ0iqcxPJNsr44j3RwhczS9OhyURX0faGWgBQ@mail.gmail.com","subject":"Re: git bug: moved file with local unstaged changes are lost during merge","fromName":"Joe Angell","fromEmail":"joe.d.angell@gmail.com","sentAt":"2012-04-12T16:13:04Z","receivedAt":"2012-04-12T16:13:04Z","isPatch":false,"sender":{"key":"joe.d.angell@gmail.com","avatar":null},"body":"Is this the right place to post bug reports?\n\n\n\nOn Wed, Apr 11, 2012 at 12:20 PM, Joe Angell <joe.d.angell@gmail.com> wrote:\n> What steps will reproduce the problem?\n> git init\n> echo \"initial checkin\" >> readme\n> git add readme\n> git commit -m \"inital checkin\"\n> git branch b1\n> git checkout b1\n> echo \"b1\" >> readme\n> git add readme\n> git commit -m \"b1 readme\"\n> git checkout master\n> git mv readme readme_master\n> git ci -m \"moved readme\"\n> echo \"master\" >> readme_master\n> git merge b1\n>\n> What is the expected output? What do you see instead?\n> I expect to have git prevent the merge due to local changes to the\n> file.  Instead it overwrites the file (erasing the local modification\n> \"master\") and you end up with:\n> cat readme_master\n> initial readme\n> b1\n>\n> What version of the product are you using? On what operating system?\n> Reproduced on 1.7.9.6 and from the git-core repo 1.7.10.128.g7945c.\n> This is on ubuntu 10.04.\n>\n> Please provide any additional information below.\n>\n> This problem only seems to occur after you check in the move, then\n> make local modifications, then do the merge.\n>\n> --\n> ---------------\n> Joe Angell\n> cell: (720) 260-2190\n\n\n\n-- \n---------------\nJoe Angell\ncell: (720) 260-2190\n"},{"id":"189176","messageId":"20120413064941.GC27214@sigill.intra.peff.net","threadId":"30215","inReplyTo":"CAAZnV3EwZ6kminW7D3ssn_Rtj2SsMHd++VCx7w14K5rQKba4_g@mail.gmail.com","subject":"Re: git bug: moved file with local unstaged changes are lost during merge","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-13T06:49:41Z","receivedAt":"2012-04-13T06:49:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 12, 2012 at 10:13:04AM -0600, Joe Angell wrote:\n\n> Is this the right place to post bug reports?\n\nIt is. Thanks for including a concise test case with your bug report.\n\nUnfortunately, the merge-recursive code is a mess, and has several known\nbuggy corner cases with renames. Elijah (cc'd) spent a lot of time\ntrying to sort these out a while ago, but there still some known\nfailures. t6042 and t6036 detect some of them. But I thought we managed\nto clean up all of the overwriting bugs.\n\nOriginal bug report is below.\n\n-Peff\n\n> On Wed, Apr 11, 2012 at 12:20 PM, Joe Angell <joe.d.angell@gmail.com> wrote:\n> > What steps will reproduce the problem?\n> > git init\n> > echo \"initial checkin\" >> readme\n> > git add readme\n> > git commit -m \"inital checkin\"\n> > git branch b1\n> > git checkout b1\n> > echo \"b1\" >> readme\n> > git add readme\n> > git commit -m \"b1 readme\"\n> > git checkout master\n> > git mv readme readme_master\n> > git ci -m \"moved readme\"\n> > echo \"master\" >> readme_master\n> > git merge b1\n> >\n> > What is the expected output? What do you see instead?\n> > I expect to have git prevent the merge due to local changes to the\n> > file.  Instead it overwrites the file (erasing the local modification\n> > \"master\") and you end up with:\n> > cat readme_master\n> > initial readme\n> > b1\n> >\n> > What version of the product are you using? On what operating system?\n> > Reproduced on 1.7.9.6 and from the git-core repo 1.7.10.128.g7945c.\n> > This is on ubuntu 10.04.\n> >\n> > Please provide any additional information below.\n> >\n> > This problem only seems to occur after you check in the move, then\n> > make local modifications, then do the merge.\n> >\n> > --\n> > ---------------\n> > Joe Angell\n> > cell: (720) 260-2190\n"},{"id":"189301","messageId":"20120414231515.GB18137@ecki","threadId":"30215","inReplyTo":"20120413064941.GC27214@sigill.intra.peff.net","subject":"Re: git bug: moved file with local unstaged changes are lost during merge","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-04-14T23:15:17Z","receivedAt":"2012-04-14T23:15:17Z","isPatch":false,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Fri, Apr 13, 2012 at 02:49:41AM -0400, Jeff King wrote:\n> \n> t6042 and t6036 detect some of them. But I thought we managed to clean\n> up all of the overwriting bugs.\n\nI could not find an existing test for this.\n\n-->o--\nSubject: [PATCH] merge overwrites unstaged changes in renamed file\n\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n t/t7607-merge-overwrite.sh |    9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh\nindex aa74184..6547eb8 100755\n--- a/t/t7607-merge-overwrite.sh\n+++ b/t/t7607-merge-overwrite.sh\n@@ -92,6 +92,15 @@ test_expect_success 'will not overwrite removed file with staged changes' '\n \ttest_cmp important c1.c\n '\n \n+test_expect_failure 'will not overwrite unstaged changes in renamed file' '\n+\tgit reset --hard c1 &&\n+\tgit mv c1.c other.c &&\n+\tgit commit -m rename &&\n+\tcp important other.c &&\n+\tgit merge c1a &&\n+\ttest_cmp important other.c\n+'\n+\n test_expect_success 'will not overwrite untracked subtree' '\n \tgit reset --hard c0 &&\n \trm -rf sub &&\n-- \n1.7.9.6\n"},{"id":"190249","messageId":"20120429141758.GA6500@ecki.lan","threadId":"30215","inReplyTo":"20120414231515.GB18137@ecki","subject":"Re: git bug: moved file with local unstaged changes are lost during merge","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-04-29T14:17:58Z","receivedAt":"2012-04-29T14:17:58Z","isPatch":false,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Sun, Apr 15, 2012 at 01:15:17AM +0200, Clemens Buchacher wrote:\n> --- a/t/t7607-merge-overwrite.sh\n> +++ b/t/t7607-merge-overwrite.sh\n> @@ -92,6 +92,15 @@ test_expect_success 'will not overwrite removed file with staged changes' '\n>  \ttest_cmp important c1.c\n>  '\n>  \n> +test_expect_failure 'will not overwrite unstaged changes in renamed file' '\n> +\tgit reset --hard c1 &&\n> +\tgit mv c1.c other.c &&\n> +\tgit commit -m rename &&\n> +\tcp important other.c &&\n> +\tgit merge c1a &&\n> +\ttest_cmp important other.c\n> +'\n\nI have looked into this today, and we have a situation that is very\nsimilar to c5ab03f2 (merge-recursive: do not clobber untracked working\ntree garbage). The only difference being that the rename was made in our\nbranch, instead of the other. However, this does make it very difficult.\n\nAs you know, we have a fundamental issue in the way we protect untracked\nor modified files from changes. Roughly, the procedure is as follows:\n\n1. merge_trees -> threeway_merge\n\nGo through each index entry and try to merge it.  Depending on the\noutcome, do one of the following:\n\n a) If the entry has conflicts, or if it merges cleanly but the result\n    is different from HEAD, check that index and work tree are not\n    dirty, so that we can checkout the result of the merge without\n    overwriting anything. Otherwise abort.\n\n b) If the entry has no changes, or if it merges cleanly and the result\n    is the same as HEAD (or if it does not exist in HEAD), remove the\n    CE_UPDATE flag from the entry so that we do not overwrite changes in\n    the index or work tree, if any.\n\n2. merge_trees -> process_entry\n\nFind possible rename pairs and try to merge renames. Due to the renames,\nentries that were classified as b) above can now become of type a).\n\nIf the rename was in the other branch, the entry is new and was\ncategorized as b). Now we realize it's a modified rename, and it should\nhave been categorized as a) above.  Untracked files used to be\noverridden here. But c5ab03f2 added a last minute safety valve, which\nchecks that a renamed entry either exists in HEAD, or does not exist in\nthe work tree. This catches at least the typical case.\n\nIf the rename was in our branch, the situation is slightly different.\nThe entry exists in HEAD, but it is still categorized as b). Due to the\nmodified rename, we later decide to update the work tree after all.  In\nthis case, the safety valve above does not help, because the entry was\ntracked, but at this point it is too late to tell if the entry was\nup-to-date. And so it is overwritten.\n\nOne possible fix is to abort in case of trivial merges that would have\nbeen classified as b) above. The reationale is that we have to assume\nthat the entry could get modified due to a rename later. But that breaks\nmany test cases where we currently assume that it is ok to have a dirty\nwork tree during trivial merges. (I did not run all tests.)\n\nMaybe we should disable merging with a dirty work tree altogether, until\nwe have a solution that is safe to use?\n\nClemens\n\n-->8--\nSubject: [PATCH] trivial merge: always verify that working tree is clean\n\nEven if the index and the merge result match at this stage, if the entry\nturns out to be a renamed file, it may still be modified. Make sure that\nthe work tree is up-to-date, in order to avoid overwriting data.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n t/t1000-read-tree-m-3way.sh |   14 +++++++-------\n t/t1004-read-tree-m-u-wf.sh |    2 +-\n t/t3903-stash.sh            |    2 +-\n t/t7607-merge-overwrite.sh  |    4 ++--\n unpack-trees.c              |    5 +++--\n 5 files changed, 14 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t1000-read-tree-m-3way.sh b/t/t1000-read-tree-m-3way.sh\nindex babcdd2..eb03339 100755\n--- a/t/t1000-read-tree-m-3way.sh\n+++ b/t/t1000-read-tree-m-3way.sh\n@@ -222,7 +222,7 @@ test_expect_success \\\n      git update-index --add NA &&\n      read_tree_must_succeed -m $tree_O $tree_A $tree_B\"\n \n-test_expect_success \\\n+test_expect_failure \\\n     '2 - matching B alone is OK in !O && !A && B case.' \\\n     \"rm -f .git/index NA &&\n      cp .orig-B/NA NA &&\n@@ -238,7 +238,7 @@ test_expect_success \\\n      read_tree_must_succeed -m $tree_O $tree_A $tree_B &&\n      check_result\"\n \n-test_expect_success \\\n+test_expect_failure \\\n     '3 - matching A alone is OK in !O && A && !B case.' \\\n     \"rm -f .git/index AN &&\n      cp .orig-A/AN AN &&\n@@ -289,7 +289,7 @@ test_expect_success \\\n      read_tree_must_succeed -m $tree_O $tree_A $tree_B &&\n      check_result\"\n \n-test_expect_success \\\n+test_expect_failure \\\n     '5 - must match in !O && A && B && A==B case.' \\\n     \"rm -f .git/index LL &&\n      cp .orig-A/LL LL &&\n@@ -417,7 +417,7 @@ test_expect_success \\\n      read_tree_must_succeed -m $tree_O $tree_A $tree_B &&\n      check_result\"\n \n-test_expect_success \\\n+test_expect_failure \\\n     '12 - must match A in O && A && B && O!=A && A==B case' \\\n     \"rm -f .git/index SS &&\n      cp .orig-A/SS SS &&\n@@ -443,7 +443,7 @@ test_expect_success \\\n      read_tree_must_succeed -m $tree_O $tree_A $tree_B &&\n      check_result\"\n \n-test_expect_success \\\n+test_expect_failure \\\n     '13 - must match A in O && A && B && O!=A && O==B case' \\\n     \"rm -f .git/index MN &&\n      cp .orig-A/MN MN &&\n@@ -460,7 +460,7 @@ test_expect_success \\\n      read_tree_must_succeed -m $tree_O $tree_A $tree_B &&\n      check_result\"\n \n-test_expect_success \\\n+test_expect_failure \\\n     '14 - may match B in O && A && B && O==A && O!=B case' \\\n     \"rm -f .git/index NM &&\n      cp .orig-B/NM NM &&\n@@ -495,7 +495,7 @@ test_expect_success \\\n      read_tree_must_succeed -m $tree_O $tree_A $tree_B &&\n      check_result\"\n \n-test_expect_success \\\n+test_expect_failure \\\n     '15 - must match A in O && A && B && O==A && O==B case' \\\n     \"rm -f .git/index NN &&\n      cp .orig-A/NN NN &&\ndiff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh\nindex b3ae7d5..5002af2 100755\n--- a/t/t1004-read-tree-m-u-wf.sh\n+++ b/t/t1004-read-tree-m-u-wf.sh\n@@ -130,7 +130,7 @@ test_expect_success '3-way not overwriting local changes (setup)' '\n \n '\n \n-test_expect_success '3-way not overwriting local changes (our side)' '\n+test_expect_failure '3-way not overwriting local changes (our side)' '\n \n \t# At this point, file1 from side-a should be kept as side-b\n \t# did not touch it.\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 3addb80..5f54c30 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -43,7 +43,7 @@ test_expect_success 'applying bogus stash does nothing' '\n \ttest_cmp expect file\n '\n \n-test_expect_success 'apply does not need clean working directory' '\n+test_expect_failure 'apply does not need clean working directory' '\n \techo 4 >other-file &&\n \tgit add other-file &&\n \techo 5 >other-file &&\ndiff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh\nindex 6547eb8..72f84b8 100755\n--- a/t/t7607-merge-overwrite.sh\n+++ b/t/t7607-merge-overwrite.sh\n@@ -92,12 +92,12 @@ test_expect_success 'will not overwrite removed file with staged changes' '\n \ttest_cmp important c1.c\n '\n \n-test_expect_failure 'will not overwrite unstaged changes in renamed file' '\n+test_expect_success 'will not overwrite unstaged changes in renamed file' '\n \tgit reset --hard c1 &&\n \tgit mv c1.c other.c &&\n \tgit commit -m rename &&\n \tcp important other.c &&\n-\tgit merge c1a &&\n+\ttest_must_fail git merge c1a &&\n \ttest_cmp important other.c\n '\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 36523da..dcaef43 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1480,6 +1480,9 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n \t\t\treturn -1;\n \t\tinvalidate_ce_path(merge, o);\n \t} else if (!(old->ce_flags & CE_CONFLICTED)) {\n+\t\tif (verify_uptodate(old, o))\n+\t\t\treturn -1;\n+\n \t\t/*\n \t\t * See if we can re-use the old CE directly?\n \t\t * That way we get the uptodate stat info.\n@@ -1491,8 +1494,6 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n \t\t\tcopy_cache_entry(merge, old);\n \t\t\tupdate = 0;\n \t\t} else {\n-\t\t\tif (verify_uptodate(old, o))\n-\t\t\t\treturn -1;\n \t\t\t/* Migrate old flags over */\n \t\t\tupdate |= old->ce_flags & (CE_SKIP_WORKTREE | CE_NEW_SKIP_WORKTREE);\n \t\t\tinvalidate_ce_path(old, o);\n-- \n1.7.10\n"},{"id":"190261","messageId":"7vr4v6hy5p.fsf@alter.siamese.dyndns.org","threadId":"30215","inReplyTo":"20120429141758.GA6500@ecki.lan","subject":"Re: git bug: moved file with local unstaged changes are lost during merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-30T00:16:18Z","receivedAt":"2012-04-30T00:16:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n> 2. merge_trees -> process_entry\n>\n> Find possible rename pairs and try to merge renames. Due to the renames,\n> entries that were classified as b) above can now become of type a).\n\nYes, unfortunately the rename-processing part of merge-recursive is known\nto be broken wrt things like this.  If you want to punt,\n\n> Maybe we should disable merging with a dirty work tree altogether, until\n> we have a solution that is safe to use?\n\nthis is not the way to go, as \"resolve\" is perfectly safe.  It would make\nmore sense to disable the rename-processing part of recursive by default,\nuntil it gets cleaned up further.\n"}]}