{"thread":{"id":"34229","subject":"[PATCH] pull: pull into void by fast-forwarding from empty tree","startedAt":"2013-06-20T12:36:03Z","lastAt":"2013-06-20T22:52:57Z","messageCount":20,"participants":["Thomas Rast","Jeff King","Ramkumar Ramachandra","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"221469","messageId":"45f6841746e5dcea03a97fc3ea24aef274728023.1371731513.git.trast@inf.ethz.ch","threadId":"34229","inReplyTo":null,"subject":"[PATCH] pull: pull into void by fast-forwarding from empty tree","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-06-20T12:36:03Z","receivedAt":"2013-06-20T12:36:03Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The logic for pulling into an unborn branch was originally designed to\nbe used on a newly-initialized repository (d09e79c, git-pull: allow\npulling into an empty repository, 2006-11-16). It thus did not\ninitially deal with uncommitted changes in the unborn branch. The\ncase of an _unstaged_ untracked file was fixed by by 4b3ffe5 (pull: do\nnot clobber untracked files on initial pull, 2011-03-25). However, it\nstill clobbered existing staged files, both when the file exists in\nthe merged commit (it will be overwritten), and when it does not (it\nwill be lost!).\n\nWe fix this by doing a two-way merge, where the \"current\" side of the\nmerge is an empty tree, and the \"target\" side is HEAD (already updated\nto FETCH_HEAD at this point). This amounts to claiming that all work\nin the index was done vs. an empty tree, and thus all content of the\nindex is precious.\n\nReported-by: Stefan Schüßler \nSigned-off-by: Thomas Rast \n---\n\nStefan Schüßler writes:\n\n> I think there's a bug in git pull. Doing a git pull in a fresh\n> repository without any commits removes files in the index.\n[...]\n\nThanks for the bug report!\n\n git-pull.sh | 9 ++++++++-\n t/t5520-pull.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex 638aabb..1f84383 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -266,10 +266,17 @@ case \"$merge_head\" in\n \t;;\n esac\n \n+# Pulling into unborn branch: a shorthand for branching off\n+# FETCH_HEAD, for lazy typers.\n if test -z \"$orig_head\"\n then\n \tgit update-ref -m \"initial pull\" HEAD $merge_head \"$curr_head\" &&\n-\tgit read-tree -m -u HEAD || exit 1\n+\t# Two-way merge: we claim the index is based on an empty tree,\n+\t# and try to fast-forward to HEAD. This ensures we will not\n+\t# lose index/worktree changes that the user already made on\n+\t# the unborn branch.\n+\tempty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n+\tgit read-tree -m -u $empty_tree HEAD || exit 1\n \texit\n fi\n \ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 6af6c63..927903c 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -57,6 +57,30 @@ test_expect_success 'pulling into void does not overwrite untracked files' '\n \t)\n '\n \n+test_expect_success 'pulling into void does not overwrite staged files' '\n+\tgit init cloned-staged-colliding &&\n+\t(\n+\t\tcd cloned-staged-colliding &&\n+\t\techo \"alternate content\" >file &&\n+\t\tgit add file &&\n+\t\ttest_must_fail git pull .. master &&\n+\t\techo \"alternate content\" >expect &&\n+\t\ttest_cmp expect file\n+\t)\n+'\n+\n+test_expect_success 'pulling into void does not remove new staged files' '\n+\tgit init cloned-staged-new &&\n+\t(\n+\t\tcd cloned-staged-new &&\n+\t\techo \"new tracked file\" >newfile &&\n+\t\tgit add newfile &&\n+\t\tgit pull .. master &&\n+\t\techo \"new tracked file\" >expect &&\n+\t\ttest_cmp expect newfile\n+\t)\n+'\n+\n test_expect_success 'test . as a remote' '\n \n \tgit branch copy master &&\n-- \n1.8.3.1.664.gae9f72a\n"},{"id":"221472","messageId":"20130620124758.GA2376@sigill.intra.peff.net","threadId":"34229","inReplyTo":"45f6841746e5dcea03a97fc3ea24aef274728023.1371731513.git.trast@inf.ethz.ch","subject":"Re: [PATCH] pull: pull into void by fast-forwarding from empty tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-20T12:47:58Z","receivedAt":"2013-06-20T12:47:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 20, 2013 at 02:36:03PM +0200, Thomas Rast wrote:\n\n> The logic for pulling into an unborn branch was originally designed to\n> be used on a newly-initialized repository (d09e79c, git-pull: allow\n> pulling into an empty repository, 2006-11-16). It thus did not\n> initially deal with uncommitted changes in the unborn branch. The\n> case of an _unstaged_ untracked file was fixed by by 4b3ffe5 (pull: do\n> not clobber untracked files on initial pull, 2011-03-25). However, it\n> still clobbered existing staged files, both when the file exists in\n> the merged commit (it will be overwritten), and when it does not (it\n> will be lost!).\n\nYeah, in 4beffe5 I just assumed that using \"read-tree -m\" would give us\nthe protections we need. But obviously I didn't think about the fact\nthat we are not giving it enough information.\n\n> We fix this by doing a two-way merge, where the \"current\" side of the\n> merge is an empty tree, and the \"target\" side is HEAD (already updated\n> to FETCH_HEAD at this point). This amounts to claiming that all work\n> in the index was done vs. an empty tree, and thus all content of the\n> index is precious.\n\nThis seems like the correct fix; it is giving read-tree the right\ninformation to make the decision. Thanks for working on this.\n\n> +test_expect_success 'pulling into void does not overwrite staged files' '\n> +\tgit init cloned-staged-colliding &&\n> +\t(\n> +\t\tcd cloned-staged-colliding &&\n> +\t\techo \"alternate content\" >file &&\n> +\t\tgit add file &&\n> +\t\ttest_must_fail git pull .. master &&\n> +\t\techo \"alternate content\" >expect &&\n> +\t\ttest_cmp expect file\n> +\t)\n> +'\n> +\n> +test_expect_success 'pulling into void does not remove new staged files' '\n> +\tgit init cloned-staged-new &&\n> +\t(\n> +\t\tcd cloned-staged-new &&\n> +\t\techo \"new tracked file\" >newfile &&\n> +\t\tgit add newfile &&\n> +\t\tgit pull .. master &&\n> +\t\techo \"new tracked file\" >expect &&\n> +\t\ttest_cmp expect newfile\n> +\t)\n> +'\n\nDo we want to also check the index state after each pull? In the former\ncase, I think it should obviously represent a conflict. In the latter,\nwe should be retaining the index contents of newfile.\n\nThese are basic things that read-tree's two-way merge should get right\n(and are presumably tested elsewhere), but it might be worth confirming\nthe desired behavior here in case somebody later tries to tweak this\ncode path not to use read-tree.\n\n-Peff\n"},{"id":"221476","messageId":"aca810600b895ed3f0a3fc575e0f6861e591de5b.1371733403.git.trast@inf.ethz.ch","threadId":"34229","inReplyTo":"20130620124758.GA2376@sigill.intra.peff.net","subject":"[PATCH v2] pull: merge into unborn by fast-forwarding from empty tree","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-06-20T13:06:01Z","receivedAt":"2013-06-20T13:06:01Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The logic for pulling into an unborn branch was originally designed to\nbe used on a newly-initialized repository (d09e79c, git-pull: allow\npulling into an empty repository, 2006-11-16). It thus did not\ninitially deal with uncommitted changes in the unborn branch. The\ncase of an _unstaged_ untracked file was fixed by 4b3ffe5 (pull: do\nnot clobber untracked files on initial pull, 2011-03-25). However, it\nstill clobbered existing staged files, both when the file exists in\nthe merged commit (it will be overwritten), and when it does not (it\nwill be deleted).\n\nWe fix this by doing a two-way merge, where the \"current\" side of the\nmerge is an empty tree, and the \"target\" side is HEAD (already updated\nto FETCH_HEAD at this point). This amounts to claiming that all work\nin the index was done vs. an empty tree, and thus all content of the\nindex is precious.\n\nReported-by: Stefan Schüßler \nSigned-off-by: Thomas Rast \n---\n\nJeff King writes:\n\n> Do we want to also check the index state after each pull? In the former\n> case, I think it should obviously represent a conflict. In the latter,\n> we should be retaining the index contents of newfile.\n>\n> These are basic things that read-tree's two-way merge should get right\n> (and are presumably tested elsewhere), but it might be worth confirming\n> the desired behavior here in case somebody later tries to tweak this\n> code path not to use read-tree.\n\nRight, good point.\n\nI also reworded the subject and message somewhat to read better.\n\n\n git-pull.sh | 9 ++++++++-\n t/t5520-pull.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 37 insertions(+), 1 deletion(-)\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex 638aabb..1f84383 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -266,10 +266,17 @@ case \"$merge_head\" in\n \t;;\n esac\n \n+# Pulling into unborn branch: a shorthand for branching off\n+# FETCH_HEAD, for lazy typers.\n if test -z \"$orig_head\"\n then\n \tgit update-ref -m \"initial pull\" HEAD $merge_head \"$curr_head\" &&\n-\tgit read-tree -m -u HEAD || exit 1\n+\t# Two-way merge: we claim the index is based on an empty tree,\n+\t# and try to fast-forward to HEAD. This ensures we will not\n+\t# lose index/worktree changes that the user already made on\n+\t# the unborn branch.\n+\tempty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n+\tgit read-tree -m -u $empty_tree HEAD || exit 1\n \texit\n fi\n \ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 6af6c63..ed4d9c8 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -57,6 +57,35 @@ test_expect_success 'pulling into void does not overwrite untracked files' '\n \t)\n '\n \n+test_expect_success 'pulling into void does not overwrite staged files' '\n+\tgit init cloned-staged-colliding &&\n+\t(\n+\t\tcd cloned-staged-colliding &&\n+\t\techo \"alternate content\" >file &&\n+\t\tgit add file &&\n+\t\ttest_must_fail git pull .. master &&\n+\t\techo \"alternate content\" >expect &&\n+\t\ttest_cmp expect file &&\n+\t\tgit cat-file blob :file >file.index &&\n+\t\ttest_cmp expect file.index\n+\t)\n+'\n+\n+\n+test_expect_success 'pulling into void does not remove new staged files' '\n+\tgit init cloned-staged-new &&\n+\t(\n+\t\tcd cloned-staged-new &&\n+\t\techo \"new tracked file\" >newfile &&\n+\t\tgit add newfile &&\n+\t\tgit pull .. master &&\n+\t\techo \"new tracked file\" >expect &&\n+\t\ttest_cmp expect newfile &&\n+\t\tgit cat-file blob :newfile >newfile.index &&\n+\t\ttest_cmp expect newfile.index\n+\t)\n+'\n+\n test_expect_success 'test . as a remote' '\n \n \tgit branch copy master &&\n-- \n1.8.3.1.664.gae9f72a\n"},{"id":"221478","messageId":"20130620131547.GA11073@sigill.intra.peff.net","threadId":"34229","inReplyTo":"aca810600b895ed3f0a3fc575e0f6861e591de5b.1371733403.git.trast@inf.ethz.ch","subject":"Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-20T13:15:47Z","receivedAt":"2013-06-20T13:15:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 20, 2013 at 03:06:01PM +0200, Thomas Rast wrote:\n\n> +test_expect_success 'pulling into void does not overwrite staged files' '\n> +\tgit init cloned-staged-colliding &&\n> +\t(\n> +\t\tcd cloned-staged-colliding &&\n> +\t\techo \"alternate content\" >file &&\n> +\t\tgit add file &&\n> +\t\ttest_must_fail git pull .. master &&\n> +\t\techo \"alternate content\" >expect &&\n> +\t\ttest_cmp expect file &&\n> +\t\tgit cat-file blob :file >file.index &&\n> +\t\ttest_cmp expect file.index\n> +\t)\n> +'\n\nI naively would have expected this to leave us in a conflicted state\nover \"file\". But I guess read-tree just rejects it, because we are not\ndoing a real three-way merge. I'm not sure it is that big a deal; this\nis more about safety than about creating a conflicted/resolvable state.\n\n-Peff\n"},{"id":"221482","messageId":"87zjuk6hr2.fsf@linux-k42r.v.cablecom.net","threadId":"34229","inReplyTo":"20130620131547.GA11073@sigill.intra.peff.net","subject":"Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-06-20T13:20:33Z","receivedAt":"2013-06-20T13:20:33Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jun 20, 2013 at 03:06:01PM +0200, Thomas Rast wrote:\n>\n>> +test_expect_success 'pulling into void does not overwrite staged files' '\n>> +\tgit init cloned-staged-colliding &&\n>> +\t(\n>> +\t\tcd cloned-staged-colliding &&\n>> +\t\techo \"alternate content\" >file &&\n>> +\t\tgit add file &&\n>> +\t\ttest_must_fail git pull .. master &&\n>> +\t\techo \"alternate content\" >expect &&\n>> +\t\ttest_cmp expect file &&\n>> +\t\tgit cat-file blob :file >file.index &&\n>> +\t\ttest_cmp expect file.index\n>> +\t)\n>> +'\n>\n> I naively would have expected this to leave us in a conflicted state\n> over \"file\". But I guess read-tree just rejects it, because we are not\n> doing a real three-way merge. I'm not sure it is that big a deal; this\n> is more about safety than about creating a conflicted/resolvable state.\n\nNote that the test_must_fail essentially tests that the merge is rejected.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"221484","messageId":"87mwqk6h5d.fsf@linux-k42r.v.cablecom.net","threadId":"34229","inReplyTo":"87zjuk6hr2.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-06-20T13:33:34Z","receivedAt":"2013-06-20T13:33:34Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Thomas Rast writes:\n\n> Jeff King writes:\n>\n>> On Thu, Jun 20, 2013 at 03:06:01PM +0200, Thomas Rast wrote:\n>>\n>>> +test_expect_success 'pulling into void does not overwrite staged files' '\n>>> +\tgit init cloned-staged-colliding &&\n>>> +\t(\n>>> +\t\tcd cloned-staged-colliding &&\n>>> +\t\techo \"alternate content\" >file &&\n>>> +\t\tgit add file &&\n>>> +\t\ttest_must_fail git pull .. master &&\n>>> +\t\techo \"alternate content\" >expect &&\n>>> +\t\ttest_cmp expect file &&\n>>> +\t\tgit cat-file blob :file >file.index &&\n>>> +\t\ttest_cmp expect file.index\n>>> +\t)\n>>> +'\n>>\n>> I naively would have expected this to leave us in a conflicted state\n>> over \"file\". But I guess read-tree just rejects it, because we are not\n>> doing a real three-way merge. I'm not sure it is that big a deal; this\n>> is more about safety than about creating a conflicted/resolvable state.\n>\n> Note that the test_must_fail essentially tests that the merge is rejected.\n\nBah, no it doesn't, a conflicting merge also returns a nonzero status.\nSigh.\n\nIf you meant we should actually conflict, I'm not sure what options\nthere would be other than actually calling a merge driver. And we could\nactually do this like so (it'll obviously break the tests):\n\ndiff --git i/git-pull.sh w/git-pull.sh\nindex 1f84383..b3d36a8 100755\n--- i/git-pull.sh\n+++ w/git-pull.sh\n@@ -276,7 +276,7 @@ then\n \t# lose index/worktree changes that the user already made on\n \t# the unborn branch.\n \tempty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n-\tgit read-tree -m -u $empty_tree HEAD || exit 1\n+\tgit merge-recursive $empty_tree -- $(git write-tree) HEAD || exit 1\n \texit\n fi\n \n\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"221486","messageId":"20130620134726.GB18200@sigill.intra.peff.net","threadId":"34229","inReplyTo":"87mwqk6h5d.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-20T13:47:26Z","receivedAt":"2013-06-20T13:47:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 20, 2013 at 03:33:34PM +0200, Thomas Rast wrote:\n\n> >> I naively would have expected this to leave us in a conflicted state\n> >> over \"file\". But I guess read-tree just rejects it, because we are not\n> >> doing a real three-way merge. I'm not sure it is that big a deal; this\n> >> is more about safety than about creating a conflicted/resolvable state.\n> >\n> > Note that the test_must_fail essentially tests that the merge is rejected.\n> \n> Bah, no it doesn't, a conflicting merge also returns a nonzero status.\n> Sigh.\n> \n> If you meant we should actually conflict,\n\nYes, that's what I meant.\n\n> I'm not sure what options there would be other than actually calling a\n> merge driver. And we could actually do this like so (it'll obviously\n> break the tests):\n\nI'd rather not invoke a merge driver directly if we can avoid it. I\nthink you could get rid of this special code-path entirely if you just\nlied to the \"git-merge\" and said \"the ancestor and current tree are fake\ncommits with an empty tree\", and then followed the usual path. But that\nlying through git-merge is ugly and complicated (and is more or less\nwhat you're doing with the merge-recursive patch here).\n\n> diff --git i/git-pull.sh w/git-pull.sh\n> index 1f84383..b3d36a8 100755\n> --- i/git-pull.sh\n> +++ w/git-pull.sh\n> @@ -276,7 +276,7 @@ then\n> \t# lose index/worktree changes that the user already made on\n> \t# the unborn branch.\n> \tempty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n> -\tgit read-tree -m -u $empty_tree HEAD || exit 1\n> +\tgit merge-recursive $empty_tree -- $(git write-tree) HEAD || exit 1\n\nI don't think there is any advantage to using merge-recursive over\nread-tree here, in the sense that there cannot be any interesting\ncontent-level merging going on (our ancestor is the empty tree, so we\nknow that differing content cannot be resolved).\n\nSo I think you could just use read-tree with a 3-way merge, but I cannot\nseem to provoke it to leave a conflict. Hrm.\n\nI also noticed that the procedure in this code path is:\n\n 1. Update HEAD with the fetched ref.\n\n 2. Checkout the contents with read-tree.\n\nI wonder if it would make sense to update HEAD only _after_ we had\nresolved successfully. As it is now, you are left in a weird state where\npull has reported failure, but we actually update the HEAD (and \"git\nstatus\" afterwards reflects that you are building on top of the pulled\nHEAD).\n\n-Peff\n"},{"id":"221488","messageId":"87hags6ewl.fsf@linux-k42r.v.cablecom.net","threadId":"34229","inReplyTo":"aca810600b895ed3f0a3fc575e0f6861e591de5b.1371733403.git.trast@inf.ethz.ch","subject":"Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-06-20T14:22:02Z","receivedAt":"2013-06-20T14:22:02Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Thomas Rast writes:\n\n> if test -z \"$orig_head\"\n> then\n> \tgit update-ref -m \"initial pull\" HEAD $merge_head \"$curr_head\" &&\n> -\tgit read-tree -m -u HEAD || exit 1\n> +\t# Two-way merge: we claim the index is based on an empty tree,\n> +\t# and try to fast-forward to HEAD. This ensures we will not\n> +\t# lose index/worktree changes that the user already made on\n> +\t# the unborn branch.\n> +\tempty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n> +\tgit read-tree -m -u $empty_tree HEAD || exit 1\n> \texit\n> fi\n\nI just noticed that I broke the && chaining here, so don't apply this\njust yet. I'll resend once we settle on the best strategy to generate\nconflicts (or not).\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"221489","messageId":"87mwqk4zzu.fsf@linux-k42r.v.cablecom.net","threadId":"34229","inReplyTo":"20130620134726.GB18200@sigill.intra.peff.net","subject":"Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-06-20T14:29:25Z","receivedAt":"2013-06-20T14:29:25Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jun 20, 2013 at 03:33:34PM +0200, Thomas Rast wrote:\n>\n>> >> I naively would have expected this to leave us in a conflicted state\n>> >> over \"file\". But I guess read-tree just rejects it, because we are not\n>> >> doing a real three-way merge. I'm not sure it is that big a deal; this\n>> >> is more about safety than about creating a conflicted/resolvable state.\n>> >\n>> > Note that the test_must_fail essentially tests that the merge is rejected.\n>> \n>> Bah, no it doesn't, a conflicting merge also returns a nonzero status.\n>> Sigh.\n>> \n>> If you meant we should actually conflict,\n>\n> Yes, that's what I meant.\n[...]\n>> diff --git i/git-pull.sh w/git-pull.sh\n>> index 1f84383..b3d36a8 100755\n>> --- i/git-pull.sh\n>> +++ w/git-pull.sh\n>> @@ -276,7 +276,7 @@ then\n>> \t# lose index/worktree changes that the user already made on\n>> \t# the unborn branch.\n>> \tempty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n>> -\tgit read-tree -m -u $empty_tree HEAD || exit 1\n>> +\tgit merge-recursive $empty_tree -- $(git write-tree) HEAD || exit 1\n>\n> I don't think there is any advantage to using merge-recursive over\n> read-tree here, in the sense that there cannot be any interesting\n> content-level merging going on (our ancestor is the empty tree, so we\n> know that differing content cannot be resolved).\n>\n> So I think you could just use read-tree with a 3-way merge, but I cannot\n> seem to provoke it to leave a conflict. Hrm.\n\nI guess read-tree doesn't consider that its job; it leaves the conflict\nin the index alright for me if I do this:\n\n git-pull.sh | 4 ++--\n t/t5520-pull.sh | 5 +++--\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git i/git-pull.sh w/git-pull.sh\nindex 1f84383..4047d02 100755\n--- i/git-pull.sh\n+++ w/git-pull.sh\n@@ -275,8 +275,8 @@ then\n \t# and try to fast-forward to HEAD. This ensures we will not\n \t# lose index/worktree changes that the user already made on\n \t# the unborn branch.\n-\tempty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n-\tgit read-tree -m -u $empty_tree HEAD || exit 1\n+\tempty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904 &&\n+\tgit read-tree -m -u $empty_tree $(git write-tree) HEAD || exit 1\n \texit\n fi\n \n\nBut it won't write the conflict markers in the worktree.\n\nOn the topic of \"do we want to conflict\": one issue is that we don't\nhave a prior state to go to, since it was never committed. Not even the\nimplicit empty tree can be passed to 'reset --keep'. So it might be\nbetter to *avoid* creating conflict hunks -- and fail -- so as to avoid\ngiving the user a state that is hard to back out of.\n\nIn the same spirit I would also support this:\n\n> I wonder if it would make sense to update HEAD only _after_ we had\n> resolved successfully. As it is now, you are left in a weird state where\n> pull has reported failure, but we actually update the HEAD (and \"git\n> status\" afterwards reflects that you are building on top of the pulled\n> HEAD).\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"221492","messageId":"CALkWK0k-SaYz2uCpeXnTTL7HvBQ=r1PhWCJeDvdpuKL6qhDoig@mail.gmail.com","threadId":"34229","inReplyTo":"45f6841746e5dcea03a97fc3ea24aef274728023.1371731513.git.trast@inf.ethz.ch","subject":"Re: [PATCH] pull: pull into void by fast-forwarding from empty tree","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-20T16:04:17Z","receivedAt":"2013-06-20T16:04:17Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Thomas Rast wrote:\n> diff --git a/git-pull.sh b/git-pull.sh\n> index 638aabb..1f84383 100755\n> --- a/git-pull.sh\n> +++ b/git-pull.sh\n> @@ -266,10 +266,17 @@ case \"$merge_head\" in\n> ;;\n> esac\n>\n> +# Pulling into unborn branch: a shorthand for branching off\n> +# FETCH_HEAD, for lazy typers.\n> if test -z \"$orig_head\"\n> then\n> git update-ref -m \"initial pull\" HEAD $merge_head \"$curr_head\" &&\n> - git read-tree -m -u HEAD || exit 1\n> + # Two-way merge: we claim the index is based on an empty tree,\n> + # and try to fast-forward to HEAD. This ensures we will not\n> + # lose index/worktree changes that the user already made on\n> + # the unborn branch.\n> + empty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n\nPerhaps replace this magic with $(git hash-object -t tree /dev/null)\nor $(git mktree writes:\n\n> The logic for pulling into an unborn branch was originally designed to\n> be used on a newly-initialized repository (d09e79c, git-pull: allow\n> pulling into an empty repository, 2006-11-16). It thus did not\n> initially deal with uncommitted changes in the unborn branch. The\n> case of an _unstaged_ untracked file was fixed by 4b3ffe5 (pull: do\n> not clobber untracked files on initial pull, 2011-03-25). However, it\n> still clobbered existing staged files, both when the file exists in\n> the merged commit (it will be overwritten), and when it does not (it\n> will be deleted).\n\nPerhaps making sure the index is empty is sufficient, then?\n"},{"id":"221512","messageId":"20130620201957.GC31364@sigill.intra.peff.net","threadId":"34229","inReplyTo":"7v8v24vd0m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-20T20:19:57Z","receivedAt":"2013-06-20T20:19:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 20, 2013 at 11:43:37AM -0700, Junio C Hamano wrote:\n\n> Thomas Rast writes:\n> \n> > The logic for pulling into an unborn branch was originally designed to\n> > be used on a newly-initialized repository (d09e79c, git-pull: allow\n> > pulling into an empty repository, 2006-11-16). It thus did not\n> > initially deal with uncommitted changes in the unborn branch. The\n> > case of an _unstaged_ untracked file was fixed by 4b3ffe5 (pull: do\n> > not clobber untracked files on initial pull, 2011-03-25). However, it\n> > still clobbered existing staged files, both when the file exists in\n> > the merged commit (it will be overwritten), and when it does not (it\n> > will be deleted).\n> \n> Perhaps making sure the index is empty is sufficient, then?\n\nThat would not let you pull when you have \"foo\" staged, but upstream\ndoes not have \"foo\" at all. To be fair, that is quite a corner case, and\nsimply rejecting the pull entirely may be OK. But read-tree already does\nthe hard work for us, so I don't think it is a lot of code either way.\n\n-Peff\n"},{"id":"221517","messageId":"7vmwqkqzhy.fsf@alter.siamese.dyndns.org","threadId":"34229","inReplyTo":"20130620201957.GC31364@sigill.intra.peff.net","subject":"Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-20T20:49:13Z","receivedAt":"2013-06-20T20:49:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Perhaps making sure the index is empty is sufficient, then?\n>\n> That would not let you pull when you have \"foo\" staged, but upstream\n> does not have \"foo\" at all. To be fair, that is quite a corner case, and\n> simply rejecting the pull entirely may be OK.\n\nThat simplicity was what I was hinting at ;-).\n\n> But read-tree already does\n> the hard work for us, so I don't think it is a lot of code either way.\n\nOK, I just got an impression from reading the back-and-forth between\nyou two that read-tree does not want to deal with that case.\n\nBut yes, if you say \"I have this index, and I am straying away from\nan empty tree to that commit\", with two-tree form \"read-tree -m -u\",\neverything should work correctly, including the bit that says \"nah,\nnah, you have added 'foo' but the other guy also adds 'foo', so I'll\nrefuse\".\n\nSo please scratch that short-cut suggestion.\n"},{"id":"221519","messageId":"20130620205533.GA8074@sigill.intra.peff.net","threadId":"34229","inReplyTo":"7vmwqkqzhy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-20T20:55:33Z","receivedAt":"2013-06-20T20:55:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 20, 2013 at 01:49:13PM -0700, Junio C Hamano wrote:\n\n> > But read-tree already does\n> > the hard work for us, so I don't think it is a lot of code either way.\n> \n> OK, I just got an impression from reading the back-and-forth between\n> you two that read-tree does not want to deal with that case.\n\nI think I got us off-track with my expectation of ending the one case\nwith a conflicted index. But caring about that is even more unlikely. I\nthink Thomas's original patch is probably a happy medium.\n\nAs an orthogonal matter, we probably should reverse the order of\nupdating HEAD and the index/working tree, as it does not make much sense\nto me to do the former if the latter is not possible (and that is the\ncase even with the current code).\n\n-Peff\n"},{"id":"221525","messageId":"7v7ghoqwwv.fsf@alter.siamese.dyndns.org","threadId":"34229","inReplyTo":"20130620205533.GA8074@sigill.intra.peff.net","subject":"Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-20T21:45:04Z","receivedAt":"2013-06-20T21:45:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think I got us off-track with my expectation of ending the one case\n> with a conflicted index. But caring about that is even more unlikely. I\n> think Thomas's original patch is probably a happy medium.\n\nOK, so should I consider it Reviewed-by: peff?\n\n> As an orthogonal matter, we probably should reverse the order of\n> updating HEAD and the index/working tree, as it does not make much sense\n> to me to do the former if the latter is not possible (and that is the\n> case even with the current code).\n\nYes.\n"},{"id":"221527","messageId":"20130620220328.GA3992@sigill.intra.peff.net","threadId":"34229","inReplyTo":"7v7ghoqwwv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-20T22:03:28Z","receivedAt":"2013-06-20T22:03:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 20, 2013 at 02:45:04PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I think I got us off-track with my expectation of ending the one case\n> > with a conflicted index. But caring about that is even more unlikely. I\n> > think Thomas's original patch is probably a happy medium.\n> \n> OK, so should I consider it Reviewed-by: peff?\n\nYes, modulo the breakage of the &&- chain that Thomas mentioned.\n\n> > As an orthogonal matter, we probably should reverse the order of\n> > updating HEAD and the index/working tree, as it does not make much sense\n> > to me to do the former if the latter is not possible (and that is the\n> > case even with the current code).\n> \n> Yes.\n\nOK. I'll prepare a series with both patches. Stand by...\n\n-Peff\n"},{"id":"221531","messageId":"20130620223550.GA21667@sigill.intra.peff.net","threadId":"34229","inReplyTo":"20130620220328.GA3992@sigill.intra.peff.net","subject":"[PATCHv3 0/2] pull into unborn branch safety tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-20T22:35:50Z","receivedAt":"2013-06-20T22:35:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 20, 2013 at 06:03:28PM -0400, Jeff King wrote:\n\n> OK. I'll prepare a series with both patches. Stand by...\n\nHere it is:\n\n [1/2]: pull: update unborn branch tip after index\n [2/2]: pull: merge into unborn by fast-forwarding from empty tree\n\nThe first one is mine. The second is Thomas's v2, rebased on top of\nmine. I also added a note to the commit message explaining the resulting\nindex state and lack of conflicts.\n\n-Peff\n"},{"id":"221532","messageId":"20130620223630.GA18675@sigill.intra.peff.net","threadId":"34229","inReplyTo":"20130620223550.GA21667@sigill.intra.peff.net","subject":"[PATCH 1/2] pull: update unborn branch tip after index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-20T22:36:31Z","receivedAt":"2013-06-20T22:36:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When commit d09e79c taught git to pull into an unborn\nbranch, it first updated the unborn branch to point at the\npulled commit, and then used read-tree to update the index\nand working tree. That ordering made sense, since any\nfailure of the latter step would be due to filesystem\nerrors, and one could then recover with \"git reset --hard\".\n\nLater, commit 4b3ffe5 added extra safety for existing files\nin the working tree by asking read-tree to bail out when it\nwould overwrite such a file. This error mode is much less\n\"your pull failed due to random errors\" and more like \"we\nreject this pull because it would lose data\". In that case,\nit makes sense not to update the HEAD ref, just as a regular\nrejected merge would do.\n\nThis patch reverses the order of the update-ref and\nread-tree calls, so that we do not touch the HEAD ref at all if a\nmerge is rejected. This also means that we would not update\nHEAD in case of a transient filesystem error, but those are\npresumably less rare (and one can still recover by repeating\nthe pull, or by accessing FETCH_HEAD directly).\n\nWhile we're reorganizing the code, we can drop the \"exit 1\"\nfrom the end of our command chain. We exit immediately\neither way, and just calling exit without an argument will\nuse the exit code from the last command.\n\nSigned-off-by: Jeff King \n---\n git-pull.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex 638aabb..7904753 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -268,8 +268,8 @@ then\n \n if test -z \"$orig_head\"\n then\n-\tgit update-ref -m \"initial pull\" HEAD $merge_head \"$curr_head\" &&\n-\tgit read-tree -m -u HEAD || exit 1\n+\tgit read-tree -m -u $merge_head &&\n+\tgit update-ref -m \"initial pull\" HEAD $merge_head \"$curr_head\"\n \texit\n fi\n \n-- \n1.8.3.rc2.14.g7eee6b3\n"},{"id":"221533","messageId":"20130620223828.GB18675@sigill.intra.peff.net","threadId":"34229","inReplyTo":"20130620223550.GA21667@sigill.intra.peff.net","subject":"[PATCH 2/2] pull: merge into unborn by fast-forwarding from empty tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-20T22:38:28Z","receivedAt":"2013-06-20T22:38:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Thomas Rast \n\nThe logic for pulling into an unborn branch was originally\ndesigned to be used on a newly-initialized repository\n(d09e79c, git-pull: allow pulling into an empty repository,\n2006-11-16). It thus did not initially deal with\nuncommitted changes in the unborn branch. The case of an\n_unstaged_ untracked file was fixed by 4b3ffe5 (pull: do not\nclobber untracked files on initial pull, 2011-03-25).\nHowever, it still clobbered existing staged files, both when\nthe file exists in the merged commit (it will be\noverwritten), and when it does not (it will be deleted).\n\nWe fix this by doing a two-way merge, where the \"current\"\nside of the merge is an empty tree, and the \"target\" side is\nHEAD (already updated to FETCH_HEAD at this point). This\namounts to claiming that all work in the index was done vs.\nan empty tree, and thus all content of the index is\nprecious.\n\nNote that this use of read-tree just gives us protection\nagainst overwriting index and working tree changes. It will\nnot actually result in a 3-way merge conflict in the index.\nThis is fine, as this is a rare situation, and the conflict\nwould not be interesting anyway (it must, by definition, be\nan add/add conflict with the whole content conflicting). And\nit makes it simpler for the user to recover, as they have no\nHEAD to \"git reset\" back to.\n\nReported-by: Stefan Schüßler \nSigned-off-by: Thomas Rast \nSigned-off-by: Jeff King \n---\n git-pull.sh | 9 ++++++++-\n t/t5520-pull.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 37 insertions(+), 1 deletion(-)\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex 7904753..6828e2c 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -266,9 +266,16 @@ then\n \t;;\n esac\n \n+# Pulling into unborn branch: a shorthand for branching off\n+# FETCH_HEAD, for lazy typers.\n if test -z \"$orig_head\"\n then\n-\tgit read-tree -m -u $merge_head &&\n+\t# Two-way merge: we claim the index is based on an empty tree,\n+\t# and try to fast-forward to HEAD. This ensures we will not\n+\t# lose index/worktree changes that the user already made on\n+\t# the unborn branch.\n+\tempty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n+\tgit read-tree -m -u $empty_tree $merge_head &&\n \tgit update-ref -m \"initial pull\" HEAD $merge_head \"$curr_head\"\n \texit\n fi\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 6af6c63..ed4d9c8 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -57,6 +57,35 @@ test_expect_success 'pulling into void does not overwrite untracked files' '\n \t)\n '\n \n+test_expect_success 'pulling into void does not overwrite staged files' '\n+\tgit init cloned-staged-colliding &&\n+\t(\n+\t\tcd cloned-staged-colliding &&\n+\t\techo \"alternate content\" >file &&\n+\t\tgit add file &&\n+\t\ttest_must_fail git pull .. master &&\n+\t\techo \"alternate content\" >expect &&\n+\t\ttest_cmp expect file &&\n+\t\tgit cat-file blob :file >file.index &&\n+\t\ttest_cmp expect file.index\n+\t)\n+'\n+\n+\n+test_expect_success 'pulling into void does not remove new staged files' '\n+\tgit init cloned-staged-new &&\n+\t(\n+\t\tcd cloned-staged-new &&\n+\t\techo \"new tracked file\" >newfile &&\n+\t\tgit add newfile &&\n+\t\tgit pull .. master &&\n+\t\techo \"new tracked file\" >expect &&\n+\t\ttest_cmp expect newfile &&\n+\t\tgit cat-file blob :newfile >newfile.index &&\n+\t\ttest_cmp expect newfile.index\n+\t)\n+'\n+\n test_expect_success 'test . as a remote' '\n \n \tgit branch copy master &&\n-- \n1.8.3.rc2.14.g7eee6b3\n"},{"id":"221535","messageId":"7vli64pf7a.fsf@alter.siamese.dyndns.org","threadId":"34229","inReplyTo":"20130620223550.GA21667@sigill.intra.peff.net","subject":"Re: [PATCHv3 0/2] pull into unborn branch safety tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-20T22:52:57Z","receivedAt":"2013-06-20T22:52:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; both look obviously good.\n"}]}