{"thread":{"id":"50023","subject":"2.20.0 - Undocumented change in submodule update wrt # parallel jobs","startedAt":"2018-12-13T09:16:07Z","lastAt":"2018-12-14T02:54:04Z","messageCount":7,"participants":["Sjon Hortensius","Ævar Arnfjörð Bjarmason","Junio C Hamano","Stefan Beller","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"365235","messageId":"CAHef355RQt9gN-7QjuAAT8mZsNFKfCo4hOYi2+bkp-0Av7W=Qw@mail.gmail.com","threadId":"50023","inReplyTo":null,"subject":"2.20.0 - Undocumented change in submodule update wrt # parallel jobs","fromName":"Sjon Hortensius","fromEmail":"sjon@parse.nl","sentAt":"2018-12-13T09:15:52Z","receivedAt":"2018-12-13T09:16:07Z","isPatch":false,"sender":{"key":"sjon@parse.nl","avatar":null},"body":"When switching to 2.20 our `git submodule update' (which clones\nthrough ssh) broke because our ssh-server rejected the ~20\nsimultaneous connections the git-client makes. This seems to be caused\nby a (possibly unintended) change in\nhttps://github.com/git/git/commit/90efe595c53f4bb1851371344c35eff71f604d2b\nwhich removed the default of max_jobs=1\n\nWhile this can easily be fixed by configuring submodule.fetchJobs I\nthink this change should be documented - or reverted back to it's\nprevious default of 1\n\n--\nKind regards,\n\nSjon Hortensius\n\n| Parse Software Development B.V.\n| http://www.parse.nl\n"},{"id":"365252","messageId":"87sgz11q4u.fsf@evledraar.gmail.com","threadId":"50023","inReplyTo":"CAHef355RQt9gN-7QjuAAT8mZsNFKfCo4hOYi2+bkp-0Av7W=Qw@mail.gmail.com","subject":"Re: 2.20.0 - Undocumented change in submodule update wrt # parallel jobs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-13T14:02:25Z","receivedAt":"2018-12-13T14:02:31Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 13 2018, Sjon Hortensius wrote:\n\n> When switching to 2.20 our `git submodule update' (which clones\n> through ssh) broke because our ssh-server rejected the ~20\n> simultaneous connections the git-client makes. This seems to be caused\n> by a (possibly unintended) change in\n> https://github.com/git/git/commit/90efe595c53f4bb1851371344c35eff71f604d2b\n> which removed the default of max_jobs=1\n>\n> While this can easily be fixed by configuring submodule.fetchJobs I\n> think this change should be documented - or reverted back to it's\n> previous default of 1\n\nJust to add to this. SSH-ing with -j (which I assume that ~20\nis) is going to run into MaxStartups in sshd_config, which is especially\nannoying as it's pre-ssh-auth, so the server doesn't even get \"so and so\ntried to login\", it's just dropped as a potential DoS attack.\n"},{"id":"365256","messageId":"xmqqva3xecjz.fsf@gitster-ct.c.googlers.com","threadId":"50023","inReplyTo":"CAHef355RQt9gN-7QjuAAT8mZsNFKfCo4hOYi2+bkp-0Av7W=Qw@mail.gmail.com","subject":"Re: 2.20.0 - Undocumented change in submodule update wrt # parallel jobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-13T14:17:20Z","receivedAt":"2018-12-13T14:17:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sjon Hortensius writes:\n\n> When switching to 2.20 our `git submodule update' (which clones\n> through ssh) broke because our ssh-server rejected the ~20\n> simultaneous connections the git-client makes. This seems to be caused\n> by a (possibly unintended) change in\n> https://github.com/git/git/commit/90efe595c53f4bb1851371344c35eff71f604d2b\n> which removed the default of max_jobs=1\n>\n> While this can easily be fixed by configuring submodule.fetchJobs I\n> think this change should be documented - or reverted back to it's\n> previous default of 1\n\nThe commit in question does not look like it _wanted_ to change the\ndefault; rather, it appears to me that it wanted to be bug-to-bug\ncompatible with the original, and any such change of behaviour is\nentirely unintended.\n\nI think the attached may be sufficient to change the default\nmax_jobs back to 1.\n\nBy the way, is there a place where we document that the default\nvalue for fetchjobs, when unconfigured, is 1? If we are not making\nsuch a concrete promise, then I would think it is OK to update the\ndefault without any fanfare, as long as we have good reasons to do\nso. For this particular one, however, as I already said, I do not\nthink we wanted to change the default to unlimited or anything like\nthat, so...\n\n builtin/submodule--helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 789d00d87d..e8cdf84f1c 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1552,7 +1552,7 @@ struct submodule_update_clone {\n #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \\\n \tSUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \\\n \tNULL, NULL, NULL, \\\n-\tNULL, 0, 0, 0, NULL, 0, 0, 0}\n+\tNULL, 0, 0, 0, NULL, 0, 0, 1}\n \n \n static void next_submodule_warn_missing(struct submodule_update_clone *suc,\n"},{"id":"365280","messageId":"CAGZ79kYsk8YEUUhMVF9fBC++fop3CPyobXTgHTuF2Lgikf9CJA@mail.gmail.com","threadId":"50023","inReplyTo":"xmqqva3xecjz.fsf@gitster-ct.c.googlers.com","subject":"Re: 2.20.0 - Undocumented change in submodule update wrt # parallel jobs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-13T18:50:20Z","receivedAt":"2018-12-13T18:50:35Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Dec 13, 2018 at 6:17 AM Junio C Hamano wrote:\n>\n> Sjon Hortensius writes:\n>\n> > When switching to 2.20 our `git submodule update' (which clones\n> > through ssh) broke because our ssh-server rejected the ~20\n> > simultaneous connections the git-client makes. This seems to be caused\n> > by a (possibly unintended) change in\n> > https://github.com/git/git/commit/90efe595c53f4bb1851371344c35eff71f604d2b\n> > which removed the default of max_jobs=1\n> >\n> > While this can easily be fixed by configuring submodule.fetchJobs I\n> > think this change should be documented - or reverted back to it's\n> > previous default of 1\n>\n> The commit in question does not look like it _wanted_ to change the\n> default; rather, it appears to me that it wanted to be bug-to-bug\n> compatible with the original, and any such change of behaviour is\n> entirely unintended.\n\nIndeed.\n\n> I think the attached may be sufficient to change the default\n> max_jobs back to 1.\n\nI think so, too. I can wrap it into a commit with a proper message.\n\n>\n> By the way, is there a place where we document that the default\n> value for fetchjobs, when unconfigured, is 1?\n\n`man git config`\n\n submodule.fetchJobs\n Specifies how many submodules are fetched/cloned at the\n same time. A positive integer allows up to that number of\n submodules fetched in parallel. A value of 0 will give some\n reasonable default. If unset, it defaults to 1.\n\nand that seems to be the only place, other places only reference\nthis place:\n\n Documentation$ git grep submodule.fetch\n config/submodule.txt:66:submodule.fetchJobs::\n git-clone.txt:259: Defaults to the `submodule.fetchJobs` option.\n git-submodule.txt:408: Defaults to the `submodule.fetchJobs` option.\n\nThe behavior of that seems to have been there since the beginning of\na028a1930c (fetching submodules: respect `submodule.fetchJobs`\nconfig option, 2016-02-29)\n\n\n> If we are not making\n> such a concrete promise, then I would think it is OK to update the\n> default without any fanfare, as long as we have good reasons to do\n> so. For this particular one, however, as I already said, I do not\n> think we wanted to change the default to unlimited or anything like\n> that, so...\n\nWe definitely want the diff below as a proper patch.\n\n> builtin/submodule--helper.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 789d00d87d..e8cdf84f1c 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1552,7 +1552,7 @@ struct submodule_update_clone {\n> #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \\\n> SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \\\n> NULL, NULL, NULL, \\\n> - NULL, 0, 0, 0, NULL, 0, 0, 0}\n> + NULL, 0, 0, 0, NULL, 0, 0, 1}\n>\n>\n> static void next_submodule_warn_missing(struct submodule_update_clone *suc,\n"},{"id":"365282","messageId":"20181213190248.247083-1-sbeller@google.com","threadId":"50023","inReplyTo":"CAGZ79kYsk8YEUUhMVF9fBC++fop3CPyobXTgHTuF2Lgikf9CJA@mail.gmail.com","subject":"[PATCH] submodule update: run at most one fetch job unless otherwise set","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-13T19:02:48Z","receivedAt":"2018-12-13T19:02:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Junio C Hamano \n\nIn a028a1930c (fetching submodules: respect `submodule.fetchJobs`\nconfig option, 2016-02-29), we made sure to keep the default behavior\nof a fetching at most one submodule at once when not setting the\nnewly introduced `submodule.fetchJobs` config.\n\nThis regressed in 90efe595c5 (builtin/submodule--helper: factor\nout submodule updating, 2018-08-03). Fix it.\n\nReported-by: Sjon Hortensius \nSigned-off-by: Junio C Hamano \nSigned-off-by: Stefan Beller \n---\n builtin/submodule--helper.c | 2 +-\n t/t5526-fetch-submodules.sh | 2 ++\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex d38113a31a..1f8a4a9d52 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1551,7 +1551,7 @@ struct submodule_update_clone {\n #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \\\n \tSUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \\\n \tNULL, NULL, NULL, \\\n-\tNULL, 0, 0, 0, NULL, 0, 0, 0}\n+\tNULL, 0, 0, 0, NULL, 0, 0, 1}\n \n \n static void next_submodule_warn_missing(struct submodule_update_clone *suc,\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 6c2f9b2ba2..a0317556c6 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -524,6 +524,8 @@ test_expect_success 'fetching submodules respects parallel settings' '\n \tgit config fetch.recurseSubmodules true &&\n \t(\n \t\tcd downstream &&\n+\t\tGIT_TRACE=$(pwd)/trace.out git fetch &&\n+\t\tgrep \"1 tasks\" trace.out &&\n \t\tGIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 &&\n \t\tgrep \"7 tasks\" trace.out &&\n \t\tgit config submodule.fetchJobs 8 &&\n-- \n2.20.0.rc2.230.gc28305e538\n\n"},{"id":"365283","messageId":"CAPig+cRHcZTS3QNXMbfFk+V2Z+HhWnXAKfkcWMePbmjDK+oKUQ@mail.gmail.com","threadId":"50023","inReplyTo":"20181213190248.247083-1-sbeller@google.com","subject":"Re: [PATCH] submodule update: run at most one fetch job unless otherwise set","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-12-13T19:04:14Z","receivedAt":"2018-12-13T19:04:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 13, 2018 at 2:03 PM Stefan Beller wrote:\n> In a028a1930c (fetching submodules: respect `submodule.fetchJobs`\n> config option, 2016-02-29), we made sure to keep the default behavior\n> of a fetching at most one submodule at once when not setting the\n\ns/of a/of/\n\n> newly introduced `submodule.fetchJobs` config.\n>\n> This regressed in 90efe595c5 (builtin/submodule--helper: factor\n> out submodule updating, 2018-08-03). Fix it.\n>\n> Reported-by: Sjon Hortensius \n> Signed-off-by: Junio C Hamano \n> Signed-off-by: Stefan Beller \n"},{"id":"365321","messageId":"xmqqwoocddiw.fsf@gitster-ct.c.googlers.com","threadId":"50023","inReplyTo":"20181213190248.247083-1-sbeller@google.com","subject":"Re: [PATCH] submodule update: run at most one fetch job unless otherwise set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-14T02:53:59Z","receivedAt":"2018-12-14T02:54:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> From: Junio C Hamano \n>\n> In a028a1930c (fetching submodules: respect `submodule.fetchJobs`\n> config option, 2016-02-29), we made sure to keep the default behavior\n> of a fetching at most one submodule at once when not setting the\n> newly introduced `submodule.fetchJobs` config.\n>\n> This regressed in 90efe595c5 (builtin/submodule--helper: factor\n> out submodule updating, 2018-08-03). Fix it.\n>\n> Reported-by: Sjon Hortensius \n> Signed-off-by: Junio C Hamano \n> Signed-off-by: Stefan Beller \n> ---\n\nThanks for tying the loose ends.\n\nWe may want to convert the _INIT macro to use the designated\ninitializers; I had to count the fields twice to make sure I was\ntweaking the right one. \n\nIt did not help that I saw, before looking at the current code, that\n90efe595c5 added one field at the end to the struct but did not\ntouch _INIT macro at all. That made me guess that max_jobs=0 was\ndue to _missing_ initialization value, leading me to an incorrect\nfix to append an extra 1 at the end, but that was bogus. The\nmissing initialization left by 90efe595 (\"builtin/submodule--helper:\nfactor out submodule updating\", 2018-08-03) was silently fixed by\nf1d15713 (\"builtin/submodule--helper: store update_clone information\nin a struct\", 2018-08-03), I think, so replacing the 0 at the end is\n1 happens to be the right fix, but with designated initializers, all\nthese confusions are more easily avoided.\n\n\n> builtin/submodule--helper.c | 2 +-\n> t/t5526-fetch-submodules.sh | 2 ++\n> 2 files changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index d38113a31a..1f8a4a9d52 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1551,7 +1551,7 @@ struct submodule_update_clone {\n> #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \\\n> \tSUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \\\n> \tNULL, NULL, NULL, \\\n> -\tNULL, 0, 0, 0, NULL, 0, 0, 0}\n> +\tNULL, 0, 0, 0, NULL, 0, 0, 1}\n> \n> \n> static void next_submodule_warn_missing(struct submodule_update_clone *suc,\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index 6c2f9b2ba2..a0317556c6 100755\n> --- a/t/t5526-fetch-submodules.sh\n> +++ b/t/t5526-fetch-submodules.sh\n> @@ -524,6 +524,8 @@ test_expect_success 'fetching submodules respects parallel settings' '\n> \tgit config fetch.recurseSubmodules true &&\n> \t(\n> \t\tcd downstream &&\n> +\t\tGIT_TRACE=$(pwd)/trace.out git fetch &&\n> +\t\tgrep \"1 tasks\" trace.out &&\n> \t\tGIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 &&\n> \t\tgrep \"7 tasks\" trace.out &&\n> \t\tgit config submodule.fetchJobs 8 &&\n"}]}