[PATCH] pull: pull into void by fast-forwarding from empty tree

The logic for pulling into an unborn branch was originally designed to
be used on a newly-initialized repository (d09e79c, git-pull: allow
pulling into an empty repository, 2006-11-16).  It thus did not
initially deal with uncommitted changes in the unborn branch.  The
case of an _unstaged_ untracked file was fixed by by 4b3ffe5 (pull: do
not clobber untracked files on initial pull, 2011-03-25).  However, it
still clobbered existing staged files, both when the file exists in
the merged commit (it will be overwritten), and when it does not (it
will be lost!).

We fix this by doing a two-way merge, where the "current" side of the
merge is an empty tree, and the "target" side is HEAD (already updated
to FETCH_HEAD at this point).  This amounts to claiming that all work
in the index was done vs. an empty tree, and thus all content of the
index is precious.

Reported-by: Stefan Schüßler <redacted>
Signed-off-by: Thomas Rast <redacted>
---

Stefan Schüßler [off-list ref] writes:

I think there's a bug in git pull. Doing a git pull in a fresh
repository without any commits removes files in the index.

[...]

Thanks for the bug report!

 git-pull.sh     |  9 ++++++++-
 t/t5520-pull.sh | 24 ++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/git-pull.sh b/git-pull.sh
index 638aabb..1f84383 100755
--- a/git-pull.sh
+++ b/git-pull.sh

@@ -266,10 +266,17 @@ case "$merge_head" in
 	;;
 esac
 
+# Pulling into unborn branch: a shorthand for branching off
+# FETCH_HEAD, for lazy typers.
 if test -z "$orig_head"
 then
 	git update-ref -m "initial pull" HEAD $merge_head "$curr_head" &&
-	git read-tree -m -u HEAD || exit 1
+	# Two-way merge: we claim the index is based on an empty tree,
+	# and try to fast-forward to HEAD.  This ensures we will not
+	# lose index/worktree changes that the user already made on
+	# the unborn branch.
+	empty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904
+	git read-tree -m -u $empty_tree HEAD || exit 1
 	exit
 fi
 

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 6af6c63..927903c 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -57,6 +57,30 @@ test_expect_success 'pulling into void does not overwrite untracked files' '
 	)
 '
 
+test_expect_success 'pulling into void does not overwrite staged files' '
+	git init cloned-staged-colliding &&
+	(
+		cd cloned-staged-colliding &&
+		echo "alternate content" >file &&
+		git add file &&
+		test_must_fail git pull .. master &&
+		echo "alternate content" >expect &&
+		test_cmp expect file
+	)
+'
+
+test_expect_success 'pulling into void does not remove new staged files' '
+	git init cloned-staged-new &&
+	(
+		cd cloned-staged-new &&
+		echo "new tracked file" >newfile &&
+		git add newfile &&
+		git pull .. master &&
+		echo "new tracked file" >expect &&
+		test_cmp expect newfile
+	)
+'
+
 test_expect_success 'test . as a remote' '
 
 	git branch copy master &&

-- 
1.8.3.1.664.gae9f72a

On Thu, Jun 20, 2013 at 02:36:03PM +0200, Thomas Rast wrote:

The logic for pulling into an unborn branch was originally designed to
be used on a newly-initialized repository (d09e79c, git-pull: allow
pulling into an empty repository, 2006-11-16).  It thus did not
initially deal with uncommitted changes in the unborn branch.  The
case of an _unstaged_ untracked file was fixed by by 4b3ffe5 (pull: do
not clobber untracked files on initial pull, 2011-03-25).  However, it
still clobbered existing staged files, both when the file exists in
the merged commit (it will be overwritten), and when it does not (it
will be lost!).

Yeah, in 4beffe5 I just assumed that using "read-tree -m" would give us
the protections we need. But obviously I didn't think about the fact
that we are not giving it enough information.

We fix this by doing a two-way merge, where the "current" side of the
merge is an empty tree, and the "target" side is HEAD (already updated
to FETCH_HEAD at this point).  This amounts to claiming that all work
in the index was done vs. an empty tree, and thus all content of the
index is precious.

This seems like the correct fix; it is giving read-tree the right
information to make the decision. Thanks for working on this.

+test_expect_success 'pulling into void does not overwrite staged files' '
+	git init cloned-staged-colliding &&
+	(
+		cd cloned-staged-colliding &&
+		echo "alternate content" >file &&
+		git add file &&
+		test_must_fail git pull .. master &&
+		echo "alternate content" >expect &&
+		test_cmp expect file
+	)
+'
+
+test_expect_success 'pulling into void does not remove new staged files' '
+	git init cloned-staged-new &&
+	(
+		cd cloned-staged-new &&
+		echo "new tracked file" >newfile &&
+		git add newfile &&
+		git pull .. master &&
+		echo "new tracked file" >expect &&
+		test_cmp expect newfile
+	)
+'

