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

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

[Elijah Newren]

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.

Changes since the last posting of these series:
  * Rewrite cover letter and commit messages for clarity
  * Avoid sed -i in the testsuite to improve portability

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 |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 12 deletions(-)

-- 
1.7.2.1

[PATCHv2 1/2] t5520-pull: Add testcases showing spurious conflicts from git pull --rebase

[Elijah Newren]

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t5520-pull.sh |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 319e389..1624dd3 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -160,4 +160,62 @@ 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 &&
+	 for i in $(seq 1 10); do echo $i; done > stuff &&
+	 git add stuff &&
+	 git commit -m "Initial revision"
+	) &&
+	git clone src dst &&
+	(cd src &&
+	 perl -pi -e s/5/43/ stuff &&
+	 git commit -a -m "5->43" &&
+	 perl -pi -e s/6/42/ stuff &&
+	 git commit -a -m "Make it bigger" &&
+	 correct=$(git rev-parse HEAD)
+	) &&
+	(cd dst &&
+	 perl -pi -e 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 &&
+	 (git rebase --abort || true) &&
+	 git reset --hard origin/master
+	) &&
+	git clone --bare src src-replace.git &&
+	rm -rf src &&
+	mv src-replace.git src &&
+	(cd dst &&
+	 perl -pi -e s/2/22/ stuff &&
+	 git commit -a -m "Change 2" &&
+	 perl -pi -e s/3/33/ stuff &&
+	 git commit -a -m "Change 3" &&
+	 perl -pi -e s/4/44/ stuff &&
+	 git commit -a -m "Change 4" &&
+	 git push &&
+
+	 perl -pi -e 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 &&
+	 (git pull --rebase || true) &&
+	 test 1 = $(find .git/rebase-apply -name "000*" | wc -l)
+	)
+'
+
 test_done
-- 
1.7.2.1

Re: [PATCHv2 1/2] t5520-pull: Add testcases showing spurious conflicts from git pull --rebase

[Ævar Arnfjörð Bjarmason]

On Sun, Aug 8, 2010 at 19:04, Elijah Newren <newren@gmail.com> wrote:
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  t/t5520-pull.sh |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 58 insertions(+), 0 deletions(-)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 319e389..1624dd3 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -160,4 +160,62 @@ 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 &&
> +        for i in $(seq 1 10); do echo $i; done > stuff &&
> +        git add stuff &&
> +        git commit -m "Initial revision"
> +       ) &&
> +       git clone src dst &&
> +       (cd src &&
> +        perl -pi -e s/5/43/ stuff &&
> +        git commit -a -m "5->43" &&
> +        perl -pi -e s/6/42/ stuff &&

Please use sed so the test doesn't depend on perl being present.

> +        git commit -a -m "Make it bigger" &&
> +        correct=$(git rev-parse HEAD)
> +       ) &&
> +       (cd dst &&
> +        perl -pi -e 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 &&
> +        (git rebase --abort || true) &&

If you're ignoring the git rebase --abort return value:

  (cd dst &&
    git rebase --abort;
    git reset ...)

> +        git reset --hard origin/master
> +       ) &&
> +       git clone --bare src src-replace.git &&
> +       rm -rf src &&
> +       mv src-replace.git src &&
> +       (cd dst &&
> +        perl -pi -e s/2/22/ stuff &&
> +        git commit -a -m "Change 2" &&
> +        perl -pi -e s/3/33/ stuff &&
> +        git commit -a -m "Change 3" &&
> +        perl -pi -e s/4/44/ stuff &&
> +        git commit -a -m "Change 4" &&
> +        git push &&
> +
> +        perl -pi -e 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 &&
> +        (git pull --rebase || true) &&
> +        test 1 = $(find .git/rebase-apply -name "000*" | wc -l)
> +       )
> +'
> +
>  test_done
> --
> 1.7.2.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCHv2 1/2] t5520-pull: Add testcases showing spurious conflicts from git pull --rebase

[Elijah Newren]

Hi,

Thanks for taking a look!

On Sun, Aug 8, 2010 at 2:01 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Sun, Aug 8, 2010 at 19:04, Elijah Newren <newren@gmail.com> wrote:
<snip>
>> +        perl -pi -e s/5/43/ stuff &&
>> +        git commit -a -m "5->43" &&
>> +        perl -pi -e s/6/42/ stuff &&
>
> Please use sed so the test doesn't depend on perl being present.

The original series used 'sed -i', but I found out that is apparently
not portable.  Should I use sed + a temporary file + mv?

