git pull --rebase should use fast forward merge if possible

git pull --rebase should use fast forward merge if possible

[neuling]

Hi, 

since I have added "pull.rebase=preserve" to my global configuration I 
wonder why "git pull" also trys to rebase if a fast forward merge is 
possible. 

A fast forward merge would speed up every pull if your local branch 
contains no new commits and the remote branch is ahead. The result would 
be the same. 

Is it possible to change the behavior of "git pull 
--rebase=true|preserve|interactive" to use a fast forward merge if the 
remote branch is ahead and the local branch contains no new commits? 


Regards, 
Mattias 

Re: git pull --rebase should use fast forward merge if possible

[Junio C Hamano]

neuling@dakosy.de writes:

> since I have added "pull.rebase=preserve" to my global configuration I 
> wonder why "git pull" also trys to rebase if a fast forward merge is 
> possible. 
>
> A fast forward merge would speed up every pull if your local branch 
> contains no new commits and the remote branch is ahead. The result would 
> be the same. 
>
> Is it possible to change the behavior of "git pull 
> --rebase=true|preserve|interactive" to use a fast forward merge if the 
> remote branch is ahead and the local branch contains no new commits? 

Interesting.  I do not think of a reason why we shouldn't.

If we were still working in scripted Porcelain, it would have been a
five minute hack, perhaps like this.

 contrib/examples/git-pull.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/contrib/examples/git-pull.sh b/contrib/examples/git-pull.sh
index 6b3a03f..3648040 100755
--- a/contrib/examples/git-pull.sh
+++ b/contrib/examples/git-pull.sh
@@ -358,6 +358,14 @@ fi
 
 if test true = "$rebase"
 then
+	if git merge-base --is-ancestor "$orig_head" "$merge_head"
+	then
+		# We can just fast-forward, as "git rebase --no-ff"
+		# is nonsense.
+		git merge --ff-only "$merge_head"
+		exit
+	fi
+
 	o=$(git show-branch --merge-base $curr_branch $merge_head $oldremoteref)
 	if test "$oldremoteref" = "$o"
 	then

Re: git pull --rebase should use fast forward merge if possible

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> Is it possible to change the behavior of "git pull 
>> --rebase=true|preserve|interactive" to use a fast forward merge if the 
>> remote branch is ahead and the local branch contains no new commits? 
>
> Interesting.  I do not think of a reason why we shouldn't.
>
> If we were still working in scripted Porcelain, it would have been a
> five minute hack, perhaps like this.
>
>  contrib/examples/git-pull.sh | 8 ++++++++
>  1 file changed, 8 insertions(+)

... and if we have to work with built-ins, it becomes a lot larger
than a five-minute hack, unfortunately.

Something like this may have a chance of working ;-)

 builtin/pull.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index bf3fd3f..777ae56 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -878,10 +878,24 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		if (merge_heads.nr > 1)
 			die(_("Cannot merge multiple branches into empty head."));
 		return pull_into_void(*merge_heads.sha1, curr_head);
-	} else if (opt_rebase) {
-		if (merge_heads.nr > 1)
-			die(_("Cannot rebase onto multiple branches."));
+	}
+	if (opt_rebase && merge_heads.nr > 1)
+		die(_("Cannot rebase onto multiple branches."));
+
+	if (opt_rebase) {
+		struct commit_list *list = NULL;
+		struct commit *merge_head, *head;
+
+		head = lookup_commit_reference(orig_head);
+		commit_list_insert(head, &list);
+		merge_head = lookup_commit_reference(merge_heads.sha1[0]);
+		if (is_descendant_of(merge_head, list)) {
+			/* we can fast-forward this without invoking rebase */
+			opt_ff = "--ff-only";
+			return run_merge();
+		}
 		return run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);
-	} else
+	}
+	else
 		return run_merge();
 }

Re: git pull --rebase should use fast forward merge if possible

[Stefan Beller]

On Wed, Jun 29, 2016 at 10:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> Is it possible to change the behavior of "git pull
>>> --rebase=true|preserve|interactive" to use a fast forward merge if the
>>> remote branch is ahead and the local branch contains no new commits?
>>
>> Interesting.  I do not think of a reason why we shouldn't.

Me neither.

