{"thread":{"id":"56740","subject":"\"Not possible to fast-forward\" when pull.ff=only and new commits on remote","startedAt":"2021-10-19T18:40:52Z","lastAt":"2021-10-21T06:38:44Z","messageCount":9,"participants":["Kenneth Arnold","Jeff King","Junio C Hamano","Alex Henrie"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"439038","messageId":"CH2PR06MB650424B4205102AC6A48F489B1BD9@CH2PR06MB6504.namprd06.prod.outlook.com","threadId":"56740","inReplyTo":null,"subject":"\"Not possible to fast-forward\" when pull.ff=only and new commits on remote","fromName":"Kenneth Arnold","fromEmail":"ka37@calvin.edu","sentAt":"2021-10-19T17:23:06Z","receivedAt":"2021-10-19T18:40:52Z","isPatch":false,"sender":{"key":"ka37@calvin.edu","avatar":null},"body":"After upgrade to 2.33.1, the behavior of `pull.ff=only` has changed in a way that breaks some workflows, notably the default used in VSCode.\n\nExample (specific repo doesn't matter)\n\n```\n$ git clone git@github.com:kcarnold/util.git\n...\n$ cd util\n$ echo \"\" > test\n$ git add test\n$ git commit -m \"Test\"\n$ git -c pull.ff=only pull\nfatal: Not possible to fast-forward, aborting.\n$ git status\nOn branch master\nYour branch is ahead of 'origin/master' by 1 commit.\n  (use \"git push\" to publish your local commits)\n```\n\nPreviously this pull succeeded without error, which was as expected because no merge was necessary. \n\nVS Code users have reported this problem because that editor has a \"sync\" option that seems to run something like `git pull && git push`.\n\n\n"},{"id":"439044","messageId":"YW83JG9t2JaX92xV@coredump.intra.peff.net","threadId":"56740","inReplyTo":"CH2PR06MB650424B4205102AC6A48F489B1BD9@CH2PR06MB6504.namprd06.prod.outlook.com","subject":"Re: \"Not possible to fast-forward\" when pull.ff=only and new commits on remote","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-19T21:22:44Z","receivedAt":"2021-10-19T21:22:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 19, 2021 at 05:23:06PM +0000, Kenneth Arnold wrote:\n\n> After upgrade to 2.33.1, the behavior of `pull.ff=only` has changed in a way that breaks some workflows, notably the default used in VSCode.\n> \n> Example (specific repo doesn't matter)\n> \n> ```\n> $ git clone git@github.com:kcarnold/util.git\n> ...\n> $ cd util\n> $ echo \"\" > test\n> $ git add test\n> $ git commit -m \"Test\"\n> $ git -c pull.ff=only pull\n> fatal: Not possible to fast-forward, aborting.\n> $ git status\n> On branch master\n> Your branch is ahead of 'origin/master' by 1 commit.\n>   (use \"git push\" to publish your local commits)\n> ```\n> \n> Previously this pull succeeded without error, which was as expected because no merge was necessary. \n\nThanks for reporting, this is an interesting case. I agree that it\nprobably ought to continue to be a noop. There is nothing to pull, and\nso the question of ff-versus-merge should not even enter into it.\n\nIt bisects to Alex's (cc'd) 3d5fc24dae (pull: abort if --ff-only is\ngiven and fast-forwarding is impossible, 2021-07-21). I'd guess it's\nprobably the hunk in pull.cc which checks \"can_ff\". The solution doesn't\nseem entirely obvious to me though (at least in a way that retains what\nthat commit was trying to do).\n\n-Peff\n"},{"id":"439117","messageId":"xmqqzgr3pso7.fsf@gitster.g","threadId":"56740","inReplyTo":"YW83JG9t2JaX92xV@coredump.intra.peff.net","subject":"Re: \"Not possible to fast-forward\" when pull.ff=only and new commits on remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-20T16:28:08Z","receivedAt":"2021-10-20T16:28:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Thanks for reporting, this is an interesting case. I agree that it\n> probably ought to continue to be a noop. There is nothing to pull, and\n> so the question of ff-versus-merge should not even enter into it.\n\nProbably something along this line?  Hasn't been tested beyond\ncompiling and passing\n\n    $ git checkout master && ./git pull --ff-only -v . maint\n\nbut that should be sufficient, I hope.\n\n\n builtin/pull.c | 29 ++++++++++++++++++++++++++++-\n 1 file changed, 28 insertions(+), 1 deletion(-)\n\ndiff --git c/builtin/pull.c w/builtin/pull.c\nindex ae9f5bd7cc..d925999543 100644\n--- c/builtin/pull.c\n+++ w/builtin/pull.c\n@@ -931,6 +931,33 @@ static int get_can_ff(struct object_id *orig_head,\n \treturn ret;\n }\n \n+/*\n+ * Is orig_head is a descendant of _all_ merge_heads?\n+ * Unfortunately is_descendant_of() cannot be used as it \n+ * asks if orig_head is a descendant of at least one of them.\n+ */\n+static int already_up_to_date(struct object_id *orig_head,\n+\t\t\t      struct oid_array *merge_heads)\n+{\n+\tint i;\n+\tstruct commit *ours;\n+\n+\tours = lookup_commit_reference(the_repository, orig_head);\n+\tfor (i = 0; i < merge_heads->nr; i++) {\n+\t\tstruct commit_list *list = NULL;\n+\t\tstruct commit *theirs;\n+\t\tint ok;\n+\n+\t\ttheirs = lookup_commit_reference(the_repository, &merge_heads->oid[i]);\n+\t\tcommit_list_insert(theirs, &list);\n+\t\tok = repo_is_descendant_of(the_repository, ours, list);\n+\t\tfree_commit_list(list);\n+\t\tif (!ok)\n+\t\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n static void show_advice_pull_non_ff(void)\n {\n \tadvise(_(\"You have divergent branches and need to specify how to reconcile them.\\n\"\n@@ -1072,7 +1099,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \t/* ff-only takes precedence over rebase */\n \tif (opt_ff && !strcmp(opt_ff, \"--ff-only\")) {\n-\t\tif (!can_ff)\n+\t\tif (!can_ff && !already_up_to_date(&orig_head, &merge_heads))\n \t\t\tdie_ff_impossible();\n \t\topt_rebase = REBASE_FALSE;\n \t}\n"},{"id":"439118","messageId":"YXBNY7/mWyxvAo/r@coredump.intra.peff.net","threadId":"56740","inReplyTo":"xmqqzgr3pso7.fsf@gitster.g","subject":"Re: \"Not possible to fast-forward\" when pull.ff=only and new commits on remote","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-20T17:09:55Z","receivedAt":"2021-10-20T17:09:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 20, 2021 at 09:28:08AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Thanks for reporting, this is an interesting case. I agree that it\n> > probably ought to continue to be a noop. There is nothing to pull, and\n> > so the question of ff-versus-merge should not even enter into it.\n> \n> Probably something along this line?  Hasn't been tested beyond\n> compiling and passing\n> \n>     $ git checkout master && ./git pull --ff-only -v . maint\n> \n> but that should be sufficient, I hope.\n\nYeah, this direction makes sense to me. Just looking over the patch...\n\n> +/*\n> + * Is orig_head is a descendant of _all_ merge_heads?\n> + * Unfortunately is_descendant_of() cannot be used as it \n> + * asks if orig_head is a descendant of at least one of them.\n> + */\n> +static int already_up_to_date(struct object_id *orig_head,\n> +\t\t\t      struct oid_array *merge_heads)\n> +{\n> +\tint i;\n> +\tstruct commit *ours;\n> +\n> +\tours = lookup_commit_reference(the_repository, orig_head);\n\nI think orig_head can be the null oid if we're on an unborn HEAD. I\nguess you'd want to return \"1\" in that case (but I could be wrong; it\nlooks like get_can_ff() assumes it's valid, so perhaps that case is\nhandled earlier).\n\nI'd expect that merge_heads can never be empty here, or we'd bail\nearlier in the command, but I didn't check (though again, get_can_ff()\nseems to assume there's at least one).\n\n> +\tfor (i = 0; i < merge_heads->nr; i++) {\n> +\t\tstruct commit_list *list = NULL;\n> +\t\tstruct commit *theirs;\n> +\t\tint ok;\n> +\n> +\t\ttheirs = lookup_commit_reference(the_repository, &merge_heads->oid[i]);\n> +\t\tcommit_list_insert(theirs, &list);\n> +\t\tok = repo_is_descendant_of(the_repository, ours, list);\n> +\t\tfree_commit_list(list);\n> +\t\tif (!ok)\n> +\t\t\treturn 0;\n> +\t}\n\nRunning a sequence of traversals like this can be slow, because we may\nwalk over the same history again and again. But I think in the usual\nnon-octopus cases we'd only have one entry, so we'd only be adding a\nsingle extra merge-base traversal in most cases.\n\nIt does feel like this could be combined with get_can_ff() somehow so\nthat we're not adding even that single traversal. But I expect that may\nbe hard to do because of the multiple heads (e.g., we cannot use the\nusual ahead/behind code).\n\n-Peff\n"},{"id":"439132","messageId":"xmqqh7dbplji.fsf@gitster.g","threadId":"56740","inReplyTo":"xmqqzgr3pso7.fsf@gitster.g","subject":"[PATCH v2] pull: --ff-only should make it a noop when already-up-to-date","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-20T19:02:09Z","receivedAt":"2021-10-20T19:02:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Earlier, we made sure that \"git pull --ff-only\" (and \"git -c\npull.ff=only pull\") errors out when our current HEAD is not an\nancestor of the tip of the history we are merging, but the condition\nto trigger the error was implemented incorrectly.\n\nImagine you forked from a remote branch, built your history on top\nof it, and then attempted to pull from them again.  If they have not\nmade any update in the meantime, our current HEAD is obviously not\ntheir ancestor, and this new error triggers.\n\nWithout the --ff-only option, we just report that there is no need\nto pull; we did the same historically with --ff-only, too.\n\nMake sure we do not fail with the recently added check to restore\nthe historycal behaviour.\n\nReported-by: Kenneth Arnold <ka37@calvin.edu>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * With tests and a proposed log message.\n\n builtin/pull.c               | 29 ++++++++++++++++++++++++++++-\n t/t7601-merge-pull-config.sh | 16 +++++++++++++++-\n 2 files changed, 43 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex b311ea6b9d..05f1f0e446 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -933,6 +933,33 @@ static int get_can_ff(struct object_id *orig_head,\n \treturn ret;\n }\n \n+/*\n+ * Is orig_head is a descendant of _all_ merge_heads?\n+ * unfortunately is_descendant_of() cannot be used as it is to\n+ * ask if orig_head is a descendant of at least one of them.\n+ */\n+static int already_up_to_date(struct object_id *orig_head,\n+\t\t\t      struct oid_array *merge_heads)\n+{\n+\tint i;\n+\tstruct commit *ours;\n+\n+\tours = lookup_commit_reference(the_repository, orig_head);\n+\tfor (i = 0; i < merge_heads->nr; i++) {\n+\t\tstruct commit_list *list = NULL;\n+\t\tstruct commit *theirs;\n+\t\tint ok;\n+\n+\t\ttheirs = lookup_commit_reference(the_repository, &merge_heads->oid[i]);\n+\t\tcommit_list_insert(theirs, &list);\n+\t\tok = repo_is_descendant_of(the_repository, ours, list);\n+\t\tfree_commit_list(list);\n+\t\tif (!ok)\n+\t\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n static void show_advice_pull_non_ff(void)\n {\n \tadvise(_(\"You have divergent branches and need to specify how to reconcile them.\\n\"\n@@ -1074,7 +1101,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \t/* ff-only takes precedence over rebase */\n \tif (opt_ff && !strcmp(opt_ff, \"--ff-only\")) {\n-\t\tif (!can_ff)\n+\t\tif (!can_ff && !already_up_to_date(&orig_head, &merge_heads))\n \t\t\tdie_ff_impossible();\n \t\topt_rebase = REBASE_FALSE;\n \t}\ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex 1f652f433e..6275641b9c 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -2,7 +2,7 @@\n \n test_description='git merge\n \n-Testing pull.* configuration parsing.'\n+Testing pull.* configuration parsing and other things.'\n \n . ./test-lib.sh\n \n@@ -387,6 +387,20 @@ test_expect_success 'pull prevents non-fast-forward with \"only\" in pull.ff' '\n \ttest_must_fail git pull . c3\n '\n \n+test_expect_success 'already-up-to-date pull succeeds with \"only\" in pull.ff' '\n+\tgit reset --hard c1 &&\n+\ttest_config pull.ff only &&\n+\tgit pull . c0 &&\n+\ttest \"$(git rev-parse HEAD)\" = \"$(git rev-parse c1)\"\n+'\n+\n+test_expect_success 'already-up-to-date pull/rebase succeeds with \"only\" in pull.ff' '\n+\tgit reset --hard c1 &&\n+\ttest_config pull.ff only &&\n+\tgit -c pull.rebase=true pull . c0 &&\n+\ttest \"$(git rev-parse HEAD)\" = \"$(git rev-parse c1)\"\n+'\n+\n test_expect_success 'merge c1 with c2 (ours in pull.twohead)' '\n \tgit reset --hard c1 &&\n \tgit config pull.twohead ours &&\n-- \n2.33.1-904-gfba30156dd\n\n\n"},{"id":"439133","messageId":"xmqqa6j3pkqw.fsf@gitster.g","threadId":"56740","inReplyTo":"YXBNY7/mWyxvAo/r@coredump.intra.peff.net","subject":"Re: \"Not possible to fast-forward\" when pull.ff=only and new commits on remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-20T19:19:19Z","receivedAt":"2021-10-20T19:19:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> +\tours = lookup_commit_reference(the_repository, orig_head);\n>\n> I think orig_head can be the null oid if we're on an unborn HEAD. I\n> guess you'd want to return \"1\" in that case (but I could be wrong; it\n> looks like get_can_ff() assumes it's valid, so perhaps that case is\n> handled earlier).\n\nIt is a good point; the main codeflow already special cases the\nunborn HEAD to delegate to pull_into_void() before it gets to the\npoint to call get_can_ff().\n\n> I'd expect that merge_heads can never be empty here, or we'd bail\n> earlier in the command\n\nYes, that happens even before that \"are we unborn\" check.\n\n> Running a sequence of traversals like this can be slow, because we may\n> walk over the same history again and again. But I think in the usual\n> non-octopus cases we'd only have one entry, so we'd only be adding a\n> single extra merge-base traversal in most cases.\n>\n> It does feel like this could be combined with get_can_ff() somehow so\n> that we're not adding even that single traversal. But I expect that may\n> be hard to do because of the multiple heads (e.g., we cannot use the\n> usual ahead/behind code).\n\nI'd leave such an optimization as a separate topic.  This was meant\nto be a regression fix.\n\n"},{"id":"439138","messageId":"YXB94Y4mqm8GpsfV@coredump.intra.peff.net","threadId":"56740","inReplyTo":"xmqqa6j3pkqw.fsf@gitster.g","subject":"Re: \"Not possible to fast-forward\" when pull.ff=only and new commits on remote","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-20T20:36:49Z","receivedAt":"2021-10-20T20:36:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 20, 2021 at 12:19:19PM -0700, Junio C Hamano wrote:\n\n> > Running a sequence of traversals like this can be slow, because we may\n> > walk over the same history again and again. But I think in the usual\n> > non-octopus cases we'd only have one entry, so we'd only be adding a\n> > single extra merge-base traversal in most cases.\n> >\n> > It does feel like this could be combined with get_can_ff() somehow so\n> > that we're not adding even that single traversal. But I expect that may\n> > be hard to do because of the multiple heads (e.g., we cannot use the\n> > usual ahead/behind code).\n> \n> I'd leave such an optimization as a separate topic.  This was meant\n> to be a regression fix.\n\nYeah, I'm definitely OK with that. You might be making performance a\nlittle worse with your patch, but getting correctness right is much more\nimportant.\n\n-Peff\n"},{"id":"439139","messageId":"YXB/1S2fid7MUmvu@coredump.intra.peff.net","threadId":"56740","inReplyTo":"xmqqh7dbplji.fsf@gitster.g","subject":"Re: [PATCH v2] pull: --ff-only should make it a noop when already-up-to-date","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-20T20:45:09Z","receivedAt":"2021-10-20T20:45:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 20, 2021 at 12:02:09PM -0700, Junio C Hamano wrote:\n\n> Earlier, we made sure that \"git pull --ff-only\" (and \"git -c\n> pull.ff=only pull\") errors out when our current HEAD is not an\n> ancestor of the tip of the history we are merging, but the condition\n> to trigger the error was implemented incorrectly.\n> \n> Imagine you forked from a remote branch, built your history on top\n> of it, and then attempted to pull from them again.  If they have not\n> made any update in the meantime, our current HEAD is obviously not\n> their ancestor, and this new error triggers.\n> \n> Without the --ff-only option, we just report that there is no need\n> to pull; we did the same historically with --ff-only, too.\n\nThanks, this looks good to me overall, and I agree this is a regression\nwe should try to fix promptly (so thank you for jumping on it).\n\n> Make sure we do not fail with the recently added check to restore\n> the historycal behaviour.\n\nNot sure if \"historycal\" is a typo or some clever pun. :)\n\n> +/*\n> + * Is orig_head is a descendant of _all_ merge_heads?\n\ns/is a/a/\n\n> +static int already_up_to_date(struct object_id *orig_head,\n> +\t\t\t      struct oid_array *merge_heads)\n> +{\n> +\tint i;\n> +\tstruct commit *ours;\n> +\n> +\tours = lookup_commit_reference(the_repository, orig_head);\n> +\tfor (i = 0; i < merge_heads->nr; i++) {\n> +\t\tstruct commit_list *list = NULL;\n> +\t\tstruct commit *theirs;\n> +\t\tint ok;\n> +\n> +\t\ttheirs = lookup_commit_reference(the_repository, &merge_heads->oid[i]);\n> +\t\tcommit_list_insert(theirs, &list);\n> +\t\tok = repo_is_descendant_of(the_repository, ours, list);\n> +\t\tfree_commit_list(list);\n> +\t\tif (!ok)\n> +\t\t\treturn 0;\n> +\t}\n> +\treturn 1;\n> +}\n\nYou answered all of my \"what about...\" questions from before elsewhere\nin the thread, so this looks correct.\n\n> +test_expect_success 'already-up-to-date pull succeeds with \"only\" in pull.ff' '\n> +\tgit reset --hard c1 &&\n> +\ttest_config pull.ff only &&\n> +\tgit pull . c0 &&\n> +\ttest \"$(git rev-parse HEAD)\" = \"$(git rev-parse c1)\"\n> +'\n> +\n> +test_expect_success 'already-up-to-date pull/rebase succeeds with \"only\" in pull.ff' '\n> +\tgit reset --hard c1 &&\n> +\ttest_config pull.ff only &&\n> +\tgit -c pull.rebase=true pull . c0 &&\n> +\ttest \"$(git rev-parse HEAD)\" = \"$(git rev-parse c1)\"\n> +'\n\nAnd these tests cover the cases I'd expect. The use of \"test\" with\nprocess substitution looks a bit funny to me these days, but it does\nmatch the surrounding code (and losing the exit codes isn't a big deal\nhere, as we are not testing rev-parse).\n\nThe combo of \"test_config\" and \"git -c\" is unusual, but I don't see\nanything wrong with it.\n\n-Peff\n"},{"id":"439179","messageId":"CAMMLpeQYSWyhM92RDQuK0TSkNcbuE06_8RQiYHtz_JDYmXJXMA@mail.gmail.com","threadId":"56740","inReplyTo":"YXB/1S2fid7MUmvu@coredump.intra.peff.net","subject":"Re: [PATCH v2] pull: --ff-only should make it a noop when already-up-to-date","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2021-10-21T06:38:23Z","receivedAt":"2021-10-21T06:38:44Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Wed, Oct 20, 2021 at 2:45 PM Jeff King <peff@peff.net> wrote:\n>\n> On Wed, Oct 20, 2021 at 12:02:09PM -0700, Junio C Hamano wrote:\n>\n> > Earlier, we made sure that \"git pull --ff-only\" (and \"git -c\n> > pull.ff=only pull\") errors out when our current HEAD is not an\n> > ancestor of the tip of the history we are merging, but the condition\n> > to trigger the error was implemented incorrectly.\n> >\n> > Imagine you forked from a remote branch, built your history on top\n> > of it, and then attempted to pull from them again.  If they have not\n> > made any update in the meantime, our current HEAD is obviously not\n> > their ancestor, and this new error triggers.\n> >\n> > Without the --ff-only option, we just report that there is no need\n> > to pull; we did the same historically with --ff-only, too.\n>\n> Thanks, this looks good to me overall, and I agree this is a regression\n> we should try to fix promptly (so thank you for jumping on it).\n\nIt looks like you guys got this under control in no time. Thank you so much!\n\n-Alex\n"}]}