<snip>
>> +test_expect_success 'setup for avoiding reapplying old patches' '
>> +       (cd dst &&
>> +        (git rebase --abort || true) &&
>
> If you're ignoring the git rebase --abort return value:
>
>  (cd dst &&
>    git rebase --abort;
>    git reset ...)

Makes sense; I'll do that.

Re: [PATCHv2 1/2] t5520-pull: Add testcases showing spurious conflicts from git pull --rebase

[Ævar Arnfjörð Bjarmason]

On Sun, Aug 8, 2010 at 20:17, Elijah Newren <newren@gmail.com> wrote:
> Hi,
>
> Thanks for taking a look!
>
> On Sun, Aug 8, 2010 at 2:01 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> On Sun, Aug 8, 2010 at 19:04, Elijah Newren <newren@gmail.com> wrote:
> <snip>
>>> +        perl -pi -e s/5/43/ stuff &&
>>> +        git commit -a -m "5->43" &&
>>> +        perl -pi -e s/6/42/ stuff &&
>>
>> Please use sed so the test doesn't depend on perl being present.
>
> The original series used 'sed -i', but I found out that is apparently
> not portable.  Should I use sed + a temporary file + mv?

Yeah, a few tests do that already. E.g.:

    sed s/6/42/ <stuff >stuff.tmp &&
    mv stuff.tmp stuff

You could also just declare a dependency on perl:

    test_expect_success PERL 'setup for detecting upstreamed changes' '

But then you'd also have to use "$PERL_PATH" instead of just perl, see
t9700-perl-git.sh.

Re: [PATCHv2 1/2] t5520-pull: Add testcases showing spurious conflicts from git pull --rebase

[Johannes Sixt]

On Sonntag, 8. August 2010, Elijah Newren wrote:
> +test_expect_success 'setup for detecting upstreamed changes' '
> +	mkdir src &&
> +	(cd src &&
> +	 git init &&
> +	 for i in $(seq 1 10); do echo $i; done > stuff &&

We don't have seq on Windows. The sequence is short enough to list it 
explicitly.

> +	 git add stuff &&
> +	 git commit -m "Initial revision"
> +	) &&
> +	git clone src dst &&
> +	(cd src &&
> +	 perl -pi -e s/5/43/ stuff &&

perl -pi is no better than sed -i; it does not work on Windows because it 
tries to remove an open file (and that is a no-go on Windows). perl -pi.bak 
does work though.

> +	 git commit -a -m "5->43" &&
> +	 perl -pi -e s/6/42/ stuff &&
> +	 git commit -a -m "Make it bigger" &&
> +	 correct=$(git rev-parse HEAD)
> +	) &&

This assignment at the end of the subshell is pointless.

> +test_expect_success 'setup for avoiding reapplying old patches' '
> +	(cd dst &&
> +	 (git rebase --abort || true) &&

Perhaps:

	test_might_fail git rebase --abort &&

but I'm not sure whether that's the intended use of test_might_fail.

> +	 git reset --hard origin/master
> +	) &&
...
> +test_expect_failure 'git pull --rebase does not reapply old patches' '
> +	(cd dst &&
> +	 (git pull --rebase || true) &&

Ditto.

> +	 test 1 = $(find .git/rebase-apply -name "000*" | wc -l)
> +	)
> +'
> +
>  test_done

-- Hannes

Re: [PATCHv2 1/2] t5520-pull: Add testcases showing spurious conflicts from git pull --rebase

[Jonathan Nieder]

Johannes Sixt wrote:
> On Sonntag, 8. August 2010, Elijah Newren wrote:

>> +test_expect_success 'setup for avoiding reapplying old patches' '
>> +	(cd dst &&
>> +	 (git rebase --abort || true) &&
>
> Perhaps:
>
> 	test_might_fail git rebase --abort &&
>
> but I'm not sure whether that's the intended use of test_might_fail.

It is.  Thanks for noticing.

[PATCHv2 2/2] pull --rebase: Avoid spurious conflicts and reapplying unnecessary patches

[Elijah Newren]

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 <newren@gmail.com>
---
Note that 24 lines of the patch are simply due to moving 12 lines; I
considered leaving those 12 lines where they were, but it'd split the
code that handles the setting of oldremoteref, which I thought should
be together.  I couldn't place the new code earlier, because it
depends on the definition of $merge_head.  I'd be happy to reshuffle
differently if anyone has any strong preferences.

 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 1624dd3..ec4d36a 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -182,7 +182,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)"
@@ -211,7 +211,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 &&
 	 (git pull --rebase || true) &&
 	 test 1 = $(find .git/rebase-apply -name "000*" | wc -l)
-- 
1.7.2.1