[PATCH v2] pull: merge into unborn 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 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 m...@stefanschuessler.de Signed-off-by: Thomas Rast tr...@inf.ethz.ch --- Jeff King p...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree
Jeff King p...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree
Thomas Rast tr...@inf.ethz.ch writes:
Jeff King p...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree
Thomas Rast tr...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree
Jeff King p...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree
Thomas Rast tr...@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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree
On Thu, Jun 20, 2013 at 11:43:37AM -0700, Junio C Hamano wrote: Thomas Rast tr...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree
Jeff King p...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree
Jeff King p...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree
On Thu, Jun 20, 2013 at 02:45:04PM -0700, Junio C Hamano wrote: Jeff King p...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
