From 3cee92369ec8cc908fe3a4307f08cfc22a9ba954 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 12 Aug 2010 19:50:49 -0600 Subject: [PATCH 1/2] t5520-pull: Add testcases showing spurious conflicts from git pull --rebase Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t5520-pull.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index dd2ee842e0..01f898e57a 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -4,6 +4,11 @@ test_description='pulling into void' . ./test-lib.sh +modify () { + sed -e "$1" <"$2" >"$2.x" && + mv "$2.x" "$2" +} + D=`pwd` test_expect_success setup ' @@ -160,4 +165,61 @@ test_expect_success 'pull --rebase works on branch yet to be born' ' test_cmp expect actual ' +test_expect_success 'setup for detecting upstreamed changes' ' + mkdir src && + (cd src && + git init && + printf "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n" > stuff && + git add stuff && + git commit -m "Initial revision" + ) && + git clone src dst && + (cd src && + modify s/5/43/ stuff && + git commit -a -m "5->43" && + modify s/6/42/ stuff && + git commit -a -m "Make it bigger" + ) && + (cd dst && + modify s/5/43/ stuff && + git commit -a -m "Independent discovery of 5->43" + ) +' + +test_expect_failure 'git pull --rebase detects upstreamed changes' ' + (cd dst && + git pull --rebase && + test -z "$(git ls-files -u)" + ) +' + +test_expect_success 'setup for avoiding reapplying old patches' ' + (cd dst && + test_might_fail git rebase --abort && + git reset --hard origin/master + ) && + git clone --bare src src-replace.git && + rm -rf src && + mv src-replace.git src && + (cd dst && + modify s/2/22/ stuff && + git commit -a -m "Change 2" && + modify s/3/33/ stuff && + git commit -a -m "Change 3" && + modify s/4/44/ stuff && + git commit -a -m "Change 4" && + git push && + + modify s/44/55/ stuff && + git commit --amend -a -m "Modified Change 4" + ) +' + +test_expect_failure 'git pull --rebase does not reapply old patches' ' + (cd dst && + test_must_fail git pull --rebase && + test 1 = $(find .git/rebase-apply -name "000*" | wc -l) + ) +' + test_done From cf65426de682bf4e336eddd2964a7c1585597f48 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 12 Aug 2010 19:50:50 -0600 Subject: [PATCH 2/2] pull --rebase: Avoid spurious conflicts and reapplying unnecessary patches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to c85c792 (pull --rebase: be cleverer with rebased upstream branches, 2008-01-26), pull --rebase would run git rebase $merge_head which resulted in a call to git format-patch ... --ignore-if-in-upstream $merge_head..$cur_branch This resulted in patches from $merge_head..$cur_branch being applied, as long as they did not already exist in $cur_branch..$merge_head. Unfortunately, when upstream is rebased, $merge_head..$cur_branch also refers to "old" commits that have already been rebased upstream, meaning that many patches that were already fixed upstream would be reapplied. This could result in many spurious conflicts, as well as reintroduce patches that were intentionally dropped upstream. So the algorithm was changed in c85c792 (pull --rebase: be cleverer with rebased upstream branches, 2008-01-26) and d44e712 (pull: support rebased upstream + fetch + pull --rebase, 2009-07-19). Defining $old_remote_ref to be the most recent entry in the reflog for @{upstream} that is an ancestor of $cur_branch, pull --rebase was changed to run git rebase --onto $merge_head $old_remote_ref which results in a call to git format-patch ... --ignore-if-in-upstream $old_remote_ref..$cur_branch The whole point of this change was to reduce the number of commits being reapplied, by avoiding commits that upstream already has or had. In the rebased upstream case, this change achieved that purpose. It is worth noting, though, that since $old_remote_ref is always an ancestor of $cur_branch (by its definition), format-patch will not know what upstream is and thus will not be able to determine if any patches are already upstream; they will all be reapplied. In the non-rebased upstream case, this new form is usually the same as the original code but in some cases $old_remote_ref can be an ancestor of $(git merge-base $merge_head $cur_branch) meaning that instead of avoiding reapplying commits that upstream already has, it actually includes more such commits. Combined with the fact that format-patch can no longer detect commits that are already upstream (since it is no longer told what upstream is), results in lots of confusion for users (e.g. "git is giving me lots of conflicts in stuff I didn't even change since my last push.") Cases where additional commits could be reapplied include forking from a commit other than the tracking branch, or amending/rebasing after pushing. Cases where the inability to detect upstreamed commits cause problems include independent discovery of a fix and having your patches get upstreamed by some alternative route (e.g. pulling your changes to a third machine, pushing from there, and then going back to your original machine and trying to pull --rebase). Fix the non-rebased upstream case by ignoring $old_remote_ref whenever it is contained in $(git merge-base $merge_head $cur_branch). This should have no affect on the rebased upstream case. Acked-by: Santi Béjar Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- git-pull.sh | 9 +++++++++ t/t5520-pull.sh | 4 ++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/git-pull.sh b/git-pull.sh index 38331a8611..5dbc4387f7 100755 --- a/git-pull.sh +++ b/git-pull.sh @@ -266,6 +266,15 @@ then exit fi +if test true = "$rebase" +then + o=$(git show-branch --merge-base $curr_branch $merge_head $oldremoteref) + if test "$oldremoteref" = "$o" + then + unset oldremoteref + fi +fi + merge_name=$(git fmt-merge-msg $log_arg <"$GIT_DIR/FETCH_HEAD") || exit case "$rebase" in true) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 01f898e57a..eee6f6d9cc 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -186,7 +186,7 @@ test_expect_success 'setup for detecting upstreamed changes' ' ) ' -test_expect_failure 'git pull --rebase detects upstreamed changes' ' +test_expect_success 'git pull --rebase detects upstreamed changes' ' (cd dst && git pull --rebase && test -z "$(git ls-files -u)" @@ -215,7 +215,7 @@ test_expect_success 'setup for avoiding reapplying old patches' ' ) ' -test_expect_failure 'git pull --rebase does not reapply old patches' ' +test_expect_success 'git pull --rebase does not reapply old patches' ' (cd dst && test_must_fail git pull --rebase && test 1 = $(find .git/rebase-apply -name "000*" | wc -l)