threads / discuss / 26861

Why git silently replaces untracked files?

Subject: Why git silently replaces untracked files?

## tl;dr

8 messages between Mar 25, 2011 and Mar 25, 2011.

replies: 7people: 2as markdown or json

igor.mikushkin· Mar 25, 2011, 14:52 UTC · lore
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· Mar 25, 2011, 16:58 UTC · re: igor.mikushkin · lore

Re: Why git silently replaces untracked files?

On Fri, Mar 25, 2011 at 07:52:34AM -0700, igor.mikushkin wrote:
Show 26 quoted lines
> 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· Mar 25, 2011, 17:53 UTC · re: Jeff King · lore

Re: Why git silently replaces untracked files?

Jeff King wrote:
Show 51 quoted lines
> 
> 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· Mar 25, 2011, 18:06 UTC · re: igor.mikushkin · lore

Re: Why git silently replaces untracked files?

On Fri, Mar 25, 2011 at 10:53:48AM -0700, igor.mikushkin wrote:
Show 22 quoted lines
> > 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· Mar 25, 2011, 18:08 UTC · re: Jeff King · lore

[PATCH 1/4] t7607: mark known breakage in test 11 as fixed

This was fixed by 1d718a51 (do not overwrite untracked symlinks, 2011-02-20).

Signed-off-by: Jeff King <peff@peff.net>
---
 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· Mar 25, 2011, 18:09 UTC · re: Jeff King · lore

[PATCH 2/4] t7607: clean up stray untracked file

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 <peff@peff.net>
---
 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· Mar 25, 2011, 18:10 UTC · re: Jeff King · lore

[PATCH 3/4] merge: merge unborn index before setting ref

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 <peff@peff.net>
---
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· Mar 25, 2011, 18:13 UTC · re: Jeff King · lore

[PATCH 4/4] pull: do not clobber untracked files on initial pull

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 <peff@peff.net>
---
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

← back to recent threads