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

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

[Thomas Rast]

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 <mail@stefanschuessler.de>
Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---

Stefan Schüßler <mail@stefanschuessler.de> 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

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

[Jeff King]

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

[PATCH v2] pull: merge into unborn by fast-forwarding from empty tree

[Thomas Rast]

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 <mail@stefanschuessler.de>
Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---

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

Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree

[Jeff King]

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

Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree

[Thomas Rast]

Jeff King <peff@peff.net> writes:

> 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.

Note that the test_must_fail essentially tests that the merge is rejected.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree

[Thomas Rast]

Thomas Rast <trast@inf.ethz.ch> writes:

> Jeff King <peff@peff.net> writes:
>
>> 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.
>
> Note that the test_must_fail essentially tests that the merge is rejected.

Bah, no it doesn't, a conflicting merge also returns a nonzero status.
Sigh.

If you meant we should actually conflict, I'm not sure what options
there would be other than actually calling a merge driver.  And we could
actually do this like so (it'll obviously break the tests):

diff --git i/git-pull.sh w/git-pull.sh
index 1f84383..b3d36a8 100755
--- i/git-pull.sh
+++ w/git-pull.sh
@@ -276,7 +276,7 @@ then
 	# 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
+	git merge-recursive $empty_tree -- $(git write-tree) HEAD || exit 1
 	exit
 fi
 


-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree

[Jeff King]

On Thu, Jun 20, 2013 at 03:33:34PM +0200, Thomas Rast wrote:

> >> 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.
> >
> > Note that the test_must_fail essentially tests that the merge is rejected.
> 
> Bah, no it doesn't, a conflicting merge also returns a nonzero status.
> Sigh.
> 
> If you meant we should actually conflict,

Yes, that's what I meant.

> I'm not sure what options there would be other than actually calling a
> merge driver.  And we could actually do this like so (it'll obviously
> break the tests):

I'd rather not invoke a merge driver directly if we can avoid it.  I
think you could get rid of this special code-path entirely if you just
lied to the "git-merge" and said "the ancestor and current tree are fake
commits with an empty tree", and then followed the usual path. But that
lying through git-merge is ugly and complicated (and is more or less
what you're doing with the merge-recursive patch here).

> diff --git i/git-pull.sh w/git-pull.sh
> index 1f84383..b3d36a8 100755
> --- i/git-pull.sh
> +++ w/git-pull.sh
> @@ -276,7 +276,7 @@ then
>  	# 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
> +	git merge-recursive $empty_tree -- $(git write-tree) HEAD || exit 1

I don't think there is any advantage to using merge-recursive over
read-tree here, in the sense that there cannot be any interesting
content-level merging going on (our ancestor is the empty tree, so we
know that differing content cannot be resolved).

So I think you could just use read-tree with a 3-way merge, but I cannot
seem to provoke it to leave a conflict. Hrm.

I also noticed that the procedure in this code path is:

  1. Update HEAD with the fetched ref.

  2. Checkout the contents with read-tree.

I wonder if it would make sense to update HEAD only _after_ we had
resolved successfully. As it is now, you are left in a weird state where
pull has reported failure, but we actually update the HEAD (and "git
status" afterwards reflects that you are building on top of the pulled
HEAD).

-Peff

Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree

[Thomas Rast]

Jeff King <peff@peff.net> writes:

> On Thu, Jun 20, 2013 at 03:33:34PM +0200, Thomas Rast wrote:
>
>> >> 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.
>> >
>> > Note that the test_must_fail essentially tests that the merge is rejected.
>> 
>> Bah, no it doesn't, a conflicting merge also returns a nonzero status.
>> Sigh.
>> 
>> If you meant we should actually conflict,
>
> Yes, that's what I meant.
[...]
>> diff --git i/git-pull.sh w/git-pull.sh
>> index 1f84383..b3d36a8 100755
>> --- i/git-pull.sh
>> +++ w/git-pull.sh
>> @@ -276,7 +276,7 @@ then
>>  	# 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
>> +	git merge-recursive $empty_tree -- $(git write-tree) HEAD || exit 1
>
> I don't think there is any advantage to using merge-recursive over
> read-tree here, in the sense that there cannot be any interesting
> content-level merging going on (our ancestor is the empty tree, so we
> know that differing content cannot be resolved).
>
> So I think you could just use read-tree with a 3-way merge, but I cannot
> seem to provoke it to leave a conflict. Hrm.

I guess read-tree doesn't consider that its job; it leaves the conflict
in the index alright for me if I do this:

 git-pull.sh     | 4 ++--
 t/t5520-pull.sh | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git i/git-pull.sh w/git-pull.sh
index 1f84383..4047d02 100755
--- i/git-pull.sh
+++ w/git-pull.sh
@@ -275,8 +275,8 @@ then
 	# 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
+	empty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904 &&
+	git read-tree -m -u $empty_tree $(git write-tree) HEAD || exit 1
 	exit
 fi
 

But it won't write the conflict markers in the worktree.

On the topic of "do we want to conflict": one issue is that we don't
have a prior state to go to, since it was never committed.  Not even the
implicit empty tree can be passed to 'reset --keep'.  So it might be
better to *avoid* creating conflict hunks -- and fail -- so as to avoid
giving the user a state that is hard to back out of.

In the same spirit I would also support this:

> I wonder if it would make sense to update HEAD only _after_ we had
> resolved successfully. As it is now, you are left in a weird state where
> pull has reported failure, but we actually update the HEAD (and "git
> status" afterwards reflects that you are building on top of the pulled
> HEAD).

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree

[Thomas Rast]

Thomas Rast <trast@inf.ethz.ch> writes:

>  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

I just noticed that I broke the && chaining here, so don't apply this
just yet.  I'll resend once we settle on the best strategy to generate
conflicts (or not).

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> 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).

Perhaps making sure the index is empty is sufficient, then?

Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree

[Jeff King]

On Thu, Jun 20, 2013 at 11:43:37AM -0700, Junio C Hamano wrote:

> Thomas Rast <trast@inf.ethz.ch> writes:
> 
> > 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).
> 
> Perhaps making sure the index is empty is sufficient, then?

