{"thread":{"id":"57049","subject":"[PATCH] builtin/fetch: skip unnecessary tasks when using --negotiate-only","startedAt":"2021-12-07T19:29:31Z","lastAt":"2022-01-21T02:18:04Z","messageCount":50,"participants":["Glen Choo","Jonathan Tan","Junio C Hamano","Jiang Xin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"443340","messageId":"20211207192925.67680-1-chooglen@google.com","threadId":"57049","inReplyTo":null,"subject":"[PATCH] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-12-07T19:29:25Z","receivedAt":"2021-12-07T19:29:31Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"`git fetch --negotiate-only` does not fetch objects and thus, it should\nnot perform certain auxiliary tasks like updating submodules, updating\nthe commit graph, or running gc. Although send_pack() invokes `git fetch\n--negotiate-only` correctly, cmd_fetch() also reads config variables,\nleading to undesirable behavior, like updating submodules if\n`submodule.recurse=true`.\n\nMake cmd_fetch() return early if --negotiate-only was specified so that\nthese auxiliary tasks are skipped.\n\nSigned-off-by: Glen Choo \n---\n`git fetch --negotiate-only` is used during push negotiation to\ndetermine the reachability of commits. As its name implies, only\nnegotiation is performed, not the actual fetching of objects. However,\ncmd_fetch() performs certain tasks with the assumption that objects are\nfetched:\n\n* Submodules are updated if enabled by recurse.submodules=true, but\n negotiation fetch doesn't actually update the repo, so this doesn't\n make sense (introduced in [1]).\n* Commit graphs will be written if enabled by\n fetch.writeCommitGraph=true. But according to\n Documentation/config/fetch.txt [2], this should only be done if a\n pack-file is downloaded\n* gc is run, but according to [3], we only do this because we expect\n `git fetch` to introduce objects\n\nInstead of disabling these tasks piecemeal, let's just make cmd_fetch()\nreturn early if --negotiate-only was given. To accommodate possible\nfuture options that don't fetch objects, I opted to introduce another\n`if` statement instead of putting the early return in the existing\n`if (negotiate_only)` block.\n\n[1] 7dce19d374 (fetch/pull: Add the --recurse-submodules option, 2010-11-12)\n[2] 50f26bd035 (fetch: add fetch.writeCommitGraph config setting, 2019-09-02)\n[3] 131b8fcbfb (fetch: run gc --auto after fetching, 2013-01-26)\n\n builtin/fetch.c | 22 +++++++++++++++++-----\n t/t5516-fetch-push.sh | 12 ++++++++++++\n 2 files changed, 29 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f7abbc31ff..01865b5c09 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1996,6 +1996,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n+\n+\tif (negotiate_only) {\n+\t\t/*\n+\t\t * --negotiate-only should never recurse into\n+\t\t * submodules, so there is no need to read .gitmodules.\n+\t\t */\n+\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n+\t\tif (!negotiation_tip.nr)\n+\t\t\tdie(_(\"--negotiate-only needs one or more --negotiate-tip=*\"));\n+\t}\n+\n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n \t\tint *sfjc = submodule_fetch_jobs_config == -1\n \t\t\t ? &submodule_fetch_jobs_config : NULL;\n@@ -2005,9 +2016,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tfetch_config_from_gitmodules(sfjc, rs);\n \t}\n \n-\tif (negotiate_only && !negotiation_tip.nr)\n-\t\tdie(_(\"--negotiate-only needs one or more --negotiate-tip=*\"));\n-\n \tif (deepen_relative) {\n \t\tif (deepen_relative < 0)\n \t\t\tdie(_(\"Negative depth in --deepen is not supported\"));\n@@ -2112,6 +2120,12 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tresult = fetch_multiple(&list, max_children);\n \t}\n \n+\tstring_list_clear(&list, 0);\n+\n+\t/* skip irrelevant tasks if objects were not fetched */\n+\tif (negotiate_only)\n+\t\treturn result;\n+\n \tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n \t\tstruct strvec options = STRVEC_INIT;\n \t\tint max_children = max_jobs;\n@@ -2132,8 +2146,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tstrvec_clear(&options);\n \t}\n \n-\tstring_list_clear(&list, 0);\n-\n \tprepare_repo_settings(the_repository);\n \tif (fetch_write_commit_graph > 0 ||\n \t (fetch_write_commit_graph < 0 &&\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 8212ca56dc..732031085e 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -229,6 +229,18 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n \ttest_i18ngrep \"push negotiation failed\" err\n '\n \n+test_expect_success 'push with negotiation does not attempt to fetch submodules' '\n+\tmk_empty submodule_upstream &&\n+\ttest_commit -C submodule_upstream submodule_commit &&\n+\tgit submodule add ./submodule_upstream submodule &&\n+\tmk_empty testrepo &&\n+\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\ttest_commit -C testrepo unrelated_commit &&\n+\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n+\tgit -c submodule.recurse=true -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n+\t! grep \"Fetching submodule\" err\n+'\n+\n test_expect_success 'push without wildcard' '\n \tmk_empty testrepo &&\n \n-- \n2.33.GIT\n\n"},{"id":"443703","messageId":"20211209221250.508257-1-jonathantanmy@google.com","threadId":"57049","inReplyTo":"20211207192925.67680-1-chooglen@google.com","subject":"Re: [PATCH] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-12-09T22:12:50Z","receivedAt":"2021-12-09T22:12:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Glen Choo writes:\n> `git fetch --negotiate-only` does not fetch objects and thus, it should\n> not perform certain auxiliary tasks like updating submodules, updating\n> the commit graph, or running gc. Although send_pack() invokes `git fetch\n> --negotiate-only` correctly, cmd_fetch() also reads config variables,\n> leading to undesirable behavior, like updating submodules if\n> `submodule.recurse=true`.\n> \n> Make cmd_fetch() return early if --negotiate-only was specified so that\n> these auxiliary tasks are skipped.\n> \n> Signed-off-by: Glen Choo \n> ---\n> `git fetch --negotiate-only` is used during push negotiation to\n> determine the reachability of commits. As its name implies, only\n> negotiation is performed, not the actual fetching of objects. However,\n> cmd_fetch() performs certain tasks with the assumption that objects are\n> fetched:\n> \n> * Submodules are updated if enabled by recurse.submodules=true, but\n> negotiation fetch doesn't actually update the repo, so this doesn't\n> make sense (introduced in [1]).\n> * Commit graphs will be written if enabled by\n> fetch.writeCommitGraph=true. But according to\n> Documentation/config/fetch.txt [2], this should only be done if a\n> pack-file is downloaded\n> * gc is run, but according to [3], we only do this because we expect\n> `git fetch` to introduce objects\n> \n> Instead of disabling these tasks piecemeal, let's just make cmd_fetch()\n> return early if --negotiate-only was given. To accommodate possible\n> future options that don't fetch objects, I opted to introduce another\n> `if` statement instead of putting the early return in the existing\n> `if (negotiate_only)` block.\n\nSome of this probably should be in the commit message too.\n\n> +\tif (negotiate_only) {\n> +\t\t/*\n> +\t\t * --negotiate-only should never recurse into\n> +\t\t * submodules, so there is no need to read .gitmodules.\n> +\t\t */\n> +\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n> +\t\tif (!negotiation_tip.nr)\n> +\t\t\tdie(_(\"--negotiate-only needs one or more --negotiate-tip=*\"));\n> +\t}\n\nMaybe add a check here that --recurse-submodules was not explicitly\ngiven.\n\n> +\t/* skip irrelevant tasks if objects were not fetched */\n> +\tif (negotiate_only)\n> +\t\treturn result;\n\nThere are other reasons too why objects were not fetched (e.g. if we\nalready have all of them). Maybe add a NEEDSWORK explaining this.\n\nBesides these code comments and the fact that the commit message could\nbe improved, this patch looks good to me.\n"},{"id":"443708","messageId":"kl6lbl1p9zjf.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57049","inReplyTo":"20211209221250.508257-1-jonathantanmy@google.com","subject":"Re: [PATCH] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-12-09T22:36:04Z","receivedAt":"2021-12-09T22:36:07Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Jonathan Tan writes:\n\n> Glen Choo writes:\n>> `git fetch --negotiate-only` does not fetch objects and thus, it should\n>> not perform certain auxiliary tasks like updating submodules, updating\n>> the commit graph, or running gc. Although send_pack() invokes `git fetch\n>> --negotiate-only` correctly, cmd_fetch() also reads config variables,\n>> leading to undesirable behavior, like updating submodules if\n>> `submodule.recurse=true`.\n>> \n>> Make cmd_fetch() return early if --negotiate-only was specified so that\n>> these auxiliary tasks are skipped.\n>> \n>> Signed-off-by: Glen Choo \n>> ---\n>> `git fetch --negotiate-only` is used during push negotiation to\n>> determine the reachability of commits. As its name implies, only\n>> negotiation is performed, not the actual fetching of objects. However,\n>> cmd_fetch() performs certain tasks with the assumption that objects are\n>> fetched:\n>> \n>> * Submodules are updated if enabled by recurse.submodules=true, but\n>> negotiation fetch doesn't actually update the repo, so this doesn't\n>> make sense (introduced in [1]).\n>> * Commit graphs will be written if enabled by\n>> fetch.writeCommitGraph=true. But according to\n>> Documentation/config/fetch.txt [2], this should only be done if a\n>> pack-file is downloaded\n>> * gc is run, but according to [3], we only do this because we expect\n>> `git fetch` to introduce objects\n>> \n>> Instead of disabling these tasks piecemeal, let's just make cmd_fetch()\n>> return early if --negotiate-only was given. To accommodate possible\n>> future options that don't fetch objects, I opted to introduce another\n>> `if` statement instead of putting the early return in the existing\n>> `if (negotiate_only)` block.\n>\n> Some of this probably should be in the commit message too.\n\nI suppose you mean the explanation of why the tasks are irrelevant to\nnegotiation fetch? i.e. \n\n * Submodules are updated if enabled by recurse.submodules=true...\n * Commit graphs will be written if enabled by...\n * gc is run, but according to [3]...\n\n>> +\tif (negotiate_only) {\n>> +\t\t/*\n>> +\t\t * --negotiate-only should never recurse into\n>> +\t\t * submodules, so there is no need to read .gitmodules.\n>> +\t\t */\n>> +\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n>> +\t\tif (!negotiation_tip.nr)\n>> +\t\t\tdie(_(\"--negotiate-only needs one or more --negotiate-tip=*\"));\n>> +\t}\n>\n> Maybe add a check here that --recurse-submodules was not explicitly\n> given.\n\nHm, that's not a bad idea, but it's not so easy because we don't have\nRECURSE_SUBMODULES_EXPLICIT so it's not easy to tell whether or not\nsubmodule recursion was enabled by CLI option or config.\n\nThis is the exact same use case I encountered with \"branch\n--recurse-submodules\" [1]. I think this means that we should consider\nstandardizing the parsing of submodule.recurse + --recurse-submodules. I\nhaven't done it yet because it's a little tricky and hard to review.\n\nSo I'll punt on this check until we get RECURSE_SUBMODULES_EXPLICIT.\n\n>\n>> +\t/* skip irrelevant tasks if objects were not fetched */\n>> +\tif (negotiate_only)\n>> +\t\treturn result;\n>\n> There are other reasons too why objects were not fetched (e.g. if we\n> already have all of them). Maybe add a NEEDSWORK explaining this.\n\nGood point, the comment doesn't distinguish between cases where we know\nthat objects won't be fetched beforehand vs cases where we found out\nthat objects weren't fetched after the fact. I'll add the NEEDSWORK.\n\n[1] https://lore.kernel.org/git/20211209184928.71413-5-chooglen@google.com\n"},{"id":"444049","messageId":"20211213225842.768852-1-jonathantanmy@google.com","threadId":"57049","inReplyTo":"kl6lbl1p9zjf.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-12-13T22:58:42Z","receivedAt":"2021-12-13T22:58:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Glen Choo writes:\n> Jonathan Tan writes:\n> \n> > Glen Choo writes:\n> >> `git fetch --negotiate-only` does not fetch objects and thus, it should\n> >> not perform certain auxiliary tasks like updating submodules, updating\n> >> the commit graph, or running gc. Although send_pack() invokes `git fetch\n> >> --negotiate-only` correctly, cmd_fetch() also reads config variables,\n> >> leading to undesirable behavior, like updating submodules if\n> >> `submodule.recurse=true`.\n> >> \n> >> Make cmd_fetch() return early if --negotiate-only was specified so that\n> >> these auxiliary tasks are skipped.\n> >> \n> >> Signed-off-by: Glen Choo \n> >> ---\n> >> `git fetch --negotiate-only` is used during push negotiation to\n> >> determine the reachability of commits. As its name implies, only\n> >> negotiation is performed, not the actual fetching of objects. However,\n> >> cmd_fetch() performs certain tasks with the assumption that objects are\n> >> fetched:\n> >> \n> >> * Submodules are updated if enabled by recurse.submodules=true, but\n> >> negotiation fetch doesn't actually update the repo, so this doesn't\n> >> make sense (introduced in [1]).\n> >> * Commit graphs will be written if enabled by\n> >> fetch.writeCommitGraph=true. But according to\n> >> Documentation/config/fetch.txt [2], this should only be done if a\n> >> pack-file is downloaded\n> >> * gc is run, but according to [3], we only do this because we expect\n> >> `git fetch` to introduce objects\n> >> \n> >> Instead of disabling these tasks piecemeal, let's just make cmd_fetch()\n> >> return early if --negotiate-only was given. To accommodate possible\n> >> future options that don't fetch objects, I opted to introduce another\n> >> `if` statement instead of putting the early return in the existing\n> >> `if (negotiate_only)` block.\n> >\n> > Some of this probably should be in the commit message too.\n> \n> I suppose you mean the explanation of why the tasks are irrelevant to\n> negotiation fetch? i.e. \n> \n> * Submodules are updated if enabled by recurse.submodules=true...\n> * Commit graphs will be written if enabled by...\n> * gc is run, but according to [3]...\n\nYes - why the behavior is undesirable, and the way you're doing it (by\nadding another \"if\" statement).\n\nAfter looking at this, more concretely, it might be better to use the\npart below the \"---\" as your commit message. :-) Just note that we're\nnot just accommodating future options that don't fetch objects - \"fetch\"\nalready may not fetch objects (e.g. if the ref we want doesn't exist or\nif we already have all the objects).\n\n> > Maybe add a check here that --recurse-submodules was not explicitly\n> > given.\n> \n> Hm, that's not a bad idea, but it's not so easy because we don't have\n> RECURSE_SUBMODULES_EXPLICIT so it's not easy to tell whether or not\n> submodule recursion was enabled by CLI option or config.\n> \n> This is the exact same use case I encountered with \"branch\n> --recurse-submodules\" [1]. I think this means that we should consider\n> standardizing the parsing of submodule.recurse + --recurse-submodules. I\n> haven't done it yet because it's a little tricky and hard to review.\n> \n> So I'll punt on this check until we get RECURSE_SUBMODULES_EXPLICIT.\n\nHmm...can we separate out the recurse_submodules variable into one\nthat's given by config and one that's given by CLI argument?\n"},{"id":"444243","messageId":"kl6lwnk4to5x.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57049","inReplyTo":"20211213225842.768852-1-jonathantanmy@google.com","subject":"Re: [PATCH] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-12-16T18:11:54Z","receivedAt":"2021-12-16T18:12:00Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Jonathan Tan writes:\n\n> Glen Choo writes:\n>> Jonathan Tan writes:\n>> \n>> > Glen Choo writes:\n>> >> `git fetch --negotiate-only` does not fetch objects and thus, it should\n>> >> not perform certain auxiliary tasks like updating submodules, updating\n>> >> the commit graph, or running gc. Although send_pack() invokes `git fetch\n>> >> --negotiate-only` correctly, cmd_fetch() also reads config variables,\n>> >> leading to undesirable behavior, like updating submodules if\n>> >> `submodule.recurse=true`.\n>> >> \n>> >> Make cmd_fetch() return early if --negotiate-only was specified so that\n>> >> these auxiliary tasks are skipped.\n>> >> \n>> >> Signed-off-by: Glen Choo \n>> >> ---\n>> >> `git fetch --negotiate-only` is used during push negotiation to\n>> >> determine the reachability of commits. As its name implies, only\n>> >> negotiation is performed, not the actual fetching of objects. However,\n>> >> cmd_fetch() performs certain tasks with the assumption that objects are\n>> >> fetched:\n>> >> \n>> >> * Submodules are updated if enabled by recurse.submodules=true, but\n>> >> negotiation fetch doesn't actually update the repo, so this doesn't\n>> >> make sense (introduced in [1]).\n>> >> * Commit graphs will be written if enabled by\n>> >> fetch.writeCommitGraph=true. But according to\n>> >> Documentation/config/fetch.txt [2], this should only be done if a\n>> >> pack-file is downloaded\n>> >> * gc is run, but according to [3], we only do this because we expect\n>> >> `git fetch` to introduce objects\n>> >> \n>> >> Instead of disabling these tasks piecemeal, let's just make cmd_fetch()\n>> >> return early if --negotiate-only was given. To accommodate possible\n>> >> future options that don't fetch objects, I opted to introduce another\n>> >> `if` statement instead of putting the early return in the existing\n>> >> `if (negotiate_only)` block.\n>> >\n>> > Some of this probably should be in the commit message too.\n>> \n>> I suppose you mean the explanation of why the tasks are irrelevant to\n>> negotiation fetch? i.e. \n>> \n>> * Submodules are updated if enabled by recurse.submodules=true...\n>> * Commit graphs will be written if enabled by...\n>> * gc is run, but according to [3]...\n>\n> Yes - why the behavior is undesirable, and the way you're doing it (by\n> adding another \"if\" statement).\n>\n> After looking at this, more concretely, it might be better to use the\n> part below the \"---\" as your commit message. :-) Just note that we're\n> not just accommodating future options that don't fetch objects - \"fetch\"\n> already may not fetch objects (e.g. if the ref we want doesn't exist or\n> if we already have all the objects).\n\nGood suggestion. I'll clean it up for brevity.\n\n>\n>> > Maybe add a check here that --recurse-submodules was not explicitly\n>> > given.\n>> \n>> Hm, that's not a bad idea, but it's not so easy because we don't have\n>> RECURSE_SUBMODULES_EXPLICIT so it's not easy to tell whether or not\n>> submodule recursion was enabled by CLI option or config.\n>> \n>> This is the exact same use case I encountered with \"branch\n>> --recurse-submodules\" [1]. I think this means that we should consider\n>> standardizing the parsing of submodule.recurse + --recurse-submodules. I\n>> haven't done it yet because it's a little tricky and hard to review.\n>> \n>> So I'll punt on this check until we get RECURSE_SUBMODULES_EXPLICIT.\n>\n> Hmm...can we separate out the recurse_submodules variable into one\n> that's given by config and one that's given by CLI argument?\n\nThis would be similar to what I did for branch --recurse-submodules [1],\nwhich, as I noted in my cover letter [2], is easier to understand, but a\nlittle inconsistent with the rest of the --recurse-submodules parsing.\n\nI'm not very enthusiastic about adding more inconsistency, and since\nthis check isn't critical to this bugfix (and I think there is very\nlittle chance that anyone would run afoul of this check any time soon)\nI'd like to hold off on it until RECURSE_SUBMODULES_EXPLICIT.\n\nUnless you think I'm missing something of course :)\n\n[1] https://lore.kernel.org/git/20211216003213.99135-6-chooglen@google.com\n[2] https://lore.kernel.org/git/20211216003213.99135-1-chooglen@google.com\n"},{"id":"444286","messageId":"20211217000235.68996-1-chooglen@google.com","threadId":"57049","inReplyTo":"20211207192925.67680-1-chooglen@google.com","subject":"[PATCH v2] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-12-17T00:02:35Z","receivedAt":"2021-12-17T00:02:43Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"cmd_fetch() performs certain tasks with the assumption that objects are\nfetched, but `git fetch --negotiate-only` does not fetch objects, as its\nname implies. This is results in behavior that is unnecessary at best,\nand incorrect at worst:\n\n* Submodules are updated if enabled by recurse.submodules=true, but\n negotiation fetch doesn't actually update the repo, so this doesn't\n make sense (introduced in [1]).\n* Commit graphs will be written if enabled by\n fetch.writeCommitGraph=true. But according to\n Documentation/config/fetch.txt [2], this should only be done if a\n pack-file is downloaded.\n* gc is run, but according to [3], we only do this because we expect\n `git fetch` to introduce objects.\n\nInstead of disabling these tasks piecemeal, make cmd_fetch() return\nearly if we know for certain that objects will not be fetched. We can\nreturn early whenever objects are not fetched, but for now this only\nconsiders --negotiate-only.\n\n[1] 7dce19d374 (fetch/pull: Add the --recurse-submodules option, 2010-11-12)\n[2] 50f26bd035 (fetch: add fetch.writeCommitGraph config setting, 2019-09-02)\n[3] 131b8fcbfb (fetch: run gc --auto after fetching, 2013-01-26)\n\nSigned-off-by: Glen Choo \n---\nThanks for the review, Jonathan! \n\nChanges since v1:\n* added more context to commit message\n* added a NEEDSWORK comment \n\nInterdiff against v1:\n diff --git a/builtin/fetch.c b/builtin/fetch.c\n index 01865b5c09..85091af99b 100644\n --- a/builtin/fetch.c\n +++ b/builtin/fetch.c\n @@ -2122,7 +2122,14 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \tstring_list_clear(&list, 0);\n \n -\t/* skip irrelevant tasks if objects were not fetched */\n +\t/*\n +\t * Skip irrelevant tasks because we know objects were not\n +\t * fetched.\n +\t *\n +\t * NEEDSWORK: as a future optimization, we can return early\n +\t * whenever objects were not fetched e.g. if we already have all\n +\t * of them.\n +\t */\n \tif (negotiate_only)\n \t\treturn result;\n \n\n builtin/fetch.c | 29 ++++++++++++++++++++++++-----\n t/t5516-fetch-push.sh | 12 ++++++++++++\n 2 files changed, 36 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f7abbc31ff..85091af99b 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1996,6 +1996,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n+\n+\tif (negotiate_only) {\n+\t\t/*\n+\t\t * --negotiate-only should never recurse into\n+\t\t * submodules, so there is no need to read .gitmodules.\n+\t\t */\n+\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n+\t\tif (!negotiation_tip.nr)\n+\t\t\tdie(_(\"--negotiate-only needs one or more --negotiate-tip=*\"));\n+\t}\n+\n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n \t\tint *sfjc = submodule_fetch_jobs_config == -1\n \t\t\t ? &submodule_fetch_jobs_config : NULL;\n@@ -2005,9 +2016,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tfetch_config_from_gitmodules(sfjc, rs);\n \t}\n \n-\tif (negotiate_only && !negotiation_tip.nr)\n-\t\tdie(_(\"--negotiate-only needs one or more --negotiate-tip=*\"));\n-\n \tif (deepen_relative) {\n \t\tif (deepen_relative < 0)\n \t\t\tdie(_(\"Negative depth in --deepen is not supported\"));\n@@ -2112,6 +2120,19 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tresult = fetch_multiple(&list, max_children);\n \t}\n \n+\tstring_list_clear(&list, 0);\n+\n+\t/*\n+\t * Skip irrelevant tasks because we know objects were not\n+\t * fetched.\n+\t *\n+\t * NEEDSWORK: as a future optimization, we can return early\n+\t * whenever objects were not fetched e.g. if we already have all\n+\t * of them.\n+\t */\n+\tif (negotiate_only)\n+\t\treturn result;\n+\n \tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n \t\tstruct strvec options = STRVEC_INIT;\n \t\tint max_children = max_jobs;\n@@ -2132,8 +2153,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tstrvec_clear(&options);\n \t}\n \n-\tstring_list_clear(&list, 0);\n-\n \tprepare_repo_settings(the_repository);\n \tif (fetch_write_commit_graph > 0 ||\n \t (fetch_write_commit_graph < 0 &&\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 8212ca56dc..732031085e 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -229,6 +229,18 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n \ttest_i18ngrep \"push negotiation failed\" err\n '\n \n+test_expect_success 'push with negotiation does not attempt to fetch submodules' '\n+\tmk_empty submodule_upstream &&\n+\ttest_commit -C submodule_upstream submodule_commit &&\n+\tgit submodule add ./submodule_upstream submodule &&\n+\tmk_empty testrepo &&\n+\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\ttest_commit -C testrepo unrelated_commit &&\n+\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n+\tgit -c submodule.recurse=true -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n+\t! grep \"Fetching submodule\" err\n+'\n+\n test_expect_success 'push without wildcard' '\n \tmk_empty testrepo &&\n \n-- \n2.33.GIT\n\n"},{"id":"444402","messageId":"xmqqilvm24bb.fsf@gitster.g","threadId":"57049","inReplyTo":"20211217000235.68996-1-chooglen@google.com","subject":"Re: [PATCH v2] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-17T23:35:04Z","receivedAt":"2021-12-17T23:35:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> cmd_fetch() performs certain tasks with the assumption that objects are\n> fetched, but `git fetch --negotiate-only` does not fetch objects, as its\n> name implies. This is results in behavior that is unnecessary at best,\n> and incorrect at worst:\n>\n> * Submodules are updated if enabled by recurse.submodules=true, but\n> negotiation fetch doesn't actually update the repo, so this doesn't\n> make sense (introduced in [1]).\n\nHmph.\n\n> * Commit graphs will be written if enabled by\n> fetch.writeCommitGraph=true. But according to\n> Documentation/config/fetch.txt [2], this should only be done if a\n> pack-file is downloaded.\n\nMakes sense, as we haven't changed any reachability, and we have no\nneed to rewrite the graph file.\n\n> * gc is run, but according to [3], we only do this because we expect\n> `git fetch` to introduce objects.\n\nMakes sense. As we haven't added any new objects, there is nothing\n(other than the passage of time) that adds to the need to collect\ngarbage.\n\nIt makes me wonder if we need to do anything upon \"fetch --dry-run\".\nI know we add to the object store without making anything reachable,\nso that the user can do pre-flight checks with the real objects. We\ndo not change the reachability so there is no reason to rewrite the\ngraph file, but we do add cruft to the object store.\n\nDoing something about \"--dry-run\" is obviously outside the scope of\nthis topic, but it may make sense to think about it while we are\nthinking about \"fetch\".\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index f7abbc31ff..85091af99b 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1996,6 +1996,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \n> \targc = parse_options(argc, argv, prefix,\n> \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n> +\n> +\tif (negotiate_only) {\n> +\t\t/*\n> +\t\t * --negotiate-only should never recurse into\n> +\t\t * submodules, so there is no need to read .gitmodules.\n> +\t\t */\n> +\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n> +\t\tif (!negotiation_tip.nr)\n> +\t\t\tdie(_(\"--negotiate-only needs one or more --negotiate-tip=*\"));\n> +\t}\n> +\n\nThis means \"fetch --negotiate-only --recurse-submodules\" silently\nignores an explicit wish by the user.\n\nI suspect that this part should be more like this.\n\n\tif (negitiate_only &&\n\t recurse_submodules != RECURSE_SUBMODULES_OFF) {\n\t\tif (recurse_submodules came from the parse_options)\n\t\t\tdie(_(\"'--%s' cannot be used with '--%s'\",\n\t\t\t \"recurse-submodules\", \"negotiate-only\"));\n\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n\t}\n\nThat is, we complain if user gives us a combination we do not\nsupport, but we are OK if the configuration is set to do so and\nsilently ignore (because we declare that the combination does not\nmake sense).\n\nBy the way, do not move the check about the number of negotiation\ntips from the original location. That check, or its location, have\nnothing to do with what you want to do in this patch, which is \"do\nnot gc or update the graph file if we are not fetching\". It is\nbetter to leave unrelated changes out of the patch.\n\nIn order to tell if recurse_submodules that is not OFF came from the\ncall to parse_options(), you may need to capture the value of the\nvariable before calling parse_options() and compare it with the\ncurrent value in the above illustration code snippet I gave.\n\nHaving said all that, is it true that recurse-submodules should not\nbe combined with negotiate-only? I naively think it would not be\nsurprising if users expect negotiate-only fetches are done also in\nthe submodules. Whatever we decide the right behaviour should be,\nwe should document it. With your patch without any of my above\ninput, I would expect at least something like\n\n diff --git i/Documentation/fetch-options.txt w/Documentation/fetch-options.txt\n index e967ff1874..baf2e9c50d 100644\n --- i/Documentation/fetch-options.txt\n +++ w/Documentation/fetch-options.txt\n @@ -73,6 +73,9 @@ configuration variables documented in linkgit:git-config[1], and the\n +\n Internally this is used to implement the `push.negotiate` option, see\n linkgit:git-config[1].\n ++\n +Note that this option silently makes various options that do not make\n +sense to be used together with it (e.g. `--recurse-submodules`) ignored.\n \n --dry-run::\n Show what would be done, without making any changes.\n\nto leave wiggling room for us to silently ignore more. We may know\nabout --recurse-submodules today, but I would not be surprised if we\nfind more.\n\n> @@ -2112,6 +2120,19 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \t\tresult = fetch_multiple(&list, max_children);\n> \t}\n> \n> +\tstring_list_clear(&list, 0);\n> +\n> +\t/*\n> +\t * Skip irrelevant tasks because we know objects were not\n> +\t * fetched.\n> +\t *\n> +\t * NEEDSWORK: as a future optimization, we can return early\n> +\t * whenever objects were not fetched e.g. if we already have all\n> +\t * of them.\n> +\t */\n> +\tif (negotiate_only)\n> +\t\treturn result;\n> +\n\nI find it somewhat misleading to have the early return before the\nblock for recurse_submodules, as we _are_ already forcing it to not\nto recurse. It would be more readable if it went before the place\nwhere we start doing the post-action clean-ups like reachability\ngraphs and garbage collection.\n\n> \tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n> \t\tstruct strvec options = STRVEC_INIT;\n> \t\tint max_children = max_jobs;\n> @@ -2132,8 +2153,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \t\tstrvec_clear(&options);\n> \t}\n> \n> -\tstring_list_clear(&list, 0);\n> -\n\nNamely, here.\n\n> \tprepare_repo_settings(the_repository);\n\nThis is existing code, but I wonder why it can be done _SO_ late in\nthe sequence. We've already called the transport API for the\nnegotiate-only communication at this point, but a call to this\nfunction is the only thing that gives fetch_negotiation_algorithm\nmember in the_repository its default value, isn't it?\n\nThanks.\n"},{"id":"444526","messageId":"kl6ltuf3ysnw.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57049","inReplyTo":"xmqqilvm24bb.fsf@gitster.g","subject":"Re: [PATCH v2] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-12-20T19:37:07Z","receivedAt":"2021-12-20T19:37:11Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n>> * gc is run, but according to [3], we only do this because we expect\n>> `git fetch` to introduce objects.\n>\n> Makes sense. As we haven't added any new objects, there is nothing\n> (other than the passage of time) that adds to the need to collect\n> garbage.\n>\n> It makes me wonder if we need to do anything upon \"fetch --dry-run\".\n> I know we add to the object store without making anything reachable,\n> so that the user can do pre-flight checks with the real objects. We\n> do not change the reachability so there is no reason to rewrite the\n> graph file, but we do add cruft to the object store.\n>\n> Doing something about \"--dry-run\" is obviously outside the scope of\n> this topic, but it may make sense to think about it while we are\n> thinking about \"fetch\".\n\nI hadn't considered \"--dry-run\". I'll think about that while I structure\nthis patch.\n\n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index f7abbc31ff..85091af99b 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -1996,6 +1996,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>> \n>> \targc = parse_options(argc, argv, prefix,\n>> \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n>> +\n>> +\tif (negotiate_only) {\n>> +\t\t/*\n>> +\t\t * --negotiate-only should never recurse into\n>> +\t\t * submodules, so there is no need to read .gitmodules.\n>> +\t\t */\n>> +\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n>> +\t\tif (!negotiation_tip.nr)\n>> +\t\t\tdie(_(\"--negotiate-only needs one or more --negotiate-tip=*\"));\n>> +\t}\n>> +\n>\n> This means \"fetch --negotiate-only --recurse-submodules\" silently\n> ignores an explicit wish by the user.\n>\n> I suspect that this part should be more like this.\n>\n> \tif (negitiate_only &&\n> \t recurse_submodules != RECURSE_SUBMODULES_OFF) {\n> \t\tif (recurse_submodules came from the parse_options)\n> \t\t\tdie(_(\"'--%s' cannot be used with '--%s'\",\n> \t\t\t \"recurse-submodules\", \"negotiate-only\"));\n> \t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n> \t}\n>\n> That is, we complain if user gives us a combination we do not\n> support, but we are OK if the configuration is set to do so and\n> silently ignore (because we declare that the combination does not\n> make sense).\n\nYes, Jonathan proposed this as well. This is identical to the approach\nin gc/branch-recurse-submodules, which as I noted in [1], is a bit\ninconsistent with submodule parsing in general.\n\nI decided *against* this because I thought that \"--negotiate-only\" is\ninternal-only. I don't see why a user would use \"--negotiate-only\", but\nif this is a user journey we want to care about, then adding the\nexplicit check sounds ok.\n\n> By the way, do not move the check about the number of negotiation\n> tips from the original location. That check, or its location, have\n> nothing to do with what you want to do in this patch, which is \"do\n> not gc or update the graph file if we are not fetching\". It is\n> better to leave unrelated changes out of the patch.\n\nAh, I see that it's not easy to tell whether or not the behavior is\ncorrect after that line is moved. I'll avoid doing this in the future.\n\nI still think that it is cleaner to move the negotiation_tip.nr check.\nShould I do this in a follow-up patch?\n\n> In order to tell if recurse_submodules that is not OFF came from the\n> call to parse_options(), you may need to capture the value of the\n> variable before calling parse_options() and compare it with the\n> current value in the above illustration code snippet I gave.\n>\n> Having said all that, is it true that recurse-submodules should not\n> be combined with negotiate-only? I naively think it would not be\n> surprising if users expect negotiate-only fetches are done also in\n> the submodules.\n\nIn the current form of \"--negotiate-only\", no it does not make sense for\nthem to be combined. I think users would have the same expectations as\nyou if they were invoking \"git fetch --negotiate-only\" directly, but I'm\nnot convinced that that they will ever do so, except to debug push\nnegotiation.\n\nI hope Jonathan can chime in to confirm whether or not users want/need\nto invoke \"--negotiate-only\".\n\n> Whatever we decide the right behaviour should be, we should document\n> it. With your patch without any of my above input, I would expect at\n> least something like\n>\n> diff --git i/Documentation/fetch-options.txt w/Documentation/fetch-options.txt\n> index e967ff1874..baf2e9c50d 100644\n> --- i/Documentation/fetch-options.txt\n> +++ w/Documentation/fetch-options.txt\n> @@ -73,6 +73,9 @@ configuration variables documented in linkgit:git-config[1], and the\n> +\n> Internally this is used to implement the `push.negotiate` option, see\n> linkgit:git-config[1].\n> ++\n> +Note that this option silently makes various options that do not make\n> +sense to be used together with it (e.g. `--recurse-submodules`) ignored.\n> \n> --dry-run::\n> Show what would be done, without making any changes.\n>\n> to leave wiggling room for us to silently ignore more. We may know\n> about --recurse-submodules today, but I would not be surprised if we\n> find more.\n\nSounds good.\n\n>> @@ -2112,6 +2120,19 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>> \t\tresult = fetch_multiple(&list, max_children);\n>> \t}\n>> \n>> +\tstring_list_clear(&list, 0);\n>> +\n>> +\t/*\n>> +\t * Skip irrelevant tasks because we know objects were not\n>> +\t * fetched.\n>> +\t *\n>> +\t * NEEDSWORK: as a future optimization, we can return early\n>> +\t * whenever objects were not fetched e.g. if we already have all\n>> +\t * of them.\n>> +\t */\n>> +\tif (negotiate_only)\n>> +\t\treturn result;\n>> +\n>\n> I find it somewhat misleading to have the early return before the\n> block for recurse_submodules, as we _are_ already forcing it to not\n> to recurse. It would be more readable if it went before the place\n> where we start doing the post-action clean-ups like reachability\n> graphs and garbage collection.\n\nYour feedback is valid, but I think it is true because negotiate_only\nand recurse_submodules just happen to have a specifal interaction. As\nindicated by the comments, this early return can apply to any situation\nwhere objects were not fetched at all, but we only consider\nnegotiate_only right now.\n\nThis early return should also apply to submodules because if no new\nobjects were found in the superproject, all of the superproject commits\nare referencing known submodule commits. If the submodule commits are\nnot known, they should be updated with \"git submodule update\", not \"git\nfetch\".\n\n>> \tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n>> \t\tstruct strvec options = STRVEC_INIT;\n>> \t\tint max_children = max_jobs;\n>> @@ -2132,8 +2153,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>> \t\tstrvec_clear(&options);\n>> \t}\n>> \n>> -\tstring_list_clear(&list, 0);\n>> -\n>\n> Namely, here.\n\nHm, I realize that I could have used a goto instead of moving\nstring_list_clear()..\n\n>\n>> \tprepare_repo_settings(the_repository);\n>\n> This is existing code, but I wonder why it can be done _SO_ late in\n> the sequence. We've already called the transport API for the\n> negotiate-only communication at this point, but a call to this\n> function is the only thing that gives fetch_negotiation_algorithm\n> member in the_repository its default value, isn't it?\n\nThat's right, this looks like it could be a bug. Maybe Jonathan knows\nmore.\n\n[1] https://lore.kernel.org/git/kl6lwnk4to5x.fsf@chooglen-macbookpro.roam.corp.google.com\n"},{"id":"444532","messageId":"xmqq4k73vymp.fsf@gitster.g","threadId":"57049","inReplyTo":"kl6ltuf3ysnw.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v2] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-20T19:56:30Z","receivedAt":"2021-12-20T19:56:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n>> By the way, do not move the check about the number of negotiation\n>> tips from the original location. That check, or its location, have\n>> nothing to do with what you want to do in this patch, which is \"do\n>> not gc or update the graph file if we are not fetching\". It is\n>> better to leave unrelated changes out of the patch.\n>\n> Ah, I see that it's not easy to tell whether or not the behavior is\n> correct after that line is moved. I'll avoid doing this in the future.\n>\n> I still think that it is cleaner to move the negotiation_tip.nr check.\n> Should I do this in a follow-up patch?\n\nI am not seeing what makes it cleaner to have it early or at the\ncurrent position, but with \"It is cleaner to do tip.nr check early\nbecause ...\", in a separate patch, it may become obvious. But let's\nnot do it in this patch.\n\n> I hope Jonathan can chime in to confirm whether or not users want/need\n> to invoke \"--negotiate-only\".\n\nYeah, I knew that this was \"internal implementation detail\", but\nthen perhaps we know that a sane developer who knows what they are\ndoing will never write combination of it with recurse-submodule\noption? If so, we'd catch a notice developer breaking the system by\nhaving a sanity check by detecting it as an error, no?\n"},{"id":"444539","messageId":"kl6lo85byp3b.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57049","inReplyTo":"xmqq4k73vymp.fsf@gitster.g","subject":"Re: [PATCH v2] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-12-20T20:54:16Z","receivedAt":"2021-12-20T20:54:20Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> Glen Choo writes:\n>\n>>> By the way, do not move the check about the number of negotiation\n>>> tips from the original location. That check, or its location, have\n>>> nothing to do with what you want to do in this patch, which is \"do\n>>> not gc or update the graph file if we are not fetching\". It is\n>>> better to leave unrelated changes out of the patch.\n>>\n>> Ah, I see that it's not easy to tell whether or not the behavior is\n>> correct after that line is moved. I'll avoid doing this in the future.\n>>\n>> I still think that it is cleaner to move the negotiation_tip.nr check.\n>> Should I do this in a follow-up patch?\n>\n> I am not seeing what makes it cleaner to have it early or at the\n> current position, but with \"It is cleaner to do tip.nr check early\n> because ...\", in a separate patch, it may become obvious. But let's\n> not do it in this patch.\n\nNoted, thanks!\n\n>> I hope Jonathan can chime in to confirm whether or not users want/need\n>> to invoke \"--negotiate-only\".\n>\n> Yeah, I knew that this was \"internal implementation detail\", but\n> then perhaps we know that a sane developer who knows what they are\n> doing will never write combination of it with recurse-submodule\n> option? If so, we'd catch a notice developer breaking the system by\n> having a sanity check by detecting it as an error, no?\n\nHm, I suppose that is true, though I have some reservations. In\nparticular, I'm not sure if the argument you are making is intended to\nbe specific to this combination of options, or all incompatible options\nin general.\n\nI think it might make sense to check for this specific combination of\n\"--negotiate-only\" and \"--recurse-submodules\" because where the stakes\nare low and behavior is reasonably simple to understand, though the\npayoff is also pretty low.\n\nIn the more general case of \"Does it *always* make sense to check for an\ninvalid combination of options?\", I find your argument a bit troubling\nbecause it seems to imply that anything we add to the Git CLI should be\ntreated as if users will depend on it. This seems like a needless burden\n(or even an antipattern) if we need to fork a Git process purely as an\nimplementation detail, but we have to treat these internal CLI options\nas user-facing.\n\nBut maybe I am misunderstanding how Git treats CLI options in general?\nWe don't ever really hide CLI options, even if they are only\ninternal. There is PARSE_OPT_HIDDEN, but those still show up in usage\nand documentation [1]. So we actually do want users to know about these\nimplementation details?\n\nPresumably, the thought process goes something like this: if we add an\noption, a user *could* find a use for it (or it might accidentally\nconflict with a user's flags), even if we never intended it for end user\nconsumption. Thus we need to treat all CLI options with care.\n\nIs this a more accurate description of how we think about CLI options?\n\n[1] This statement doesn't apply to the -- commands (like\nsubmodule--helper). Those are 'truly' hidden because they don't have\npublic docs AFAIK.\n"},{"id":"444550","messageId":"xmqqmtkvsz6y.fsf@gitster.g","threadId":"57049","inReplyTo":"kl6lo85byp3b.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v2] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-20T22:12:37Z","receivedAt":"2021-12-20T22:12:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> But maybe I am misunderstanding how Git treats CLI options in general?\n> We don't ever really hide CLI options, even if they are only\n> internal. There is PARSE_OPT_HIDDEN, but those still show up in usage\n> and documentation [1]. So we actually do want users to know about these\n> implementation details?\n\nAt least they should be told not to use from the command line when\nthey have no business triggering whatever feature these options\ntrigger. \n\n> Presumably, the thought process goes something like this: if we add an\n> option, a user *could* find a use for it (or it might accidentally\n> conflict with a user's flags), even if we never intended it for end user\n> consumption. Thus we need to treat all CLI options with care.\n\nRather, they can keep both halves ;-)\n"},{"id":"444565","messageId":"kl6lee66zu7c.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57049","inReplyTo":"xmqqmtkvsz6y.fsf@gitster.g","subject":"Re: [PATCH v2] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-12-21T00:18:31Z","receivedAt":"2021-12-21T00:18:35Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> Glen Choo writes:\n>\n>> But maybe I am misunderstanding how Git treats CLI options in general?\n>> We don't ever really hide CLI options, even if they are only\n>> internal. There is PARSE_OPT_HIDDEN, but those still show up in usage\n>> and documentation [1]. So we actually do want users to know about these\n>> implementation details?\n>\n> At least they should be told not to use from the command line when\n> they have no business triggering whatever feature these options\n> trigger. \n\nMakes sense. I can add this to the docs if we think --negotiate-only\nisn't meant for end users.\n\n>> Presumably, the thought process goes something like this: if we add an\n>> option, a user *could* find a use for it (or it might accidentally\n>> conflict with a user's flags), even if we never intended it for end user\n>> consumption. Thus we need to treat all CLI options with care.\n>\n> Rather, they can keep both halves ;-)\n\nHm, do you mean \"both halves\" as in, they should be able to choose\nwhether or not they want to use an internal option?\n"},{"id":"444670","messageId":"kl6lczlpzhdn.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57049","inReplyTo":"kl6ltuf3ysnw.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v2] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-12-21T23:07:48Z","receivedAt":"2021-12-21T23:07:52Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Glen Choo writes:\n\n>>> \tprepare_repo_settings(the_repository);\n>>\n>> This is existing code, but I wonder why it can be done _SO_ late in\n>> the sequence. We've already called the transport API for the\n>> negotiate-only communication at this point, but a call to this\n>> function is the only thing that gives fetch_negotiation_algorithm\n>> member in the_repository its default value, isn't it?\n>\n> That's right, this looks like it could be a bug. Maybe Jonathan knows\n> more.\n\nIt seems that fetch negotiation always calls prepare_repo_settings().\nFetch negotiation uses negotiate_using_fetch(), which calls\nfetch_negotiator_init(), which calls prepare_repo_settings():\n\n void fetch_negotiator_init(struct repository *r,\n struct fetch_negotiator *negotiator)\n {\n prepare_repo_settings(r);\n switch(r->settings.fetch_negotiation_algorithm) {\n case FETCH_NEGOTIATION_SKIPPING:\n skipping_negotiator_init(negotiator);\n return;\n\n case FETCH_NEGOTIATION_NOOP:\n noop_negotiator_init(negotiator);\n return;\n\n case FETCH_NEGOTIATION_DEFAULT:\n default_negotiator_init(negotiator);\n return;\n }\n }\n\nIf anything, this seems safer than calling prepare_repo_settings() in\ncmd_fetch() :)\n"},{"id":"444683","messageId":"20211222001134.28933-1-chooglen@google.com","threadId":"57049","inReplyTo":"20211217000235.68996-1-chooglen@google.com","subject":"[PATCH v3 0/3] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-12-22T00:11:31Z","receivedAt":"2021-12-22T00:11:50Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"cmd_fetch() performs certain tasks with the assumption that objects are\nfetched, but `git fetch --negotiate-only` does not fetch objects, as its\nname implies. This is results in behavior that is unnecessary at best,\nand incorrect at worst:\n\n* Submodules are updated if enabled by recurse.submodules=true, but\n negotiation fetch doesn't actually update the repo, so this doesn't\n make sense (introduced in [1]).\n* Commit graphs will be written if enabled by\n fetch.writeCommitGraph=true. But according to\n Documentation/config/fetch.txt [2], this should only be done if a\n pack-file is downloaded.\n* gc is run, but according to [3], we only do this because we expect\n `git fetch` to introduce objects.\n\nMake `git fetch --negotiate-only` handle these tasks more rigorously by\ndoing the following:\n\n* Make cmd_fetch() return early if we know for certain that objects will\n not be fetched\n* Disable submodule recursion and die() if a user explicitly asks for it\n\n[1] 7dce19d374 (fetch/pull: Add the --recurse-submodules option, 2010-11-12)\n[2] 50f26bd035 (fetch: add fetch.writeCommitGraph config setting, 2019-09-02)\n[3] 131b8fcbfb (fetch: run gc --auto after fetching, 2013-01-26)\n\nChanges since v2:\n* added a prepatory patch that introduces a \"goto cleanup\"\n* drop an unnecessary line move (as suggested by Junio)\n* check for user-given --recurse-submodules and die() (as suggested by\n Jonathan and Junio)\n* update --negotiate-only's documentation \n\nChanges since v1:\n* added more context to commit message\n* added a NEEDSWORK comment \n\nGlen Choo (3):\n builtin/fetch: use goto cleanup in cmd_fetch()\n builtin/fetch: skip unnecessary tasks when using --negotiate-only\n builtin/fetch: die on --negotiate-only and --recurse-submodules\n\n Documentation/fetch-options.txt | 1 +\n builtin/fetch.c | 41 +++++++++++++++++++++++++++++----\n t/t5516-fetch-push.sh | 12 ++++++++++\n t/t5702-protocol-v2.sh | 17 ++++++++++++++\n 4 files changed, 67 insertions(+), 4 deletions(-)\n\nRange-diff against v2:\n-: ---------- > 1: 97e2cba633 builtin/fetch: use goto cleanup in cmd_fetch()\n1: 9d18270a41 ! 2: 3a3a04b649 builtin/fetch: skip unnecessary tasks when using --negotiate-only\n @@ Commit message\n return early whenever objects are not fetched, but for now this only\n considers --negotiate-only.\n \n + A similar optimization would be to skip irrelevant tasks in `git fetch\n + --dry-run`. This optimization was not done in this commit because a dry\n + run will actually fetch objects; we'd presumably still want to recurse\n + into submodules and run gc.\n +\n [1] 7dce19d374 (fetch/pull: Add the --recurse-submodules option, 2010-11-12)\n [2] 50f26bd035 (fetch: add fetch.writeCommitGraph config setting, 2019-09-02)\n [3] 131b8fcbfb (fetch: run gc --auto after fetching, 2013-01-26)\n @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n +\t\t * submodules, so there is no need to read .gitmodules.\n +\t\t */\n +\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n -+\t\tif (!negotiation_tip.nr)\n -+\t\t\tdie(_(\"--negotiate-only needs one or more --negotiate-tip=*\"));\n +\t}\n +\n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n \t\tint *sfjc = submodule_fetch_jobs_config == -1\n \t\t\t ? &submodule_fetch_jobs_config : NULL;\n -@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n - \t\tfetch_config_from_gitmodules(sfjc, rs);\n - \t}\n - \n --\tif (negotiate_only && !negotiation_tip.nr)\n --\t\tdie(_(\"--negotiate-only needs one or more --negotiate-tip=*\"));\n --\n - \tif (deepen_relative) {\n - \t\tif (deepen_relative < 0)\n - \t\t\tdie(_(\"Negative depth in --deepen is not supported\"));\n @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tresult = fetch_multiple(&list, max_children);\n \t}\n \n -+\tstring_list_clear(&list, 0);\n -+\n +\t/*\n +\t * Skip irrelevant tasks because we know objects were not\n +\t * fetched.\n @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n +\t * of them.\n +\t */\n +\tif (negotiate_only)\n -+\t\treturn result;\n ++\t\tgoto cleanup;\n +\n \tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n \t\tstruct strvec options = STRVEC_INIT;\n \t\tint max_children = max_jobs;\n -@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n - \t\tstrvec_clear(&options);\n - \t}\n - \n --\tstring_list_clear(&list, 0);\n --\n - \tprepare_repo_settings(the_repository);\n - \tif (fetch_write_commit_graph > 0 ||\n - \t (fetch_write_commit_graph < 0 &&\n \n ## t/t5516-fetch-push.sh ##\n @@ t/t5516-fetch-push.sh: test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n-: ---------- > 3: 97792b5214 builtin/fetch: die on --negotiate-only and --recurse-submodules\n\nbase-commit: abe6bb3905392d5eb6b01fa6e54d7e784e0522aa\n-- \n2.33.GIT\n\n"},{"id":"444684","messageId":"20211222001134.28933-2-chooglen@google.com","threadId":"57049","inReplyTo":"20211222001134.28933-1-chooglen@google.com","subject":"[PATCH v3 1/3] builtin/fetch: use goto cleanup in cmd_fetch()","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-12-22T00:11:32Z","receivedAt":"2021-12-22T00:11:54Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Replace an early return with 'goto cleanup' in cmd_fetch() so that the\nstring_list is always cleared.\n\nThe string_list_clear() call is purely cleanup; the string_list was not\nreused.\n\nSigned-off-by: Glen Choo \n---\n builtin/fetch.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f7abbc31ff..eab73d7417 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2076,7 +2076,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tgtransport->smart_options->acked_commits = &acked_commits;\n \t\t} else {\n \t\t\twarning(_(\"Protocol does not support --negotiate-only, exiting.\"));\n-\t\t\treturn 1;\n+\t\t\tresult = 1;\n+\t\t\tgoto cleanup;\n \t\t}\n \t\tif (server_options.nr)\n \t\t\tgtransport->server_options = &server_options;\n@@ -2132,8 +2133,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tstrvec_clear(&options);\n \t}\n \n-\tstring_list_clear(&list, 0);\n-\n \tprepare_repo_settings(the_repository);\n \tif (fetch_write_commit_graph > 0 ||\n \t (fetch_write_commit_graph < 0 &&\n@@ -2151,5 +2150,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (enable_auto_gc)\n \t\trun_auto_maintenance(verbosity < 0);\n \n+ cleanup:\n+\tstring_list_clear(&list, 0);\n \treturn result;\n }\n-- \n2.33.GIT\n\n"},{"id":"444685","messageId":"20211222001134.28933-3-chooglen@google.com","threadId":"57049","inReplyTo":"20211222001134.28933-1-chooglen@google.com","subject":"[PATCH v3 2/3] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-12-22T00:11:33Z","receivedAt":"2021-12-22T00:11:54Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"cmd_fetch() performs certain tasks with the assumption that objects are\nfetched, but `git fetch --negotiate-only` does not fetch objects, as its\nname implies. This is results in behavior that is unnecessary at best,\nand incorrect at worst:\n\n* Submodules are updated if enabled by recurse.submodules=true, but\n negotiation fetch doesn't actually update the repo, so this doesn't\n make sense (introduced in [1]).\n* Commit graphs will be written if enabled by\n fetch.writeCommitGraph=true. But according to\n Documentation/config/fetch.txt [2], this should only be done if a\n pack-file is downloaded.\n* gc is run, but according to [3], we only do this because we expect\n `git fetch` to introduce objects.\n\nInstead of disabling these tasks piecemeal, make cmd_fetch() return\nearly if we know for certain that objects will not be fetched. We can\nreturn early whenever objects are not fetched, but for now this only\nconsiders --negotiate-only.\n\nA similar optimization would be to skip irrelevant tasks in `git fetch\n--dry-run`. This optimization was not done in this commit because a dry\nrun will actually fetch objects; we'd presumably still want to recurse\ninto submodules and run gc.\n\n[1] 7dce19d374 (fetch/pull: Add the --recurse-submodules option, 2010-11-12)\n[2] 50f26bd035 (fetch: add fetch.writeCommitGraph config setting, 2019-09-02)\n[3] 131b8fcbfb (fetch: run gc --auto after fetching, 2013-01-26)\n\nSigned-off-by: Glen Choo \n---\n builtin/fetch.c | 20 ++++++++++++++++++++\n t/t5516-fetch-push.sh | 12 ++++++++++++\n 2 files changed, 32 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex eab73d7417..883bb1b10c 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1996,6 +1996,15 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n+\n+\tif (negotiate_only) {\n+\t\t/*\n+\t\t * --negotiate-only should never recurse into\n+\t\t * submodules, so there is no need to read .gitmodules.\n+\t\t */\n+\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n+\t}\n+\n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n \t\tint *sfjc = submodule_fetch_jobs_config == -1\n \t\t\t ? &submodule_fetch_jobs_config : NULL;\n@@ -2113,6 +2122,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tresult = fetch_multiple(&list, max_children);\n \t}\n \n+\t/*\n+\t * Skip irrelevant tasks because we know objects were not\n+\t * fetched.\n+\t *\n+\t * NEEDSWORK: as a future optimization, we can return early\n+\t * whenever objects were not fetched e.g. if we already have all\n+\t * of them.\n+\t */\n+\tif (negotiate_only)\n+\t\tgoto cleanup;\n+\n \tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n \t\tstruct strvec options = STRVEC_INIT;\n \t\tint max_children = max_jobs;\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 8212ca56dc..732031085e 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -229,6 +229,18 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n \ttest_i18ngrep \"push negotiation failed\" err\n '\n \n+test_expect_success 'push with negotiation does not attempt to fetch submodules' '\n+\tmk_empty submodule_upstream &&\n+\ttest_commit -C submodule_upstream submodule_commit &&\n+\tgit submodule add ./submodule_upstream submodule &&\n+\tmk_empty testrepo &&\n+\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\ttest_commit -C testrepo unrelated_commit &&\n+\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n+\tgit -c submodule.recurse=true -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n+\t! grep \"Fetching submodule\" err\n+'\n+\n test_expect_success 'push without wildcard' '\n \tmk_empty testrepo &&\n \n-- \n2.33.GIT\n\n"},{"id":"444686","messageId":"20211222001134.28933-4-chooglen@google.com","threadId":"57049","inReplyTo":"20211222001134.28933-1-chooglen@google.com","subject":"[PATCH v3 3/3] builtin/fetch: die on --negotiate-only and --recurse-submodules","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-12-22T00:11:34Z","receivedAt":"2021-12-22T00:11:56Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"The previous commit ignores the value of --recurse-submodules if\n--negotiate-only is given. Since non \"no\" values of --recurse-submodules\nare not supported with --negotiate-only, make cmd_fetch() check for\nthis invalid combination and die.\n\nThis is unlikely to affect internal usage of --negotiate-only, but it\nmay be helpful for users. We may also want to discourage users from\nusing --negotiate-only altogether because it was not intended for them.\n\nSigned-off-by: Glen Choo \n---\n Documentation/fetch-options.txt | 1 +\n builtin/fetch.c | 24 ++++++++++++++++++------\n t/t5702-protocol-v2.sh | 17 +++++++++++++++++\n 3 files changed, 36 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex e967ff1874..81a100d593 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -71,6 +71,7 @@ configuration variables documented in linkgit:git-config[1], and the\n \tancestors of the provided `--negotiation-tip=*` arguments,\n \twhich we have in common with the server.\n +\n+This is incompatible with `--recurse-submodules[=yes|on-demand]`.\n Internally this is used to implement the `push.negotiate` option, see\n linkgit:git-config[1].\n \ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 883bb1b10c..54f970130e 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -75,6 +75,7 @@ static struct transport *gtransport;\n static struct transport *gsecondary;\n static const char *submodule_prefix = \"\";\n static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+static int recurse_submodules_explicit = RECURSE_SUBMODULES_DEFAULT;\n static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;\n static int shown_url = 0;\n static struct refspec refmap = REFSPEC_INIT_FETCH;\n@@ -166,7 +167,7 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"prune remote-tracking branches no longer on remote\")),\n \tOPT_BOOL('P', \"prune-tags\", &prune_tags,\n \t\t N_(\"prune local tags no longer on remote and clobber changed tags\")),\n-\tOPT_CALLBACK_F(0, \"recurse-submodules\", &recurse_submodules, N_(\"on-demand\"),\n+\tOPT_CALLBACK_F(0, \"recurse-submodules\", &recurse_submodules_explicit, N_(\"on-demand\"),\n \t\t N_(\"control recursive fetching of submodules\"),\n \t\t PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),\n \tOPT_BOOL(0, \"dry-run\", &dry_run,\n@@ -1997,12 +1998,23 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n \n+\tif (recurse_submodules_explicit != RECURSE_SUBMODULES_DEFAULT)\n+\t\trecurse_submodules = recurse_submodules_explicit;\n+\n \tif (negotiate_only) {\n-\t\t/*\n-\t\t * --negotiate-only should never recurse into\n-\t\t * submodules, so there is no need to read .gitmodules.\n-\t\t */\n-\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n+\t\tswitch (recurse_submodules_explicit) {\n+\t\tcase RECURSE_SUBMODULES_OFF:\n+\t\tcase RECURSE_SUBMODULES_DEFAULT: {\n+\t\t\t/*\n+\t\t\t * --negotiate-only should never recurse into\n+\t\t\t * submodules, so there is no need to read .gitmodules.\n+\t\t\t */\n+\t\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n+\t\t\tbreak;\n+\t\t}\n+\t\tdefault:\n+\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n+\t\t}\n \t}\n \n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex d527cf6c49..d099c46499 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -628,6 +628,23 @@ test_expect_success 'usage: --negotiate-only without --negotiation-tip' '\n \ttest_cmp err.expect err.actual\n '\n \n+test_expect_success 'usage: --negotiate-only with --recurse-submodules' '\n+\tSERVER=\"server\" &&\n+\tURI=\"file://$(pwd)/server\" &&\n+\n+\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n+\n+\tcat >err.expect <<-\\EOF &&\n+\tfatal: --negotiate-only and --recurse-submodules cannot be used together\n+\tEOF\n+\n+\ttest_must_fail git -c protocol.version=2 -C client fetch \\\n+\t\t--negotiate-only \\\n+\t\t--recurse-submodules \\\n+\t\torigin 2>err.actual &&\n+\ttest_cmp err.expect err.actual\n+'\n+\n test_expect_success 'file:// --negotiate-only' '\n \tSERVER=\"server\" &&\n \tURI=\"file://$(pwd)/server\" &&\n-- \n2.33.GIT\n\n"},{"id":"444751","messageId":"xmqqa6gtkumz.fsf@gitster.g","threadId":"57049","inReplyTo":"20211222001134.28933-3-chooglen@google.com","subject":"Re: [PATCH v3 2/3] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-22T06:42:44Z","receivedAt":"2021-12-22T06:42:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> @@ -2113,6 +2122,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \t\tresult = fetch_multiple(&list, max_children);\n> \t}\n> \n> +\t/*\n> +\t * Skip irrelevant tasks because we know objects were not\n> +\t * fetched.\n> +\t *\n> +\t * NEEDSWORK: as a future optimization, we can return early\n> +\t * whenever objects were not fetched e.g. if we already have all\n> +\t * of them.\n> +\t */\n> +\tif (negotiate_only)\n> +\t\tgoto cleanup;\n\nSorry if I did not mention this in the review of the earlier round,\nbut I think the location this patch places the jump is wrong,\nespecially with the NEEDSWORK comment.\n\nWhen we are not under negotiate_only, if our earlier call to\ntransport_fetch_refs() learns to tell us that that we did not add\nany new objects, we would be able to jump to the clean-up label,\nmaking the above code to:\n\n\tif (negotiate_only || !num_fetched_objects)\n\t\tgoto cleanup;\n\nBut such a future enhancement is made harder by having this jump\nhere---the optimization the NEEDSWORK comment alludes to has no\nreason to be incompatible with \"--recurse-submodules\".\n\nIf the above block is placed _after_ the \"if the main fetch was\nsuccessful, and we are not told not to recurse into submodules, then\ndo this\" block we see below, then\n\n (1) this patch still achieves its goal, as we have manually\n and unconditionally turned recursion off;\n\n (2) such a future enhancement will not be forbidden from working\n with recurse-submodules feature.\n\n> \tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n> \t\tstruct strvec options = STRVEC_INIT;\n> \t\tint max_children = max_jobs;\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 8212ca56dc..732031085e 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -229,6 +229,18 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n> \ttest_i18ngrep \"push negotiation failed\" err\n> '\n> \n> +test_expect_success 'push with negotiation does not attempt to fetch submodules' '\n> +\tmk_empty submodule_upstream &&\n> +\ttest_commit -C submodule_upstream submodule_commit &&\n> +\tgit submodule add ./submodule_upstream submodule &&\n> +\tmk_empty testrepo &&\n> +\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n> +\ttest_commit -C testrepo unrelated_commit &&\n> +\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n> +\tgit -c submodule.recurse=true -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n> +\t! grep \"Fetching submodule\" err\n> +'\n> +\n> test_expect_success 'push without wildcard' '\n> \tmk_empty testrepo &&\n"},{"id":"444752","messageId":"xmqq5yrhkuhc.fsf@gitster.g","threadId":"57049","inReplyTo":"20211222001134.28933-4-chooglen@google.com","subject":"Re: [PATCH v3 3/3] builtin/fetch: die on --negotiate-only and --recurse-submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-22T06:46:07Z","receivedAt":"2021-12-22T06:46:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> +This is incompatible with `--recurse-submodules[=yes|on-demand]`.\n> Internally this is used to implement the `push.negotiate` option, see\n> linkgit:git-config[1].\n\nMakes sense.\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 883bb1b10c..54f970130e 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -75,6 +75,7 @@ static struct transport *gtransport;\n> static struct transport *gsecondary;\n> static const char *submodule_prefix = \"\";\n> static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n> +static int recurse_submodules_explicit = RECURSE_SUBMODULES_DEFAULT;\n> static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;\n> static int shown_url = 0;\n> static struct refspec refmap = REFSPEC_INIT_FETCH;\n> @@ -166,7 +167,7 @@ static struct option builtin_fetch_options[] = {\n> \t\t N_(\"prune remote-tracking branches no longer on remote\")),\n> \tOPT_BOOL('P', \"prune-tags\", &prune_tags,\n> \t\t N_(\"prune local tags no longer on remote and clobber changed tags\")),\n> -\tOPT_CALLBACK_F(0, \"recurse-submodules\", &recurse_submodules, N_(\"on-demand\"),\n> +\tOPT_CALLBACK_F(0, \"recurse-submodules\", &recurse_submodules_explicit, N_(\"on-demand\"),\n> \t\t N_(\"control recursive fetching of submodules\"),\n> \t\t PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),\n> \tOPT_BOOL(0, \"dry-run\", &dry_run,\n> @@ -1997,12 +1998,23 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \targc = parse_options(argc, argv, prefix,\n> \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n> \n> +\tif (recurse_submodules_explicit != RECURSE_SUBMODULES_DEFAULT)\n> +\t\trecurse_submodules = recurse_submodules_explicit;\n> +\n> \tif (negotiate_only) {\n> -\t\t/*\n> -\t\t * --negotiate-only should never recurse into\n> -\t\t * submodules, so there is no need to read .gitmodules.\n> -\t\t */\n> -\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n> +\t\tswitch (recurse_submodules_explicit) {\n> +\t\tcase RECURSE_SUBMODULES_OFF:\n> +\t\tcase RECURSE_SUBMODULES_DEFAULT: {\n> +\t\t\t/*\n> +\t\t\t * --negotiate-only should never recurse into\n> +\t\t\t * submodules, so there is no need to read .gitmodules.\n> +\t\t\t */\n> +\t\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tdefault:\n> +\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n> +\t\t}\n> \t}\n\nNice. Very cleanly done.\n"},{"id":"444788","messageId":"kl6l8rwczgzy.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57049","inReplyTo":"xmqqa6gtkumz.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-12-22T17:28:17Z","receivedAt":"2021-12-22T17:28:23Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> Glen Choo writes:\n>\n>> @@ -2113,6 +2122,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>> \t\tresult = fetch_multiple(&list, max_children);\n>> \t}\n>> \n>> +\t/*\n>> +\t * Skip irrelevant tasks because we know objects were not\n>> +\t * fetched.\n>> +\t *\n>> +\t * NEEDSWORK: as a future optimization, we can return early\n>> +\t * whenever objects were not fetched e.g. if we already have all\n>> +\t * of them.\n>> +\t */\n>> +\tif (negotiate_only)\n>> +\t\tgoto cleanup;\n>\n> Sorry if I did not mention this in the review of the earlier round,\n> but I think the location this patch places the jump is wrong,\n> especially with the NEEDSWORK comment.\n>\n> When we are not under negotiate_only, if our earlier call to\n> transport_fetch_refs() learns to tell us that that we did not add\n> any new objects, we would be able to jump to the clean-up label,\n> making the above code to:\n>\n> \tif (negotiate_only || !num_fetched_objects)\n> \t\tgoto cleanup;\n\nThanks for the clarification. Yes, we agree that the location of the\njump in this patch should be the same as the location of the jump after\nthe future optimization.\n\n> But such a future enhancement is made harder by having this jump\n> here---the optimization the NEEDSWORK comment alludes to has no\n> reason to be incompatible with \"--recurse-submodules\".\n>\n> If the above block is placed _after_ the \"if the main fetch was\n> successful, and we are not told not to recurse into submodules, then\n> do this\" block we see below, then\n>\n> (1) this patch still achieves its goal, as we have manually\n> and unconditionally turned recursion off;\n>\n> (2) such a future enhancement will not be forbidden from working\n> with recurse-submodules feature.\n>\n\nI would have come to same conclusion if I agreed that we should recurse\ninto submodules even if no objects are fetched. When I first wrote this\npatch, I was convinced that \"no new objects\" implies \"no need to update\nsubmodules\" (see my response at [1]), but I'm not sure any more and I'd\nlike to check my understanding.\n\nThe way \"fetch --recurse-submodules\" works is that the changed\nsubmodules are calcuated from the newly updated tips fetched from the\nremote. If no objects were fetched, we already have all of the\nsuperproject commits.\n\nIn ~99% of the time, no objects were fetched because the remote doesn't\nhave any info we do not know about - there are no new commits and no\nrefs were updated. In this scenario, 'git fetch' can avoid recursing\ninto submodules because there's no need to. But if we choose to recurse,\nthe worst thing that happens is that we do some file I/O and realize\nthat there are no changed submodules - essentially a no-op given that\nfetch is slow.\n\n(This is where my understanding of objects vs refs needs to be checked)\nIn the other ~1%, we might already have all commits, but a remote ref\nmight still have moved, albeit to a known commit. In this case, the\nsubmodule would need to be updated because it might have changed.\n\nIf my understanding is correct, then my patch produces the wrong\nbehavior in that ~1%. But even if my understanding is wrong, and we\ndon't need to worry about that edge case, I see that there's unnecessary\nrisk in trying to be too be clever in my reasoning and skipping what is\nessentially a no-op.\n\nIs my understanding accurate? At any rate, I'm somewhat convinced to\nmove the jump to just after the \"if main fetch was successful, and we\nare not told not to recurse into submodules\" block, i.e. before the \"if\nwe should write the commit graph\" block.\n\n[1] https://lore.kernel.org/git/kl6ltuf3ysnw.fsf@chooglen-macbookpro.roam.corp.google.com\n\n>> \tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n>> \t\tstruct strvec options = STRVEC_INIT;\n>> \t\tint max_children = max_jobs;\n>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n>> index 8212ca56dc..732031085e 100755\n>> --- a/t/t5516-fetch-push.sh\n>> +++ b/t/t5516-fetch-push.sh\n>> @@ -229,6 +229,18 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n>> \ttest_i18ngrep \"push negotiation failed\" err\n>> '\n>> \n>> +test_expect_success 'push with negotiation does not attempt to fetch submodules' '\n>> +\tmk_empty submodule_upstream &&\n>> +\ttest_commit -C submodule_upstream submodule_commit &&\n>> +\tgit submodule add ./submodule_upstream submodule &&\n>> +\tmk_empty testrepo &&\n>> +\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n>> +\ttest_commit -C testrepo unrelated_commit &&\n>> +\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n>> +\tgit -c submodule.recurse=true -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n>> +\t! grep \"Fetching submodule\" err\n>> +'\n>> +\n>> test_expect_success 'push without wildcard' '\n>> \tmk_empty testrepo &&\n"},{"id":"444815","messageId":"xmqqpmpojv48.fsf@gitster.g","threadId":"57049","inReplyTo":"kl6l8rwczgzy.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v3 2/3] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-22T19:29:59Z","receivedAt":"2021-12-22T19:30:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> I would have come to same conclusion if I agreed that we should recurse\n> into submodules even if no objects are fetched. When I first wrote this\n> patch, I was convinced that \"no new objects\" implies \"no need to update\n> submodules\" (see my response at [1]), but I'm not sure any more and I'd\n> like to check my understanding.\n\nFor example, there is a \"everything_local()\" optimization in the\nfetch-pack codepath where the following steps happen:\n\n (1) we look at the current value of the remote refs we are fetching\n from their ls_refs output (let's call it \"new tips\");\n\n (2) we notice that all of these objects happen to exist in our\n object store.\n\n (3) we make sure that we do not see any \"missing links\" if we run a\n reachability traversal that starts from these objects and our\n existing refs, and stops when the traversal intersect.\n\nWhen the last step finds that all objects necessary to point at\nthese \"new tips\" with our refs safely, then we have no reason to\nperform physical transfer of objects. Yet, we'd update our refs\nto the \"new tips\".\n\nThis can happen in a number of ways. \n\nImagine that you have a clone of https://github.com/git/git/ for\nonly its 'main' branch (i.e. a single-branch clone). If you then\nsay \"git fetch origin maint:maint\", we'll learn that the tip of\ntheir 'maint' branch points at a commit, we look into our object\nstore, find that there is no missing object to reach from it to the\npart of the object graph that is reachable from our refs (i.e. my\n'maint' is always an ancestor of my 'main'), and we find that there\nis no reason to transfer any object. Yet we will carete a new ref\nand point at the commit.\n\nOr if you did \"git branch -d\" locally, making objects unreachable in\nyour object store, and then fetch from your upstream, which had fast\nforwarded to the contents of the branch you just deleted.\n\nOr they rewound and rebuilt their branches since you fetched the\nlast time, and then they realized their mistake and now their refs\npoint at a commit that you have already seen but are different from\nwhat your remote-tracking branches point at now.\n\nOr you are using Derrick's \"prefetch\" (in \"git maintenance run\") and\na background process already downloaded the objects needed for the\nbranch you are fetching in the past.\n\nDepending on what happened when these objects were pre-fetched, such\na real fetch that did not have to perform an object transfer may\nlikely to need to adjust things in the submodule repository.\n\"prefetch\" is designed not to disrupt and to be invisible to the\nnormal operation as much as possible, so I would expect that it\nwon't do any priming of the submodules based on what it prefetched\nfor the superproject, for example.\n\nSo in short, physical object transfer can be optimized out, even\nwhen the external world view, i.e. where in the history graph the\nrefs point at, changes and makes it necessary to check in the\nsubmodule repositories.\n\n"},{"id":"444823","messageId":"kl6l4k70z8po.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57049","inReplyTo":"xmqqpmpojv48.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] builtin/fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-12-22T20:27:15Z","receivedAt":"2021-12-22T20:27:28Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> Glen Choo writes:\n>\n>> I would have come to same conclusion if I agreed that we should recurse\n>> into submodules even if no objects are fetched. When I first wrote this\n>> patch, I was convinced that \"no new objects\" implies \"no need to update\n>> submodules\" (see my response at [1]), but I'm not sure any more and I'd\n>> like to check my understanding.\n>\n> For example, there is a \"everything_local()\" optimization in the\n> fetch-pack codepath where the following steps happen:\n>\n> (1) we look at the current value of the remote refs we are fetching\n> from their ls_refs output (let's call it \"new tips\");\n>\n> (2) we notice that all of these objects happen to exist in our\n> object store.\n>\n> (3) we make sure that we do not see any \"missing links\" if we run a\n> reachability traversal that starts from these objects and our\n> existing refs, and stops when the traversal intersect.\n>\n> When the last step finds that all objects necessary to point at\n> these \"new tips\" with our refs safely, then we have no reason to\n> perform physical transfer of objects. Yet, we'd update our refs\n> to the \"new tips\".\n>\n> This can happen in a number of ways. \n>\n> Imagine that you have a clone of https://github.com/git/git/ for\n> only its 'main' branch (i.e. a single-branch clone). If you then\n> say \"git fetch origin maint:maint\", we'll learn that the tip of\n> their 'maint' branch points at a commit, we look into our object\n> store, find that there is no missing object to reach from it to the\n> part of the object graph that is reachable from our refs (i.e. my\n> 'maint' is always an ancestor of my 'main'), and we find that there\n> is no reason to transfer any object. Yet we will carete a new ref\n> and point at the commit.\n>\n> Or if you did \"git branch -d\" locally, making objects unreachable in\n> your object store, and then fetch from your upstream, which had fast\n> forwarded to the contents of the branch you just deleted.\n>\n> Or they rewound and rebuilt their branches since you fetched the\n> last time, and then they realized their mistake and now their refs\n> point at a commit that you have already seen but are different from\n> what your remote-tracking branches point at now.\n>\n> Or you are using Derrick's \"prefetch\" (in \"git maintenance run\") and\n> a background process already downloaded the objects needed for the\n> branch you are fetching in the past.\n>\n> Depending on what happened when these objects were pre-fetched, such\n> a real fetch that did not have to perform an object transfer may\n> likely to need to adjust things in the submodule repository.\n> \"prefetch\" is designed not to disrupt and to be invisible to the\n> normal operation as much as possible, so I would expect that it\n> won't do any priming of the submodules based on what it prefetched\n> for the superproject, for example.\n\nThanks for providing concrete examples; that's a lot more convincing\nthan my hypothetical.\n\n> So in short, physical object transfer can be optimized out, even\n> when the external world view, i.e. where in the history graph the\n> refs point at, changes and makes it necessary to check in the\n> submodule repositories.\n\nYes, you're right. I'll move the jump accordingly :)\n"},{"id":"444874","messageId":"20211223190808.1285008-1-jonathantanmy@google.com","threadId":"57049","inReplyTo":"20211222001134.28933-4-chooglen@google.com","subject":"Re: [PATCH v3 3/3] builtin/fetch: die on --negotiate-only and --recurse-submodules","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-12-23T19:08:08Z","receivedAt":"2021-12-23T19:08:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Glen Choo writes:\n> The previous commit ignores the value of --recurse-submodules if\n> --negotiate-only is given. Since non \"no\" values of --recurse-submodules\n> are not supported with --negotiate-only, make cmd_fetch() check for\n> this invalid combination and die.\n> \n> This is unlikely to affect internal usage of --negotiate-only, but it\n> may be helpful for users. We may also want to discourage users from\n> using --negotiate-only altogether because it was not intended for them.\n\nAll 3 patches look good, and I'm happy if they were merged as-is.\n\nHaving said that, I would delete the last sentence - I envision that\nother tools may want to use --negotiate-only to be able to better query\na Git server.\n\n> @@ -75,6 +75,7 @@ static struct transport *gtransport;\n> static struct transport *gsecondary;\n> static const char *submodule_prefix = \"\";\n> static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n> +static int recurse_submodules_explicit = RECURSE_SUBMODULES_DEFAULT;\n\nI would call this \"recurse_submodules_cli\" as a config variable could\nalso be considered explicit.\n\nBut again, you can consider all these suggestions as optional.\n"},{"id":"446078","messageId":"20220113004501.78822-1-chooglen@google.com","threadId":"57049","inReplyTo":"20211222001134.28933-1-chooglen@google.com","subject":"[PATCH v4 0/3] fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-13T00:44:58Z","receivedAt":"2022-01-13T00:45:33Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"`git fetch --negotiate-only` is used internally by push negotation and\nit behaves very differently from other uses of `git fetch`, e.g. it does\nnot update refs or fetch objects. But because of how cmd_fetch() is\nwritten, `git fetch --negotiate-only` performs tasks that it shouldn't.\nThis is results in behavior that is unnecessary at best, and incorrect\nat worst:\n\n* Submodules are updated if enabled by recurse.submodules=true, but\n negotiation fetch doesn't actually update the repo, so this doesn't\n make sense.\n* Commit graphs will be written if enabled by\n fetch.writeCommitGraph=true, but this is unnecessary because no\n objects are fetched [1]. \n* gc is run, but according to the commit message in [2], we only do this\n because we expect `git fetch` to introduce objects.\n\nMake `git fetch --negotiate-only` handle these tasks more rigorously by\ndoing the following:\n\n* Make cmd_fetch() skip irrelevant tasks if we know for certain that\nobjects will not be fetched\n* Disable submodule recursion and die() if a user explicitly asks for it\n\n[1] This is also confirmed by Documentation/config/fetch.txt, which\n states that Git should only write commit graphs if a pack-file is\n downloaded.\n[2] 131b8fcbfb (fetch: run gc --auto after fetching, 2013-01-26)\n\nChanges since v3:\n* change commit message subject: builtin/fetch -> fetch --negotiate-only\n* move the 'goto cleanup' to _after_ the submodule updating task because\n we may want to update submodules even if objects were not fetched (as\n pointed out by Junio, thanks!)\n* disable submodule recursion in the patch that checks for\n --negotiate-only + --recurse-submodules, so we never silently ignore\n --recurse-submodules.\n* incorporate some of Jonathan's suggestions (thanks!)\n\nChanges since v2:\n* added a prepatory patch that introduces a \"goto cleanup\"\n* drop an unnecessary line move (as suggested by Junio)\n* check for user-given --recurse-submodules and die() (as suggested by\n Jonathan and Junio)\n* update --negotiate-only's documentation\n\nChanges since v1:\n* added more context to commit message\n* added a NEEDSWORK comment\n\nGlen Choo (3):\n fetch: use goto cleanup in cmd_fetch()\n fetch: skip tasks related to fetching objects\n fetch --negotiate-only: do not update submodules\n\n Documentation/fetch-options.txt | 1 +\n builtin/fetch.c | 40 ++++++++++++++++++++++++++++++---\n t/t5516-fetch-push.sh | 12 ++++++++++\n t/t5702-protocol-v2.sh | 12 ++++++++++\n 4 files changed, 62 insertions(+), 3 deletions(-)\n\nRange-diff against v3:\n1: 97e2cba633 ! 1: ffa1a24109 builtin/fetch: use goto cleanup in cmd_fetch()\n @@ Metadata\n Author: Glen Choo \n \n ## Commit message ##\n - builtin/fetch: use goto cleanup in cmd_fetch()\n + fetch: use goto cleanup in cmd_fetch()\n \n Replace an early return with 'goto cleanup' in cmd_fetch() so that the\n - string_list is always cleared.\n -\n - The string_list_clear() call is purely cleanup; the string_list was not\n - reused.\n + string_list is always cleared (the string_list_clear() call is purely\n + cleanup; the string_list is not reused). This makes cleanup consistent\n + so that a subsequent commit can use 'goto cleanup' to bail out early.\n \n Signed-off-by: Glen Choo \n \n @@ builtin/fetch.c\n @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tgtransport->smart_options->acked_commits = &acked_commits;\n \t\t} else {\n - \t\t\twarning(_(\"Protocol does not support --negotiate-only, exiting.\"));\n + \t\t\twarning(_(\"protocol does not support --negotiate-only, exiting\"));\n -\t\t\treturn 1;\n +\t\t\tresult = 1;\n +\t\t\tgoto cleanup;\n2: 3a3a04b649 < -: ---------- builtin/fetch: skip unnecessary tasks when using --negotiate-only\n-: ---------- > 2: b0c73e8135 fetch: skip tasks related to fetching objects\n3: 97792b5214 ! 3: 914d30866f builtin/fetch: die on --negotiate-only and --recurse-submodules\n @@ Metadata\n Author: Glen Choo \n \n ## Commit message ##\n - builtin/fetch: die on --negotiate-only and --recurse-submodules\n + fetch --negotiate-only: do not update submodules\n \n - The previous commit ignores the value of --recurse-submodules if\n - --negotiate-only is given. Since non \"no\" values of --recurse-submodules\n - are not supported with --negotiate-only, make cmd_fetch() check for\n - this invalid combination and die.\n + `git fetch --negotiate-only` is an implementation detail of push\n + negotiation and, unlike most `git fetch` invocations, does not actually\n + update the main repository. Thus it should not update submodules even\n + if submodule recursion is enabled.\n \n - This is unlikely to affect internal usage of --negotiate-only, but it\n - may be helpful for users. We may also want to discourage users from\n - using --negotiate-only altogether because it was not intended for them.\n + This is not just slow, it is wrong e.g. push negotiation with\n + \"submodule.recurse=true\" will cause submodules to be updated because it\n + invokes `git fetch --negotiate-only`.\n +\n + Fix this by disabling submodule recursion if --negotiate-only was given.\n + Since this makes --negotiate-only and --recurse-submodules incompatible,\n + check for this invalid combination and die.\n +\n + This does not use the \"goto cleanup\" introduced in the previous commit\n + because we want to recurse through submodules whenever a ref is fetched,\n + and this can happen without introducing new objects.\n \n Signed-off-by: Glen Choo \n \n @@ Documentation/fetch-options.txt: configuration variables documented in linkgit:g\n \tancestors of the provided `--negotiation-tip=*` arguments,\n \twhich we have in common with the server.\n +\n -+This is incompatible with `--recurse-submodules[=yes|on-demand]`.\n ++This is incompatible with `--recurse-submodules=[yes|on-demand]`.\n Internally this is used to implement the `push.negotiate` option, see\n linkgit:git-config[1].\n \n @@ builtin/fetch.c: static struct transport *gtransport;\n static struct transport *gsecondary;\n static const char *submodule_prefix = \"\";\n static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n -+static int recurse_submodules_explicit = RECURSE_SUBMODULES_DEFAULT;\n ++static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;\n static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;\n static int shown_url = 0;\n static struct refspec refmap = REFSPEC_INIT_FETCH;\n @@ builtin/fetch.c: static struct option builtin_fetch_options[] = {\n \tOPT_BOOL('P', \"prune-tags\", &prune_tags,\n \t\t N_(\"prune local tags no longer on remote and clobber changed tags\")),\n -\tOPT_CALLBACK_F(0, \"recurse-submodules\", &recurse_submodules, N_(\"on-demand\"),\n -+\tOPT_CALLBACK_F(0, \"recurse-submodules\", &recurse_submodules_explicit, N_(\"on-demand\"),\n ++\tOPT_CALLBACK_F(0, \"recurse-submodules\", &recurse_submodules_cli, N_(\"on-demand\"),\n \t\t N_(\"control recursive fetching of submodules\"),\n \t\t PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),\n \tOPT_BOOL(0, \"dry-run\", &dry_run,\n @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n + \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n - \n -+\tif (recurse_submodules_explicit != RECURSE_SUBMODULES_DEFAULT)\n -+\t\trecurse_submodules = recurse_submodules_explicit;\n +\n - \tif (negotiate_only) {\n --\t\t/*\n --\t\t * --negotiate-only should never recurse into\n --\t\t * submodules, so there is no need to read .gitmodules.\n --\t\t */\n --\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n -+\t\tswitch (recurse_submodules_explicit) {\n ++\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n ++\t\trecurse_submodules = recurse_submodules_cli;\n ++\n ++\tif (negotiate_only) {\n ++\t\tswitch (recurse_submodules_cli) {\n +\t\tcase RECURSE_SUBMODULES_OFF:\n +\t\tcase RECURSE_SUBMODULES_DEFAULT: {\n +\t\t\t/*\n +\t\t\t * --negotiate-only should never recurse into\n -+\t\t\t * submodules, so there is no need to read .gitmodules.\n ++\t\t\t * submodules. Skip it by setting recurse_submodules to\n ++\t\t\t * RECURSE_SUBMODULES_OFF.\n +\t\t\t */\n +\t\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n +\t\t\tbreak;\n @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n +\t\tdefault:\n +\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n +\t\t}\n - \t}\n - \n ++\t}\n ++\n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n + \t\tint *sfjc = submodule_fetch_jobs_config == -1\n + \t\t\t ? &submodule_fetch_jobs_config : NULL;\n +\n + ## t/t5516-fetch-push.sh ##\n +@@ t/t5516-fetch-push.sh: test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n + \ttest_i18ngrep \"push negotiation failed\" err\n + '\n + \n ++test_expect_success 'push with negotiation does not attempt to fetch submodules' '\n ++\tmk_empty submodule_upstream &&\n ++\ttest_commit -C submodule_upstream submodule_commit &&\n ++\tgit submodule add ./submodule_upstream submodule &&\n ++\tmk_empty testrepo &&\n ++\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n ++\ttest_commit -C testrepo unrelated_commit &&\n ++\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n ++\tgit -c submodule.recurse=true -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n ++\t! grep \"Fetching submodule\" err\n ++'\n ++\n + test_expect_success 'push without wildcard' '\n + \tmk_empty testrepo &&\n + \n \n ## t/t5702-protocol-v2.sh ##\n @@ t/t5702-protocol-v2.sh: test_expect_success 'usage: --negotiate-only without --negotiation-tip' '\n @@ t/t5702-protocol-v2.sh: test_expect_success 'usage: --negotiate-only without --n\n '\n \n +test_expect_success 'usage: --negotiate-only with --recurse-submodules' '\n -+\tSERVER=\"server\" &&\n -+\tURI=\"file://$(pwd)/server\" &&\n -+\n -+\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n -+\n +\tcat >err.expect <<-\\EOF &&\n +\tfatal: --negotiate-only and --recurse-submodules cannot be used together\n +\tEOF\n\nbase-commit: 90d242d36e248acfae0033274b524bfa55a947fd\n-- \n2.33.GIT\n\n"},{"id":"446079","messageId":"20220113004501.78822-2-chooglen@google.com","threadId":"57049","inReplyTo":"20220113004501.78822-1-chooglen@google.com","subject":"[PATCH v4 1/3] fetch: use goto cleanup in cmd_fetch()","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-13T00:44:59Z","receivedAt":"2022-01-13T00:45:37Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Replace an early return with 'goto cleanup' in cmd_fetch() so that the\nstring_list is always cleared (the string_list_clear() call is purely\ncleanup; the string_list is not reused). This makes cleanup consistent\nso that a subsequent commit can use 'goto cleanup' to bail out early.\n\nSigned-off-by: Glen Choo \n---\n builtin/fetch.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex eaab8056bf..0a625cb137 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2095,7 +2095,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tgtransport->smart_options->acked_commits = &acked_commits;\n \t\t} else {\n \t\t\twarning(_(\"protocol does not support --negotiate-only, exiting\"));\n-\t\t\treturn 1;\n+\t\t\tresult = 1;\n+\t\t\tgoto cleanup;\n \t\t}\n \t\tif (server_options.nr)\n \t\t\tgtransport->server_options = &server_options;\n@@ -2151,8 +2152,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tstrvec_clear(&options);\n \t}\n \n-\tstring_list_clear(&list, 0);\n-\n \tprepare_repo_settings(the_repository);\n \tif (fetch_write_commit_graph > 0 ||\n \t (fetch_write_commit_graph < 0 &&\n@@ -2170,5 +2169,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (enable_auto_gc)\n \t\trun_auto_maintenance(verbosity < 0);\n \n+ cleanup:\n+\tstring_list_clear(&list, 0);\n \treturn result;\n }\n-- \n2.33.GIT\n\n"},{"id":"446080","messageId":"20220113004501.78822-3-chooglen@google.com","threadId":"57049","inReplyTo":"20220113004501.78822-1-chooglen@google.com","subject":"[PATCH v4 2/3] fetch: skip tasks related to fetching objects","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-13T00:45:00Z","receivedAt":"2022-01-13T00:45:45Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"cmd_fetch() does the following with the assumption that objects are\nfetched:\n\n* Run gc\n* Write commit graphs (if enabled by fetch.writeCommitGraph=true)\n\nHowever, neither of these tasks makes sense if objects are not fetched\ne.g. `git fetch --negotiate-only` never fetches objects.\n\nSpeed up cmd_fetch() by bailing out early if we know for certain that\nobjects will not be fetched. cmd_fetch() can bail out early whenever\nobjects are not fetched, but for now this only considers\n--negotiate-only.\n\nThe same optimization does not apply to `git fetch --dry-run` because\nthat actually fetches objects; the dry run refers to not updating refs.\n\nSigned-off-by: Glen Choo \n---\n builtin/fetch.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0a625cb137..7bbff5a029 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2152,6 +2152,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tstrvec_clear(&options);\n \t}\n \n+\t/*\n+\t * Skip irrelevant tasks because we know objects were not\n+\t * fetched.\n+\t *\n+\t * NEEDSWORK: as a future optimization, we can return early\n+\t * whenever objects were not fetched e.g. if we already have all\n+\t * of them.\n+\t */\n+\tif (negotiate_only)\n+\t\tgoto cleanup;\n+\n \tprepare_repo_settings(the_repository);\n \tif (fetch_write_commit_graph > 0 ||\n \t (fetch_write_commit_graph < 0 &&\n-- \n2.33.GIT\n\n"},{"id":"446081","messageId":"20220113004501.78822-4-chooglen@google.com","threadId":"57049","inReplyTo":"20220113004501.78822-1-chooglen@google.com","subject":"[PATCH v4 3/3] fetch --negotiate-only: do not update submodules","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-13T00:45:01Z","receivedAt":"2022-01-13T00:45:46Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"`git fetch --negotiate-only` is an implementation detail of push\nnegotiation and, unlike most `git fetch` invocations, does not actually\nupdate the main repository. Thus it should not update submodules even\nif submodule recursion is enabled.\n\nThis is not just slow, it is wrong e.g. push negotiation with\n\"submodule.recurse=true\" will cause submodules to be updated because it\ninvokes `git fetch --negotiate-only`.\n\nFix this by disabling submodule recursion if --negotiate-only was given.\nSince this makes --negotiate-only and --recurse-submodules incompatible,\ncheck for this invalid combination and die.\n\nThis does not use the \"goto cleanup\" introduced in the previous commit\nbecause we want to recurse through submodules whenever a ref is fetched,\nand this can happen without introducing new objects.\n\nSigned-off-by: Glen Choo \n---\n Documentation/fetch-options.txt | 1 +\n builtin/fetch.c | 24 +++++++++++++++++++++++-\n t/t5516-fetch-push.sh | 12 ++++++++++++\n t/t5702-protocol-v2.sh | 12 ++++++++++++\n 4 files changed, 48 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex e967ff1874..f903683189 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -71,6 +71,7 @@ configuration variables documented in linkgit:git-config[1], and the\n \tancestors of the provided `--negotiation-tip=*` arguments,\n \twhich we have in common with the server.\n +\n+This is incompatible with `--recurse-submodules=[yes|on-demand]`.\n Internally this is used to implement the `push.negotiate` option, see\n linkgit:git-config[1].\n \ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 7bbff5a029..8b8bde8809 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -76,6 +76,7 @@ static struct transport *gtransport;\n static struct transport *gsecondary;\n static const char *submodule_prefix = \"\";\n static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;\n static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;\n static int shown_url = 0;\n static struct refspec refmap = REFSPEC_INIT_FETCH;\n@@ -167,7 +168,7 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"prune remote-tracking branches no longer on remote\")),\n \tOPT_BOOL('P', \"prune-tags\", &prune_tags,\n \t\t N_(\"prune local tags no longer on remote and clobber changed tags\")),\n-\tOPT_CALLBACK_F(0, \"recurse-submodules\", &recurse_submodules, N_(\"on-demand\"),\n+\tOPT_CALLBACK_F(0, \"recurse-submodules\", &recurse_submodules_cli, N_(\"on-demand\"),\n \t\t N_(\"control recursive fetching of submodules\"),\n \t\t PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),\n \tOPT_BOOL(0, \"dry-run\", &dry_run,\n@@ -2014,6 +2015,27 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n+\n+\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n+\t\trecurse_submodules = recurse_submodules_cli;\n+\n+\tif (negotiate_only) {\n+\t\tswitch (recurse_submodules_cli) {\n+\t\tcase RECURSE_SUBMODULES_OFF:\n+\t\tcase RECURSE_SUBMODULES_DEFAULT: {\n+\t\t\t/*\n+\t\t\t * --negotiate-only should never recurse into\n+\t\t\t * submodules. Skip it by setting recurse_submodules to\n+\t\t\t * RECURSE_SUBMODULES_OFF.\n+\t\t\t */\n+\t\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n+\t\t\tbreak;\n+\t\t}\n+\t\tdefault:\n+\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n+\t\t}\n+\t}\n+\n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n \t\tint *sfjc = submodule_fetch_jobs_config == -1\n \t\t\t ? &submodule_fetch_jobs_config : NULL;\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 2f04cf9a1c..87881726ed 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -229,6 +229,18 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n \ttest_i18ngrep \"push negotiation failed\" err\n '\n \n+test_expect_success 'push with negotiation does not attempt to fetch submodules' '\n+\tmk_empty submodule_upstream &&\n+\ttest_commit -C submodule_upstream submodule_commit &&\n+\tgit submodule add ./submodule_upstream submodule &&\n+\tmk_empty testrepo &&\n+\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\ttest_commit -C testrepo unrelated_commit &&\n+\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n+\tgit -c submodule.recurse=true -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n+\t! grep \"Fetching submodule\" err\n+'\n+\n test_expect_success 'push without wildcard' '\n \tmk_empty testrepo &&\n \ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 710f33e2aa..1b9023d3f0 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -628,6 +628,18 @@ test_expect_success 'usage: --negotiate-only without --negotiation-tip' '\n \ttest_cmp err.expect err.actual\n '\n \n+test_expect_success 'usage: --negotiate-only with --recurse-submodules' '\n+\tcat >err.expect <<-\\EOF &&\n+\tfatal: --negotiate-only and --recurse-submodules cannot be used together\n+\tEOF\n+\n+\ttest_must_fail git -c protocol.version=2 -C client fetch \\\n+\t\t--negotiate-only \\\n+\t\t--recurse-submodules \\\n+\t\torigin 2>err.actual &&\n+\ttest_cmp err.expect err.actual\n+'\n+\n test_expect_success 'file:// --negotiate-only' '\n \tSERVER=\"server\" &&\n \tURI=\"file://$(pwd)/server\" &&\n-- \n2.33.GIT\n\n"},{"id":"446082","messageId":"xmqqr19cjuzw.fsf@gitster.g","threadId":"57049","inReplyTo":"20220113004501.78822-4-chooglen@google.com","subject":"Re: [PATCH v4 3/3] fetch --negotiate-only: do not update submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-13T01:16:03Z","receivedAt":"2022-01-13T01:16:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 7bbff5a029..8b8bde8809 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -76,6 +76,7 @@ static struct transport *gtransport;\n> static struct transport *gsecondary;\n> static const char *submodule_prefix = \"\";\n> static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n> +static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;\n> static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;\n> static int shown_url = 0;\n> static struct refspec refmap = REFSPEC_INIT_FETCH;\n> @@ -167,7 +168,7 @@ static struct option builtin_fetch_options[] = {\n> \t\t N_(\"prune remote-tracking branches no longer on remote\")),\n> \tOPT_BOOL('P', \"prune-tags\", &prune_tags,\n> \t\t N_(\"prune local tags no longer on remote and clobber changed tags\")),\n> -\tOPT_CALLBACK_F(0, \"recurse-submodules\", &recurse_submodules, N_(\"on-demand\"),\n> +\tOPT_CALLBACK_F(0, \"recurse-submodules\", &recurse_submodules_cli, N_(\"on-demand\"),\n> \t\t N_(\"control recursive fetching of submodules\"),\n> \t\t PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),\n> \tOPT_BOOL(0, \"dry-run\", &dry_run,\n> @@ -2014,6 +2015,27 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \n> \targc = parse_options(argc, argv, prefix,\n> \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n> +\n> +\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n> +\t\trecurse_submodules = recurse_submodules_cli;\n\nThis made me wonder what should happen if the command line option\nwas given and explicitly told us to use the default, but after\nfollowing the option_fetch_parse_recurse_submodules() codeflow, I\nrealized that it will never return RECURSE_SUBMODULES_DEFAULT, so it\nis OK. It is a bit misleading that _DEFAULT does not mean \"use the\ndefault settings\" here---it merely means \"this variable was left\nuntouched\". But I suppose that it is in line with all the other\nuses of RECURSE_SUBMODULES_DEFAULT, in which case it is OK for now.\n\n> +\tif (negotiate_only) {\n> +\t\tswitch (recurse_submodules_cli) {\n> +\t\tcase RECURSE_SUBMODULES_OFF:\n> +\t\tcase RECURSE_SUBMODULES_DEFAULT: {\n> +\t\t\t/*\n> +\t\t\t * --negotiate-only should never recurse into\n> +\t\t\t * submodules. Skip it by setting recurse_submodules to\n> +\t\t\t * RECURSE_SUBMODULES_OFF.\n> +\t\t\t */\n> +\t\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n> +\t\t\tbreak;\n> +\t\t}\n\nIt is not immediately obvious to me why we need an extra block\nhere. If there is no reason, let's not have it---there is no reason\nto puzzle readers into wondering if anything funny is going on if\nthere is nothing unusual.\n\n> +\t\tdefault:\n> +\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n> +\t\t}\n> +\t}\n> +\n\nOther than that, looking good.\n\nThanks.\n"},{"id":"446440","messageId":"20220118185453.33710-1-chooglen@google.com","threadId":"57049","inReplyTo":"20220113004501.78822-1-chooglen@google.com","subject":"[PATCH v5 0/3] fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-18T18:54:50Z","receivedAt":"2022-01-18T18:55:01Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"`git fetch --negotiate-only` is used internally by push negotation and\nit behaves very differently from other uses of `git fetch`, e.g. it does\nnot update refs or fetch objects. But because of how cmd_fetch() is\nwritten, `git fetch --negotiate-only` performs tasks that it shouldn't.\nThis is results in behavior that is unnecessary at best, and incorrect\nat worst:\n\n* Submodules are updated if enabled by recurse.submodules=true, but\n negotiation fetch doesn't actually update the repo, so this doesn't\n make sense.\n* Commit graphs will be written if enabled by\n fetch.writeCommitGraph=true, but this is unnecessary because no\n objects are fetched [1]. \n* gc is run, but according to the commit message in [2], we only do this\n because we expect `git fetch` to introduce objects.\n\nMake `git fetch --negotiate-only` handle these tasks more rigorously by\ndoing the following:\n\n* Make cmd_fetch() skip irrelevant tasks if we know for certain that\nobjects will not be fetched\n* Disable submodule recursion and die() if a user explicitly asks for it\n\n[1] This is also confirmed by Documentation/config/fetch.txt, which\n states that Git should only write commit graphs if a pack-file is\n downloaded.\n[2] 131b8fcbfb (fetch: run gc --auto after fetching, 2013-01-26)\n\nChanges since v4:\n* drop an unnecessary block (thanks Junio!)\n\nChanges since v3:\n* change commit message subject: builtin/fetch -> fetch --negotiate-only\n* move the 'goto cleanup' to _after_ the submodule updating task because\n we may want to update submodules even if objects were not fetched (as\n pointed out by Junio, thanks!)\n* disable submodule recursion in the patch that checks for\n --negotiate-only + --recurse-submodules, so we never silently ignore\n --recurse-submodules.\n* incorporate some of Jonathan's suggestions (thanks!)\n\nChanges since v2:\n* added a prepatory patch that introduces a \"goto cleanup\"\n* drop an unnecessary line move (as suggested by Junio)\n* check for user-given --recurse-submodules and die() (as suggested by\n Jonathan and Junio)\n* update --negotiate-only's documentation\n\nChanges since v1:\n* added more context to commit message\n* added a NEEDSWORK comment\n\nGlen Choo (3):\n fetch: use goto cleanup in cmd_fetch()\n fetch: skip tasks related to fetching objects\n fetch --negotiate-only: do not update submodules\n\n Documentation/fetch-options.txt | 1 +\n builtin/fetch.c | 40 ++++++++++++++++++++++++++++++---\n t/t5516-fetch-push.sh | 12 ++++++++++\n t/t5702-protocol-v2.sh | 12 ++++++++++\n 4 files changed, 62 insertions(+), 3 deletions(-)\n\nRange-diff against v4:\n1: ffa1a24109 = 1: ffa1a24109 fetch: use goto cleanup in cmd_fetch()\n2: b0c73e8135 = 2: b0c73e8135 fetch: skip tasks related to fetching objects\n3: 914d30866f ! 3: f929297961 fetch --negotiate-only: do not update submodules\n @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n +\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n +\t\trecurse_submodules = recurse_submodules_cli;\n +\n -+\tif (negotiate_only) {\n ++\tif (negotiate_only)\n +\t\tswitch (recurse_submodules_cli) {\n +\t\tcase RECURSE_SUBMODULES_OFF:\n +\t\tcase RECURSE_SUBMODULES_DEFAULT: {\n @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n +\t\tdefault:\n +\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n +\t\t}\n -+\t}\n ++\n +\n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n \t\tint *sfjc = submodule_fetch_jobs_config == -1\n\nbase-commit: 90d242d36e248acfae0033274b524bfa55a947fd\n-- \n2.33.GIT\n\n"},{"id":"446441","messageId":"20220118185453.33710-2-chooglen@google.com","threadId":"57049","inReplyTo":"20220118185453.33710-1-chooglen@google.com","subject":"[PATCH v5 1/3] fetch: use goto cleanup in cmd_fetch()","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-18T18:54:51Z","receivedAt":"2022-01-18T18:55:06Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Replace an early return with 'goto cleanup' in cmd_fetch() so that the\nstring_list is always cleared (the string_list_clear() call is purely\ncleanup; the string_list is not reused). This makes cleanup consistent\nso that a subsequent commit can use 'goto cleanup' to bail out early.\n\nSigned-off-by: Glen Choo \n---\n builtin/fetch.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex eaab8056bf..0a625cb137 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2095,7 +2095,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tgtransport->smart_options->acked_commits = &acked_commits;\n \t\t} else {\n \t\t\twarning(_(\"protocol does not support --negotiate-only, exiting\"));\n-\t\t\treturn 1;\n+\t\t\tresult = 1;\n+\t\t\tgoto cleanup;\n \t\t}\n \t\tif (server_options.nr)\n \t\t\tgtransport->server_options = &server_options;\n@@ -2151,8 +2152,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tstrvec_clear(&options);\n \t}\n \n-\tstring_list_clear(&list, 0);\n-\n \tprepare_repo_settings(the_repository);\n \tif (fetch_write_commit_graph > 0 ||\n \t (fetch_write_commit_graph < 0 &&\n@@ -2170,5 +2169,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (enable_auto_gc)\n \t\trun_auto_maintenance(verbosity < 0);\n \n+ cleanup:\n+\tstring_list_clear(&list, 0);\n \treturn result;\n }\n-- \n2.33.GIT\n\n"},{"id":"446442","messageId":"20220118185453.33710-3-chooglen@google.com","threadId":"57049","inReplyTo":"20220118185453.33710-1-chooglen@google.com","subject":"[PATCH v5 2/3] fetch: skip tasks related to fetching objects","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-18T18:54:52Z","receivedAt":"2022-01-18T18:55:08Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"cmd_fetch() does the following with the assumption that objects are\nfetched:\n\n* Run gc\n* Write commit graphs (if enabled by fetch.writeCommitGraph=true)\n\nHowever, neither of these tasks makes sense if objects are not fetched\ne.g. `git fetch --negotiate-only` never fetches objects.\n\nSpeed up cmd_fetch() by bailing out early if we know for certain that\nobjects will not be fetched. cmd_fetch() can bail out early whenever\nobjects are not fetched, but for now this only considers\n--negotiate-only.\n\nThe same optimization does not apply to `git fetch --dry-run` because\nthat actually fetches objects; the dry run refers to not updating refs.\n\nSigned-off-by: Glen Choo \n---\n builtin/fetch.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0a625cb137..7bbff5a029 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2152,6 +2152,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tstrvec_clear(&options);\n \t}\n \n+\t/*\n+\t * Skip irrelevant tasks because we know objects were not\n+\t * fetched.\n+\t *\n+\t * NEEDSWORK: as a future optimization, we can return early\n+\t * whenever objects were not fetched e.g. if we already have all\n+\t * of them.\n+\t */\n+\tif (negotiate_only)\n+\t\tgoto cleanup;\n+\n \tprepare_repo_settings(the_repository);\n \tif (fetch_write_commit_graph > 0 ||\n \t (fetch_write_commit_graph < 0 &&\n-- \n2.33.GIT\n\n"},{"id":"446443","messageId":"20220118185453.33710-4-chooglen@google.com","threadId":"57049","inReplyTo":"20220118185453.33710-1-chooglen@google.com","subject":"[PATCH v5 3/3] fetch --negotiate-only: do not update submodules","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-18T18:54:53Z","receivedAt":"2022-01-18T18:55:09Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"`git fetch --negotiate-only` is an implementation detail of push\nnegotiation and, unlike most `git fetch` invocations, does not actually\nupdate the main repository. Thus it should not update submodules even\nif submodule recursion is enabled.\n\nThis is not just slow, it is wrong e.g. push negotiation with\n\"submodule.recurse=true\" will cause submodules to be updated because it\ninvokes `git fetch --negotiate-only`.\n\nFix this by disabling submodule recursion if --negotiate-only was given.\nSince this makes --negotiate-only and --recurse-submodules incompatible,\ncheck for this invalid combination and die.\n\nThis does not use the \"goto cleanup\" introduced in the previous commit\nbecause we want to recurse through submodules whenever a ref is fetched,\nand this can happen without introducing new objects.\n\nSigned-off-by: Glen Choo \n---\n Documentation/fetch-options.txt | 1 +\n builtin/fetch.c | 24 +++++++++++++++++++++++-\n t/t5516-fetch-push.sh | 12 ++++++++++++\n t/t5702-protocol-v2.sh | 12 ++++++++++++\n 4 files changed, 48 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex e967ff1874..f903683189 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -71,6 +71,7 @@ configuration variables documented in linkgit:git-config[1], and the\n \tancestors of the provided `--negotiation-tip=*` arguments,\n \twhich we have in common with the server.\n +\n+This is incompatible with `--recurse-submodules=[yes|on-demand]`.\n Internally this is used to implement the `push.negotiate` option, see\n linkgit:git-config[1].\n \ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 7bbff5a029..63f74776a2 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -76,6 +76,7 @@ static struct transport *gtransport;\n static struct transport *gsecondary;\n static const char *submodule_prefix = \"\";\n static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;\n static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;\n static int shown_url = 0;\n static struct refspec refmap = REFSPEC_INIT_FETCH;\n@@ -167,7 +168,7 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"prune remote-tracking branches no longer on remote\")),\n \tOPT_BOOL('P', \"prune-tags\", &prune_tags,\n \t\t N_(\"prune local tags no longer on remote and clobber changed tags\")),\n-\tOPT_CALLBACK_F(0, \"recurse-submodules\", &recurse_submodules, N_(\"on-demand\"),\n+\tOPT_CALLBACK_F(0, \"recurse-submodules\", &recurse_submodules_cli, N_(\"on-demand\"),\n \t\t N_(\"control recursive fetching of submodules\"),\n \t\t PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),\n \tOPT_BOOL(0, \"dry-run\", &dry_run,\n@@ -2014,6 +2015,27 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n+\n+\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n+\t\trecurse_submodules = recurse_submodules_cli;\n+\n+\tif (negotiate_only)\n+\t\tswitch (recurse_submodules_cli) {\n+\t\tcase RECURSE_SUBMODULES_OFF:\n+\t\tcase RECURSE_SUBMODULES_DEFAULT: {\n+\t\t\t/*\n+\t\t\t * --negotiate-only should never recurse into\n+\t\t\t * submodules. Skip it by setting recurse_submodules to\n+\t\t\t * RECURSE_SUBMODULES_OFF.\n+\t\t\t */\n+\t\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n+\t\t\tbreak;\n+\t\t}\n+\t\tdefault:\n+\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n+\t\t}\n+\n+\n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n \t\tint *sfjc = submodule_fetch_jobs_config == -1\n \t\t\t ? &submodule_fetch_jobs_config : NULL;\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 2f04cf9a1c..87881726ed 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -229,6 +229,18 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n \ttest_i18ngrep \"push negotiation failed\" err\n '\n \n+test_expect_success 'push with negotiation does not attempt to fetch submodules' '\n+\tmk_empty submodule_upstream &&\n+\ttest_commit -C submodule_upstream submodule_commit &&\n+\tgit submodule add ./submodule_upstream submodule &&\n+\tmk_empty testrepo &&\n+\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\ttest_commit -C testrepo unrelated_commit &&\n+\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n+\tgit -c submodule.recurse=true -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n+\t! grep \"Fetching submodule\" err\n+'\n+\n test_expect_success 'push without wildcard' '\n \tmk_empty testrepo &&\n \ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 710f33e2aa..1b9023d3f0 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -628,6 +628,18 @@ test_expect_success 'usage: --negotiate-only without --negotiation-tip' '\n \ttest_cmp err.expect err.actual\n '\n \n+test_expect_success 'usage: --negotiate-only with --recurse-submodules' '\n+\tcat >err.expect <<-\\EOF &&\n+\tfatal: --negotiate-only and --recurse-submodules cannot be used together\n+\tEOF\n+\n+\ttest_must_fail git -c protocol.version=2 -C client fetch \\\n+\t\t--negotiate-only \\\n+\t\t--recurse-submodules \\\n+\t\torigin 2>err.actual &&\n+\ttest_cmp err.expect err.actual\n+'\n+\n test_expect_success 'file:// --negotiate-only' '\n \tSERVER=\"server\" &&\n \tURI=\"file://$(pwd)/server\" &&\n-- \n2.33.GIT\n\n"},{"id":"446457","messageId":"xmqqtue0r978.fsf@gitster.g","threadId":"57049","inReplyTo":"20220118185453.33710-4-chooglen@google.com","subject":"Re: [PATCH v5 3/3] fetch --negotiate-only: do not update submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-18T22:05:31Z","receivedAt":"2022-01-18T22:05:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> +\tif (negotiate_only)\n> +\t\tswitch (recurse_submodules_cli) {\n> +\t\tcase RECURSE_SUBMODULES_OFF:\n> +\t\tcase RECURSE_SUBMODULES_DEFAULT: {\n> +\t\t\t/*\n> +\t\t\t * --negotiate-only should never recurse into\n> +\t\t\t * submodules. Skip it by setting recurse_submodules to\n> +\t\t\t * RECURSE_SUBMODULES_OFF.\n> +\t\t\t */\n> +\t\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tdefault:\n> +\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n> +\t\t}\n\nI think that this part has the only difference since the previous\nround, but I am puzzled about it. Everything else looks as good as\nthe previous round was.\n\nI did not (and I do not) mind the block for the body of this if\nstatement. Even though technically a single switch() statement\nmakes a single statement block that does not need {} around, it is\nlarge enough that extra {} around (which you had in the previous\nround) may make it clear where the body begins and ends.\n\nBut do we really need the extra block _inside_ the switch statement?\nIOW I would have expected to see this instead:\n\n\t\tswitch (recurse_submodules_cli) {\n\t\tcase RECURSE_SUBMODULES_OFF:\n\t\tcase RECURSE_SUBMODULES_DEFAULT:\n\t\t\t/*\n\t\t\t * --negotiate-only should never recurse into\n\t\t\t * submodules. Skip it by setting recurse_submodules to\n\t\t\t * RECURSE_SUBMODULES_OFF.\n\t\t\t */\n\t\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n\t\t\tbreak;\n\t\tdefault:\n\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n\t\t}\n\nThanks.\n"},{"id":"446463","messageId":"kl6lpmooy5lq.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57049","inReplyTo":"xmqqtue0r978.fsf@gitster.g","subject":"Re: [PATCH v5 3/3] fetch --negotiate-only: do not update submodules","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-18T23:41:21Z","receivedAt":"2022-01-18T23:41:25Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> Glen Choo writes:\n>\n>> +\tif (negotiate_only)\n>> +\t\tswitch (recurse_submodules_cli) {\n>> +\t\tcase RECURSE_SUBMODULES_OFF:\n>> +\t\tcase RECURSE_SUBMODULES_DEFAULT: {\n>> +\t\t\t/*\n>> +\t\t\t * --negotiate-only should never recurse into\n>> +\t\t\t * submodules. Skip it by setting recurse_submodules to\n>> +\t\t\t * RECURSE_SUBMODULES_OFF.\n>> +\t\t\t */\n>> +\t\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t\tdefault:\n>> +\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n>> +\t\t}\n>\n> I think that this part has the only difference since the previous\n> round, but I am puzzled about it. Everything else looks as good as\n> the previous round was.\n>\n> I did not (and I do not) mind the block for the body of this if\n> statement. Even though technically a single switch() statement\n> makes a single statement block that does not need {} around, it is\n> large enough that extra {} around (which you had in the previous\n> round) may make it clear where the body begins and ends.\n\nYes, that makes sense. This was why I added it initially.\n\n> But do we really need the extra block _inside_ the switch statement?\n> IOW I would have expected to see this instead:\n>\n> \t\tswitch (recurse_submodules_cli) {\n> \t\tcase RECURSE_SUBMODULES_OFF:\n> \t\tcase RECURSE_SUBMODULES_DEFAULT:\n> \t\t\t/*\n> \t\t\t * --negotiate-only should never recurse into\n> \t\t\t * submodules. Skip it by setting recurse_submodules to\n> \t\t\t * RECURSE_SUBMODULES_OFF.\n> \t\t\t */\n> \t\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n> \t\t\tbreak;\n> \t\tdefault:\n> \t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n> \t\t}\n>\n> Thanks.\n\nAh, I misunderstood. I'll blame the fact that I'm recovering from\nCOVID ;) I'll remove the block from the \"case\".\n"},{"id":"446465","messageId":"20220119000056.58503-1-chooglen@google.com","threadId":"57049","inReplyTo":"20220118185453.33710-1-chooglen@google.com","subject":"[PATCH v6 0/3] fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-19T00:00:53Z","receivedAt":"2022-01-19T00:01:06Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Changes since v5:\n* revert v4\n* drop the unnecessary block that Junio actually meant\n\nChanges since v4:\n* drop an unnecessary block (thanks Junio!)\n\nChanges since v3:\n* change commit message subject: builtin/fetch -> fetch --negotiate-only\n* move the 'goto cleanup' to _after_ the submodule updating task because\n we may want to update submodules even if objects were not fetched (as\n pointed out by Junio, thanks!)\n* disable submodule recursion in the patch that checks for\n --negotiate-only + --recurse-submodules, so we never silently ignore\n --recurse-submodules.\n* incorporate some of Jonathan's suggestions (thanks!)\n\nChanges since v2:\n* added a prepatory patch that introduces a \"goto cleanup\"\n* drop an unnecessary line move (as suggested by Junio)\n* check for user-given --recurse-submodules and die() (as suggested by\n Jonathan and Junio)\n* update --negotiate-only's documentation\n\nChanges since v1:\n* added more context to commit message\n* added a NEEDSWORK comment\n\nGlen Choo (3):\n fetch: use goto cleanup in cmd_fetch()\n fetch: skip tasks related to fetching objects\n fetch --negotiate-only: do not update submodules\n\n Documentation/fetch-options.txt | 1 +\n builtin/fetch.c | 40 ++++++++++++++++++++++++++++++---\n t/t5516-fetch-push.sh | 12 ++++++++++\n t/t5702-protocol-v2.sh | 12 ++++++++++\n 4 files changed, 62 insertions(+), 3 deletions(-)\n\nRange-diff against v5:\n1: ffa1a24109 = 1: ffa1a24109 fetch: use goto cleanup in cmd_fetch()\n2: b0c73e8135 = 2: b0c73e8135 fetch: skip tasks related to fetching objects\n3: f929297961 ! 3: 239101e752 fetch --negotiate-only: do not update submodules\n @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n +\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n +\t\trecurse_submodules = recurse_submodules_cli;\n +\n -+\tif (negotiate_only)\n ++\tif (negotiate_only) {\n +\t\tswitch (recurse_submodules_cli) {\n +\t\tcase RECURSE_SUBMODULES_OFF:\n -+\t\tcase RECURSE_SUBMODULES_DEFAULT: {\n ++\t\tcase RECURSE_SUBMODULES_DEFAULT:\n +\t\t\t/*\n +\t\t\t * --negotiate-only should never recurse into\n +\t\t\t * submodules. Skip it by setting recurse_submodules to\n @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n +\t\t\t */\n +\t\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n +\t\t\tbreak;\n -+\t\t}\n ++\n +\t\tdefault:\n +\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n +\t\t}\n -+\n ++\t}\n +\n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n \t\tint *sfjc = submodule_fetch_jobs_config == -1\n\nbase-commit: 90d242d36e248acfae0033274b524bfa55a947fd\n-- \n2.33.GIT\n\n"},{"id":"446466","messageId":"20220119000056.58503-2-chooglen@google.com","threadId":"57049","inReplyTo":"20220119000056.58503-1-chooglen@google.com","subject":"[PATCH v6 1/3] fetch: use goto cleanup in cmd_fetch()","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-19T00:00:54Z","receivedAt":"2022-01-19T00:01:08Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Replace an early return with 'goto cleanup' in cmd_fetch() so that the\nstring_list is always cleared (the string_list_clear() call is purely\ncleanup; the string_list is not reused). This makes cleanup consistent\nso that a subsequent commit can use 'goto cleanup' to bail out early.\n\nSigned-off-by: Glen Choo \n---\n builtin/fetch.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex eaab8056bf..0a625cb137 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2095,7 +2095,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tgtransport->smart_options->acked_commits = &acked_commits;\n \t\t} else {\n \t\t\twarning(_(\"protocol does not support --negotiate-only, exiting\"));\n-\t\t\treturn 1;\n+\t\t\tresult = 1;\n+\t\t\tgoto cleanup;\n \t\t}\n \t\tif (server_options.nr)\n \t\t\tgtransport->server_options = &server_options;\n@@ -2151,8 +2152,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tstrvec_clear(&options);\n \t}\n \n-\tstring_list_clear(&list, 0);\n-\n \tprepare_repo_settings(the_repository);\n \tif (fetch_write_commit_graph > 0 ||\n \t (fetch_write_commit_graph < 0 &&\n@@ -2170,5 +2169,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (enable_auto_gc)\n \t\trun_auto_maintenance(verbosity < 0);\n \n+ cleanup:\n+\tstring_list_clear(&list, 0);\n \treturn result;\n }\n-- \n2.33.GIT\n\n"},{"id":"446467","messageId":"20220119000056.58503-3-chooglen@google.com","threadId":"57049","inReplyTo":"20220119000056.58503-1-chooglen@google.com","subject":"[PATCH v6 2/3] fetch: skip tasks related to fetching objects","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-19T00:00:55Z","receivedAt":"2022-01-19T00:01:11Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"cmd_fetch() does the following with the assumption that objects are\nfetched:\n\n* Run gc\n* Write commit graphs (if enabled by fetch.writeCommitGraph=true)\n\nHowever, neither of these tasks makes sense if objects are not fetched\ne.g. `git fetch --negotiate-only` never fetches objects.\n\nSpeed up cmd_fetch() by bailing out early if we know for certain that\nobjects will not be fetched. cmd_fetch() can bail out early whenever\nobjects are not fetched, but for now this only considers\n--negotiate-only.\n\nThe same optimization does not apply to `git fetch --dry-run` because\nthat actually fetches objects; the dry run refers to not updating refs.\n\nSigned-off-by: Glen Choo \n---\n builtin/fetch.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0a625cb137..7bbff5a029 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2152,6 +2152,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tstrvec_clear(&options);\n \t}\n \n+\t/*\n+\t * Skip irrelevant tasks because we know objects were not\n+\t * fetched.\n+\t *\n+\t * NEEDSWORK: as a future optimization, we can return early\n+\t * whenever objects were not fetched e.g. if we already have all\n+\t * of them.\n+\t */\n+\tif (negotiate_only)\n+\t\tgoto cleanup;\n+\n \tprepare_repo_settings(the_repository);\n \tif (fetch_write_commit_graph > 0 ||\n \t (fetch_write_commit_graph < 0 &&\n-- \n2.33.GIT\n\n"},{"id":"446468","messageId":"20220119000056.58503-4-chooglen@google.com","threadId":"57049","inReplyTo":"20220119000056.58503-1-chooglen@google.com","subject":"[PATCH v6 3/3] fetch --negotiate-only: do not update submodules","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-19T00:00:56Z","receivedAt":"2022-01-19T00:01:12Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"`git fetch --negotiate-only` is an implementation detail of push\nnegotiation and, unlike most `git fetch` invocations, does not actually\nupdate the main repository. Thus it should not update submodules even\nif submodule recursion is enabled.\n\nThis is not just slow, it is wrong e.g. push negotiation with\n\"submodule.recurse=true\" will cause submodules to be updated because it\ninvokes `git fetch --negotiate-only`.\n\nFix this by disabling submodule recursion if --negotiate-only was given.\nSince this makes --negotiate-only and --recurse-submodules incompatible,\ncheck for this invalid combination and die.\n\nThis does not use the \"goto cleanup\" introduced in the previous commit\nbecause we want to recurse through submodules whenever a ref is fetched,\nand this can happen without introducing new objects.\n\nSigned-off-by: Glen Choo \n---\n Documentation/fetch-options.txt | 1 +\n builtin/fetch.c | 24 +++++++++++++++++++++++-\n t/t5516-fetch-push.sh | 12 ++++++++++++\n t/t5702-protocol-v2.sh | 12 ++++++++++++\n 4 files changed, 48 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex e967ff1874..f903683189 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -71,6 +71,7 @@ configuration variables documented in linkgit:git-config[1], and the\n \tancestors of the provided `--negotiation-tip=*` arguments,\n \twhich we have in common with the server.\n +\n+This is incompatible with `--recurse-submodules=[yes|on-demand]`.\n Internally this is used to implement the `push.negotiate` option, see\n linkgit:git-config[1].\n \ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 7bbff5a029..fef4332fb1 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -76,6 +76,7 @@ static struct transport *gtransport;\n static struct transport *gsecondary;\n static const char *submodule_prefix = \"\";\n static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;\n static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;\n static int shown_url = 0;\n static struct refspec refmap = REFSPEC_INIT_FETCH;\n@@ -167,7 +168,7 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"prune remote-tracking branches no longer on remote\")),\n \tOPT_BOOL('P', \"prune-tags\", &prune_tags,\n \t\t N_(\"prune local tags no longer on remote and clobber changed tags\")),\n-\tOPT_CALLBACK_F(0, \"recurse-submodules\", &recurse_submodules, N_(\"on-demand\"),\n+\tOPT_CALLBACK_F(0, \"recurse-submodules\", &recurse_submodules_cli, N_(\"on-demand\"),\n \t\t N_(\"control recursive fetching of submodules\"),\n \t\t PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),\n \tOPT_BOOL(0, \"dry-run\", &dry_run,\n@@ -2014,6 +2015,27 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n+\n+\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n+\t\trecurse_submodules = recurse_submodules_cli;\n+\n+\tif (negotiate_only) {\n+\t\tswitch (recurse_submodules_cli) {\n+\t\tcase RECURSE_SUBMODULES_OFF:\n+\t\tcase RECURSE_SUBMODULES_DEFAULT:\n+\t\t\t/*\n+\t\t\t * --negotiate-only should never recurse into\n+\t\t\t * submodules. Skip it by setting recurse_submodules to\n+\t\t\t * RECURSE_SUBMODULES_OFF.\n+\t\t\t */\n+\t\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n+\t\t\tbreak;\n+\n+\t\tdefault:\n+\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n+\t\t}\n+\t}\n+\n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n \t\tint *sfjc = submodule_fetch_jobs_config == -1\n \t\t\t ? &submodule_fetch_jobs_config : NULL;\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 2f04cf9a1c..87881726ed 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -229,6 +229,18 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n \ttest_i18ngrep \"push negotiation failed\" err\n '\n \n+test_expect_success 'push with negotiation does not attempt to fetch submodules' '\n+\tmk_empty submodule_upstream &&\n+\ttest_commit -C submodule_upstream submodule_commit &&\n+\tgit submodule add ./submodule_upstream submodule &&\n+\tmk_empty testrepo &&\n+\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\ttest_commit -C testrepo unrelated_commit &&\n+\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n+\tgit -c submodule.recurse=true -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n+\t! grep \"Fetching submodule\" err\n+'\n+\n test_expect_success 'push without wildcard' '\n \tmk_empty testrepo &&\n \ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 710f33e2aa..1b9023d3f0 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -628,6 +628,18 @@ test_expect_success 'usage: --negotiate-only without --negotiation-tip' '\n \ttest_cmp err.expect err.actual\n '\n \n+test_expect_success 'usage: --negotiate-only with --recurse-submodules' '\n+\tcat >err.expect <<-\\EOF &&\n+\tfatal: --negotiate-only and --recurse-submodules cannot be used together\n+\tEOF\n+\n+\ttest_must_fail git -c protocol.version=2 -C client fetch \\\n+\t\t--negotiate-only \\\n+\t\t--recurse-submodules \\\n+\t\torigin 2>err.actual &&\n+\ttest_cmp err.expect err.actual\n+'\n+\n test_expect_success 'file:// --negotiate-only' '\n \tSERVER=\"server\" &&\n \tURI=\"file://$(pwd)/server\" &&\n-- \n2.33.GIT\n\n"},{"id":"446472","messageId":"xmqqpmoopo4m.fsf@gitster.g","threadId":"57049","inReplyTo":"kl6lpmooy5lq.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v5 3/3] fetch --negotiate-only: do not update submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-19T00:26:01Z","receivedAt":"2022-01-19T00:26:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> Ah, I misunderstood. I'll blame the fact that I'm recovering from\n> COVID ;) I'll remove the block from the \"case\".\n\nThanks. Take it easy and be safe.\n\nWill queue the updated one. It looks good to me otherwise; unless I\nhear otherwise from others, let's have it in 'next' soon, and\ninclude it in the first batch after the current cycle.\n\n"},{"id":"446533","messageId":"20220120023821.12320-1-worldhello.net@gmail.com","threadId":"57049","inReplyTo":"20220119000056.58503-4-chooglen@google.com","subject":"RE: [PATCH v6 3/3] fetch --negotiate-only: do not update submodules","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2022-01-20T02:38:21Z","receivedAt":"2022-01-20T02:38:40Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Tue, 18 Jan 2022 at 16:00:56, Glen Choo wrote:\n\n> @@ -2014,6 +2015,27 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \n> \targc = parse_options(argc, argv, prefix,\n> \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n> +\n> +\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n> +\t\trecurse_submodules = recurse_submodules_cli;\n> +\n> +\tif (negotiate_only) {\n> +\t\tswitch (recurse_submodules_cli) {\n> +\t\tcase RECURSE_SUBMODULES_OFF:\n> +\t\tcase RECURSE_SUBMODULES_DEFAULT:\n> +\t\t\t/*\n> +\t\t\t * --negotiate-only should never recurse into\n> +\t\t\t * submodules. Skip it by setting recurse_submodules to\n> +\t\t\t * RECURSE_SUBMODULES_OFF.\n> +\t\t\t */\n> +\t\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n> +\t\t\tbreak;\n> +\n> +\t\tdefault:\n> +\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n\nFound this new l10n message in \"po/git.pot\" in the next branch[1]. To be\nl10n-friendly, it's better to follow the standard Jean-Noël provided in\ncommit 246cac8505 (i18n: turn even more messages into \"cannot be used\ntogether\" ones, 2022-01-05). And rewrite the l10n messageas as:\n\n-\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n+\t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t\t \"--negotiate-only\", \"--recurse-submodules\");\n\n[1]: https://github.com/git-l10n/git-po/blob/pot/next/2022-01-19.diff#L23\n\n--\nJiang Xin\n\n"},{"id":"446581","messageId":"kl6lh79yxq3h.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57049","inReplyTo":"20220120023821.12320-1-worldhello.net@gmail.com","subject":"RE: [PATCH v6 3/3] fetch --negotiate-only: do not update submodules","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-20T17:40:50Z","receivedAt":"2022-01-20T17:40:55Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Jiang Xin writes:\n\n> On Tue, 18 Jan 2022 at 16:00:56, Glen Choo wrote:\n>\n>> @@ -2014,6 +2015,27 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>> \n>> \targc = parse_options(argc, argv, prefix,\n>> \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n>> +\n>> +\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n>> +\t\trecurse_submodules = recurse_submodules_cli;\n>> +\n>> +\tif (negotiate_only) {\n>> +\t\tswitch (recurse_submodules_cli) {\n>> +\t\tcase RECURSE_SUBMODULES_OFF:\n>> +\t\tcase RECURSE_SUBMODULES_DEFAULT:\n>> +\t\t\t/*\n>> +\t\t\t * --negotiate-only should never recurse into\n>> +\t\t\t * submodules. Skip it by setting recurse_submodules to\n>> +\t\t\t * RECURSE_SUBMODULES_OFF.\n>> +\t\t\t */\n>> +\t\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n>> +\t\t\tbreak;\n>> +\n>> +\t\tdefault:\n>> +\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n>\n> Found this new l10n message in \"po/git.pot\" in the next branch[1]. To be\n> l10n-friendly, it's better to follow the standard Jean-Noël provided in\n> commit 246cac8505 (i18n: turn even more messages into \"cannot be used\n> together\" ones, 2022-01-05). And rewrite the l10n messageas as:\n>\n> -\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n> +\t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n> +\t\t\t \"--negotiate-only\", \"--recurse-submodules\");\n>\n> [1]: https://github.com/git-l10n/git-po/blob/pot/next/2022-01-19.diff#L23\n>\n> --\n> Jiang Xin\n\nThanks for spotting this! I didn't realize we had standardized this.\nI'll send a reroll soon.\n"},{"id":"446582","messageId":"20220120174942.94231-1-chooglen@google.com","threadId":"57049","inReplyTo":"20220119000056.58503-1-chooglen@google.com","subject":"[PATCH v7 0/3] fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-20T17:49:39Z","receivedAt":"2022-01-20T17:49:50Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Changes since v6:\n* use standard message format introduced in 246cac8505 (i18n: turn even\n more messages into \"cannot be used together\" ones, 2022-01-05) (thanks\n Jiang Xin!)\n\nChanges since v5:\n* revert v4\n* drop the unnecessary block that Junio actually meant\n\nChanges since v4:\n* drop an unnecessary block (thanks Junio!)\n\nChanges since v3:\n* change commit message subject: builtin/fetch -> fetch --negotiate-only\n* move the 'goto cleanup' to _after_ the submodule updating task because\n we may want to update submodules even if objects were not fetched (as\n pointed out by Junio, thanks!)\n* disable submodule recursion in the patch that checks for\n --negotiate-only + --recurse-submodules, so we never silently ignore\n --recurse-submodules.\n* incorporate some of Jonathan's suggestions (thanks!)\n\nChanges since v2:\n* added a prepatory patch that introduces a \"goto cleanup\"\n* drop an unnecessary line move (as suggested by Junio)\n* check for user-given --recurse-submodules and die() (as suggested by\n Jonathan and Junio)\n* update --negotiate-only's documentation\n\nChanges since v1:\n* added more context to commit message\n* added a NEEDSWORK comment\n\nGlen Choo (3):\n fetch: use goto cleanup in cmd_fetch()\n fetch: skip tasks related to fetching objects\n fetch --negotiate-only: do not update submodules\n\n Documentation/fetch-options.txt | 1 +\n builtin/fetch.c | 41 ++++++++++++++++++++++++++++++---\n t/t5516-fetch-push.sh | 12 ++++++++++\n t/t5702-protocol-v2.sh | 12 ++++++++++\n 4 files changed, 63 insertions(+), 3 deletions(-)\n\nRange-diff against v6:\n1: ffa1a24109 = 1: ffa1a24109 fetch: use goto cleanup in cmd_fetch()\n2: b0c73e8135 = 2: b0c73e8135 fetch: skip tasks related to fetching objects\n3: 239101e752 ! 3: 7e0de7232b fetch --negotiate-only: do not update submodules\n @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n +\t\t\tbreak;\n +\n +\t\tdefault:\n -+\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n ++\t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n ++\t\t\t \"--negotiate-only\", \"--recurse-submodules\");\n +\t\t}\n +\t}\n +\n\nbase-commit: 90d242d36e248acfae0033274b524bfa55a947fd\n-- \n2.33.GIT\n\n"},{"id":"446583","messageId":"20220120174942.94231-2-chooglen@google.com","threadId":"57049","inReplyTo":"20220120174942.94231-1-chooglen@google.com","subject":"[PATCH v7 1/3] fetch: use goto cleanup in cmd_fetch()","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-20T17:49:40Z","receivedAt":"2022-01-20T17:49:53Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Replace an early return with 'goto cleanup' in cmd_fetch() so that the\nstring_list is always cleared (the string_list_clear() call is purely\ncleanup; the string_list is not reused). This makes cleanup consistent\nso that a subsequent commit can use 'goto cleanup' to bail out early.\n\nSigned-off-by: Glen Choo \n---\n builtin/fetch.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex eaab8056bf..0a625cb137 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2095,7 +2095,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tgtransport->smart_options->acked_commits = &acked_commits;\n \t\t} else {\n \t\t\twarning(_(\"protocol does not support --negotiate-only, exiting\"));\n-\t\t\treturn 1;\n+\t\t\tresult = 1;\n+\t\t\tgoto cleanup;\n \t\t}\n \t\tif (server_options.nr)\n \t\t\tgtransport->server_options = &server_options;\n@@ -2151,8 +2152,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tstrvec_clear(&options);\n \t}\n \n-\tstring_list_clear(&list, 0);\n-\n \tprepare_repo_settings(the_repository);\n \tif (fetch_write_commit_graph > 0 ||\n \t (fetch_write_commit_graph < 0 &&\n@@ -2170,5 +2169,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (enable_auto_gc)\n \t\trun_auto_maintenance(verbosity < 0);\n \n+ cleanup:\n+\tstring_list_clear(&list, 0);\n \treturn result;\n }\n-- \n2.33.GIT\n\n"},{"id":"446584","messageId":"20220120174942.94231-3-chooglen@google.com","threadId":"57049","inReplyTo":"20220120174942.94231-1-chooglen@google.com","subject":"[PATCH v7 2/3] fetch: skip tasks related to fetching objects","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-20T17:49:41Z","receivedAt":"2022-01-20T17:49:55Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"cmd_fetch() does the following with the assumption that objects are\nfetched:\n\n* Run gc\n* Write commit graphs (if enabled by fetch.writeCommitGraph=true)\n\nHowever, neither of these tasks makes sense if objects are not fetched\ne.g. `git fetch --negotiate-only` never fetches objects.\n\nSpeed up cmd_fetch() by bailing out early if we know for certain that\nobjects will not be fetched. cmd_fetch() can bail out early whenever\nobjects are not fetched, but for now this only considers\n--negotiate-only.\n\nThe same optimization does not apply to `git fetch --dry-run` because\nthat actually fetches objects; the dry run refers to not updating refs.\n\nSigned-off-by: Glen Choo \n---\n builtin/fetch.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0a625cb137..7bbff5a029 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2152,6 +2152,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tstrvec_clear(&options);\n \t}\n \n+\t/*\n+\t * Skip irrelevant tasks because we know objects were not\n+\t * fetched.\n+\t *\n+\t * NEEDSWORK: as a future optimization, we can return early\n+\t * whenever objects were not fetched e.g. if we already have all\n+\t * of them.\n+\t */\n+\tif (negotiate_only)\n+\t\tgoto cleanup;\n+\n \tprepare_repo_settings(the_repository);\n \tif (fetch_write_commit_graph > 0 ||\n \t (fetch_write_commit_graph < 0 &&\n-- \n2.33.GIT\n\n"},{"id":"446585","messageId":"20220120174942.94231-4-chooglen@google.com","threadId":"57049","inReplyTo":"20220120174942.94231-1-chooglen@google.com","subject":"[PATCH v7 3/3] fetch --negotiate-only: do not update submodules","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-20T17:49:42Z","receivedAt":"2022-01-20T17:49:56Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"`git fetch --negotiate-only` is an implementation detail of push\nnegotiation and, unlike most `git fetch` invocations, does not actually\nupdate the main repository. Thus it should not update submodules even\nif submodule recursion is enabled.\n\nThis is not just slow, it is wrong e.g. push negotiation with\n\"submodule.recurse=true\" will cause submodules to be updated because it\ninvokes `git fetch --negotiate-only`.\n\nFix this by disabling submodule recursion if --negotiate-only was given.\nSince this makes --negotiate-only and --recurse-submodules incompatible,\ncheck for this invalid combination and die.\n\nThis does not use the \"goto cleanup\" introduced in the previous commit\nbecause we want to recurse through submodules whenever a ref is fetched,\nand this can happen without introducing new objects.\n\nSigned-off-by: Glen Choo \n---\n Documentation/fetch-options.txt | 1 +\n builtin/fetch.c | 25 ++++++++++++++++++++++++-\n t/t5516-fetch-push.sh | 12 ++++++++++++\n t/t5702-protocol-v2.sh | 12 ++++++++++++\n 4 files changed, 49 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex e967ff1874..f903683189 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -71,6 +71,7 @@ configuration variables documented in linkgit:git-config[1], and the\n \tancestors of the provided `--negotiation-tip=*` arguments,\n \twhich we have in common with the server.\n +\n+This is incompatible with `--recurse-submodules=[yes|on-demand]`.\n Internally this is used to implement the `push.negotiate` option, see\n linkgit:git-config[1].\n \ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 7bbff5a029..efd1c9bb41 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -76,6 +76,7 @@ static struct transport *gtransport;\n static struct transport *gsecondary;\n static const char *submodule_prefix = \"\";\n static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;\n static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;\n static int shown_url = 0;\n static struct refspec refmap = REFSPEC_INIT_FETCH;\n@@ -167,7 +168,7 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"prune remote-tracking branches no longer on remote\")),\n \tOPT_BOOL('P', \"prune-tags\", &prune_tags,\n \t\t N_(\"prune local tags no longer on remote and clobber changed tags\")),\n-\tOPT_CALLBACK_F(0, \"recurse-submodules\", &recurse_submodules, N_(\"on-demand\"),\n+\tOPT_CALLBACK_F(0, \"recurse-submodules\", &recurse_submodules_cli, N_(\"on-demand\"),\n \t\t N_(\"control recursive fetching of submodules\"),\n \t\t PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),\n \tOPT_BOOL(0, \"dry-run\", &dry_run,\n@@ -2014,6 +2015,28 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n+\n+\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n+\t\trecurse_submodules = recurse_submodules_cli;\n+\n+\tif (negotiate_only) {\n+\t\tswitch (recurse_submodules_cli) {\n+\t\tcase RECURSE_SUBMODULES_OFF:\n+\t\tcase RECURSE_SUBMODULES_DEFAULT:\n+\t\t\t/*\n+\t\t\t * --negotiate-only should never recurse into\n+\t\t\t * submodules. Skip it by setting recurse_submodules to\n+\t\t\t * RECURSE_SUBMODULES_OFF.\n+\t\t\t */\n+\t\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n+\t\t\tbreak;\n+\n+\t\tdefault:\n+\t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t\t \"--negotiate-only\", \"--recurse-submodules\");\n+\t\t}\n+\t}\n+\n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n \t\tint *sfjc = submodule_fetch_jobs_config == -1\n \t\t\t ? &submodule_fetch_jobs_config : NULL;\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 2f04cf9a1c..87881726ed 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -229,6 +229,18 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n \ttest_i18ngrep \"push negotiation failed\" err\n '\n \n+test_expect_success 'push with negotiation does not attempt to fetch submodules' '\n+\tmk_empty submodule_upstream &&\n+\ttest_commit -C submodule_upstream submodule_commit &&\n+\tgit submodule add ./submodule_upstream submodule &&\n+\tmk_empty testrepo &&\n+\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\ttest_commit -C testrepo unrelated_commit &&\n+\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n+\tgit -c submodule.recurse=true -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n+\t! grep \"Fetching submodule\" err\n+'\n+\n test_expect_success 'push without wildcard' '\n \tmk_empty testrepo &&\n \ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 710f33e2aa..1b9023d3f0 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -628,6 +628,18 @@ test_expect_success 'usage: --negotiate-only without --negotiation-tip' '\n \ttest_cmp err.expect err.actual\n '\n \n+test_expect_success 'usage: --negotiate-only with --recurse-submodules' '\n+\tcat >err.expect <<-\\EOF &&\n+\tfatal: --negotiate-only and --recurse-submodules cannot be used together\n+\tEOF\n+\n+\ttest_must_fail git -c protocol.version=2 -C client fetch \\\n+\t\t--negotiate-only \\\n+\t\t--recurse-submodules \\\n+\t\torigin 2>err.actual &&\n+\ttest_cmp err.expect err.actual\n+'\n+\n test_expect_success 'file:// --negotiate-only' '\n \tSERVER=\"server\" &&\n \tURI=\"file://$(pwd)/server\" &&\n-- \n2.33.GIT\n\n"},{"id":"446600","messageId":"xmqqk0eugjcc.fsf_-_@gitster.g","threadId":"57049","inReplyTo":"20220120174942.94231-1-chooglen@google.com","subject":"Re* [PATCH v7 0/3] fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-20T21:58:43Z","receivedAt":"2022-01-20T21:58:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> Changes since v6:\n> * use standard message format introduced in 246cac8505 (i18n: turn even\n> more messages into \"cannot be used together\" ones, 2022-01-05) (thanks\n> Jiang Xin!)\n\nAs v6 is already in 'next' since yesterday, let's make it an\nincremental update. It would give us a place to spell out why we\nconsider this change is desirable.\n\nThis is a tangent, but I recall there was a talk about \"reviewer\nchecklist\". Things like:\n\n - check if we can reuse existing and identical message to reduce\n load on translators\n\n - when we are creating a file in a subdirectory of $GIT_DIR, be\n prepared to see any directories other than $GIT_DIR itself\n missing and create them as necessary\n\n - use safe_create_leading_directories() and adjust_shared_perm() on\n things under $GIT_DIR but not in the working tree\n\n\nmay belong there.\n\nI am not sure if it is feasible to create and maintain such a list,\nthough.\n\n----- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: [PATCH] fetch: help translators by reusing the same message template\n\nFollow the example set by 12909b6b (i18n: turn \"options are\nincompatible\" into \"cannot be used together\", 2022-01-05) and use\nthe same message string to reduce the need for translation.\n\nReported-by: Jiang Xin \nHelped-by: Glen Choo \nSigned-off-by: Junio C Hamano \n---\n builtin/fetch.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git c/builtin/fetch.c w/builtin/fetch.c\nindex dc6e637428..5c329f9835 100644\n--- c/builtin/fetch.c\n+++ w/builtin/fetch.c\n@@ -2014,7 +2014,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tbreak;\n \n \t\tdefault:\n-\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n+\t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t\t \"--negotiate-only\", \"--recurse-submodules\");\n \t\t}\n \t}\n \n"},{"id":"446610","messageId":"xmqqy23af1ka.fsf@gitster.g","threadId":"57049","inReplyTo":"20220120174942.94231-4-chooglen@google.com","subject":"Re: [PATCH v7 3/3] fetch --negotiate-only: do not update submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-20T23:08:05Z","receivedAt":"2022-01-20T23:08:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index 710f33e2aa..1b9023d3f0 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -628,6 +628,18 @@ test_expect_success 'usage: --negotiate-only without --negotiation-tip' '\n> \ttest_cmp err.expect err.actual\n> '\n> \n> +test_expect_success 'usage: --negotiate-only with --recurse-submodules' '\n> +\tcat >err.expect <<-\\EOF &&\n> +\tfatal: --negotiate-only and --recurse-submodules cannot be used together\n\nThis also needs to be updated to match. I've fixed up locally.\n\n"},{"id":"446611","messageId":"kl6lee52xalp.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57049","inReplyTo":"xmqqk0eugjcc.fsf_-_@gitster.g","subject":"Re: Re* [PATCH v7 0/3] fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-20T23:15:30Z","receivedAt":"2022-01-20T23:16:42Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> Glen Choo writes:\n>\n>> Changes since v6:\n>> * use standard message format introduced in 246cac8505 (i18n: turn even\n>> more messages into \"cannot be used together\" ones, 2022-01-05) (thanks\n>> Jiang Xin!)\n>\n> As v6 is already in 'next' since yesterday, let's make it an\n> incremental update. It would give us a place to spell out why we\n> consider this change is desirable.\n\nAh, yes. I will take note of this for the future.\n\n>\n> This is a tangent, but I recall there was a talk about \"reviewer\n> checklist\". Things like:\n>\n> - check if we can reuse existing and identical message to reduce\n> load on translators\n>\n> - when we are creating a file in a subdirectory of $GIT_DIR, be\n> prepared to see any directories other than $GIT_DIR itself\n> missing and create them as necessary\n>\n> - use safe_create_leading_directories() and adjust_shared_perm() on\n> things under $GIT_DIR but not in the working tree\n>\n>\n> may belong there.\n>\n> I am not sure if it is feasible to create and maintain such a list,\n> though.\n\nThis sounds like a combination of low-hanging fruit things to check when\nsubmitting/reviewing. I think that even a minimal list is preferable to\nthe toil of spotting and fixing the same mistakes over and over.\n\nA ReviewingPatches doc has been discussed internally for a while, but I\ndon't recall if this checklist was part of it.\n\n> ----- >8 --------- >8 --------- >8 --------- >8 -----\n> Subject: [PATCH] fetch: help translators by reusing the same message template\n>\n> Follow the example set by 12909b6b (i18n: turn \"options are\n> incompatible\" into \"cannot be used together\", 2022-01-05) and use\n> the same message string to reduce the need for translation.\n>\n> Reported-by: Jiang Xin \n> Helped-by: Glen Choo \n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/fetch.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git c/builtin/fetch.c w/builtin/fetch.c\n> index dc6e637428..5c329f9835 100644\n> --- c/builtin/fetch.c\n> +++ w/builtin/fetch.c\n> @@ -2014,7 +2014,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \t\t\tbreak;\n> \n> \t\tdefault:\n> -\t\t\tdie(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n> +\t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n> +\t\t\t \"--negotiate-only\", \"--recurse-submodules\");\n> \t\t}\n> \t}\n> \n\nThe diff looks good. Thanks!\n"},{"id":"446612","messageId":"kl6lbl06xajy.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57049","inReplyTo":"xmqqy23af1ka.fsf@gitster.g","subject":"Re: [PATCH v7 3/3] fetch --negotiate-only: do not update submodules","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-20T23:16:33Z","receivedAt":"2022-01-20T23:16:42Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> Glen Choo writes:\n>\n>> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n>> index 710f33e2aa..1b9023d3f0 100755\n>> --- a/t/t5702-protocol-v2.sh\n>> +++ b/t/t5702-protocol-v2.sh\n>> @@ -628,6 +628,18 @@ test_expect_success 'usage: --negotiate-only without --negotiation-tip' '\n>> \ttest_cmp err.expect err.actual\n>> '\n>> \n>> +test_expect_success 'usage: --negotiate-only with --recurse-submodules' '\n>> +\tcat >err.expect <<-\\EOF &&\n>> +\tfatal: --negotiate-only and --recurse-submodules cannot be used together\n>\n> This also needs to be updated to match. I've fixed up locally.\n\nAh, my mistake. Thanks for the catch :)\n"},{"id":"446614","messageId":"CANYiYbGyJU-HQAgRPEB=Bdw8Oj+jvMnL3z=VguXh21OEQcfNgQ@mail.gmail.com","threadId":"57049","inReplyTo":"xmqqk0eugjcc.fsf_-_@gitster.g","subject":"Re: Re* [PATCH v7 0/3] fetch: skip unnecessary tasks when using --negotiate-only","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2022-01-21T02:17:32Z","receivedAt":"2022-01-21T02:18:04Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Fri, Jan 21, 2022 at 5:58 AM Junio C Hamano wrote:\n>\n> Glen Choo writes:\n>\n> > Changes since v6:\n> > * use standard message format introduced in 246cac8505 (i18n: turn even\n> > more messages into \"cannot be used together\" ones, 2022-01-05) (thanks\n> > Jiang Xin!)\n>\n> As v6 is already in 'next' since yesterday, let's make it an\n> incremental update. It would give us a place to spell out why we\n> consider this change is desirable.\n>\n> This is a tangent, but I recall there was a talk about \"reviewer\n> checklist\". Things like:\n>\n> - check if we can reuse existing and identical message to reduce\n> load on translators\n\nHow to spot typos in l10n messages in time, instead of waiting until\nthe very end of the release cycle, has always puzzled me for a long\ntime. Until two months ago, by introducing a new GitHub Actions[1] in\nthe \"git-l10n/git-po\" repository[2], it was possible to generate\ntemporary \"po/git.pot\" file based on new changes in \"git.git\" in time,\nand create incremental diffs of \"po/git.pot\" as separate files in the\n\"pot/master\", \"pot/next\", and \"pot/seen\" branches in the \"git-po\"\nrepository. By checking the diffs daily, several i18n issues were\nfound in recent months.\n\nWe can make some improvements to the workflow:\n\n1. Send each incremental diff of \"po/git.pot\" to active l10n team\nleaders, and also to this mailing list. Git contributors can find\n\"side effects\" introduced by their patches.\n\n2. Just like using the \"git-po-helper\" program[3] to check \"msgstr\" in\nl10n translations, develop a new helper to check \"msgid\". E.g.: check\nif we can reuse existing and identical message to reduce load on\ntranslators.\n\nThis workflow only works when patches go into the branch \"seen\" of\ngit.git repository. Lagging behind code reviews.\n\n[1]: https://github.com/git-l10n/git-po/blob/pot/CI/.github/workflows/git-pot.yml\n[2]: https://github.com/git-l10n/git-po\n[3]: https://github.com/git-l10n/git-po-helper\n\n> - when we are creating a file in a subdirectory of $GIT_DIR, be\n> prepared to see any directories other than $GIT_DIR itself\n> missing and create them as necessary\n>\n> - use safe_create_leading_directories() and adjust_shared_perm() on\n> things under $GIT_DIR but not in the working tree\n>\n>\n> may belong there.\n>\n> I am not sure if it is feasible to create and maintain such a list,\n> though.\n>\n> ----- >8 --------- >8 --------- >8 --------- >8 -----\n> Subject: [PATCH] fetch: help translators by reusing the same message template\n>\n> Follow the example set by 12909b6b (i18n: turn \"options are\n> incompatible\" into \"cannot be used together\", 2022-01-05) and use\n> the same message string to reduce the need for translation.\n>\n> Reported-by: Jiang Xin \n> Helped-by: Glen Choo \n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/fetch.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git c/builtin/fetch.c w/builtin/fetch.c\n> index dc6e637428..5c329f9835 100644\n> --- c/builtin/fetch.c\n> +++ w/builtin/fetch.c\n> @@ -2014,7 +2014,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> break;\n>\n> default:\n> - die(_(\"--negotiate-only and --recurse-submodules cannot be used together\"));\n> + die(_(\"options '%s' and '%s' cannot be used together\"),\n> + \"--negotiate-only\", \"--recurse-submodules\");\n> }\n> }\n>\n"}]}