[PATCHv4 0/2] Fix spurious conflicts with pull --rebase

Changes since v1 (since v3, only Junio's testcase style issue was fixed):
  * Wording changes in the commit log
  * Address style and format issues for the testcase, raised by Ævar
    Arnfjörð Bjarmason, Junio, and Hannes.

This patch series fixes spurious conflict issues with git pull
--rebase for the case where the upstream repository is *not* rebased.
(There is no change in the case where the upstream repository is
rebased.)

In c85c79279d and d44e71261f, the call to git-rebase was modified in
an effort to reduce the number of commits being reapplied, by trying
to avoid commits that upstream already had or has.  It was
specifically designed with an upstream that is rebased in mind.
Unfortunately, it had two side effects for the non-rebased upstream
case: (1) it prevented detecting if "local" patches are already
upstream, and (2) it could in some cases cause more patches known to
be upstream to be reapplied rather than less.  This series fixes both
of these issues for the non-rebased upstream case.  See the commit
message of the second patch for details.

It's worth noting that issue (1) above also affects the case where the
upstream repository has been rebased, which this patch series does not
address.  As far as I can tell, fixing it would require changes
(including new syntax) to format-patch to allow it to be told what
'upstream' is, and some changes to git-pull.sh/git-rebase.sh to pass
it this information.

Elijah Newren (2):
  t5520-pull: Add testcases showing spurious conflicts from git pull
    --rebase
  pull --rebase: Avoid spurious conflicts and reapplying unnecessary
    patches

 git-pull.sh     |   34 +++++++++++++++++++----------
 t/t5520-pull.sh |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+), 12 deletions(-)

-- 
1.7.2.1.43.gbae63

Signed-off-by: Elijah Newren <redacted>
---
 t/t5520-pull.sh |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 319e389..85a6b23 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

-- 
1.7.2.1.43.gbae63

Prior to c85c79279df2c8a583d95449d1029baba41f8660, 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 c85c79279df2c8a583d95449d1029baba41f8660
and d44e71261f91d3cc81293e0976bb40daa8abb583.  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.")

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.

Signed-off-by: Elijah Newren <redacted>
---
 git-pull.sh     |   34 ++++++++++++++++++++++------------
 t/t5520-pull.sh |    4 ++--
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index a09a44e..54da07b 100755
--- a/git-pull.sh
+++ b/git-pull.sh

@@ -206,18 +206,6 @@ test true = "$rebase" && {
 		git diff-index --ignore-submodules --cached --quiet HEAD -- ||
 		die "refusing to pull with rebase: your working tree is not up-to-date"
 	fi
-	oldremoteref= &&
-	. git-parse-remote &&
-	remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
-	oldremoteref="$(git rev-parse -q --verify "$remoteref")" &&
-	for reflog in $(git rev-list -g $remoteref 2>/dev/null)
-	do
-		if test "$reflog" = "$(git merge-base $reflog $curr_branch)"
-		then
-			oldremoteref="$reflog"
-			break
-		fi
-	done
 }
 orig_head=$(git rev-parse -q --verify HEAD)
 git fetch $verbosity $progress $dry_run --update-head-ok "$@" || exit 1

@@ -273,6 +261,28 @@ then
 	exit
 fi
 
+if test true = "$rebase"
+then
+	oldremoteref= &&
+	. git-parse-remote &&
+	remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
+	oldremoteref="$(git rev-parse -q --verify "$remoteref")" &&
+	for reflog in $(git rev-list -g $remoteref 2>/dev/null)
+	do
+		if test "$reflog" = "$(git merge-base $reflog $curr_branch)"
+		then
+			oldremoteref="$reflog"
+			break
+		fi
+	done
+
+	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 85a6b23..0b489f5 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)

-- 
1.7.2.1.43.gbae63

On Thu, Aug 12, 2010 at 7:56 AM, Elijah Newren [off-list ref] wrote:

Prior to c85c79279df2c8a583d95449d1029baba41f8660, 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 c85c79279df2c8a583d95449d1029baba41f8660
and d44e71261f91d3cc81293e0976bb40daa8abb583.  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.")

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.

All this makes sense.

But can you explain when it happens? One possibility is when you don't
fork from the tracking branch as in:

Subject: Difference between pull --rebase and fetch+rebase 						
Message-ID: [off-list ref]
From: martinvz <redacted>

and this patch should also fix martinvz's issue (I've CC martinvz, can
you test this patch? Thanks).

Can you refer to commits with something like this?

c85c792 (pull --rebase: be cleverer with rebased upstream branches, 2008-01-26)