That would not let you pull when you have "foo" staged, but upstream
does not have "foo" at all. To be fair, that is quite a corner case, and
simply rejecting the pull entirely may be OK. But read-tree already does
the hard work for us, so I don't think it is a lot of code either way.

-Peff

Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Perhaps making sure the index is empty is sufficient, then?
>
> That would not let you pull when you have "foo" staged, but upstream
> does not have "foo" at all. To be fair, that is quite a corner case, and
> simply rejecting the pull entirely may be OK.

That simplicity was what I was hinting at ;-).

> But read-tree already does
> the hard work for us, so I don't think it is a lot of code either way.

OK, I just got an impression from reading the back-and-forth between
you two that read-tree does not want to deal with that case.

But yes, if you say "I have this index, and I am straying away from
an empty tree to that commit", with two-tree form "read-tree -m -u",
everything should work correctly, including the bit that says "nah,
nah, you have added 'foo' but the other guy also adds 'foo', so I'll
refuse".

So please scratch that short-cut suggestion.

Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree

[Jeff King]

On Thu, Jun 20, 2013 at 01:49:13PM -0700, Junio C Hamano wrote:

> > But read-tree already does
> > the hard work for us, so I don't think it is a lot of code either way.
> 
> OK, I just got an impression from reading the back-and-forth between
> you two that read-tree does not want to deal with that case.

I think I got us off-track with my expectation of ending the one case
with a conflicted index. But caring about that is even more unlikely.  I
think Thomas's original patch is probably a happy medium.

As an orthogonal matter, we probably should reverse the order of
updating HEAD and the index/working tree, as it does not make much sense
to me to do the former if the latter is not possible (and that is the
case even with the current code).

-Peff

Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think I got us off-track with my expectation of ending the one case
> with a conflicted index. But caring about that is even more unlikely.  I
> think Thomas's original patch is probably a happy medium.

OK, so should I consider it Reviewed-by: peff?

> As an orthogonal matter, we probably should reverse the order of
> updating HEAD and the index/working tree, as it does not make much sense
> to me to do the former if the latter is not possible (and that is the
> case even with the current code).

