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

Stefan Schüßler <mail@stefanschuessler.de> writes:

[...]

Thanks for the bug report!

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

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

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.

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

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

Jeff King <peff@peff.net> writes:

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

On Thu, Jun 20, 2013 at 03:06:01PM +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.

-Peff

Jeff King <peff@peff.net> writes:

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

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

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

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

Yes, that's what I meant.

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

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

Jeff King <peff@peff.net> writes:

[...]

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

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

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.ethz.ch> writes:

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

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

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

Jeff King <peff@peff.net> writes:

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

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.

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

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

Jeff King <peff@peff.net> writes:

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

Yes.

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

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

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

-Peff

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

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

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

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

Thanks; both look obviously good.

Thomas Rast wrote:

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