fetch --multiple: respect --jobs=
cover fetch --multiple: respect --jobs=
I saw with sadness that pd/fetch-jobs went nowhere, and read in the most recent What's Cooking mail that it was even dropped.
This is my attempt to resurrect the idea (although without the overhead of trying to support a first-class UI to control submodule and multiple-remote fetches independently, of which I was a rather outspoken opponent).
To make things a bit safer, this patch uses the --end-of-options marker, and is therefore based on top of jk/eoo.
Johannes Schindelin (1):
fetch: let --jobs=<n> parallelize --multiple, too
Documentation/config/fetch.txt | 10 +++ Documentation/fetch-options.txt | 13 ++-- builtin/fetch.c | 124 +++++++++++++++++++++++++++----- t/t5514-fetch-multiple.sh | 11 +++ 4 files changed, 137 insertions(+), 21 deletions(-)
base-commit: 67feca3b1c45a51b204253039139b46cc07e145f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-369%2Fdscho%2Ffetch-jobs-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-369/dscho/fetch-jobs-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/369
Signature · 1 line
gitgitgadget
PATCH 1/1 fetch: let --jobs=<n> parallelize --multiple, too
From: Johannes Schindelin <[email protected]>
So far, --jobs=<n> only parallelizes submodule fetches/clones, not --multiple fetches, which is unintuitive, given that the option's name does not say anything about submodules in particular.
Let's change that. With this patch, also fetches from multiple remotes are parallelized.
For backwards-compatibility (and to prepare for a use case where submodule and multiple-remote fetches may need different parallelization limits), the config setting submodule.fetchJobs still only controls the submodule part of git fetch, while the newly-introduced setting fetch.parallel controls both (but can be overridden for submodules with submodule.fetchJobs).
--- Documentation/config/fetch.txt | 10 +++ Documentation/fetch-options.txt | 13 ++-- builtin/fetch.c | 124 +++++++++++++++++++++++++++----- t/t5514-fetch-multiple.sh | 11 +++ 4 files changed, 137 insertions(+), 21 deletions(-)
Documentation/config/fetch.txt +10 -0
@@ -68,3 +68,13 @@ fetch.showForcedUpdates:: Set to false to enable `--no-show-forced-updates` in linkgit:git-fetch[1] and linkgit:git-pull[1] commands. Defaults to true. + +fetch.parallel:: + Specifies the maximal number of fetch operations to be run in parallel + at a time (submodules, or remotes when the `--multiple` option of + linkgit:git-fetch[1] is in effect). ++ +A value of 0 will give some reasonable default. If unset, it defaults to 1. ++ +For submodules, this setting can be overridden using the `submodule.fetchJobs` +config setting.
Documentation/fetch-options.txt +9 -4
@@ -160,10 +160,15 @@ ifndef::git-pull[] -j:: --jobs=<n>:: - Number of parallel children to be used for fetching submodules. - Each will fetch from different submodules, such that fetching many - submodules will be faster. By default submodules will be fetched - one at a time. + Number of parallel children to be used for all forms of fetching. ++ +If the `--multiple` option was specified, the different remotes will be fetched +in parallel. If multiple submodules are fetched, they will be fetched in +parallel. To control them independently, use the config settings +`fetch.parallel` and `submodule.fetchJobs` (see linkgit:git-config[1]). ++ +Typically, parallel recursive and multi-remote fetches will be faster. By +default fetches are performed sequentially, not in parallel. --no-recurse-submodules:: Disable recursive fetching of submodules (this has the same effect as
builtin/fetch.c +107 -17
183 lines not on this page · raw message
t/t5514-fetch-multiple.sh +11 -0
@@ -183,4 +183,15 @@ test_expect_success 'git fetch --all --tags' ' test_cmp expect test8/output ' +test_expect_success 'parallel' ' + git remote add one ./bogus1 && + git remote add two ./bogus2 && + + test_must_fail env GIT_TRACE="$PWD/trace" \ + git fetch --jobs=2 --multiple one two 2>err && + grep "2 tasks" trace && + grep "one.*128" err && + grep "two.*128" err +' + test_done
Signature · 1 line
gitgitgadget
Re: [PATCH 1/1] fetch: let --jobs=<n> parallelize --multiple, too
"Johannes Schindelin via GitGitGadget" <[email protected]>
Review
Quoted 7 lines · writes:
(quoted diff) +7
-0+test_expect_success 'parallel' ' + git remote add one ./bogus1 && + git remote add two ./bogus2 && + + test_must_fail env GIT_TRACE="$PWD/trace" \ + git fetch --jobs=2 --multiple one two 2>err && + grep "2 tasks" trace &&
I think this one expects to match this in run-command.c:
trace_printf("run_processes_parallel: preparing to run up to %d tasks", n);Review
Quoted 2 lines
(quoted diff) +2
-0+ grep "one.*128" err && + grep "two.*128" err
and these expect to match this in fetch.c
strbuf_addf(out, _("could not fetch '%s' (exit code: %d)\n"),It would have been nice to fellow contributors, if the grep patterns were written a bit more tightly. It would allow people who debug test failure to more easily identify which message the patterns are trying to catch.
In any case, the latter two needs to be guarded against gettext-poison, I would think. Without addressing the vagueness of the pattern, at least the following needs to be squashed to help the CI.
Thanks.
--- t/t5514-fetch-multiple.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
t/t5514-fetch-multiple.sh +2 -2
@@ -190,8 +190,8 @@ test_expect_success 'parallel' ' test_must_fail env GIT_TRACE="$PWD/trace" \ git fetch --jobs=2 --multiple one two 2>err && grep "2 tasks" trace && - grep "one.*128" err && - grep "two.*128" err + test_i18ngrep "one.*128" err && + test_i18ngrep "two.*128" err ' test_done
Signature · 1 line
2.23.0-686-g3bf927a9c0
Re: [PATCH 1/1] fetch: let --jobs=<n> parallelize --multiple, too
Hi Junio,
Quoted 20 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Review
Quoted 7 lines · writes:
(quoted diff) +7
-0+test_expect_success 'parallel' ' + git remote add one ./bogus1 && + git remote add two ./bogus2 && + + test_must_fail env GIT_TRACE="$PWD/trace" \ + git fetch --jobs=2 --multiple one two 2>err && + grep "2 tasks" trace &&I think this one expects to match this in
run-command.c:trace_printf("run_processes_parallel: preparing to run up to %d tasks", n);Review
Quoted 2 lines
(quoted diff) +2
-0+ grep "one.*128" err && + grep "two.*128" errand these expect to match this in
fetch.c
Quoted 6 lines
strbuf_addf(out, _("could not fetch '%s' (exit code: %d)\n"),It would have been nice to fellow contributors, if the grep patterns were written a bit more tightly. It would allow people who debug test failure to more easily identify which message the patterns are trying to catch.
This is a two-edged sword: when those messages change (for whatever reason), the regression test will fail, too, but it actually wants to test the parallel fetch, not the trace message of run_processes_parallel.
So I tried to prevent such an unactionable regression test failure. But I see your reasoning, and I now thought about it and consider those error messages to be rather stable.
Will fix.
Quoted 4 lines
In any case, the latter two needs to be guarded against gettext-poison, I would think. Without addressing the vagueness of the pattern, at least the following needs to be squashed to help the CI.
Indeed. I missed this because the GitGitGadget PR build was all green. My guess is that I messed up the definition of that PR build (it is _not_ what's in azure-pipelines.yml because that would not work correctly when PRs target older commits). I _think_ I fixed it by setting export GIT_TEST_GETTEXT_POISON=true explicitly (previously I only set the jobname, expecting ci/run-build-and-tests.sh to pick up on that.
Thanks,
Dscho
Quoted 26 lines
Thanks.
--- t/t5514-fetch-multiple.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)t/t5514-fetch-multiple.sh +2
-3@@ -190,8 +190,8 @@ test_expect_success 'parallel' ' test_must_fail env GIT_TRACE="$PWD/trace" \ git fetch --jobs=2 --multiple one two 2>err && grep "2 tasks" trace && - grep "one.*128" err && - grep "two.*128" err + test_i18ngrep "one.*128" err && + test_i18ngrep "two.*128" err ' test_done --2.23.0-686-g3bf927a9c0
v2 cover fetch --multiple: respect --jobs=
In reply to Johannes Schindelin via GitGitGadgetI saw with sadness that pd/fetch-jobs went nowhere, and read in the most recent What's Cooking mail that it was even dropped.
This is my attempt to resurrect the idea (although without the overhead of trying to support a first-class UI to control submodule and multiple-remote fetches independently, of which I was a rather outspoken opponent).
To make things a bit safer, this patch uses the --end-of-options marker, and is therefore based on top of jk/eoo.
Changes since v1:
* The regression test now passes even under GETTEXT_POISON.
* The needles used in the regression test are now more indicative of the code producing them.
Johannes Schindelin (1):
fetch: let --jobs=<n> parallelize --multiple, too
Documentation/config/fetch.txt | 10 +++ Documentation/fetch-options.txt | 13 ++-- builtin/fetch.c | 124 +++++++++++++++++++++++++++----- t/t5514-fetch-multiple.sh | 11 +++ 4 files changed, 137 insertions(+), 21 deletions(-)
base-commit: 67feca3b1c45a51b204253039139b46cc07e145f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-369%2Fdscho%2Ffetch-jobs-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-369/dscho/fetch-jobs-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/369
Range-diff vs v1:
1: 818936f1e0 ! 1: 93a155a000 fetch: let --jobs=<n> parallelize --multiple, too
@@ -267,9 +267,9 @@
+
+ test_must_fail env GIT_TRACE="$PWD/trace" \
+ git fetch --jobs=2 --multiple one two 2>err &&
-+ grep "2 tasks" trace &&
-+ grep "one.*128" err &&
-+ grep "two.*128" err
++ grep "preparing to run up to 2 tasks" trace &&
++ test_i18ngrep "could not fetch .one.*128" err &&
++ test_i18ngrep "could not fetch .two.*128" err
+'
+
test_doneSignature · 1 line
gitgitgadget
PATCH v2 1/1 fetch: let --jobs=<n> parallelize --multiple, too
From: Johannes Schindelin <[email protected]>
So far, --jobs=<n> only parallelizes submodule fetches/clones, not --multiple fetches, which is unintuitive, given that the option's name does not say anything about submodules in particular.
Let's change that. With this patch, also fetches from multiple remotes are parallelized.
For backwards-compatibility (and to prepare for a use case where submodule and multiple-remote fetches may need different parallelization limits), the config setting submodule.fetchJobs still only controls the submodule part of git fetch, while the newly-introduced setting fetch.parallel controls both (but can be overridden for submodules with submodule.fetchJobs).
--- Documentation/config/fetch.txt | 10 +++ Documentation/fetch-options.txt | 13 ++-- builtin/fetch.c | 124 +++++++++++++++++++++++++++----- t/t5514-fetch-multiple.sh | 11 +++ 4 files changed, 137 insertions(+), 21 deletions(-)
Documentation/config/fetch.txt +10 -0
@@ -68,3 +68,13 @@ fetch.showForcedUpdates:: Set to false to enable `--no-show-forced-updates` in linkgit:git-fetch[1] and linkgit:git-pull[1] commands. Defaults to true. + +fetch.parallel:: + Specifies the maximal number of fetch operations to be run in parallel + at a time (submodules, or remotes when the `--multiple` option of + linkgit:git-fetch[1] is in effect). ++ +A value of 0 will give some reasonable default. If unset, it defaults to 1. ++ +For submodules, this setting can be overridden using the `submodule.fetchJobs` +config setting.
Documentation/fetch-options.txt +9 -4
@@ -160,10 +160,15 @@ ifndef::git-pull[] -j:: --jobs=<n>:: - Number of parallel children to be used for fetching submodules. - Each will fetch from different submodules, such that fetching many - submodules will be faster. By default submodules will be fetched - one at a time. + Number of parallel children to be used for all forms of fetching. ++ +If the `--multiple` option was specified, the different remotes will be fetched +in parallel. If multiple submodules are fetched, they will be fetched in +parallel. To control them independently, use the config settings +`fetch.parallel` and `submodule.fetchJobs` (see linkgit:git-config[1]). ++ +Typically, parallel recursive and multi-remote fetches will be faster. By +default fetches are performed sequentially, not in parallel. --no-recurse-submodules:: Disable recursive fetching of submodules (this has the same effect as
builtin/fetch.c +107 -17
183 lines not on this page · raw message
t/t5514-fetch-multiple.sh +11 -0
@@ -183,4 +183,15 @@ test_expect_success 'git fetch --all --tags' ' test_cmp expect test8/output ' +test_expect_success 'parallel' ' + git remote add one ./bogus1 && + git remote add two ./bogus2 && + + test_must_fail env GIT_TRACE="$PWD/trace" \ + git fetch --jobs=2 --multiple one two 2>err && + grep "preparing to run up to 2 tasks" trace && + test_i18ngrep "could not fetch .one.*128" err && + test_i18ngrep "could not fetch .two.*128" err +' + test_done
Signature · 1 line
gitgitgadget
Re: [PATCH v2 1/1] fetch: let --jobs=<n> parallelize --multiple, too
In reply to a message not in the archiveHi Junio,
Quoted 21 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
t/t5514-fetch-multiple.sh test_expect_success 'git fetch --all --tags' ' · line 197
Quoted 12 lines · writes:
t/t5514-fetch-multiple.sh +11
-0+test_expect_success 'parallel' ' + git remote add one ./bogus1 && + git remote add two ./bogus2 && + + test_must_fail env GIT_TRACE="$PWD/trace" \ + git fetch --jobs=2 --multiple one two 2>err && + grep "preparing to run up to 2 tasks" trace && + test_i18ngrep "could not fetch .one.*128" err && + test_i18ngrep "could not fetch .two.*128" err +' + test_doneThanks. I think it is much better to prepare these tests like this patch does to be broken when phrasing changes---that would give feedback and confidence to the person who is changing the message and/or the logic to emit the message.
Quoted 3 lines
Where does the constant 128 come from, by the way? If it is from
errno.hthen we will soon hear breakage report from NonStop folks, I predict ;-)
It comes from die():
-- snip --
static NORETURN void die_builtin(const char *err, va_list params)
{
/*
* We call this trace2 function first and expect it to va_copy 'params'
* before using it (because an 'ap' can only be walked once).
*/
trace2_cmd_error_va(err, params);
vreportf("fatal: ", err, params);
exit(128);
}-- snap --
Thanks,
Dscho
Re: [PATCH v2 1/1] fetch: let --jobs=<n> parallelize --multiple, too
Quoted 7 lines · Johannes Schindelin
Hi Junio,
...Quoted 5 lines
Thanks. I think it is much better to prepare these tests like this patch does to be broken when phrasing changes---that would give feedback and confidence to the person who is changing the message and/or the logic to emit the message.
Quoted 7 lines
Quoted 3 lines
Where does the constant 128 come from, by the way? If it is from
errno.hthen we will soon hear breakage report from NonStop folks, I predict ;-)It comes from
die():
...exit(128);
OK, so hopefully we wouldn't see any platform specific variations. Good.
Thanks for digging.
Re: [PATCH v2 1/1] fetch: let --jobs=<n> parallelize --multiple, too
Hi Junio,
Quoted 13 lines · Junio C Hamano
Quoted 11 lines · Johannes Schindelin
Hi Junio,
...Quoted 8 lines
Thanks. I think it is much better to prepare these tests like this patch does to be broken when phrasing changes---that would give feedback and confidence to the person who is changing the message and/or the logic to emit the message.
Where does the constant 128 come from, by the way? If it is from
errno.hthen we will soon hear breakage report from NonStop folks, I predict ;-)
Quoted 5 lines
Quoted 3 lines
It comes from
die():
...exit(128);OK, so hopefully we wouldn't see any platform specific variations.
I am certain of it, as the matched 128 is not printed implicitly, it is printed by these two lines that I added in this patch (as part of fetch_finished()):
+ strbuf_addf(out, _("could not fetch '%s' (exit code: %d)\n"),
+ remote, result);
Thanks,
Dscho
Re: [PATCH v2 1/1] fetch: let --jobs=<n> parallelize --multiple, too
Hi Junio,
Quoted 25 lines · Johannes Schindelin
Quoted 13 lines · Junio C Hamano
Quoted 11 lines · Johannes Schindelin
Hi Junio,
...Quoted 8 lines
Thanks. I think it is much better to prepare these tests like this patch does to be broken when phrasing changes---that would give feedback and confidence to the person who is changing the message and/or the logic to emit the message.
Where does the constant 128 come from, by the way? If it is from
errno.hthen we will soon hear breakage report from NonStop folks, I predict ;-)Quoted 5 lines
Quoted 3 lines
It comes from
die():
...exit(128);OK, so hopefully we wouldn't see any platform specific variations.
I am certain of it, as the matched
128is not printed implicitly, it is printed by these two lines that I added in this patch (as part offetch_finished()):
builtin/fetch.c static void add_options_to_argv(struct argv_array *argv) · line 1515
Quoted 2 lines
builtin/fetch.c +2
-0+strbuf_addf (out,_ ("could not fetch '%s' (exit code: %d)\n" ), + remote, result);
Oh, and I forgot to say: the exit code is expected to be 128 because the added regression test case uses a non-existing path to fetch from, essentially saying "No, git fetch, I expect you to die()".
Ciao,
Dscho