Yes.

Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree

[Jeff King]

On Thu, Jun 20, 2013 at 02:45:04PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I think I got us off-track with my expectation of ending the one case
> > with a conflicted index. But caring about that is even more unlikely.  I
> > think Thomas's original patch is probably a happy medium.
> 
> OK, so should I consider it Reviewed-by: peff?

Yes, modulo the breakage of the &&- chain that Thomas mentioned.

> > As an orthogonal matter, we probably should reverse the order of
> > updating HEAD and the index/working tree, as it does not make much sense
> > to me to do the former if the latter is not possible (and that is the
> > case even with the current code).
> 
> Yes.

OK. I'll prepare a series with both patches. Stand by...

-Peff

[PATCHv3 0/2] pull into unborn branch safety tree

[Jeff King]

On Thu, Jun 20, 2013 at 06:03:28PM -0400, Jeff King wrote:

> OK. I'll prepare a series with both patches. Stand by...

Here it is:

  [1/2]: pull: update unborn branch tip after index
  [2/2]: pull: merge into unborn by fast-forwarding from empty tree

The first one is mine. The second is Thomas's v2, rebased on top of
mine. I also added a note to the commit message explaining the resulting
index state and lack of conflicts.

-Peff

[PATCH 1/2] pull: update unborn branch tip after index

[Jeff King]

When commit d09e79c taught git to pull into an unborn
branch, it first updated the unborn branch to point at the
pulled commit, and then used read-tree to update the index
and working tree. That ordering made sense, since any
failure of the latter step would be due to filesystem
errors, and one could then recover with "git reset --hard".

Later, commit 4b3ffe5 added extra safety for existing files
in the working tree by asking read-tree to bail out when it
would overwrite such a file. This error mode is much less
"your pull failed due to random errors" and more like "we
reject this pull because it would lose data". In that case,
it makes sense not to update the HEAD ref, just as a regular
rejected merge would do.

This patch reverses the order of the update-ref and
read-tree calls, so that we do not touch the HEAD ref at all if a
merge is rejected. This also means that we would not update
HEAD in case of a transient filesystem error, but those are
presumably less rare (and one can still recover by repeating
the pull, or by accessing FETCH_HEAD directly).

While we're reorganizing the code, we can drop the "exit 1"
from the end of our command chain. We exit immediately
either way, and just calling exit without an argument will
use the exit code from the last command.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-pull.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index 638aabb..7904753 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -268,8 +268,8 @@ then
 
 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
+	git read-tree -m -u $merge_head &&
+	git update-ref -m "initial pull" HEAD $merge_head "$curr_head"
 	exit
 fi
 
-- 
1.8.3.rc2.14.g7eee6b3

[PATCH 2/2] pull: merge into unborn by fast-forwarding from empty tree

[Jeff King]

From: Thomas Rast <trast@inf.ethz.ch>

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.

Note that this use of read-tree just gives us protection
against overwriting index and working tree changes. It will
not actually result in a 3-way merge conflict in the index.
This is fine, as this is a rare situation, and the conflict
would not be interesting anyway (it must, by definition, be
an add/add conflict with the whole content conflicting). And
it makes it simpler for the user to recover, as they have no
HEAD to "git reset" back to.

Reported-by: Stefan Schüßler <mail@stefanschuessler.de>
Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
Signed-off-by: Jeff King <peff@peff.net>
---
 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 7904753..6828e2c 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -266,9 +266,16 @@ then
 	;;
 esac
 
+# Pulling into unborn branch: a shorthand for branching off
+# FETCH_HEAD, for lazy typers.
 if test -z "$orig_head"
 then
-	git read-tree -m -u $merge_head &&
+	# 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 $merge_head &&
 	git update-ref -m "initial pull" HEAD $merge_head "$curr_head"
 	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.rc2.14.g7eee6b3

Re: [PATCHv3 0/2] pull into unborn branch safety tree

[Junio C Hamano]

Thanks; both look obviously good.

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

[Ramkumar Ramachandra]

Thomas Rast wrote:
> 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

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