alias.one !git --no-pager show -s --pretty='tformat:%h (%s, %ad)' --date=short

You've moved all the lines after the call to "git fetch". It changes
the behavior when the reflog is not enabled, as the old value of
remoteref is lost.

Thanks
Santi

On Thu, Aug 12, 2010 at 3:34 PM, Santi Béjar [off-list ref] wrote:

On Thu, Aug 12, 2010 at 7:56 AM, Elijah Newren [off-list ref] wrote:

[...]

quoted

Signed-off-by: Elijah Newren <redacted>
---
 git-pull.sh     |   34 ++++++++++++++++++++++------------
 t/t5520-pull.sh |    4 ++--
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index a09a44e..54da07b 100755
--- a/git-pull.sh
+++ b/git-pull.sh

@@ -206,18 +206,6 @@ test true = "$rebase" && {

               git diff-index --ignore-submodules --cached --quiet HEAD -- ||
               die "refusing to pull with rebase: your working tree is not up-to-date"
       fi
-       oldremoteref= &&
-       . git-parse-remote &&
-       remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
-       oldremoteref="$(git rev-parse -q --verify "$remoteref")" &&
-       for reflog in $(git rev-list -g $remoteref 2>/dev/null)
-       do
-               if test "$reflog" = "$(git merge-base $reflog $curr_branch)"
-               then
-                       oldremoteref="$reflog"
-                       break
-               fi
-       done
 }
 orig_head=$(git rev-parse -q --verify HEAD)
 git fetch $verbosity $progress $dry_run --update-head-ok "$@" || exit 1

@@ -273,6 +261,28 @@ then

       exit
 fi

+if test true = "$rebase"
+then
+       oldremoteref= &&
+       . git-parse-remote &&
+       remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
+       oldremoteref="$(git rev-parse -q --verify "$remoteref")" &&
+       for reflog in $(git rev-list -g $remoteref 2>/dev/null)
+       do
+               if test "$reflog" = "$(git merge-base $reflog $curr_branch)"
+               then
+                       oldremoteref="$reflog"
+                       break
+               fi
+       done
+
+       o=$(git show-branch --merge-base $curr_branch $merge_head $oldremoteref)
+       if test "$oldremoteref" = "$o"
+       then
+               unset oldremoteref
+       fi
+fi
+

You've moved all the lines after the call to "git fetch". It changes
the behavior when the reflog is not enabled, as the old value of
remoteref is lost.

Something like this?

Fix the non-rebased upstream case by only setting $old_remote_ref
with commits not ancestors of $remoteref or $merge_head. This should
have no affect on the rebased upstream case.

diff --git c/git-pull.sh w/git-pull.sh
index a09a44e..c1617d5 100755
--- c/git-pull.sh
+++ w/git-pull.sh

@@ -214,7 +214,10 @@ test true = "$rebase" && {
        do
                if test "$reflog" = "$(git merge-base $reflog $curr_branch)"
                then
-                       oldremoteref="$reflog"
+                       if test "$reflog" != $(git merge-base $reflog

$remoteref)
+                       then
+                               oldremoteref="$reflog"
+                       fi
                        break
                fi
        done

@@ -273,6 +276,14 @@ then
        exit
 fi

+if test true = "$rebase"
+then
+       if test "$oldremoteref" = $(git merge-base $oldremoteref $merge_head)
+       then
+               unset oldremoteref
+       fi
+fi
+
 merge_name=$(git fmt-merge-msg $log_arg <"$GIT_DIR/FETCH_HEAD") || exit
 case "$rebase" in
 true)

(also attached because of whitespace damage)

HTH,
Santi

And now the attachment.

Santi

Hi,

Thanks for the review and comments!

On Thu, Aug 12, 2010 at 7:34 AM, Santi Béjar [off-list ref] wrote:
<snip>

All this makes sense.

But can you explain when it happens? One possibility is when you don't
fork from the tracking branch as in:

That's one possibility.  Patch 1/2 in this thread contains testcases
for two others.  Another possibility is having your patches get
upstream 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).

Subject: Difference between pull --rebase and fetch+rebase
Message-ID: [off-list ref]
From: martinvz <redacted>

and this patch should also fix martinvz's issue (I've CC martinvz, can
you test this patch? Thanks).

Since you've cc'd martinvz, I'll note for his benefit that in the
thread you reference above, you stated,

"By the way, when Git tries to apply these two commits it should detect
that they are already applied so it should do nothing, isn't it?"

