# git pull --rebase should use fast forward merge if possible 9 messages from 2016-06-29 to 2016-12-01. Participants: neuling@dakosy.de, Junio C Hamano, Stefan Beller. Thread: https://gitlist.dev/t/42748 ## neuling@dakosy.de, 2016-06-29 16:18 Subject: git pull --rebase should use fast forward merge if possible Message-ID: URL: https://gitlist.dev/e/OF95D98CB6.47969C1C-ONC1257FE1.0058D980-C1257FE1.0059986D%40dakosy.de ``` 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 ``` ## Junio C Hamano, 2016-06-29 16:32 Subject: Re: git pull --rebase should use fast forward merge if possible Message-ID: URL: https://gitlist.dev/e/xmqqd1n0552i.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` 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 ``` ## Junio C Hamano, 2016-06-29 17:23 Subject: Re: git pull --rebase should use fast forward merge if possible Message-ID: URL: https://gitlist.dev/e/xmqqtwgb52py.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Junio C Hamano 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(); } ``` ## Stefan Beller, 2016-06-29 20:40 Subject: Re: git pull --rebase should use fast forward merge if possible Message-ID: URL: https://gitlist.dev/e/CAGZ79kYS%3DXWBpEBJsj95FV%2Bj%2BHLxg%3D5xqwD73618t7ERYrKUfA%40mail.gmail.com In-Reply-To: ``` On Wed, Jun 29, 2016 at 10:23 AM, Junio C Hamano wrote: > Junio C Hamano 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 ``` ## Junio C Hamano, 2016-06-29 20:43 Subject: Re: git pull --rebase should use fast forward merge if possible Message-ID: URL: https://gitlist.dev/e/CAPc5daURyXO6-yaOWPhvvdS8Dr5psEEc8MVP4wQJ_AuxyZraRg%40mail.gmail.com In-Reply-To: ``` On Wed, Jun 29, 2016 at 1:40 PM, Stefan Beller 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 ;-) ``` ## Junio C Hamano, 2016-12-01 17:59 Subject: Re* git pull --rebase should use fast forward merge if possible Message-ID: URL: https://gitlist.dev/e/xmqqa8cfbkeq.fsf_-_%40gitster.mtv.corp.google.com In-Reply-To: ``` Junio C Hamano 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 --- 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 ``` ## Stefan Beller, 2016-12-01 18:24 Subject: Re: Re* git pull --rebase should use fast forward merge if possible Message-ID: URL: https://gitlist.dev/e/CAGZ79kZSEan5uXCUn4iVCWEc9zohMSr%2BUDyHDyQUHz84H%3DtR8w%40mail.gmail.com In-Reply-To: ``` On Thu, Dec 1, 2016 at 9:59 AM, Junio C Hamano 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? ``` ## Junio C Hamano, 2016-12-01 18:50 Subject: Re: Re* git pull --rebase should use fast forward merge if possible Message-ID: URL: https://gitlist.dev/e/xmqqoa0va3fw.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Stefan Beller writes: > On Thu, Dec 1, 2016 at 9:59 AM, Junio C Hamano 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. ``` ## Stefan Beller, 2016-12-01 18:51 Subject: Re: Re* git pull --rebase should use fast forward merge if possible Message-ID: URL: https://gitlist.dev/e/CAGZ79kbSApFA3G2nKjxJdyxF%3DYFzodLVwuYMhKy9fm9359frNg%40mail.gmail.com In-Reply-To: ``` On Thu, Dec 1, 2016 at 10:50 AM, Junio C Hamano wrote: > Stefan Beller writes: > >> On Thu, Dec 1, 2016 at 9:59 AM, Junio C Hamano 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. ```