Do we want to also check the index state after each pull? In the former
case, I think it should obviously represent a conflict. In the latter,
we should be retaining the index contents of newfile.

These are basic things that read-tree's two-way merge should get right
(and are presumably tested elsewhere), but it might be worth confirming
the desired behavior here in case somebody later tries to tweak this
code path not to use read-tree.

-Peff

The logic for pulling into an unborn branch was originally designed to
be used on a newly-initialized repository (d09e79c, git-pull: allow
pulling into an empty repository, 2006-11-16).  It thus did not
initially deal with uncommitted changes in the unborn branch.  The
case of an _unstaged_ untracked file was fixed by 4b3ffe5 (pull: do
not clobber untracked files on initial pull, 2011-03-25).  However, it
still clobbered existing staged files, both when the file exists in
the merged commit (it will be overwritten), and when it does not (it
will be deleted).

We fix this by doing a two-way merge, where the "current" side of the
merge is an empty tree, and the "target" side is HEAD (already updated
to FETCH_HEAD at this point).  This amounts to claiming that all work
in the index was done vs. an empty tree, and thus all content of the
index is precious.

Reported-by: Stefan Schüßler <redacted>
Signed-off-by: Thomas Rast <redacted>
---

Jeff King [off-list ref] writes:

Do we want to also check the index state after each pull? In the former
case, I think it should obviously represent a conflict. In the latter,
we should be retaining the index contents of newfile.

These are basic things that read-tree's two-way merge should get right
(and are presumably tested elsewhere), but it might be worth confirming
the desired behavior here in case somebody later tries to tweak this
code path not to use read-tree.

Right, good point.

I also reworded the subject and message somewhat to read better.


 git-pull.sh     |  9 ++++++++-
 t/t5520-pull.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/git-pull.sh b/git-pull.sh
index 638aabb..1f84383 100755
--- a/git-pull.sh
+++ b/git-pull.sh

@@ -266,10 +266,17 @@ case "$merge_head" in
 	;;
 esac
 
+# Pulling into unborn branch: a shorthand for branching off
+# FETCH_HEAD, for lazy typers.
 if test -z "$orig_head"
 then
 	git update-ref -m "initial pull" HEAD $merge_head "$curr_head" &&
-	git read-tree -m -u HEAD || exit 1
+	# Two-way merge: we claim the index is based on an empty tree,
+	# and try to fast-forward to HEAD.  This ensures we will not
+	# lose index/worktree changes that the user already made on
+	# the unborn branch.
+	empty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904
+	git read-tree -m -u $empty_tree HEAD || exit 1
 	exit
 fi
 

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 6af6c63..ed4d9c8 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -57,6 +57,35 @@ test_expect_success 'pulling into void does not overwrite untracked files' '
 	)
 '
 
+test_expect_success 'pulling into void does not overwrite staged files' '
+	git init cloned-staged-colliding &&
+	(
+		cd cloned-staged-colliding &&
+		echo "alternate content" >file &&
+		git add file &&
+		test_must_fail git pull .. master &&
+		echo "alternate content" >expect &&
+		test_cmp expect file &&
+		git cat-file blob :file >file.index &&
+		test_cmp expect file.index
+	)
+'
+
+
+test_expect_success 'pulling into void does not remove new staged files' '
+	git init cloned-staged-new &&
+	(
+		cd cloned-staged-new &&
+		echo "new tracked file" >newfile &&
+		git add newfile &&
+		git pull .. master &&
+		echo "new tracked file" >expect &&
+		test_cmp expect newfile &&
+		git cat-file blob :newfile >newfile.index &&
+		test_cmp expect newfile.index
+	)
+'
+
 test_expect_success 'test . as a remote' '
 
 	git branch copy master &&

-- 
1.8.3.1.664.gae9f72a

On Thu, Jun 20, 2013 at 03:06:01PM +0200, Thomas Rast wrote:

+test_expect_success 'pulling into void does not overwrite staged files' '
+	git init cloned-staged-colliding &&
+	(
+		cd cloned-staged-colliding &&
+		echo "alternate content" >file &&
+		git add file &&
+		test_must_fail git pull .. master &&
+		echo "alternate content" >expect &&
+		test_cmp expect file &&
+		git cat-file blob :file >file.index &&
+		test_cmp expect file.index
+	)
+'

I naively would have expected this to leave us in a conflicted state
over "file".  But I guess read-tree just rejects it, because we are not
doing a real three-way merge.  I'm not sure it is that big a deal; this
is more about safety than about creating a conflicted/resolvable state.

-Peff

Thomas Rast wrote:

Perhaps replace this magic with $(git hash-object -t tree /dev/null)
or $(git mktree </dev/null)?