>>
>> If we were still working in scripted Porcelain, it would have been a
>> five minute hack, perhaps like this.
>>
>>  contrib/examples/git-pull.sh | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>
> ... and if we have to work with built-ins, it becomes a lot larger
> than a five-minute hack, unfortunately.
>
> Something like this may have a chance of working ;-)
>
>  builtin/pull.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index bf3fd3f..777ae56 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -878,10 +878,24 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                 if (merge_heads.nr > 1)
>                         die(_("Cannot merge multiple branches into empty head."));
>                 return pull_into_void(*merge_heads.sha1, curr_head);
> -       } else if (opt_rebase) {
> -               if (merge_heads.nr > 1)
> -                       die(_("Cannot rebase onto multiple branches."));
> +       }
> +       if (opt_rebase && merge_heads.nr > 1)
> +               die(_("Cannot rebase onto multiple branches."));
> +
> +       if (opt_rebase) {
> +               struct commit_list *list = NULL;
> +               struct commit *merge_head, *head;
> +
> +               head = lookup_commit_reference(orig_head);
> +               commit_list_insert(head, &list);
> +               merge_head = lookup_commit_reference(merge_heads.sha1[0]);

The crashes for merge_heads.nr == 0.
(I did not inspect code further up if this is caught before, I think
it would trigger if
you and the remote are on an initial commit with no parents?)

> +               if (is_descendant_of(merge_head, list)) {
> +                       /* we can fast-forward this without invoking rebase */
> +                       opt_ff = "--ff-only";
> +                       return run_merge();
> +               }
>                 return run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);
> -       } else
> +       }
> +       else

Keep the style ?

>                 return run_merge();
>  }
> --
> 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: git pull --rebase should use fast forward merge if possible

[Junio C Hamano]

On Wed, Jun 29, 2016 at 1:40 PM, Stefan Beller <sbeller@google.com> wrote:
>> +               merge_head = lookup_commit_reference(merge_heads.sha1[0]);
>
> The crashes for merge_heads.nr == 0.
> (I did not inspect code further up if this is caught before, I think
> it would trigger if
> you and the remote are on an initial commit with no parents?)

Perhaps you can inspect code before you start typing?

die_no_merge_candidates() would have triggered, I would imagine.

Note that I won't be applying this without test updates and proper log message,
so no need to worry about the style yet ;-)

Re* git pull --rebase should use fast forward merge if possible

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> die_no_merge_candidates() would have triggered, I would imagine.
>
> Note that I won't be applying this without test updates and proper log message,
> so no need to worry about the style yet ;-)

This time with a bit of explanation in the log and a trivial test.

I am no longer sure if this is a good idea myself, though.  The
trivial case the new test covers is not interesting, even though it
may be worth protecting the behaviour with the test.  What's more
interesting to think about is what should happen in various corner
cases.  E.g. what should happen when there are local changes that
would conflict with the fast-forwarding?  what should happen if the
user has rebase.autostash set in such a case?  etc.

-- >8 --
Subject: [PATCH] pull: fast-forward "pull --rebase=true"

"git pull --rebase" always runs "git rebase" after fetching the
commit to serve as the new base, even when the new base is a
descendant of the current HEAD, i.e. we haven't done any work.

In such a case, we can instead fast-forward to the new base without
invoking the rebase process.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/pull.c  | 22 ++++++++++++++++++----
 t/t5520-pull.sh | 17 +++++++++++++++++
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index bf3fd3f9c8..2a41d415b2 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -878,10 +878,24 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		if (merge_heads.nr > 1)
 			die(_("Cannot merge multiple branches into empty head."));
 		return pull_into_void(*merge_heads.sha1, curr_head);
-	} else if (opt_rebase) {
-		if (merge_heads.nr > 1)
-			die(_("Cannot rebase onto multiple branches."));
+	}
+	if (opt_rebase && merge_heads.nr > 1)
+		die(_("Cannot rebase onto multiple branches."));
+
+	if (opt_rebase) {
+		struct commit_list *list = NULL;
+		struct commit *merge_head, *head;
+
+		head = lookup_commit_reference(orig_head);
+		commit_list_insert(head, &list);
+		merge_head = lookup_commit_reference(merge_heads.sha1[0]);
+		if (is_descendant_of(merge_head, list)) {
+			/* we can fast-forward this without invoking rebase */
+			opt_ff = "--ff-only";
+			return run_merge();
+		}
 		return run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);
