# Why git silently replaces untracked files? 8 messages from 2011-03-25 to 2011-03-25. Participants: igor.mikushkin, Jeff King. Thread: https://gitlist.dev/t/26861 ## igor.mikushkin, 2011-03-25 14:52 Subject: Why git silently replaces untracked files? Message-ID: <1301064754576-6207950.post@n2.nabble.com> URL: https://gitlist.dev/e/1301064754576-6207950.post%40n2.nabble.com ``` Why git silently replaces untracked files? # mkdir test.git # mkdir 1 # mkdir 2 # echo 1 > 1/test # echo 2 > 2/test # cd test.git # git init --bare # cd .. # git clone test.git # cp -r test/.git 1 # cp -r test/.git 2 # cd 1 # git add test # git commit -am 1 # git push origin master # cd ../2 # git pull # cat test 1 In my opinion it is wrong behavior. I've just lost important file due to it. Should not "git pull" fail here? Anyhow it looks more correct then silent replacing. Thanks, Igor -- View this message in context: http://git.661346.n2.nabble.com/Why-git-silently-replaces-untracked-files-tp6207950p6207950.html Sent from the git mailing list archive at Nabble.com. ``` ## Jeff King, 2011-03-25 16:58 Subject: Re: Why git silently replaces untracked files? Message-ID: <20110325165811.GB25851@sigill.intra.peff.net> URL: https://gitlist.dev/e/20110325165811.GB25851%40sigill.intra.peff.net In-Reply-To: <1301064754576-6207950.post@n2.nabble.com> ``` On Fri, Mar 25, 2011 at 07:52:34AM -0700, igor.mikushkin wrote: > Why git silently replaces untracked files? > > # mkdir test.git > # mkdir 1 > # mkdir 2 > # echo 1 > 1/test > # echo 2 > 2/test > # cd test.git > # git init --bare > # cd .. > # git clone test.git > # cp -r test/.git 1 > # cp -r test/.git 2 > # cd 1 > # git add test > # git commit -am 1 > # git push origin master > # cd ../2 > # git pull > # cat test > 1 > > In my opinion it is wrong behavior. > I've just lost important file due to it. > > Should not "git pull" fail here? Ick, definitely it's wrong behavior. The culprit seems to be a special code path for the initial pull which doesn't merge at all, but calls read-tree --reset. It should probably be: diff --git a/git-pull.sh b/git-pull.sh index a3159c3..fb9e2df 100755 --- a/git-pull.sh +++ b/git-pull.sh @@ -253,7 +253,7 @@ esac if test -z "$orig_head" then git update-ref -m "initial pull" HEAD $merge_head "$curr_head" && - git read-tree --reset -u HEAD || exit 1 + git read-tree -m -u HEAD || exit 1 exit fi Though I don't know if there are any cases where the --reset would be beneficial over "-m". I couldn't think of any. -Peff ``` ## igor.mikushkin, 2011-03-25 17:53 Subject: Re: Why git silently replaces untracked files? Message-ID: <1301075628970-6208585.post@n2.nabble.com> URL: https://gitlist.dev/e/1301075628970-6208585.post%40n2.nabble.com In-Reply-To: <20110325165811.GB25851@sigill.intra.peff.net> ``` Jeff King wrote: > > On Fri, Mar 25, 2011 at 07:52:34AM -0700, igor.mikushkin wrote: > > > Why git silently replaces untracked files? > > > > # mkdir test.git > > # mkdir 1 > > # mkdir 2 > > # echo 1 > 1/test > > # echo 2 > 2/test > > # cd test.git > > # git init --bare > > # cd .. > > # git clone test.git > > # cp -r test/.git 1 > > # cp -r test/.git 2 > > # cd 1 > > # git add test > > # git commit -am 1 > > # git push origin master > > # cd ../2 > > # git pull > > # cat test > > 1 > > > > In my opinion it is wrong behavior. > > I've just lost important file due to it. > > > > Should not "git pull" fail here? > > Ick, definitely it's wrong behavior. The culprit seems to be a special > code path for the initial pull which doesn't merge at all, but calls > read-tree --reset. It should probably be: > > diff --git a/git-pull.sh b/git-pull.sh > index a3159c3..fb9e2df 100755 > --- a/git-pull.sh > +++ b/git-pull.sh > @@ -253,7 +253,7 @@ esac > if test -z "$orig_head" > then > git update-ref -m "initial pull" HEAD $merge_head > "$curr_head" && > - git read-tree --reset -u HEAD || exit 1 > + git read-tree -m -u HEAD || exit 1 > exit > fi > > Though I don't know if there are any cases where the --reset would be > beneficial over "-m". I couldn't think of any. > Thanks Jeff, My opinion is that you are right and merging is best here (Though just fail would be probably OK either). Love one line fixes. Igor -- View this message in context: http://git.661346.n2.nabble.com/Why-git-silently-replaces-untracked-files-tp6207950p6208585.html Sent from the git mailing list archive at Nabble.com. ``` ## Jeff King, 2011-03-25 18:06 Subject: Re: Why git silently replaces untracked files? Message-ID: <20110325180644.GA30838@sigill.intra.peff.net> URL: https://gitlist.dev/e/20110325180644.GA30838%40sigill.intra.peff.net In-Reply-To: <1301075628970-6208585.post@n2.nabble.com> ``` On Fri, Mar 25, 2011 at 10:53:48AM -0700, igor.mikushkin wrote: > > diff --git a/git-pull.sh b/git-pull.sh > > index a3159c3..fb9e2df 100755 > > --- a/git-pull.sh > > +++ b/git-pull.sh > > @@ -253,7 +253,7 @@ esac > > if test -z "$orig_head" > > then > > git update-ref -m "initial pull" HEAD $merge_head > > "$curr_head" && > > - git read-tree --reset -u HEAD || exit 1 > > + git read-tree -m -u HEAD || exit 1 > > exit > > fi > > > > Though I don't know if there are any cases where the --reset would be > > beneficial over "-m". I couldn't think of any. > > > > Thanks Jeff, > My opinion is that you are right and merging is best here > (Though just fail would be probably OK either). > Love one line fixes. It will fail with "untracked file 'test' would be overwritten..."; it's just that --reset turns off the safety features of read-tree, which I don't see a point in doing. While looking at this, I also noticed that "git merge" behaves in a funny way on this case. So I came up with this patch series: [1/4]: t7607: mark known breakage in test 11 as fixed [2/4]: t7607: clean up stray untracked file [3/4]: merge: merge unborn index before setting ref [4/4]: pull: do not clobber untracked files on initial pull -Peff ``` ## Jeff King, 2011-03-25 18:08 Subject: [PATCH 1/4] t7607: mark known breakage in test 11 as fixed Message-ID: <20110325180836.GA14898@sigill.intra.peff.net> URL: https://gitlist.dev/e/20110325180836.GA14898%40sigill.intra.peff.net In-Reply-To: <20110325180644.GA30838@sigill.intra.peff.net> ``` This was fixed by 1d718a51 (do not overwrite untracked symlinks, 2011-02-20). Signed-off-by: Jeff King --- t/t7607-merge-overwrite.sh | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh index 5f731a1..691c5fd 100755 --- a/t/t7607-merge-overwrite.sh +++ b/t/t7607-merge-overwrite.sh @@ -122,7 +122,7 @@ test_expect_success 'will not overwrite untracked file in leading path' ' rm -f sub sub2 ' -test_expect_failure SYMLINKS 'will not overwrite untracked symlink in leading path' ' +test_expect_success SYMLINKS 'will not overwrite untracked symlink in leading path' ' git reset --hard c0 && rm -rf sub && mkdir sub2 && -- 1.7.4.33.gb8855.dirty ``` ## Jeff King, 2011-03-25 18:09 Subject: [PATCH 2/4] t7607: clean up stray untracked file Message-ID: <20110325180903.GB14898@sigill.intra.peff.net> URL: https://gitlist.dev/e/20110325180903.GB14898%40sigill.intra.peff.net In-Reply-To: <20110325180644.GA30838@sigill.intra.peff.net> ``` This file ends up conflicting with the test just after it (causing the "git merge" to fail). Neither test is to blame for the bug, though. It looks like the merge in 1a9fe45 (Merge branch 'tr/merge-unborn-clobber', 2011-02-09) is what caused the conflict. We didn't notice because the follow-on test is already marked as expect_failure (even though it has since been fixed, and now succeeds once the untracked file is moved out of the way). Signed-off-by: Jeff King --- t/t7607-merge-overwrite.sh | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh index 691c5fd..c86e298 100755 --- a/t/t7607-merge-overwrite.sh +++ b/t/t7607-merge-overwrite.sh @@ -150,6 +150,7 @@ test_expect_success 'will not overwrite untracked file on unborn branch' ' git rm -fr . && git checkout --orphan new && cp important c0.c && + test_when_finished "rm c0.c" && test_must_fail git merge c0 2>out && test_cmp out expect && test_path_is_missing .git/MERGE_HEAD && @@ -164,7 +165,7 @@ test_expect_success 'set up unborn branch and content' ' echo bar > untracked-file ' -test_expect_failure 'will not clobber WT/index when merging into unborn' ' +test_expect_success 'will not clobber WT/index when merging into unborn' ' git merge master && grep foo tracked-file && git show :tracked-file >expect && -- 1.7.4.33.gb8855.dirty ``` ## Jeff King, 2011-03-25 18:10 Subject: [PATCH 3/4] merge: merge unborn index before setting ref Message-ID: <20110325181038.GC14898@sigill.intra.peff.net> URL: https://gitlist.dev/e/20110325181038.GC14898%40sigill.intra.peff.net In-Reply-To: <20110325180644.GA30838@sigill.intra.peff.net> ``` When we merge into an unborn branch, there are basically two steps: 1. Write the sha1 of the new commit into the ref pointed to by HEAD. 2. Update the index with the new content, and check it out to the working tree. We currently do them in this order. However, (2) is the step that is much more likely to fail, since it can be blocked by things like untracked working tree files. When it does, the merge fails and we are left with an empty index but an updated HEAD. This patch switches the order, so that a failure in updating the index leaves us unchanged. Of course, a failure in updating the ref now leaves us with an updated index and mis-matched HEAD. That is arguably not much better, but it is probably less likely to actually happen. Signed-off-by: Jeff King --- I noticed this while diagnosing the pull problem fixed in 4/4. As discused, this is just trading one set of error conditions for another. The "right" thing to do is probably to rollback, but of course that can fail, too, and it's more effort. I think in practice this is fine. builtin/merge.c | 2 +- t/t7607-merge-overwrite.sh | 4 ++++ 2 files changed, 5 insertions(+), 1 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index aa3453c..c8d028c 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1063,9 +1063,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix) remote_head = peel_to_type(argv[0], 0, NULL, OBJ_COMMIT); if (!remote_head) die("%s - not something we can merge", argv[0]); + read_empty(remote_head->sha1, 0); update_ref("initial pull", "HEAD", remote_head->sha1, NULL, 0, DIE_ON_ERR); - read_empty(remote_head->sha1, 0); return 0; } else { struct strbuf merge_names = STRBUF_INIT; diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh index c86e298..b54e840 100755 --- a/t/t7607-merge-overwrite.sh +++ b/t/t7607-merge-overwrite.sh @@ -157,6 +157,10 @@ test_expect_success 'will not overwrite untracked file on unborn branch' ' test_cmp important c0.c ' +test_expect_success 'failed merge leaves unborn branch in the womb' ' + test_must_fail git rev-parse --verify HEAD +' + test_expect_success 'set up unborn branch and content' ' git symbolic-ref HEAD refs/heads/unborn && rm -f .git/index && -- 1.7.4.33.gb8855.dirty ``` ## Jeff King, 2011-03-25 18:13 Subject: [PATCH 4/4] pull: do not clobber untracked files on initial pull Message-ID: <20110325181331.GD14898@sigill.intra.peff.net> URL: https://gitlist.dev/e/20110325181331.GD14898%40sigill.intra.peff.net In-Reply-To: <20110325180644.GA30838@sigill.intra.peff.net> ``` For a pull into an unborn branch, we do not use "git merge" at all. Instead, we call read-tree directly. However, we used the --reset parameter instead of "-m", which turns off the safety features. Signed-off-by: Jeff King --- This blames all the way back to d09e79c (git-pull: allow pulling into an empty repository, 2006-11-16) by Linus. I couldn't think of a good reason to use "--reset" instead of "-m". git-pull.sh | 2 +- t/t5520-pull.sh | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletions(-) diff --git a/git-pull.sh b/git-pull.sh index 63b063a..e31226b 100755 --- a/git-pull.sh +++ b/git-pull.sh @@ -274,7 +274,7 @@ esac if test -z "$orig_head" then git update-ref -m "initial pull" HEAD $merge_head "$curr_head" && - git read-tree --reset -u HEAD || exit 1 + git read-tree -m -u HEAD || exit 1 exit fi diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 0470a81..0e5eb67 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -46,6 +46,17 @@ test_expect_success 'pulling into void using master:master' ' test_cmp file cloned-uho/file ' +test_expect_success 'pulling into void does not overwrite untracked files' ' + git init cloned-untracked && + ( + cd cloned-untracked && + echo untracked >file && + test_must_fail git pull .. master && + echo untracked >expect && + test_cmp expect file + ) +' + test_expect_success 'test . as a remote' ' git branch copy master && -- 1.7.4.33.gb8855.dirty ```