{"thread":{"id":"42748","subject":"git pull --rebase should use fast forward merge if possible","startedAt":"2016-06-29T16:18:43Z","lastAt":"2016-12-01T18:51:42Z","messageCount":9,"participants":["neuling@dakosy.de","Junio C Hamano","Stefan Beller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"290508","messageId":"OF95D98CB6.47969C1C-ONC1257FE1.0058D980-C1257FE1.0059986D@dakosy.de","threadId":"42748","inReplyTo":null,"subject":"git pull --rebase should use fast forward merge if possible","fromName":"","fromEmail":"neuling@dakosy.de","sentAt":"2016-06-29T16:18:37Z","receivedAt":"2016-06-29T16:18:43Z","isPatch":false,"sender":{"key":"neuling@dakosy.de","avatar":null},"body":"Hi, \n\nsince I have added \"pull.rebase=preserve\" to my global configuration I \nwonder why \"git pull\" also trys to rebase if a fast forward merge is \npossible. \n\nA fast forward merge would speed up every pull if your local branch \ncontains no new commits and the remote branch is ahead. The result would \nbe the same. \n\nIs it possible to change the behavior of \"git pull \n--rebase=true|preserve|interactive\" to use a fast forward merge if the \nremote branch is ahead and the local branch contains no new commits? \n\n\nRegards, \nMattias \n"},{"id":"290509","messageId":"xmqqd1n0552i.fsf@gitster.mtv.corp.google.com","threadId":"42748","inReplyTo":"OF95D98CB6.47969C1C-ONC1257FE1.0058D980-C1257FE1.0059986D@dakosy.de","subject":"Re: git pull --rebase should use fast forward merge if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-29T16:32:21Z","receivedAt":"2016-06-29T16:32:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"neuling@dakosy.de writes:\n\n> since I have added \"pull.rebase=preserve\" to my global configuration I \n> wonder why \"git pull\" also trys to rebase if a fast forward merge is \n> possible. \n>\n> A fast forward merge would speed up every pull if your local branch \n> contains no new commits and the remote branch is ahead. The result would \n> be the same. \n>\n> Is it possible to change the behavior of \"git pull \n> --rebase=true|preserve|interactive\" to use a fast forward merge if the \n> remote branch is ahead and the local branch contains no new commits? \n\nInteresting.  I do not think of a reason why we shouldn't.\n\nIf we were still working in scripted Porcelain, it would have been a\nfive minute hack, perhaps like this.\n\n contrib/examples/git-pull.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/contrib/examples/git-pull.sh b/contrib/examples/git-pull.sh\nindex 6b3a03f..3648040 100755\n--- a/contrib/examples/git-pull.sh\n+++ b/contrib/examples/git-pull.sh\n@@ -358,6 +358,14 @@ fi\n \n if test true = \"$rebase\"\n then\n+\tif git merge-base --is-ancestor \"$orig_head\" \"$merge_head\"\n+\tthen\n+\t\t# We can just fast-forward, as \"git rebase --no-ff\"\n+\t\t# is nonsense.\n+\t\tgit merge --ff-only \"$merge_head\"\n+\t\texit\n+\tfi\n+\n \to=$(git show-branch --merge-base $curr_branch $merge_head $oldremoteref)\n \tif test \"$oldremoteref\" = \"$o\"\n \tthen\n"},{"id":"290513","messageId":"xmqqtwgb52py.fsf@gitster.mtv.corp.google.com","threadId":"42748","inReplyTo":"xmqqd1n0552i.fsf@gitster.mtv.corp.google.com","subject":"Re: git pull --rebase should use fast forward merge if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-29T17:23:05Z","receivedAt":"2016-06-29T17:24:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> Is it possible to change the behavior of \"git pull \n>> --rebase=true|preserve|interactive\" to use a fast forward merge if the \n>> remote branch is ahead and the local branch contains no new commits? \n>\n> Interesting.  I do not think of a reason why we shouldn't.\n>\n> If we were still working in scripted Porcelain, it would have been a\n> five minute hack, perhaps like this.\n>\n>  contrib/examples/git-pull.sh | 8 ++++++++\n>  1 file changed, 8 insertions(+)\n\n... and if we have to work with built-ins, it becomes a lot larger\nthan a five-minute hack, unfortunately.\n\nSomething like this may have a chance of working ;-)\n\n builtin/pull.c | 22 ++++++++++++++++++----\n 1 file changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex bf3fd3f..777ae56 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -878,10 +878,24 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\tif (merge_heads.nr > 1)\n \t\t\tdie(_(\"Cannot merge multiple branches into empty head.\"));\n \t\treturn pull_into_void(*merge_heads.sha1, curr_head);\n-\t} else if (opt_rebase) {\n-\t\tif (merge_heads.nr > 1)\n-\t\t\tdie(_(\"Cannot rebase onto multiple branches.\"));\n+\t}\n+\tif (opt_rebase && merge_heads.nr > 1)\n+\t\tdie(_(\"Cannot rebase onto multiple branches.\"));\n+\n+\tif (opt_rebase) {\n+\t\tstruct commit_list *list = NULL;\n+\t\tstruct commit *merge_head, *head;\n+\n+\t\thead = lookup_commit_reference(orig_head);\n+\t\tcommit_list_insert(head, &list);\n+\t\tmerge_head = lookup_commit_reference(merge_heads.sha1[0]);\n+\t\tif (is_descendant_of(merge_head, list)) {\n+\t\t\t/* we can fast-forward this without invoking rebase */\n+\t\t\topt_ff = \"--ff-only\";\n+\t\t\treturn run_merge();\n+\t\t}\n \t\treturn run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);\n-\t} else\n+\t}\n+\telse\n \t\treturn run_merge();\n }\n"},{"id":"290530","messageId":"CAGZ79kYS=XWBpEBJsj95FV+j+HLxg=5xqwD73618t7ERYrKUfA@mail.gmail.com","threadId":"42748","inReplyTo":"xmqqtwgb52py.fsf@gitster.mtv.corp.google.com","subject":"Re: git pull --rebase should use fast forward merge if possible","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-29T20:40:27Z","receivedAt":"2016-06-29T20:41:53Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jun 29, 2016 at 10:23 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>>> Is it possible to change the behavior of \"git pull\n>>> --rebase=true|preserve|interactive\" to use a fast forward merge if the\n>>> remote branch is ahead and the local branch contains no new commits?\n>>\n>> Interesting.  I do not think of a reason why we shouldn't.\n\nMe neither.\n\n>>\n>> If we were still working in scripted Porcelain, it would have been a\n>> five minute hack, perhaps like this.\n>>\n>>  contrib/examples/git-pull.sh | 8 ++++++++\n>>  1 file changed, 8 insertions(+)\n>\n> ... and if we have to work with built-ins, it becomes a lot larger\n> than a five-minute hack, unfortunately.\n>\n> Something like this may have a chance of working ;-)\n>\n>  builtin/pull.c | 22 ++++++++++++++++++----\n>  1 file changed, 18 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index bf3fd3f..777ae56 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -878,10 +878,24 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n>                 if (merge_heads.nr > 1)\n>                         die(_(\"Cannot merge multiple branches into empty head.\"));\n>                 return pull_into_void(*merge_heads.sha1, curr_head);\n> -       } else if (opt_rebase) {\n> -               if (merge_heads.nr > 1)\n> -                       die(_(\"Cannot rebase onto multiple branches.\"));\n> +       }\n> +       if (opt_rebase && merge_heads.nr > 1)\n> +               die(_(\"Cannot rebase onto multiple branches.\"));\n> +\n> +       if (opt_rebase) {\n> +               struct commit_list *list = NULL;\n> +               struct commit *merge_head, *head;\n> +\n> +               head = lookup_commit_reference(orig_head);\n> +               commit_list_insert(head, &list);\n> +               merge_head = lookup_commit_reference(merge_heads.sha1[0]);\n\nThe crashes for merge_heads.nr == 0.\n(I did not inspect code further up if this is caught before, I think\nit would trigger if\nyou and the remote are on an initial commit with no parents?)\n\n> +               if (is_descendant_of(merge_head, list)) {\n> +                       /* we can fast-forward this without invoking rebase */\n> +                       opt_ff = \"--ff-only\";\n> +                       return run_merge();\n> +               }\n>                 return run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);\n> -       } else\n> +       }\n> +       else\n\nKeep the style ?\n\n>                 return run_merge();\n>  }\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"290531","messageId":"CAPc5daURyXO6-yaOWPhvvdS8Dr5psEEc8MVP4wQJ_AuxyZraRg@mail.gmail.com","threadId":"42748","inReplyTo":"CAGZ79kYS=XWBpEBJsj95FV+j+HLxg=5xqwD73618t7ERYrKUfA@mail.gmail.com","subject":"Re: git pull --rebase should use fast forward merge if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-29T20:43:21Z","receivedAt":"2016-06-29T20:44:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Wed, Jun 29, 2016 at 1:40 PM, Stefan Beller <sbeller@google.com> wrote:\n>> +               merge_head = lookup_commit_reference(merge_heads.sha1[0]);\n>\n> The crashes for merge_heads.nr == 0.\n> (I did not inspect code further up if this is caught before, I think\n> it would trigger if\n> you and the remote are on an initial commit with no parents?)\n\nPerhaps you can inspect code before you start typing?\n\ndie_no_merge_candidates() would have triggered, I would imagine.\n\nNote that I won't be applying this without test updates and proper log message,\nso no need to worry about the style yet ;-)\n"},{"id":"306699","messageId":"xmqqa8cfbkeq.fsf_-_@gitster.mtv.corp.google.com","threadId":"42748","inReplyTo":"CAPc5daURyXO6-yaOWPhvvdS8Dr5psEEc8MVP4wQJ_AuxyZraRg@mail.gmail.com","subject":"Re* git pull --rebase should use fast forward merge if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-01T17:59:09Z","receivedAt":"2016-12-01T17:59:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> die_no_merge_candidates() would have triggered, I would imagine.\n>\n> Note that I won't be applying this without test updates and proper log message,\n> so no need to worry about the style yet ;-)\n\nThis time with a bit of explanation in the log and a trivial test.\n\nI am no longer sure if this is a good idea myself, though.  The\ntrivial case the new test covers is not interesting, even though it\nmay be worth protecting the behaviour with the test.  What's more\ninteresting to think about is what should happen in various corner\ncases.  E.g. what should happen when there are local changes that\nwould conflict with the fast-forwarding?  what should happen if the\nuser has rebase.autostash set in such a case?  etc.\n\n-- >8 --\nSubject: [PATCH] pull: fast-forward \"pull --rebase=true\"\n\n\"git pull --rebase\" always runs \"git rebase\" after fetching the\ncommit to serve as the new base, even when the new base is a\ndescendant of the current HEAD, i.e. we haven't done any work.\n\nIn such a case, we can instead fast-forward to the new base without\ninvoking the rebase process.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/pull.c  | 22 ++++++++++++++++++----\n t/t5520-pull.sh | 17 +++++++++++++++++\n 2 files changed, 35 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex bf3fd3f9c8..2a41d415b2 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -878,10 +878,24 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\tif (merge_heads.nr > 1)\n \t\t\tdie(_(\"Cannot merge multiple branches into empty head.\"));\n \t\treturn pull_into_void(*merge_heads.sha1, curr_head);\n-\t} else if (opt_rebase) {\n-\t\tif (merge_heads.nr > 1)\n-\t\t\tdie(_(\"Cannot rebase onto multiple branches.\"));\n+\t}\n+\tif (opt_rebase && merge_heads.nr > 1)\n+\t\tdie(_(\"Cannot rebase onto multiple branches.\"));\n+\n+\tif (opt_rebase) {\n+\t\tstruct commit_list *list = NULL;\n+\t\tstruct commit *merge_head, *head;\n+\n+\t\thead = lookup_commit_reference(orig_head);\n+\t\tcommit_list_insert(head, &list);\n+\t\tmerge_head = lookup_commit_reference(merge_heads.sha1[0]);\n+\t\tif (is_descendant_of(merge_head, list)) {\n+\t\t\t/* we can fast-forward this without invoking rebase */\n+\t\t\topt_ff = \"--ff-only\";\n+\t\t\treturn run_merge();\n+\t\t}\n \t\treturn run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);\n-\t} else\n+\t} else {\n \t\treturn run_merge();\n+\t}\n }\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex a0013ee32f..7887b6d97b 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -237,6 +237,23 @@ test_expect_success '--rebase' '\n \ttest new = \"$(git show HEAD:file2)\"\n '\n \n+test_expect_success '--rebase fast forward' '\n+\tgit reset --hard before-rebase &&\n+\tgit checkout -b ff &&\n+\techo another modification >file &&\n+\tgit commit -m third file &&\n+\n+\tgit checkout to-rebase &&\n+\tgit pull --rebase . ff &&\n+\ttest \"$(git rev-parse HEAD)\" = \"$(git rev-parse ff)\" &&\n+\n+\t# The above only validates the result.  Did we actually bypass rebase?\n+\tgit reflog -1 >reflog.actual &&\n+\tsed \"s/^[0-9a-f][0-9a-f]*/OBJID/\" reflog.actual >reflog.fuzzy &&\n+\techo \"OBJID HEAD@{0}: pull --rebase . ff: Fast-forward\" >reflog.expected &&\n+\ttest_cmp reflog.expected reflog.fuzzy\n+'\n+\n test_expect_success '--rebase fails with multiple branches' '\n \tgit reset --hard before-rebase &&\n \ttest_must_fail git pull --rebase . copy master 2>err &&\n-- \n2.11.0-192-gbadfaabe38\n\n"},{"id":"306702","messageId":"CAGZ79kZSEan5uXCUn4iVCWEc9zohMSr+UDyHDyQUHz84H=tR8w@mail.gmail.com","threadId":"42748","inReplyTo":"xmqqa8cfbkeq.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: Re* git pull --rebase should use fast forward merge if possible","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-01T18:24:49Z","receivedAt":"2016-12-01T18:24:55Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Dec 1, 2016 at 9:59 AM, Junio C Hamano <gitster@pobox.com> wrote:\n\n> +test_expect_success '--rebase fast forward' '\n> +       git reset --hard before-rebase &&\n> +       git checkout -b ff &&\n> +       echo another modification >file &&\n> +       git commit -m third file &&\n> +\n> +       git checkout to-rebase &&\n> +       git pull --rebase . ff &&\n> +       test \"$(git rev-parse HEAD)\" = \"$(git rev-parse ff)\" &&\n> +\n> +       # The above only validates the result.  Did we actually bypass rebase?\n\nGood catch for the test, but I think we can make the sed regexp simpler, as we\ncan leave out the second \"[0-9a-f]\"? (git reflog |sed\n\"s/^[0-9a-f]*/OBJID/\" works here)\n\nThe implication of that we'd also match if there is no object id at\nall at the beginning,\nwhich sounds fine.\n\nI shortly debated the idea to just cut off anything before the first\nspace and then expect\n\"HEAD@{0}: pull --rebase . ff: Fast-forward\" only, but why cut off\nwhat we produce\nin the first place?\n\n    git reflog --format=\"%s\" -1 >actual &&\n    echo \"pull --rebase . ff: Fast-forward\" >expect &&\n    test_cmp expect actual\n\nmaybe?\n"},{"id":"306707","messageId":"xmqqoa0va3fw.fsf@gitster.mtv.corp.google.com","threadId":"42748","inReplyTo":"CAGZ79kZSEan5uXCUn4iVCWEc9zohMSr+UDyHDyQUHz84H=tR8w@mail.gmail.com","subject":"Re: Re* git pull --rebase should use fast forward merge if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-01T18:50:59Z","receivedAt":"2016-12-01T18:51:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Thu, Dec 1, 2016 at 9:59 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> +test_expect_success '--rebase fast forward' '\n>> +       git reset --hard before-rebase &&\n>> +       git checkout -b ff &&\n>> +       echo another modification >file &&\n>> +       git commit -m third file &&\n>> +\n>> +       git checkout to-rebase &&\n>> +       git pull --rebase . ff &&\n>> +       test \"$(git rev-parse HEAD)\" = \"$(git rev-parse ff)\" &&\n>> +\n>> +       # The above only validates the result.  Did we actually bypass rebase?\n>\n> Good catch for the test, but I think we can make the sed regexp simpler, as we\n> can leave out the second \"[0-9a-f]\"? (git reflog |sed\n> \"s/^[0-9a-f]*/OBJID/\" works here)\n\nThis mimics the existing tests around there for consistency.\nSimplifying or cleaning of this test script as a whole is outside\nthe scope.\n"},{"id":"306709","messageId":"CAGZ79kbSApFA3G2nKjxJdyxF=YFzodLVwuYMhKy9fm9359frNg@mail.gmail.com","threadId":"42748","inReplyTo":"xmqqoa0va3fw.fsf@gitster.mtv.corp.google.com","subject":"Re: Re* git pull --rebase should use fast forward merge if possible","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-01T18:51:36Z","receivedAt":"2016-12-01T18:51:42Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Dec 1, 2016 at 10:50 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> On Thu, Dec 1, 2016 at 9:59 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>>> +test_expect_success '--rebase fast forward' '\n>>> +       git reset --hard before-rebase &&\n>>> +       git checkout -b ff &&\n>>> +       echo another modification >file &&\n>>> +       git commit -m third file &&\n>>> +\n>>> +       git checkout to-rebase &&\n>>> +       git pull --rebase . ff &&\n>>> +       test \"$(git rev-parse HEAD)\" = \"$(git rev-parse ff)\" &&\n>>> +\n>>> +       # The above only validates the result.  Did we actually bypass rebase?\n>>\n>> Good catch for the test, but I think we can make the sed regexp simpler, as we\n>> can leave out the second \"[0-9a-f]\"? (git reflog |sed\n>> \"s/^[0-9a-f]*/OBJID/\" works here)\n>\n> This mimics the existing tests around there for consistency.\n> Simplifying or cleaning of this test script as a whole is outside\n> the scope.\n\nOk, in that case the patch looks fine.\n"}]}