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

> 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? 

>> 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(+)

> 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();
>  }
> --
> 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

>> +               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?)

> +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?

> 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)

> 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.