Why git silently replaces untracked files?

Why git silently replaces untracked files?

[igor.mikushkin]

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.

Re: Why git silently replaces untracked files?

[Jeff King]

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

Re: Why git silently replaces untracked files?

[igor.mikushkin]

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.

Re: Why git silently replaces untracked files?

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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