-	} else
+	} else {
 		return run_merge();
+	}
 }
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index a0013ee32f..7887b6d97b 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -237,6 +237,23 @@ test_expect_success '--rebase' '
 	test new = "$(git show HEAD:file2)"
 '
 
+test_expect_success '--rebase fast forward' '
+	git reset --hard before-rebase &&
+	git checkout -b ff &&
+	echo another modification >file &&
+	git commit -m third file &&
+
+	git checkout to-rebase &&
+	git pull --rebase . ff &&
+	test "$(git rev-parse HEAD)" = "$(git rev-parse ff)" &&
+
+	# The above only validates the result.  Did we actually bypass rebase?
+	git reflog -1 >reflog.actual &&
+	sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
+	echo "OBJID HEAD@{0}: pull --rebase . ff: Fast-forward" >reflog.expected &&
+	test_cmp reflog.expected reflog.fuzzy
+'
+
 test_expect_success '--rebase fails with multiple branches' '
 	git reset --hard before-rebase &&
 	test_must_fail git pull --rebase . copy master 2>err &&
-- 
2.11.0-192-gbadfaabe38

Re: Re* git pull --rebase should use fast forward merge if possible

[Stefan Beller]

On Thu, Dec 1, 2016 at 9:59 AM, Junio C Hamano <gitster@pobox.com> wrote:

> +test_expect_success '--rebase fast forward' '
> +       git reset --hard before-rebase &&
> +       git checkout -b ff &&
> +       echo another modification >file &&
> +       git commit -m third file &&
> +
> +       git checkout to-rebase &&
> +       git pull --rebase . ff &&
> +       test "$(git rev-parse HEAD)" = "$(git rev-parse ff)" &&
> +
> +       # The above only validates the result.  Did we actually bypass rebase?

Good catch for the test, but I think we can make the sed regexp simpler, as we
can leave out the second "[0-9a-f]"? (git reflog |sed
"s/^[0-9a-f]*/OBJID/" works here)

The implication of that we'd also match if there is no object id at
all at the beginning,
which sounds fine.

I shortly debated the idea to just cut off anything before the first
space and then expect
"HEAD@{0}: pull --rebase . ff: Fast-forward" only, but why cut off
what we produce
in the first place?

    git reflog --format="%s" -1 >actual &&
    echo "pull --rebase . ff: Fast-forward" >expect &&
    test_cmp expect actual

maybe?

Re: Re* git pull --rebase should use fast forward merge if possible

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Thu, Dec 1, 2016 at 9:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> +test_expect_success '--rebase fast forward' '
>> +       git reset --hard before-rebase &&
>> +       git checkout -b ff &&
>> +       echo another modification >file &&
>> +       git commit -m third file &&
>> +
>> +       git checkout to-rebase &&
>> +       git pull --rebase . ff &&
>> +       test "$(git rev-parse HEAD)" = "$(git rev-parse ff)" &&
>> +
>> +       # The above only validates the result.  Did we actually bypass rebase?
>
> Good catch for the test, but I think we can make the sed regexp simpler, as we
> can leave out the second "[0-9a-f]"? (git reflog |sed
> "s/^[0-9a-f]*/OBJID/" works here)

This mimics the existing tests around there for consistency.
Simplifying or cleaning of this test script as a whole is outside
the scope.

Re: Re* git pull --rebase should use fast forward merge if possible

[Stefan Beller]

On Thu, Dec 1, 2016 at 10:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Thu, Dec 1, 2016 at 9:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> +test_expect_success '--rebase fast forward' '
>>> +       git reset --hard before-rebase &&
>>> +       git checkout -b ff &&
>>> +       echo another modification >file &&
>>> +       git commit -m third file &&
>>> +
>>> +       git checkout to-rebase &&
>>> +       git pull --rebase . ff &&
>>> +       test "$(git rev-parse HEAD)" = "$(git rev-parse ff)" &&
>>> +
>>> +       # The above only validates the result.  Did we actually bypass rebase?
>>
>> Good catch for the test, but I think we can make the sed regexp simpler, as we
>> can leave out the second "[0-9a-f]"? (git reflog |sed
>> "s/^[0-9a-f]*/OBJID/" works here)
>
> This mimics the existing tests around there for consistency.
> Simplifying or cleaning of this test script as a whole is outside
> the scope.

Ok, in that case the patch looks fine.