* 2.20.0 - Undocumented change in submodule update wrt # parallel jobs @ 2018-12-13 9:15 Sjon Hortensius 2018-12-13 14:02 ` Ævar Arnfjörð Bjarmason 2018-12-13 14:17 ` Junio C Hamano 0 siblings, 2 replies; 7+ messages in thread From: Sjon Hortensius @ 2018-12-13 9:15 UTC (permalink / raw) To: git When switching to 2.20 our `git submodule update' (which clones through ssh) broke because our ssh-server rejected the ~20 simultaneous connections the git-client makes. This seems to be caused by a (possibly unintended) change in https://github.com/git/git/commit/90efe595c53f4bb1851371344c35eff71f604d2b which removed the default of max_jobs=1 While this can easily be fixed by configuring submodule.fetchJobs I think this change should be documented - or reverted back to it's previous default of 1 -- Kind regards, Sjon Hortensius | Parse Software Development B.V. | http://www.parse.nl ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: 2.20.0 - Undocumented change in submodule update wrt # parallel jobs 2018-12-13 9:15 2.20.0 - Undocumented change in submodule update wrt # parallel jobs Sjon Hortensius @ 2018-12-13 14:02 ` Ævar Arnfjörð Bjarmason 2018-12-13 14:17 ` Junio C Hamano 1 sibling, 0 replies; 7+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-12-13 14:02 UTC (permalink / raw) To: Sjon Hortensius; +Cc: git On Thu, Dec 13 2018, Sjon Hortensius wrote: > When switching to 2.20 our `git submodule update' (which clones > through ssh) broke because our ssh-server rejected the ~20 > simultaneous connections the git-client makes. This seems to be caused > by a (possibly unintended) change in > https://github.com/git/git/commit/90efe595c53f4bb1851371344c35eff71f604d2b > which removed the default of max_jobs=1 > > While this can easily be fixed by configuring submodule.fetchJobs I > think this change should be documented - or reverted back to it's > previous default of 1 Just to add to this. SSH-ing with -j<ncores> (which I assume that ~20 is) is going to run into MaxStartups in sshd_config, which is especially annoying as it's pre-ssh-auth, so the server doesn't even get "so and so tried to login", it's just dropped as a potential DoS attack. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: 2.20.0 - Undocumented change in submodule update wrt # parallel jobs 2018-12-13 9:15 2.20.0 - Undocumented change in submodule update wrt # parallel jobs Sjon Hortensius 2018-12-13 14:02 ` Ævar Arnfjörð Bjarmason @ 2018-12-13 14:17 ` Junio C Hamano 2018-12-13 18:50 ` Stefan Beller 1 sibling, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2018-12-13 14:17 UTC (permalink / raw) To: Stefan Beller; +Cc: git, Sjon Hortensius Sjon Hortensius <sjon@parse.nl> writes: > When switching to 2.20 our `git submodule update' (which clones > through ssh) broke because our ssh-server rejected the ~20 > simultaneous connections the git-client makes. This seems to be caused > by a (possibly unintended) change in > https://github.com/git/git/commit/90efe595c53f4bb1851371344c35eff71f604d2b > which removed the default of max_jobs=1 > > While this can easily be fixed by configuring submodule.fetchJobs I > think this change should be documented - or reverted back to it's > previous default of 1 The commit in question does not look like it _wanted_ to change the default; rather, it appears to me that it wanted to be bug-to-bug compatible with the original, and any such change of behaviour is entirely unintended. I think the attached may be sufficient to change the default max_jobs back to 1. By the way, is there a place where we document that the default value for fetchjobs, when unconfigured, is 1? If we are not making such a concrete promise, then I would think it is OK to update the default without any fanfare, as long as we have good reasons to do so. For this particular one, however, as I already said, I do not think we wanted to change the default to unlimited or anything like that, so... builtin/submodule--helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 789d00d87d..e8cdf84f1c 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1552,7 +1552,7 @@ struct submodule_update_clone { #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \ SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \ NULL, NULL, NULL, \ - NULL, 0, 0, 0, NULL, 0, 0, 0} + NULL, 0, 0, 0, NULL, 0, 0, 1} static void next_submodule_warn_missing(struct submodule_update_clone *suc, ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: 2.20.0 - Undocumented change in submodule update wrt # parallel jobs 2018-12-13 14:17 ` Junio C Hamano @ 2018-12-13 18:50 ` Stefan Beller 2018-12-13 19:02 ` [PATCH] submodule update: run at most one fetch job unless otherwise set Stefan Beller 0 siblings, 1 reply; 7+ messages in thread From: Stefan Beller @ 2018-12-13 18:50 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, sjon On Thu, Dec 13, 2018 at 6:17 AM Junio C Hamano <gitster@pobox.com> wrote: > > Sjon Hortensius <sjon@parse.nl> writes: > > > When switching to 2.20 our `git submodule update' (which clones > > through ssh) broke because our ssh-server rejected the ~20 > > simultaneous connections the git-client makes. This seems to be caused > > by a (possibly unintended) change in > > https://github.com/git/git/commit/90efe595c53f4bb1851371344c35eff71f604d2b > > which removed the default of max_jobs=1 > > > > While this can easily be fixed by configuring submodule.fetchJobs I > > think this change should be documented - or reverted back to it's > > previous default of 1 > > The commit in question does not look like it _wanted_ to change the > default; rather, it appears to me that it wanted to be bug-to-bug > compatible with the original, and any such change of behaviour is > entirely unintended. Indeed. > I think the attached may be sufficient to change the default > max_jobs back to 1. I think so, too. I can wrap it into a commit with a proper message. > > By the way, is there a place where we document that the default > value for fetchjobs, when unconfigured, is 1? `man git config` submodule.fetchJobs Specifies how many submodules are fetched/cloned at the same time. A positive integer allows up to that number of submodules fetched in parallel. A value of 0 will give some reasonable default. If unset, it defaults to 1. and that seems to be the only place, other places only reference this place: Documentation$ git grep submodule.fetch config/submodule.txt:66:submodule.fetchJobs:: git-clone.txt:259: Defaults to the `submodule.fetchJobs` option. git-submodule.txt:408: Defaults to the `submodule.fetchJobs` option. The behavior of that seems to have been there since the beginning of a028a1930c (fetching submodules: respect `submodule.fetchJobs` config option, 2016-02-29) > If we are not making > such a concrete promise, then I would think it is OK to update the > default without any fanfare, as long as we have good reasons to do > so. For this particular one, however, as I already said, I do not > think we wanted to change the default to unlimited or anything like > that, so... We definitely want the diff below as a proper patch. > builtin/submodule--helper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 789d00d87d..e8cdf84f1c 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -1552,7 +1552,7 @@ struct submodule_update_clone { > #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \ > SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \ > NULL, NULL, NULL, \ > - NULL, 0, 0, 0, NULL, 0, 0, 0} > + NULL, 0, 0, 0, NULL, 0, 0, 1} > > > static void next_submodule_warn_missing(struct submodule_update_clone *suc, ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] submodule update: run at most one fetch job unless otherwise set 2018-12-13 18:50 ` Stefan Beller @ 2018-12-13 19:02 ` Stefan Beller 2018-12-13 19:04 ` Eric Sunshine 2018-12-14 2:53 ` Junio C Hamano 0 siblings, 2 replies; 7+ messages in thread From: Stefan Beller @ 2018-12-13 19:02 UTC (permalink / raw) To: sbeller; +Cc: git, gitster, sjon From: Junio C Hamano <gitster@pobox.com> In a028a1930c (fetching submodules: respect `submodule.fetchJobs` config option, 2016-02-29), we made sure to keep the default behavior of a fetching at most one submodule at once when not setting the newly introduced `submodule.fetchJobs` config. This regressed in 90efe595c5 (builtin/submodule--helper: factor out submodule updating, 2018-08-03). Fix it. Reported-by: Sjon Hortensius <sjon@parse.nl> Signed-off-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Stefan Beller <sbeller@google.com> --- builtin/submodule--helper.c | 2 +- t/t5526-fetch-submodules.sh | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index d38113a31a..1f8a4a9d52 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1551,7 +1551,7 @@ struct submodule_update_clone { #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \ SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \ NULL, NULL, NULL, \ - NULL, 0, 0, 0, NULL, 0, 0, 0} + NULL, 0, 0, 0, NULL, 0, 0, 1} static void next_submodule_warn_missing(struct submodule_update_clone *suc, diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh index 6c2f9b2ba2..a0317556c6 100755 --- a/t/t5526-fetch-submodules.sh +++ b/t/t5526-fetch-submodules.sh @@ -524,6 +524,8 @@ test_expect_success 'fetching submodules respects parallel settings' ' git config fetch.recurseSubmodules true && ( cd downstream && + GIT_TRACE=$(pwd)/trace.out git fetch && + grep "1 tasks" trace.out && GIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 && grep "7 tasks" trace.out && git config submodule.fetchJobs 8 && -- 2.20.0.rc2.230.gc28305e538 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] submodule update: run at most one fetch job unless otherwise set 2018-12-13 19:02 ` [PATCH] submodule update: run at most one fetch job unless otherwise set Stefan Beller @ 2018-12-13 19:04 ` Eric Sunshine 2018-12-14 2:53 ` Junio C Hamano 1 sibling, 0 replies; 7+ messages in thread From: Eric Sunshine @ 2018-12-13 19:04 UTC (permalink / raw) To: Stefan Beller; +Cc: Git List, Junio C Hamano, sjon On Thu, Dec 13, 2018 at 2:03 PM Stefan Beller <sbeller@google.com> wrote: > In a028a1930c (fetching submodules: respect `submodule.fetchJobs` > config option, 2016-02-29), we made sure to keep the default behavior > of a fetching at most one submodule at once when not setting the s/of a/of/ > newly introduced `submodule.fetchJobs` config. > > This regressed in 90efe595c5 (builtin/submodule--helper: factor > out submodule updating, 2018-08-03). Fix it. > > Reported-by: Sjon Hortensius <sjon@parse.nl> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: Stefan Beller <sbeller@google.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] submodule update: run at most one fetch job unless otherwise set 2018-12-13 19:02 ` [PATCH] submodule update: run at most one fetch job unless otherwise set Stefan Beller 2018-12-13 19:04 ` Eric Sunshine @ 2018-12-14 2:53 ` Junio C Hamano 1 sibling, 0 replies; 7+ messages in thread From: Junio C Hamano @ 2018-12-14 2:53 UTC (permalink / raw) To: Stefan Beller; +Cc: git, sjon Stefan Beller <sbeller@google.com> writes: > From: Junio C Hamano <gitster@pobox.com> > > In a028a1930c (fetching submodules: respect `submodule.fetchJobs` > config option, 2016-02-29), we made sure to keep the default behavior > of a fetching at most one submodule at once when not setting the > newly introduced `submodule.fetchJobs` config. > > This regressed in 90efe595c5 (builtin/submodule--helper: factor > out submodule updating, 2018-08-03). Fix it. > > Reported-by: Sjon Hortensius <sjon@parse.nl> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: Stefan Beller <sbeller@google.com> > --- Thanks for tying the loose ends. We may want to convert the _INIT macro to use the designated initializers; I had to count the fields twice to make sure I was tweaking the right one. It did not help that I saw, before looking at the current code, that 90efe595c5 added one field at the end to the struct but did not touch _INIT macro at all. That made me guess that max_jobs=0 was due to _missing_ initialization value, leading me to an incorrect fix to append an extra 1 at the end, but that was bogus. The missing initialization left by 90efe595 ("builtin/submodule--helper: factor out submodule updating", 2018-08-03) was silently fixed by f1d15713 ("builtin/submodule--helper: store update_clone information in a struct", 2018-08-03), I think, so replacing the 0 at the end is 1 happens to be the right fix, but with designated initializers, all these confusions are more easily avoided. > builtin/submodule--helper.c | 2 +- > t/t5526-fetch-submodules.sh | 2 ++ > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index d38113a31a..1f8a4a9d52 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -1551,7 +1551,7 @@ struct submodule_update_clone { > #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \ > SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \ > NULL, NULL, NULL, \ > - NULL, 0, 0, 0, NULL, 0, 0, 0} > + NULL, 0, 0, 0, NULL, 0, 0, 1} > > > static void next_submodule_warn_missing(struct submodule_update_clone *suc, > diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh > index 6c2f9b2ba2..a0317556c6 100755 > --- a/t/t5526-fetch-submodules.sh > +++ b/t/t5526-fetch-submodules.sh > @@ -524,6 +524,8 @@ test_expect_success 'fetching submodules respects parallel settings' ' > git config fetch.recurseSubmodules true && > ( > cd downstream && > + GIT_TRACE=$(pwd)/trace.out git fetch && > + grep "1 tasks" trace.out && > GIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 && > grep "7 tasks" trace.out && > git config submodule.fetchJobs 8 && ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-12-14 2:54 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-13 9:15 2.20.0 - Undocumented change in submodule update wrt # parallel jobs Sjon Hortensius 2018-12-13 14:02 ` Ævar Arnfjörð Bjarmason 2018-12-13 14:17 ` Junio C Hamano 2018-12-13 18:50 ` Stefan Beller 2018-12-13 19:02 ` [PATCH] submodule update: run at most one fetch job unless otherwise set Stefan Beller 2018-12-13 19:04 ` Eric Sunshine 2018-12-14 2:53 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).