The answer is no, it won't detect they are already applied, as
explained in the commit message that started the current thread.  (If
git did detect the changes were already applied, this bug would have
been innocuous.)

Can you refer to commits with something like this?

c85c792 (pull --rebase: be cleverer with rebased upstream branches, 2008-01-26)

Sure, I'll start doing that.

You've moved all the lines after the call to "git fetch". It changes
the behavior when the reflog is not enabled, as the old value of
remoteref is lost.

Doh.  That's what I get for trying to 'clean up' some code and put all
the references to setting oldremoteref together.  I should have stuck
with my original 7-line addition patch.  I'll resend the simplified
patch.

Elijah

Hi,

On Thu, Aug 12, 2010 at 8:37 AM, Santi Béjar [off-list ref] wrote:

How does this help?  I've been trying to scratch my head trying to
figure out a case where it could affect the outcome, and am struggling
to come up with one.

This was indeed my original patch, then I just (incorrectly, as you
pointed out) moved other bits of code to be with this so that the
determination of oldremoteref was all in one place.

On Thu, Aug 12, 2010 at 10:19 PM, Elijah Newren [off-list ref] wrote:

Hi,

Thanks for the review and comments!

On Thu, Aug 12, 2010 at 7:34 AM, Santi Béjar [off-list ref] wrote:
<snip>

quoted

All this makes sense.

But can you explain when it happens? One possibility is when you don't
fork from the tracking branch as in:

That's one possibility.  Patch 1/2 in this thread contains testcases
for two others.  Another possibility is having your patches get
upstream 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).

I think this is commit message material.

quoted

Subject: Difference between pull --rebase and fetch+rebase
Message-ID: [off-list ref]
From: martinvz <redacted>

and this patch should also fix martinvz's issue (I've CC martinvz, can
you test this patch? Thanks).

Since you've cc'd martinvz, I'll note for his benefit that in the
thread you reference above, you stated,

"By the way, when Git tries to apply these two commits it should detect
that they are already applied so it should do nothing, isn't it?"

The answer is no, it won't detect they are already applied, as
explained in the commit message that started the current thread.  (If
git did detect the changes were already applied, this bug would have
been innocuous.)

Thanks, you are right.

Santi

On Thu, Aug 12, 2010 at 11:02 PM, Elijah Newren [off-list ref] wrote:

Hi,

On Thu, Aug 12, 2010 at 8:37 AM, Santi Béjar [off-list ref] wrote:

quoted

diff --git c/git-pull.sh w/git-pull.sh
index a09a44e..c1617d5 100755
--- c/git-pull.sh
+++ w/git-pull.sh

@@ -214,7 +214,10 @@ test true = "$rebase" && {

       do
               if test "$reflog" = "$(git merge-base $reflog $curr_branch)"
               then
-                       oldremoteref="$reflog"
+                       if test "$reflog" != $(git merge-base $reflog
$remoteref)
+                       then
+                               oldremoteref="$reflog"
+                       fi

How does this help?  I've been trying to scratch my head trying to
figure out a case where it could affect the outcome, and am struggling
to come up with one.

Me too :)

The only case where it makes a difference, it is wrong. I was trying
to be extra cautious, but ... too much in this case.

quoted

@@ -273,6 +276,14 @@ then

       exit
 fi

+if test true = "$rebase"
+then
+       if test "$oldremoteref" = $(git merge-base $oldremoteref $merge_head)
+       then
+               unset oldremoteref
+       fi
+fi
+

This was indeed my original patch, then I just (incorrectly, as you
pointed out) moved other bits of code to be with this so that the
determination of oldremoteref was all in one place.

OK. Then for your original patch:

Acked-by: Santi Béjar <redacted>

Thanks,
Santi

On Fri, Aug 13, 2010 at 12:08 AM, Santi Béjar [off-list ref] wrote:

On Thu, Aug 12, 2010 at 10:19 PM, Elijah Newren [off-list ref] wrote:

quoted

Hi,

Thanks for the review and comments!

On Thu, Aug 12, 2010 at 7:34 AM, Santi Béjar [off-list ref] wrote:
<snip>

quoted

All this makes sense.

But can you explain when it happens? One possibility is when you don't
fork from the tracking branch as in:

That's one possibility.  Patch 1/2 in this thread contains testcases
for two others.  Another possibility is having your patches get
upstream 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).

I think this is commit message material.

I just want to add one thing, thanks for your great commit message.
Although the patch itself if 7 lines it is fantastic to have a commit
message explaining all the historical context and this level of
detail.

Thanks,
Santi