{"thread":{"id":"57817","subject":"Bugreport - submodules are fetched twice in some cases","startedAt":"2022-04-29T14:47:05Z","lastAt":"2022-05-18T15:53:27Z","messageCount":17,"participants":["Benedek Kozma","Junio C Hamano","Glen Choo"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"454617","messageId":"CAN8jHOgwVF5g=jM_KYT0Oh+j+Lk3qvdyA4zNRbzf8e1Xp5WAUw@mail.gmail.com","threadId":"57817","inReplyTo":null,"subject":"Bugreport - submodules are fetched twice in some cases","fromName":"Benedek Kozma","fromEmail":"cyberbeni@gmail.com","sentAt":"2022-04-29T14:46:36Z","receivedAt":"2022-04-29T14:47:05Z","isPatch":false,"sender":{"key":"cyberbeni@gmail.com","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\nRun the following command with 1 remote:\n`git fetch --all --prune --prune-tags`\nI have the following in my config that could affect this:\n```\n[submodule]\nrecurse = true\n[fetch]\nparallel = 0\n```\n\nWhat did you expect to happen? (Expected behavior)\nSubmodules are only fetched once.\n\nWhat happened instead? (Actual behavior)\nSubmodules are fetched twice.\n\nWhat's different between what you expected and what actually happened?\nThe process takes longer\n\nAnything else you want to add:\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.36.0\ncpu: arm64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nfeature: fsmonitor--daemon\nuname: Darwin 21.4.0 Darwin Kernel Version 21.4.0: Fri Mar 18 00:46:32\nPDT 2022; root:xnu-8020.101.4~15/RELEASE_ARM64_T6000 arm64\ncompiler info: clang: 13.1.6 (clang-1316.0.21.2)\nlibc info: no libc information available\n$SHELL (typically, interactive shell): /bin/zsh\n\n\n[Enabled Hooks]\npre-commit\n"},{"id":"454627","messageId":"xmqqczgzdc1r.fsf@gitster.g","threadId":"57817","inReplyTo":"CAN8jHOgwVF5g=jM_KYT0Oh+j+Lk3qvdyA4zNRbzf8e1Xp5WAUw@mail.gmail.com","subject":"Re: Bugreport - submodules are fetched twice in some cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-29T17:39:12Z","receivedAt":"2022-04-29T17:39:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"A simple and concrete reproduction \n\n git init top\n cd top\n date >file1\n git init sub\n cd sub\n date >subfile1\n git add .\n git commit -m subinitial\n cd .. ;# back to top\n git submodule add ./sub sub\n git add file1\n git commit -m initial\n cd .. ;# out of top\n git clone --recurse-submodules top copy\n cd copy\n git config submodule.recurse true\n git config fetch.parallel 0\n GIT_TRACE2=$(pwd)/trace git fetch --all --prune --prune-tags\n\nThis throws the three lines to the output.\n\nFetching origin\nFetching submodule sub\nFetching submodule sub\n\nThe two \"Fetching submodule\" messages are coming from two separate\ncalls to get_fetch_task_from_index(), and the trace does show that\nthe code is doing \"git-upload-pack\" three times (one for the top\nlevel, twice for the same top/sub). We can see it by grepping\nfor \"git-upload-pack\" in the resulting 'trace' file above.\n\nGlen, as submodule.c::fetch_submodules() was created in your heavy\nrefactoring quite recently, I thought I'd redirect this report in\nyour direction, as I expect you'd be the most clueful in this area\n;-)\n\nThanks.\n\n\n\n\nBenedek Kozma writes:\n\n> Thank you for filling out a Git bug report!\n> Please answer the following questions to help us understand your issue.\n>\n> What did you do before the bug happened? (Steps to reproduce your issue)\n> Run the following command with 1 remote:\n> `git fetch --all --prune --prune-tags`\n> I have the following in my config that could affect this:\n> ```\n> [submodule]\n> recurse = true\n> [fetch]\n> parallel = 0\n> ```\n>\n> What did you expect to happen? (Expected behavior)\n> Submodules are only fetched once.\n>\n> What happened instead? (Actual behavior)\n> Submodules are fetched twice.\n>\n> What's different between what you expected and what actually happened?\n> The process takes longer\n>\n> Anything else you want to add:\n>\n> Please review the rest of the bug report below.\n> You can delete any lines you don't wish to share.\n>\n>\n> [System Info]\n> git version:\n> git version 2.36.0\n> cpu: arm64\n> no commit associated with this build\n> sizeof-long: 8\n> sizeof-size_t: 8\n> shell-path: /bin/sh\n> feature: fsmonitor--daemon\n> uname: Darwin 21.4.0 Darwin Kernel Version 21.4.0: Fri Mar 18 00:46:32\n> PDT 2022; root:xnu-8020.101.4~15/RELEASE_ARM64_T6000 arm64\n> compiler info: clang: 13.1.6 (clang-1316.0.21.2)\n> libc info: no libc information available\n> $SHELL (typically, interactive shell): /bin/zsh\n>\n>\n> [Enabled Hooks]\n> pre-commit\n"},{"id":"454632","messageId":"kl6l4k2bpv61.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57817","inReplyTo":"xmqqczgzdc1r.fsf@gitster.g","subject":"Re: Bugreport - submodules are fetched twice in some cases","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-29T19:05:26Z","receivedAt":"2022-04-29T19:05:56Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> A simple and concrete reproduction \n>\n> git init top\n> cd top\n> date >file1\n> git init sub\n> cd sub\n> date >subfile1\n> git add .\n> git commit -m subinitial\n> cd .. ;# back to top\n> git submodule add ./sub sub\n> git add file1\n> git commit -m initial\n> cd .. ;# out of top\n> git clone --recurse-submodules top copy\n> cd copy\n> git config submodule.recurse true\n> git config fetch.parallel 0\n> GIT_TRACE2=$(pwd)/trace git fetch --all --prune --prune-tags\n>\n> This throws the three lines to the output.\n>\n> Fetching origin\n> Fetching submodule sub\n> Fetching submodule sub\n>\n> The two \"Fetching submodule\" messages are coming from two separate\n> calls to get_fetch_task_from_index(), and the trace does show that\n> the code is doing \"git-upload-pack\" three times (one for the top\n> level, twice for the same top/sub). We can see it by grepping\n> for \"git-upload-pack\" in the resulting 'trace' file above.\n\n \nThanks for the reproduction recipe and findings, that'll be very helpful\n:)\n\n> Glen, as submodule.c::fetch_submodules() was created in your heavy\n> refactoring quite recently, I thought I'd redirect this report in\n> your direction, as I expect you'd be the most clueful in this area\n> ;-)\n\nHm, this does look like something that I probably introduced. But even\nif it turns out to be older than that, I think I am the right person to\nfix it, yes.\n\nI'm a little caught up with the embedded bare repo work, but I think I\ncan find time to debug this within the next 2 working days or so.\n"},{"id":"454636","messageId":"xmqq1qxfbqtq.fsf@gitster.g","threadId":"57817","inReplyTo":"kl6l4k2bpv61.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bugreport - submodules are fetched twice in some cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-29T20:02:57Z","receivedAt":"2022-04-29T20:03:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> Junio C Hamano writes:\n>\n>> A simple and concrete reproduction \n>>\n>> git init top\n>> cd top\n>> date >file1\n>> git init sub\n>> cd sub\n>> date >subfile1\n>> git add .\n>> git commit -m subinitial\n>> cd .. ;# back to top\n>> git submodule add ./sub sub\n>> git add file1\n>> git commit -m initial\n>> cd .. ;# out of top\n>> git clone --recurse-submodules top copy\n>> cd copy\n>> git config submodule.recurse true\n>> git config fetch.parallel 0\n>> GIT_TRACE2=$(pwd)/trace git fetch --all --prune --prune-tags\n>>\n>> This throws the three lines to the output.\n>>\n>> Fetching origin\n>> Fetching submodule sub\n>> Fetching submodule sub\n>>\n>> The two \"Fetching submodule\" messages are coming from two separate\n>> calls to get_fetch_task_from_index(), and the trace does show that\n>> the code is doing \"git-upload-pack\" three times (one for the top\n>> level, twice for the same top/sub). We can see it by grepping\n>> for \"git-upload-pack\" in the resulting 'trace' file above.\n>\n> \n> Thanks for the reproduction recipe and findings, that'll be very helpful\n> :)\n>\n>> Glen, as submodule.c::fetch_submodules() was created in your heavy\n>> refactoring quite recently, I thought I'd redirect this report in\n>> your direction, as I expect you'd be the most clueful in this area\n>> ;-)\n>\n> Hm, this does look like something that I probably introduced. But even\n> if it turns out to be older than that, I think I am the right person to\n> fix it, yes.\n\nIt seems that ever since the introduction of the --prune-tags option\nat v2.16.1-16-g97716d217c (fetch: add a --prune-tags option and\nfetch.pruneTags config, 2018-02-09), we always behaved this way.\n\nWithout \"--prune-tags\" (but still with \"--prune\"), we can go even\nolder than that version, and v2.10.0 seems to fetch only once.\n\nAnd the command keeps working that way all the way back to the\ncommit that starts honoring submodule.recurse configuration, at\nv2.13.0-137-g58f4203e7d (builtin/fetch.c: respect\n'submodule.recurse' option, 2017-05-31)\n\nIf we instead use \"git fetch --recurse-submodules\" with versions\nof GIt older than that, we can go even older. I saw v2.5.0 behaves\nthat way before I got tired and gave up.\n\nSo, we still would want to eventually get to it, but no rush. This\nis an old thing and not as urgent as fixing a recent regression.\n\nFWIW, without \"--all\", we do not make an extra fetch at all, with\nthe current code or with code as ancient as v2.5.0\n\nThanks.\n"},{"id":"454640","messageId":"kl6l1qxfpqwj.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57817","inReplyTo":"xmqq1qxfbqtq.fsf@gitster.g","subject":"Re: Bugreport - submodules are fetched twice in some cases","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-29T20:37:32Z","receivedAt":"2022-04-29T20:38:15Z","isPatch":false,"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>> Junio C Hamano writes:\n>>\n>>> A simple and concrete reproduction \n>>>\n>>> git init top\n>>> cd top\n>>> date >file1\n>>> git init sub\n>>> cd sub\n>>> date >subfile1\n>>> git add .\n>>> git commit -m subinitial\n>>> cd .. ;# back to top\n>>> git submodule add ./sub sub\n>>> git add file1\n>>> git commit -m initial\n>>> cd .. ;# out of top\n>>> git clone --recurse-submodules top copy\n>>> cd copy\n>>> git config submodule.recurse true\n>>> git config fetch.parallel 0\n>>> GIT_TRACE2=$(pwd)/trace git fetch --all --prune --prune-tags\n>>>\n>>> This throws the three lines to the output.\n>>>\n>>> Fetching origin\n>>> Fetching submodule sub\n>>> Fetching submodule sub\n>>>\n>>> The two \"Fetching submodule\" messages are coming from two separate\n>>> calls to get_fetch_task_from_index(), and the trace does show that\n>>> the code is doing \"git-upload-pack\" three times (one for the top\n>>> level, twice for the same top/sub). We can see it by grepping\n>>> for \"git-upload-pack\" in the resulting 'trace' file above.\n>>\n>> \n>> Thanks for the reproduction recipe and findings, that'll be very helpful\n>> :)\n>>\n>>> Glen, as submodule.c::fetch_submodules() was created in your heavy\n>>> refactoring quite recently, I thought I'd redirect this report in\n>>> your direction, as I expect you'd be the most clueful in this area\n>>> ;-)\n>>\n>> Hm, this does look like something that I probably introduced. But even\n>> if it turns out to be older than that, I think I am the right person to\n>> fix it, yes.\n>\n> It seems that ever since the introduction of the --prune-tags option\n> at v2.16.1-16-g97716d217c (fetch: add a --prune-tags option and\n> fetch.pruneTags config, 2018-02-09), we always behaved this way.\n>\n> Without \"--prune-tags\" (but still with \"--prune\"), we can go even\n> older than that version, and v2.10.0 seems to fetch only once.\n>\n> And the command keeps working that way all the way back to the\n> commit that starts honoring submodule.recurse configuration, at\n> v2.13.0-137-g58f4203e7d (builtin/fetch.c: respect\n> 'submodule.recurse' option, 2017-05-31)\n>\n> If we instead use \"git fetch --recurse-submodules\" with versions\n> of GIt older than that, we can go even older. I saw v2.5.0 behaves\n> that way before I got tired and gave up.\n>\n> So, we still would want to eventually get to it, but no rush. This\n> is an old thing and not as urgent as fixing a recent regression.\n\nAh, thanks a lot for investigating even further. I'll still take a look\nwhen I can, though this alleviates the time pressure a lot :)\n"},{"id":"455281","messageId":"kl6la6blj7fa.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57817","inReplyTo":"xmqq1qxfbqtq.fsf@gitster.g","subject":"Re: Bugreport - submodules are fetched twice in some cases","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-05-14T00:15:37Z","receivedAt":"2022-05-14T02:51:36Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"> FWIW, without \"--all\", we do not make an extra fetch at all, with\n> the current code or with code as ancient as v2.5.0\n\nAha, the reason why I wasn't able to override \"--recurse-submodules\" is\nthat I didn't notice add_options_to_argv() was setting\n\"--recurse-submodules\" for each of the 'inner' fetch-es in\nfetch_multiple().\n\nSo a PoC fix might look like this\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e3791f09ed..d42646e68c 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1856,10 +1856,6 @@ static void add_options_to_argv(struct strvec *argv)\n \t\tstrvec_push(argv, \"--force\");\n \tif (keep)\n \t\tstrvec_push(argv, \"--keep\");\n-\tif (recurse_submodules == RECURSE_SUBMODULES_ON)\n-\t\tstrvec_push(argv, \"--recurse-submodules\");\n-\telse if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)\n-\t\tstrvec_push(argv, \"--recurse-submodules=on-demand\");\n \tif (tags == TAGS_SET)\n \t\tstrvec_push(argv, \"--tags\");\n \telse if (tags == TAGS_UNSET)\n@@ -1942,8 +1938,7 @@ static int fetch_multiple(struct string_list *list, int max_children)\n \t\t\treturn errcode;\n \t}\n \n-\tstrvec_pushl(&argv, \"fetch\", \"--append\", \"--no-auto-gc\",\n-\t\t \"--no-write-commit-graph\", NULL);\n+\tstrvec_pushl(&argv, \"fetch\", \"--append\", \"--no-auto-gc\", \"--no-write-commit-graph\", \"--recurse-submodules=false\", NULL);\n \tadd_options_to_argv(&argv);\n \n \tif (max_children != 1 && list->nr != 1) {\n@@ -2271,6 +2266,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tmax_children = fetch_parallel_config;\n \n \t\tadd_options_to_argv(&options);\n+\t\tif (recurse_submodules == RECURSE_SUBMODULES_ON)\n+\t\t\tstrvec_push(&options, \"--recurse-submodules\");\n+\t\telse if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)\n+\t\t\tstrvec_push(&options, \"--recurse-submodules=on-demand\");\n \t\tresult = fetch_submodules(the_repository,\n \t\t\t\t\t &options,\n \t\t\t\t\t submodule_prefix,\n"},{"id":"455282","messageId":"kl6lczghj7tn.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57817","inReplyTo":"xmqq1qxfbqtq.fsf@gitster.g","subject":"Re: Bugreport - submodules are fetched twice in some cases","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-05-14T00:07:00Z","receivedAt":"2022-05-14T02:57:07Z","isPatch":false,"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>> Junio C Hamano writes:\n>>\n>>> A simple and concrete reproduction \n>>>\n>>> git init top\n>>> cd top\n>>> date >file1\n>>> git init sub\n>>> cd sub\n>>> date >subfile1\n>>> git add .\n>>> git commit -m subinitial\n>>> cd .. ;# back to top\n>>> git submodule add ./sub sub\n>>> git add file1\n>>> git commit -m initial\n>>> cd .. ;# out of top\n>>> git clone --recurse-submodules top copy\n>>> cd copy\n>>> git config submodule.recurse true\n>>> git config fetch.parallel 0\n>>> GIT_TRACE2=$(pwd)/trace git fetch --all --prune --prune-tags\n>>>\n>>> This throws the three lines to the output.\n>>>\n>>> Fetching origin\n>>> Fetching submodule sub\n>>> Fetching submodule sub\n>>>\n>>> The two \"Fetching submodule\" messages are coming from two separate\n>>> calls to get_fetch_task_from_index(), and the trace does show that\n>>> the code is doing \"git-upload-pack\" three times (one for the top\n>>> level, twice for the same top/sub). We can see it by grepping\n>>> for \"git-upload-pack\" in the resulting 'trace' file above.\n>>\n>> \n>> Thanks for the reproduction recipe and findings, that'll be very helpful\n>> :)\n>>\n>>> Glen, as submodule.c::fetch_submodules() was created in your heavy\n>>> refactoring quite recently, I thought I'd redirect this report in\n>>> your direction, as I expect you'd be the most clueful in this area\n>>> ;-)\n>>\n>> Hm, this does look like something that I probably introduced. But even\n>> if it turns out to be older than that, I think I am the right person to\n>> fix it, yes.\n>\n> It seems that ever since the introduction of the --prune-tags option\n> at v2.16.1-16-g97716d217c (fetch: add a --prune-tags option and\n> fetch.pruneTags config, 2018-02-09), we always behaved this way.\n>\n> Without \"--prune-tags\" (but still with \"--prune\"), we can go even\n> older than that version, and v2.10.0 seems to fetch only once.\n>\n> And the command keeps working that way all the way back to the\n> commit that starts honoring submodule.recurse configuration, at\n> v2.13.0-137-g58f4203e7d (builtin/fetch.c: respect\n> 'submodule.recurse' option, 2017-05-31)\n>\n> If we instead use \"git fetch --recurse-submodules\" with versions\n> of GIt older than that, we can go even older. I saw v2.5.0 behaves\n> that way before I got tired and gave up.\n>\n> So, we still would want to eventually get to it, but no rush. This\n> is an old thing and not as urgent as fixing a recent regression.\n>\n> FWIW, without \"--all\", we do not make an extra fetch at all, with\n> the current code or with code as ancient as v2.5.0\n\nI had a little free time, so I dug into it a bit more. \"--all\" does seem\nto be the crux of it and I _very strongly_ suspect it is because \"--all\"\ncreates a child process which also fetches submodules.\n\nHere's a hint inside of builtin/fetch.c:fetch_multiple..\n\n static int fetch_multiple(struct string_list *list, int max_children)\n {\n // ....\n strvec_pushl(&argv, \"fetch\", \"--append\", \"--no-auto-gc\", \"--no-write-commit-graph\", NULL);\n\nNotice how we pass \"--no-auto-gc\" and \"--no-write-commit-graph\"? This\nmakes sense because those tasks run _after_ the main fetch and only need\nto run once. If we didn't pass these flags, then we'd run those tasks\nonce per remote, and once more after the main fetch (R + 1 times, where\nR is the number of remotes).\n\nAnd obviously, we aren't passing \"--recurse-submodules=false\", so there's\ngood reason to believe that \"--all\" will fetch submodules R + 1 times.\n\nBut confusingly, I can't validate this hypothesis because I can't get\nthe \"inner fetch\" to respect \"--recurse-submodules\" (I hope this isn't\ndue to that recent change I made).\n\nI did most of this testing without \"--prune\" or \"--prune-tags\". Your\nfindings suggest that they're somewhat relevant though, so maybe I'll\nlook at those versions to get more clues.\n"},{"id":"455283","messageId":"xmqqpmkg8z58.fsf@gitster.g","threadId":"57817","inReplyTo":"kl6lczghj7tn.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bugreport - submodules are fetched twice in some cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-14T05:24:35Z","receivedAt":"2022-05-14T05:24:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> And obviously, we aren't passing \"--recurse-submodules=false\", so there's\n> good reason to believe that \"--all\" will fetch submodules R + 1 times.\n\nGood find.\n\nGiven your recent work on enumerating the commits in the submodule\nrepository that are needed to complement \"git fetch\" made in the\nsuperproject, the above finding raises an interesting question.\n\nImagine that we have two remotes for the current repository, and\nthis superproject uses one submodule.\n\nWhen we run \"git fetch --all --resurse-submodules\", from one remote,\nwe may grab a range of history in the superproject that mentions\nsubmodule commits C1 and C2 that are not in our clone of the\nsubmodule, while the other remote gives a different range of history\nin the superproject that mentions submodule commit C3 that we do not\nhave.\n\nWhat should happen in our submodule? In other words, how do we make\nsure that we grab C1, C2 and C3?\n\nIdeally, we probably would want to run a non-recursive fetch of the\nsuperproject twice (i.e. once for each of the two remotes we have),\nthen traverse the superproject history to find that these three\ncommits are needed in the submodule, and run a single (possibly\nrecursive) fetch in the submodule and ask for C1, C2 and C3. But I\nam not sure if we are set up to do so. Does the \"parent\" process\ntake a snapshot of our refs before spawning the two \"child\" fetches\nfor each remote when handling \"fetch --all\", so that we can later\nfigure out what superproject commits were obtained during the\nfetches from these two remotes? Without that information, we cannot\nfind out that C1, C2 and C3 are new in the submodule, so we cannot\nimplement the \"fetch without recursion from each remote and then do\na single fetch in submodule to grab everything we need at once\"\napproach.\n\nProvided if we have the \"make sure everything needed in the\nsubmodule is fetched by inspecting the range of commits we fetch for\na superproject\" working correctly for a single remote, an\nalternative approach is to run \"git fetch --recurse-submodules\" for\neach remote separately, without the \"parent\" process doing anything\nin the submodule (i.e. you earlier counted R+1 fetches, but instead,\nwe make R fetches in the submodule. It is less than ideal but it\nmay be easier to implement).\n\nThoughts?\n"},{"id":"455330","messageId":"kl6l7d6ljrrg.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57817","inReplyTo":"xmqqpmkg8z58.fsf@gitster.g","subject":"Re: Bugreport - submodules are fetched twice in some cases","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-05-16T17:45:23Z","receivedAt":"2022-05-16T17:45:32Z","isPatch":false,"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>> And obviously, we aren't passing \"--recurse-submodules=false\", so there's\n>> good reason to believe that \"--all\" will fetch submodules R + 1 times.\n>\n> Good find.\n>\n> Given your recent work on enumerating the commits in the submodule\n> repository that are needed to complement \"git fetch\" made in the\n> superproject, the above finding raises an interesting question.\n>\n> Imagine that we have two remotes for the current repository, and\n> this superproject uses one submodule.\n>\n> When we run \"git fetch --all --resurse-submodules\", from one remote,\n> we may grab a range of history in the superproject that mentions\n> submodule commits C1 and C2 that are not in our clone of the\n> submodule, while the other remote gives a different range of history\n> in the superproject that mentions submodule commit C3 that we do not\n> have.\n>\n> What should happen in our submodule? In other words, how do we make\n> sure that we grab C1, C2 and C3?\n>\n> Ideally, we probably would want to run a non-recursive fetch of the\n> superproject twice (i.e. once for each of the two remotes we have),\n> then traverse the superproject history to find that these three\n> commits are needed in the submodule, and run a single (possibly\n> recursive) fetch in the submodule and ask for C1, C2 and C3. But I\n> am not sure if we are set up to do so. Does the \"parent\" process\n> take a snapshot of our refs before spawning the two \"child\" fetches\n> for each remote when handling \"fetch --all\", so that we can later\n> figure out what superproject commits were obtained during the\n> fetches from these two remotes?\n\nI know that we do this \"snapshot\"-ing for a single process. Looking in\nthe code, I see that check_for_new_submodule_commits() creates the\nsnapshot (in addition to registering a newly fetched commit).\n\nI don't think we are in the ideal scenario because we only snapshot when\nwe fetch without \"--all\":\n\n cmd_fetch() > *fetch_one()* > do_fetch() > backfill_tags() >\n fetch_and_consume_refs() > store_updated_refs() >\n check_for_new_submodule_commits()\n\nIn the ideal scenario, it would be something like:\n\n cmd_fetch() >\n TODO_snapshot_old_refs(), fetch_[one|multiple](),\n TODO_register_new_refs()\n\nIt looks non-trivial enough that I don't think I'll try to fix this\nsoon, but it does not look prohibitively hard.\n\n> Provided if we have the \"make sure everything needed in the\n> submodule is fetched by inspecting the range of commits we fetch for\n> a superproject\" working correctly for a single remote, an\n> alternative approach is to run \"git fetch --recurse-submodules\" for\n> each remote separately, without the \"parent\" process doing anything\n> in the submodule (i.e. you earlier counted R+1 fetches, but instead,\n> we make R fetches in the submodule. It is less than ideal but it\n> may be easier to implement).\n>\n> Thoughts?\n\nThe +1 fetch is redundant, so it's probably good to get rid of it\nanyway. I also wonder what it does if we don't snapshot the old refs in\nthe \"parent\" process. Perhaps it does nothing in the\n\"--recurse-submodules=on-demand\" case because it wouldn't know what\nsubmodules have changed?\n\nSince it is only one fetch, it might not be worth the effort though.\nIt would only look correct in some odd situation where the user has only\none configured remote, but fetches with \"--all\".\n"},{"id":"455342","messageId":"xmqq4k1p2v40.fsf@gitster.g","threadId":"57817","inReplyTo":"kl6l7d6ljrrg.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bugreport - submodules are fetched twice in some cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-16T18:25:03Z","receivedAt":"2022-05-16T18:25:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> I don't think we are in the ideal scenario because we only snapshot when\n> we fetch without \"--all\":\n>\n> cmd_fetch() > *fetch_one()* > do_fetch() > backfill_tags() >\n> fetch_and_consume_refs() > store_updated_refs() >\n> check_for_new_submodule_commits()\n>\n> In the ideal scenario, it would be something like:\n>\n> cmd_fetch() >\n> TODO_snapshot_old_refs(), fetch_[one|multiple](),\n> TODO_register_new_refs()\n>\n> It looks non-trivial enough that I don't think I'll try to fix this\n> soon, but it does not look prohibitively hard.\n\nIt matches my gut feeling.\n\n>> Provided if we have the \"make sure everything needed in the\n>> submodule is fetched by inspecting the range of commits we fetch for\n>> a superproject\" working correctly for a single remote, an\n>> alternative approach is to run \"git fetch --recurse-submodules\" for\n>> each remote separately, without the \"parent\" process doing anything\n>> in the submodule (i.e. you earlier counted R+1 fetches, but instead,\n>> we make R fetches in the submodule. It is less than ideal but it\n>> may be easier to implement).\n>>\n>> Thoughts?\n>\n> The +1 fetch is redundant, so it's probably good to get rid of it\n> anyway.\n\nSounds sensible. It should be a single-liner, i.e.\n\n builtin/fetch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git i/builtin/fetch.c w/builtin/fetch.c\nindex eeee5ac8f1..be61c390c1 100644\n--- i/builtin/fetch.c\n+++ w/builtin/fetch.c\n@@ -2262,7 +2262,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tresult = fetch_multiple(&list, max_children);\n \t}\n \n-\tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n+\tif (!result && remote && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n \t\tstruct strvec options = STRVEC_INIT;\n \t\tint max_children = max_jobs;\n \n"},{"id":"455346","messageId":"xmqqwnel1eqb.fsf@gitster.g","threadId":"57817","inReplyTo":"xmqq4k1p2v40.fsf@gitster.g","subject":"Re: Bugreport - submodules are fetched twice in some cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-16T19:04:12Z","receivedAt":"2022-05-16T19:04:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> The +1 fetch is redundant, so it's probably good to get rid of it\n>> anyway.\n\nAnother thing I noticed is that this +1 is *not* limited to \"--all\".\nIf you give a group that expands to multiple remotes, you'd take the\nsame fetch_multiple() code path (that is why the \"single liner\"\npatch in the message I am responding to looks at the \"is remote\nset?\", which is the same condition that decides if we use\nfetch_one() or fetch_multiple()).\n\n\nI also notice that there is a strange \"optimzation\" that is used\nafter expanding a group into one or more remotes and when it turns\nout it was a group of one. In such a case (and only in such a\ncase), we set \"remote\" and bypass the code path that uses &list.\n\nI wonder if this \"optimization\" should be also used for \"--all\".\n\n builtin/fetch.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git c/builtin/fetch.c w/builtin/fetch.c\nindex e3791f09ed..9093455e81 100644\n--- c/builtin/fetch.c\n+++ w/builtin/fetch.c\n@@ -2187,6 +2187,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\telse if (argc > 1)\n \t\t\tdie(_(\"fetch --all does not make sense with refspecs\"));\n \t\t(void) for_each_remote(get_one_remote_for_fetch, &list);\n+\t\t/* no point doing fetch_multiple() of one */\n+\t\tif (list.nr == 1)\n+\t\t\tremote = remote_get(list.items[0].string);\n \t} else if (argc == 0) {\n \t\t/* No arguments -- use default remote */\n \t\tremote = remote_get(NULL);\n\n\n"},{"id":"455364","messageId":"xmqqczgd16wx.fsf_-_@gitster.g","threadId":"57817","inReplyTo":"xmqqwnel1eqb.fsf@gitster.g","subject":"[PATCH] fetch: do not run a redundant fetch from submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-16T21:53:02Z","receivedAt":"2022-05-16T21:53:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When 7dce19d3 (fetch/pull: Add the --recurse-submodules option,\n2010-11-12) introduced the \"--recurse-submodule\" option, the\napproach taken was to perform fetches in submodules only once, after\nall the main fetching (it may usually be a fetch from a single\nremote, but it could be fetching from a group of remotes using\nfetch_multiple()) succeeded. Later we added \"--all\" to fetch from\nall defined remotes, which complicated things even more.\n\nIf your project has a submodule, and you try to run \"git fetch\n--recurse-submodule --all\", you'd see a fetch for the top-level,\nwhich invokes another fetch for the submodule, followed by another\nfetch for the same submodule. All but the last fetch for the\nsubmodule come from a \"git fetch --recurse-submodules\" subprocess\nthat is spawned via the fetch_multiple() interface for the remotes,\nand the last fetch comes from the code at the end.\n\nBecause recursive fetching from submodules is done in each fetch for\nthe top-level in fetch_multiple(), the last fetch in the submodule\nis redundant. It only matters when fetch_one() interacts with a\nsingle remote at the top-level.\n\nWhile we are at it, there is one optimization that exists in dealing\nwith a group of remote, but is missing when \"--all\" is used. In the\nformer, when the group turns out to be a group of one, instead of\nspawning \"git fetch\" as a subprocess via the fetch_multiple()\ninterface, we use the normal fetch_one() code path. Do the same\nwhen handing \"--all\", if it turns out that we have only one remote\ndefined.\n\nSigned-off-by: Junio C Hamano \n---\n\n * So, here is a mixed thing that fixes the issue in two different\n ways, which makes it unsuitable to be the final patch. Either\n \"turning --all into a single fetch with optimizaiton\" that is in\n the first hunk, or \"don't do the final sweep unless doing a\n single fetch\" that is in the second hunk, is sufficient to make\n the symptom disappear. Of course, using them both does not break\n anything, but it somehow feels unsatisfactory, and makes readers\n feel that we should be able to do better.\n\n But the better thing being what Glen alluded to as \"non-trivial\n not prohibitivey hard\", I'll stop here.\n\n builtin/fetch.c | 6 +++++-\n t/t5617-clone-submodules-remote.sh | 13 +++++++++++++\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e3791f09ed..359321e731 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2187,6 +2187,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\telse if (argc > 1)\n \t\t\tdie(_(\"fetch --all does not make sense with refspecs\"));\n \t\t(void) for_each_remote(get_one_remote_for_fetch, &list);\n+\n+\t\t/* no point doing fetch_multiple() of one */\n+\t\tif (list.nr == 1)\n+\t\t\tremote = remote_get(list.items[0].string);\n \t} else if (argc == 0) {\n \t\t/* No arguments -- use default remote */\n \t\tremote = remote_get(NULL);\n@@ -2261,7 +2265,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tresult = fetch_multiple(&list, max_children);\n \t}\n \n-\tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n+\tif (!result && remote && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n \t\tstruct strvec options = STRVEC_INIT;\n \t\tint max_children = max_jobs;\n \ndiff --git a/t/t5617-clone-submodules-remote.sh b/t/t5617-clone-submodules-remote.sh\nindex ca8f80083a..8b6b482a39 100755\n--- a/t/t5617-clone-submodules-remote.sh\n+++ b/t/t5617-clone-submodules-remote.sh\n@@ -72,6 +72,19 @@ test_expect_success 'clone with --single-branch' '\n \t)\n '\n \n+test_expect_success 'fetch --all with --recurse-submodules' '\n+\ttest_when_finished \"rm -fr super_clone\" &&\n+\tgit clone --recurse-submodules srv.bare super_clone &&\n+\t(\n+\t\tcd super_clone &&\n+\t\tgit config submodule.recurse true &&\n+\t\tgit config fetch.parallel 0 &&\n+\t\tgit fetch --all 2>../fetch-log\n+\t) &&\n+\tgrep \"Fetching sub\" fetch-log >fetch-subs &&\n+\ttest_line_count = 1 fetch-subs\n+'\n+\n # do basic partial clone from \"srv.bare\"\n # confirm partial clone was registered in the local config for super and sub.\n test_expect_success 'clone with --filter' '\n-- \n2.36.1-379-g841af8e974\n\n"},{"id":"455368","messageId":"kl6lr14thysl.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57817","inReplyTo":"xmqqczgd16wx.fsf_-_@gitster.g","subject":"Re: [PATCH] fetch: do not run a redundant fetch from submodule","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-05-16T22:56:26Z","receivedAt":"2022-05-16T22:56:39Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index e3791f09ed..359321e731 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -2187,6 +2187,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \t\telse if (argc > 1)\n> \t\t\tdie(_(\"fetch --all does not make sense with refspecs\"));\n> \t\t(void) for_each_remote(get_one_remote_for_fetch, &list);\n> +\n> +\t\t/* no point doing fetch_multiple() of one */\n> +\t\tif (list.nr == 1)\n> +\t\t\tremote = remote_get(list.items[0].string);\n> \t} else if (argc == 0) {\n> \t\t/* No arguments -- use default remote */\n> \t\tremote = remote_get(NULL);\n> @@ -2261,7 +2265,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \t\tresult = fetch_multiple(&list, max_children);\n> \t}\n> \n> -\tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n> +\tif (!result && remote && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n> \t\tstruct strvec options = STRVEC_INIT;\n> \t\tint max_children = max_jobs;\n\nIMO \"&& remote\" is non-inuitive enough to warrant a comment, e.g.\n\n /* do not update submodules if fetch_multiple() was called */\n if (!result && remote && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n\nbut I suppose that the commit message explains this well enough.\n\n> diff --git a/t/t5617-clone-submodules-remote.sh b/t/t5617-clone-submodules-remote.sh\n> index ca8f80083a..8b6b482a39 100755\n> --- a/t/t5617-clone-submodules-remote.sh\n> +++ b/t/t5617-clone-submodules-remote.sh\n> @@ -72,6 +72,19 @@ test_expect_success 'clone with --single-branch' '\n> \t)\n> '\n> \n> +test_expect_success 'fetch --all with --recurse-submodules' '\n> +\ttest_when_finished \"rm -fr super_clone\" &&\n> +\tgit clone --recurse-submodules srv.bare super_clone &&\n> +\t(\n> +\t\tcd super_clone &&\n> +\t\tgit config submodule.recurse true &&\n> +\t\tgit config fetch.parallel 0 &&\n> +\t\tgit fetch --all 2>../fetch-log\n> +\t) &&\n> +\tgrep \"Fetching sub\" fetch-log >fetch-subs &&\n> +\ttest_line_count = 1 fetch-subs\n> +'\n> +\n\nThe test looks good, but I think it belongs in\nt/t5526-fetch-submodules.sh. I don't see anything else in\nt5617-clone-submodules-remote.sh that calls \"git fetch\".\n\nIn addition, I think we need one more test that adds another remote. In\nthe above test, we only have one remote, so we hit your new optimization\nand already pass the test without the need for \"&& remote\".\n\nWhereas this test fails if we remove \"&& remote\".\n\n test_expect_success 'fetch --all with --recurse-submodules and more remotes' '\n test_when_finished \"rm -fr super_clone\" &&\n git clone --recurse-submodules srv.bare super_clone &&\n cp -r srv.bare srv.bare2 &&\n (\n cd super_clone &&\n git config submodule.recurse true &&\n git config fetch.parallel 0 &&\n git remote add other ../srv.bare2 &&\n git fetch --all 2>../fetch-log\n ) &&\n grep \"Fetching sub\" fetch-log >fetch-subs &&\n test_line_count = 2 fetch-subs\n '\n"},{"id":"455370","messageId":"xmqqpmkdyrw9.fsf@gitster.g","threadId":"57817","inReplyTo":"kl6lr14thysl.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH] fetch: do not run a redundant fetch from submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-16T23:33:26Z","receivedAt":"2022-05-16T23:33:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> IMO \"&& remote\" is non-inuitive enough to warrant a comment, e.g.\n>\n> /* do not update submodules if fetch_multiple() was called */\n> if (!result && remote && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n\nAs I am not so familiar with this codepath when submodules are\ninvolved, I need to be explained why having called fetch_multiple()\nmeans there is no need to update submodules\".\n\n\t/* \n\t * This is only needed after fetch_one(), which does not\n * fetch submodules by itself.\n\t *\n\t * fetch_multiple() has already updated submodules to grab\n\t * commits necessary for the fetched history from each remote,\n\t * so there is no need to fetch submodules from here.\n\t */\n\nperhaps?\n\n> The test looks good, but I think it belongs in\n> t/t5526-fetch-submodules.sh. I don't see anything else in\n> t5617-clone-submodules-remote.sh that calls \"git fetch\".\n\nGood eyes.\n\n> In addition, I think we need one more test that adds another remote. In\n> the above test, we only have one remote, so we hit your new optimization\n> and already pass the test without the need for \"&& remote\".\n\nTrue, too.\n"},{"id":"455371","messageId":"xmqqk0alyqyj.fsf_-_@gitster.g","threadId":"57817","inReplyTo":"xmqqczgd16wx.fsf_-_@gitster.g","subject":"[PATCH v2] fetch: do not run a redundant fetch from submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-16T23:53:40Z","receivedAt":"2022-05-16T23:53:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When 7dce19d3 (fetch/pull: Add the --recurse-submodules option,\n2010-11-12) introduced the \"--recurse-submodule\" option, the\napproach taken was to perform fetches in submodules only once, after\nall the main fetching (it may usually be a fetch from a single\nremote, but it could be fetching from a group of remotes using\nfetch_multiple()) succeeded. Later we added \"--all\" to fetch from\nall defined remotes, which complicated things even more.\n\nIf your project has a submodule, and you try to run \"git fetch\n--recurse-submodule --all\", you'd see a fetch for the top-level,\nwhich invokes another fetch for the submodule, followed by another\nfetch for the same submodule. All but the last fetch for the\nsubmodule come from a \"git fetch --recurse-submodules\" subprocess\nthat is spawned via the fetch_multiple() interface for the remotes,\nand the last fetch comes from the code at the end.\n\nBecause recursive fetching from submodules is done in each fetch for\nthe top-level in fetch_multiple(), the last fetch in the submodule\nis redundant. It only matters when fetch_one() interacts with a\nsingle remote at the top-level.\n\nWhile we are at it, there is one optimization that exists in dealing\nwith a group of remote, but is missing when \"--all\" is used. In the\nformer, when the group turns out to be a group of one, instead of\nspawning \"git fetch\" as a subprocess via the fetch_multiple()\ninterface, we use the normal fetch_one() code path. Do the same\nwhen handing \"--all\", if it turns out that we have only one remote\ndefined.\n\nHelped-by: Glen Choo \nSigned-off-by: Junio C Hamano \n---\n\nSo here is a second attempt. It demonstrates a bit interesting\nfunny in range-diff where similar changes from the previous round\ngets applied to a different target.\n\nt5617 is much cleanly organized than t5526, and we may want to clean\nup the latter after dust settles.\n\n1: 006fe43da1 ! 1: da0a4e341b fetch: do not run a redundant fetch from submodule\n @@ Commit message\n when handing \"--all\", if it turns out that we have only one remote\n defined.\n \n + Helped-by: Glen Choo \n Signed-off-by: Junio C Hamano \n \n ## builtin/fetch.c ##\n @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"fetch --all does not make sense with refspecs\"));\n \t\t(void) for_each_remote(get_one_remote_for_fetch, &list);\n +\n -+\t\t/* no point doing fetch_multiple() of one */\n ++\t\t/* do not do fetch_multiple() of one */\n +\t\tif (list.nr == 1)\n +\t\t\tremote = remote_get(list.items[0].string);\n \t} else if (argc == 0) {\n @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t}\n \n -\tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n ++\n ++\t/*\n ++\t * This is only needed after fetch_one(), which does not fetch\n ++\t * submodules by itself.\n ++\t *\n ++\t * When we fetch from multiple remotes, fetch_multiple() has\n ++\t * already updated submodules to grab commits necessary for\n ++\t * the fetched history from each remote, so there is no need\n ++\t * to fetch submodules from here.\n ++\t */\n +\tif (!result && remote && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n \t\tstruct strvec options = STRVEC_INIT;\n \t\tint max_children = max_jobs;\n \n \n - ## t/t5617-clone-submodules-remote.sh ##\n -@@ t/t5617-clone-submodules-remote.sh: test_expect_success 'clone with --single-branch' '\n + ## t/t5526-fetch-submodules.sh ##\n +@@ t/t5526-fetch-submodules.sh: test_expect_success 'fetch --recurse-submodules updates name-conflicted, unpopul\n \t)\n '\n \n +test_expect_success 'fetch --all with --recurse-submodules' '\n -+\ttest_when_finished \"rm -fr super_clone\" &&\n -+\tgit clone --recurse-submodules srv.bare super_clone &&\n ++\ttest_when_finished \"rm -fr src_clone\" &&\n ++\tgit clone --recurse-submodules src src_clone &&\n +\t(\n -+\t\tcd super_clone &&\n ++\t\tcd src_clone &&\n +\t\tgit config submodule.recurse true &&\n +\t\tgit config fetch.parallel 0 &&\n +\t\tgit fetch --all 2>../fetch-log\n +\t) &&\n -+\tgrep \"Fetching sub\" fetch-log >fetch-subs &&\n ++\tgrep \"^Fetching submodule sub$\" fetch-log >fetch-subs &&\n +\ttest_line_count = 1 fetch-subs\n +'\n +\n - # do basic partial clone from \"srv.bare\"\n - # confirm partial clone was registered in the local config for super and sub.\n - test_expect_success 'clone with --filter' '\n ++test_expect_success 'fetch --all with --recurse-submodules with multiple' '\n ++\ttest_when_finished \"rm -fr src_clone\" &&\n ++\tgit clone --recurse-submodules src src_clone &&\n ++\t(\n ++\t\tcd src_clone &&\n ++\t\tgit remote add secondary ../src &&\n ++\t\tgit config submodule.recurse true &&\n ++\t\tgit config fetch.parallel 0 &&\n ++\t\tgit fetch --all 2>../fetch-log\n ++\t) &&\n ++\tgrep \"Fetching submodule sub\" fetch-log >fetch-subs &&\n ++\ttest_line_count = 2 fetch-subs\n ++'\n ++\n + test_done\n\n\n builtin/fetch.c | 16 +++++++++++++++-\n t/t5526-fetch-submodules.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e3791f09ed..8b15c40bb2 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2187,6 +2187,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\telse if (argc > 1)\n \t\t\tdie(_(\"fetch --all does not make sense with refspecs\"));\n \t\t(void) for_each_remote(get_one_remote_for_fetch, &list);\n+\n+\t\t/* do not do fetch_multiple() of one */\n+\t\tif (list.nr == 1)\n+\t\t\tremote = remote_get(list.items[0].string);\n \t} else if (argc == 0) {\n \t\t/* No arguments -- use default remote */\n \t\tremote = remote_get(NULL);\n@@ -2261,7 +2265,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tresult = fetch_multiple(&list, max_children);\n \t}\n \n-\tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n+\n+\t/*\n+\t * This is only needed after fetch_one(), which does not fetch\n+\t * submodules by itself.\n+\t *\n+\t * When we fetch from multiple remotes, fetch_multiple() has\n+\t * already updated submodules to grab commits necessary for\n+\t * the fetched history from each remote, so there is no need\n+\t * to fetch submodules from here.\n+\t */\n+\tif (!result && remote && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n \t\tstruct strvec options = STRVEC_INIT;\n \t\tint max_children = max_jobs;\n \ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 43dada8544..a301b56db8 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -1125,4 +1125,31 @@ test_expect_success 'fetch --recurse-submodules updates name-conflicted, unpopul\n \t)\n '\n \n+test_expect_success 'fetch --all with --recurse-submodules' '\n+\ttest_when_finished \"rm -fr src_clone\" &&\n+\tgit clone --recurse-submodules src src_clone &&\n+\t(\n+\t\tcd src_clone &&\n+\t\tgit config submodule.recurse true &&\n+\t\tgit config fetch.parallel 0 &&\n+\t\tgit fetch --all 2>../fetch-log\n+\t) &&\n+\tgrep \"^Fetching submodule sub$\" fetch-log >fetch-subs &&\n+\ttest_line_count = 1 fetch-subs\n+'\n+\n+test_expect_success 'fetch --all with --recurse-submodules with multiple' '\n+\ttest_when_finished \"rm -fr src_clone\" &&\n+\tgit clone --recurse-submodules src src_clone &&\n+\t(\n+\t\tcd src_clone &&\n+\t\tgit remote add secondary ../src &&\n+\t\tgit config submodule.recurse true &&\n+\t\tgit config fetch.parallel 0 &&\n+\t\tgit fetch --all 2>../fetch-log\n+\t) &&\n+\tgrep \"Fetching submodule sub\" fetch-log >fetch-subs &&\n+\ttest_line_count = 2 fetch-subs\n+'\n+\n test_done\n-- \n2.36.1-384-ga138d62aa5\n\n"},{"id":"455390","messageId":"kl6lo7zwhzsl.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57817","inReplyTo":"xmqqk0alyqyj.fsf_-_@gitster.g","subject":"Re: [PATCH v2] fetch: do not run a redundant fetch from submodule","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-05-17T16:47:06Z","receivedAt":"2022-05-17T16:47:27Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\nThis version looks good to me, thanks :)\n\n Reviewed-by: Glen Choo \n\nJunio C Hamano writes:\n\n> t5617 is much cleanly organized than t5526, and we may want to clean\n> up the latter after dust settles.\n\nYeah, t5526 has so many tests for the 'core' functionality that it's\nhard to fit something 'tangential' like \"--all\". I might touch it again\nsoon, so I'll keep this in mind.\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index e3791f09ed..8b15c40bb2 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -2261,7 +2265,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \t\tresult = fetch_multiple(&list, max_children);\n> \t}\n> \n> -\tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n> +\n> +\t/*\n> +\t * This is only needed after fetch_one(), which does not fetch\n> +\t * submodules by itself.\n> +\t *\n> +\t * When we fetch from multiple remotes, fetch_multiple() has\n> +\t * already updated submodules to grab commits necessary for\n> +\t * the fetched history from each remote, so there is no need\n> +\t * to fetch submodules from here.\n> +\t */\n> +\tif (!result && remote && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n> \t\tstruct strvec options = STRVEC_INIT;\n> \t\tint max_children = max_jobs;\n\nLooks good; the comment is easier to understand than my suggestion for\nsure.\n\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index 43dada8544..a301b56db8 100755\n> --- a/t/t5526-fetch-submodules.sh\n> +++ b/t/t5526-fetch-submodules.sh\n> @@ -1125,4 +1125,31 @@ test_expect_success 'fetch --recurse-submodules updates name-conflicted, unpopul\n> \t)\n> '\n> \n> +test_expect_success 'fetch --all with --recurse-submodules' '\n> +\ttest_when_finished \"rm -fr src_clone\" &&\n> +\tgit clone --recurse-submodules src src_clone &&\n> +\t(\n> +\t\tcd src_clone &&\n> +\t\tgit config submodule.recurse true &&\n> +\t\tgit config fetch.parallel 0 &&\n> +\t\tgit fetch --all 2>../fetch-log\n> +\t) &&\n> +\tgrep \"^Fetching submodule sub$\" fetch-log >fetch-subs &&\n> +\ttest_line_count = 1 fetch-subs\n> +'\n> +\n> +test_expect_success 'fetch --all with --recurse-submodules with multiple' '\n> +\ttest_when_finished \"rm -fr src_clone\" &&\n> +\tgit clone --recurse-submodules src src_clone &&\n> +\t(\n> +\t\tcd src_clone &&\n> +\t\tgit remote add secondary ../src &&\n> +\t\tgit config submodule.recurse true &&\n> +\t\tgit config fetch.parallel 0 &&\n> +\t\tgit fetch --all 2>../fetch-log\n> +\t) &&\n> +\tgrep \"Fetching submodule sub\" fetch-log >fetch-subs &&\n> +\ttest_line_count = 2 fetch-subs\n> +'\n> +\n\nAlso looks good.\n"},{"id":"455403","messageId":"xmqqczgayh01.fsf@gitster.g","threadId":"57817","inReplyTo":"kl6lo7zwhzsl.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v2] fetch: do not run a redundant fetch from submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-18T15:53:18Z","receivedAt":"2022-05-18T15:53:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n>> +\n>> +\t/*\n>> +\t * This is only needed after fetch_one(), which does not fetch\n>> +\t * submodules by itself.\n>> +\t *\n>> +\t * When we fetch from multiple remotes, fetch_multiple() has\n>> +\t * already updated submodules to grab commits necessary for\n>> +\t * the fetched history from each remote, so there is no need\n>> +\t * to fetch submodules from here.\n>> +\t */\n>> +\tif (!result && remote && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n>> \t\tstruct strvec options = STRVEC_INIT;\n>> \t\tint max_children = max_jobs;\n>\n> Looks good; the comment is easier to understand than my suggestion for\n> sure.\n\nThanks. Today's code has diverged too much from the original code I\nwrote long time ago (before submodules), and I needed an extra set\nof eyeballs to double check and tell me that what I (wishfully)\nwrote how the code works with submodules is in line with today's\ncode ;-)\n\n"}]}