{"thread":{"id":"51949","subject":"[PATCH 0/1] fetch --multiple: respect --jobs=","startedAt":"2019-10-01T11:53:40Z","lastAt":"2019-10-07T10:17:53Z","messageCount":10,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"383200","messageId":"pull.369.git.gitgitgadget@gmail.com","threadId":"51949","inReplyTo":null,"subject":"[PATCH 0/1] fetch --multiple: respect --jobs=","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-01T11:53:36Z","receivedAt":"2019-10-01T11:53:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I saw with sadness that pd/fetch-jobs went nowhere, and read in the most\nrecent What's Cooking mail that it was even dropped.\n\nThis is my attempt to resurrect the idea (although without the overhead of\ntrying to support a first-class UI to control submodule and multiple-remote\nfetches independently, of which I was a rather outspoken opponent).\n\nTo make things a bit safer, this patch uses the --end-of-options marker, and\nis therefore based on top of jk/eoo.\n\nJohannes Schindelin (1):\n fetch: let --jobs= parallelize --multiple, too\n\n Documentation/config/fetch.txt | 10 +++\n Documentation/fetch-options.txt | 13 ++--\n builtin/fetch.c | 124 +++++++++++++++++++++++++++-----\n t/t5514-fetch-multiple.sh | 11 +++\n 4 files changed, 137 insertions(+), 21 deletions(-)\n\n\nbase-commit: 67feca3b1c45a51b204253039139b46cc07e145f\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-369%2Fdscho%2Ffetch-jobs-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-369/dscho/fetch-jobs-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/369\n-- \ngitgitgadget\n"},{"id":"383201","messageId":"818936f1e0389820fdfa31f36c6f75e8a4bb1a07.1569930816.git.gitgitgadget@gmail.com","threadId":"51949","inReplyTo":"pull.369.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] fetch: let --jobs= parallelize --multiple, too","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-01T11:53:37Z","receivedAt":"2019-10-01T11:53:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nSo far, `--jobs=` only parallelizes submodule fetches/clones, not\n`--multiple` fetches, which is unintuitive, given that the option's name\ndoes not say anything about submodules in particular.\n\nLet's change that. With this patch, also fetches from multiple remotes\nare parallelized.\n\nFor backwards-compatibility (and to prepare for a use case where\nsubmodule and multiple-remote fetches may need different parallelization\nlimits), the config setting `submodule.fetchJobs` still only controls\nthe submodule part of `git fetch`, while the newly-introduced setting\n`fetch.parallel` controls both (but can be overridden for submodules\nwith `submodule.fetchJobs`).\n\nSigned-off-by: Johannes Schindelin \n---\n Documentation/config/fetch.txt | 10 +++\n Documentation/fetch-options.txt | 13 ++--\n builtin/fetch.c | 124 +++++++++++++++++++++++++++-----\n t/t5514-fetch-multiple.sh | 11 +++\n 4 files changed, 137 insertions(+), 21 deletions(-)\n\ndiff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\nindex ba890b5884..b200634065 100644\n--- a/Documentation/config/fetch.txt\n+++ b/Documentation/config/fetch.txt\n@@ -68,3 +68,13 @@ fetch.showForcedUpdates::\n \tSet to false to enable `--no-show-forced-updates` in\n \tlinkgit:git-fetch[1] and linkgit:git-pull[1] commands.\n \tDefaults to true.\n+\n+fetch.parallel::\n+\tSpecifies the maximal number of fetch operations to be run in parallel\n+\tat a time (submodules, or remotes when the `--multiple` option of\n+\tlinkgit:git-fetch[1] is in effect).\n++\n+A value of 0 will give some reasonable default. If unset, it defaults to 1.\n++\n+For submodules, this setting can be overridden using the `submodule.fetchJobs`\n+config setting.\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 3c9b4f9e09..8f269d3baa 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -160,10 +160,15 @@ ifndef::git-pull[]\n \n -j::\n --jobs=::\n-\tNumber of parallel children to be used for fetching submodules.\n-\tEach will fetch from different submodules, such that fetching many\n-\tsubmodules will be faster. By default submodules will be fetched\n-\tone at a time.\n+\tNumber of parallel children to be used for all forms of fetching.\n++\n+If the `--multiple` option was specified, the different remotes will be fetched\n+in parallel. If multiple submodules are fetched, they will be fetched in\n+parallel. To control them independently, use the config settings\n+`fetch.parallel` and `submodule.fetchJobs` (see linkgit:git-config[1]).\n++\n+Typically, parallel recursive and multi-remote fetches will be faster. By\n+default fetches are performed sequentially, not in parallel.\n \n --no-recurse-submodules::\n \tDisable recursive fetching of submodules (this has the same effect as\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 53ce99d2bb..e2d374724d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -54,7 +54,8 @@ static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosit\n static int progress = -1;\n static int enable_auto_gc = 1;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;\n-static int max_children = 1;\n+static int max_jobs = -1, submodule_fetch_jobs_config = -1;\n+static int fetch_parallel_config = 1;\n static enum transport_family family;\n static const char *depth;\n static const char *deepen_since;\n@@ -96,13 +97,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n \t}\n \n \tif (!strcmp(k, \"submodule.fetchjobs\")) {\n-\t\tmax_children = parse_submodule_fetchjobs(k, v);\n+\t\tsubmodule_fetch_jobs_config = parse_submodule_fetchjobs(k, v);\n \t\treturn 0;\n \t} else if (!strcmp(k, \"fetch.recursesubmodules\")) {\n \t\trecurse_submodules = parse_fetch_recurse_submodules_arg(k, v);\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(k, \"fetch.parallel\")) {\n+\t\tfetch_parallel_config = git_config_int(k, v);\n+\t\tif (fetch_parallel_config < 0)\n+\t\t\tdie(_(\"fetch.parallel cannot be negative\"));\n+\t\treturn 0;\n+\t}\n+\n \treturn git_default_config(k, v, cb);\n }\n \n@@ -134,7 +142,7 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"fetch all tags and associated objects\"), TAGS_SET),\n \tOPT_SET_INT('n', NULL, &tags,\n \t\t N_(\"do not fetch all tags (--no-tags)\"), TAGS_UNSET),\n-\tOPT_INTEGER('j', \"jobs\", &max_children,\n+\tOPT_INTEGER('j', \"jobs\", &max_jobs,\n \t\t N_(\"number of submodules fetched in parallel\")),\n \tOPT_BOOL('p', \"prune\", &prune,\n \t\t N_(\"prune remote-tracking branches no longer on remote\")),\n@@ -1456,7 +1464,62 @@ static void add_options_to_argv(struct argv_array *argv)\n \n }\n \n-static int fetch_multiple(struct string_list *list)\n+/* Fetch multiple remotes in parallel */\n+\n+struct parallel_fetch_state {\n+\tconst char **argv;\n+\tstruct string_list *remotes;\n+\tint next, result;\n+};\n+\n+static int fetch_next_remote(struct child_process *cp, struct strbuf *out,\n+\t\t\t void *cb, void **task_cb)\n+{\n+\tstruct parallel_fetch_state *state = cb;\n+\tchar *remote;\n+\n+\tif (state->next < 0 || state->next >= state->remotes->nr)\n+\t\treturn 0;\n+\n+\tremote = state->remotes->items[state->next++].string;\n+\t*task_cb = remote;\n+\n+\targv_array_pushv(&cp->args, state->argv);\n+\targv_array_push(&cp->args, remote);\n+\tcp->git_cmd = 1;\n+\n+\tif (verbosity >= 0)\n+\t\tprintf(_(\"Fetching %s\\n\"), remote);\n+\n+\treturn 1;\n+}\n+\n+static int fetch_failed_to_start(struct strbuf *out, void *cb, void *task_cb)\n+{\n+\tstruct parallel_fetch_state *state = cb;\n+\tconst char *remote = task_cb;\n+\n+\tstate->result = error(_(\"Could not fetch %s\"), remote);\n+\n+\treturn 0;\n+}\n+\n+static int fetch_finished(int result, struct strbuf *out,\n+\t\t\t void *cb, void *task_cb)\n+{\n+\tstruct parallel_fetch_state *state = cb;\n+\tconst char *remote = task_cb;\n+\n+\tif (result) {\n+\t\tstrbuf_addf(out, _(\"could not fetch '%s' (exit code: %d)\\n\"),\n+\t\t\t remote, result);\n+\t\tstate->result = -1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int fetch_multiple(struct string_list *list, int max_children)\n {\n \tint i, result = 0;\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n@@ -1470,20 +1533,34 @@ static int fetch_multiple(struct string_list *list)\n \targv_array_pushl(&argv, \"fetch\", \"--append\", \"--no-auto-gc\", NULL);\n \tadd_options_to_argv(&argv);\n \n-\tfor (i = 0; i < list->nr; i++) {\n-\t\tconst char *name = list->items[i].string;\n-\t\targv_array_push(&argv, name);\n-\t\tif (verbosity >= 0)\n-\t\t\tprintf(_(\"Fetching %s\\n\"), name);\n-\t\tif (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {\n-\t\t\terror(_(\"Could not fetch %s\"), name);\n-\t\t\tresult = 1;\n+\tif (max_children != 1 && list->nr != 1) {\n+\t\tstruct parallel_fetch_state state = { argv.argv, list, 0, 0 };\n+\n+\t\targv_array_push(&argv, \"--end-of-options\");\n+\t\tresult = run_processes_parallel_tr2(max_children,\n+\t\t\t\t\t\t &fetch_next_remote,\n+\t\t\t\t\t\t &fetch_failed_to_start,\n+\t\t\t\t\t\t &fetch_finished,\n+\t\t\t\t\t\t &state,\n+\t\t\t\t\t\t \"fetch\", \"parallel/fetch\");\n+\n+\t\tif (!result)\n+\t\t\tresult = state.result;\n+\t} else\n+\t\tfor (i = 0; i < list->nr; i++) {\n+\t\t\tconst char *name = list->items[i].string;\n+\t\t\targv_array_push(&argv, name);\n+\t\t\tif (verbosity >= 0)\n+\t\t\t\tprintf(_(\"Fetching %s\\n\"), name);\n+\t\t\tif (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {\n+\t\t\t\terror(_(\"Could not fetch %s\"), name);\n+\t\t\t\tresult = 1;\n+\t\t\t}\n+\t\t\targv_array_pop(&argv);\n \t\t}\n-\t\targv_array_pop(&argv);\n-\t}\n \n \targv_array_clear(&argv);\n-\treturn result;\n+\treturn !!result;\n }\n \n /*\n@@ -1626,7 +1703,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tfor (i = 1; i < argc; i++)\n \t\tstrbuf_addf(&default_rla, \" %s\", argv[i]);\n \n-\tfetch_config_from_gitmodules(&max_children, &recurse_submodules);\n+\tfetch_config_from_gitmodules(&submodule_fetch_jobs_config,\n+\t\t\t\t &recurse_submodules);\n \tgit_config(git_fetch_config, NULL);\n \n \targc = parse_options(argc, argv, prefix,\n@@ -1692,15 +1770,27 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tfetch_one_setup_partial(remote);\n \t\tresult = fetch_one(remote, argc, argv, prune_tags_ok);\n \t} else {\n+\t\tint max_children = max_jobs;\n+\n \t\tif (filter_options.choice)\n \t\t\tdie(_(\"--filter can only be used with the remote \"\n \t\t\t \"configured in extensions.partialclone\"));\n+\n+\t\tif (max_children < 0)\n+\t\t\tmax_children = fetch_parallel_config;\n+\n \t\t/* TODO should this also die if we have a previous partial-clone? */\n-\t\tresult = fetch_multiple(&list);\n+\t\tresult = fetch_multiple(&list, max_children);\n \t}\n \n \tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n \t\tstruct argv_array options = ARGV_ARRAY_INIT;\n+\t\tint max_children = max_jobs;\n+\n+\t\tif (max_children < 0)\n+\t\t\tmax_children = submodule_fetch_jobs_config;\n+\t\tif (max_children < 0)\n+\t\t\tmax_children = fetch_parallel_config;\n \n \t\tadd_options_to_argv(&options);\n \t\tresult = fetch_populated_submodules(the_repository,\ndiff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh\nindex 5426d4b5ab..cce829b989 100755\n--- a/t/t5514-fetch-multiple.sh\n+++ b/t/t5514-fetch-multiple.sh\n@@ -183,4 +183,15 @@ test_expect_success 'git fetch --all --tags' '\n \ttest_cmp expect test8/output\n '\n \n+test_expect_success 'parallel' '\n+\tgit remote add one ./bogus1 &&\n+\tgit remote add two ./bogus2 &&\n+\n+\ttest_must_fail env GIT_TRACE=\"$PWD/trace\" \\\n+\t\tgit fetch --jobs=2 --multiple one two 2>err &&\n+\tgrep \"2 tasks\" trace &&\n+\tgrep \"one.*128\" err &&\n+\tgrep \"two.*128\" err\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"383396","messageId":"xmqqftk9870h.fsf@gitster-ct.c.googlers.com","threadId":"51949","inReplyTo":"818936f1e0389820fdfa31f36c6f75e8a4bb1a07.1569930816.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] fetch: let --jobs= parallelize --multiple, too","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-04T04:33:02Z","receivedAt":"2019-10-04T04:33:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> +test_expect_success 'parallel' '\n> +\tgit remote add one ./bogus1 &&\n> +\tgit remote add two ./bogus2 &&\n> +\n> +\ttest_must_fail env GIT_TRACE=\"$PWD/trace\" \\\n> +\t\tgit fetch --jobs=2 --multiple one two 2>err &&\n> +\tgrep \"2 tasks\" trace &&\n\nI think this one expects to match this in run-command.c:\n\n\ttrace_printf(\"run_processes_parallel: preparing to run up to %d tasks\", n);\n\n> +\tgrep \"one.*128\" err &&\n> +\tgrep \"two.*128\" err\n\nand these expect to match this in fetch.c\n\n\t\tstrbuf_addf(out, _(\"could not fetch '%s' (exit code: %d)\\n\"),\n\nIt would have been nice to fellow contributors, if the grep patterns\nwere written a bit more tightly. It would allow people who debug\ntest failure to more easily identify which message the patterns are\ntrying to catch.\n\nIn any case, the latter two needs to be guarded against\ngettext-poison, I would think. Without addressing the vagueness of\nthe pattern, at least the following needs to be squashed to help the\nCI.\n\nThanks.\n\n---\n t/t5514-fetch-multiple.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh\nindex cce829b989..33f5220a53 100755\n--- a/t/t5514-fetch-multiple.sh\n+++ b/t/t5514-fetch-multiple.sh\n@@ -190,8 +190,8 @@ test_expect_success 'parallel' '\n \ttest_must_fail env GIT_TRACE=\"$PWD/trace\" \\\n \t\tgit fetch --jobs=2 --multiple one two 2>err &&\n \tgrep \"2 tasks\" trace &&\n-\tgrep \"one.*128\" err &&\n-\tgrep \"two.*128\" err\n+\ttest_i18ngrep \"one.*128\" err &&\n+\ttest_i18ngrep \"two.*128\" err\n '\n \n test_done\n-- \n2.23.0-686-g3bf927a9c0\n\n"},{"id":"383448","messageId":"nycvar.QRO.7.76.6.1910042356450.46@tvgsbejvaqbjf.bet","threadId":"51949","inReplyTo":"xmqqftk9870h.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] fetch: let --jobs= parallelize --multiple, too","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-04T22:03:43Z","receivedAt":"2019-10-04T22:04:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 4 Oct 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > +test_expect_success 'parallel' '\n> > +\tgit remote add one ./bogus1 &&\n> > +\tgit remote add two ./bogus2 &&\n> > +\n> > +\ttest_must_fail env GIT_TRACE=\"$PWD/trace\" \\\n> > +\t\tgit fetch --jobs=2 --multiple one two 2>err &&\n> > +\tgrep \"2 tasks\" trace &&\n>\n> I think this one expects to match this in run-command.c:\n>\n> \ttrace_printf(\"run_processes_parallel: preparing to run up to %d tasks\", n);\n>\n> > +\tgrep \"one.*128\" err &&\n> > +\tgrep \"two.*128\" err\n>\n> and these expect to match this in fetch.c\n>\n> \t\tstrbuf_addf(out, _(\"could not fetch '%s' (exit code: %d)\\n\"),\n>\n> It would have been nice to fellow contributors, if the grep patterns\n> were written a bit more tightly. It would allow people who debug\n> test failure to more easily identify which message the patterns are\n> trying to catch.\n\nThis is a two-edged sword: when those messages change (for whatever\nreason), the regression test will fail, too, but it actually wants to\ntest the parallel fetch, not the trace message of\n`run_processes_parallel`.\n\nSo I tried to prevent such an unactionable regression test failure. But\nI see your reasoning, and I now thought about it and consider those\nerror messages to be rather stable.\n\nWill fix.\n\n> In any case, the latter two needs to be guarded against\n> gettext-poison, I would think. Without addressing the vagueness of\n> the pattern, at least the following needs to be squashed to help the\n> CI.\n\nIndeed. I missed this because the GitGitGadget PR build was all green.\nMy guess is that I messed up the definition of that PR build (it is\n_not_ what's in `azure-pipelines.yml` because that would not work\ncorrectly when PRs target older commits). I _think_ I fixed it by\nsetting `export GIT_TEST_GETTEXT_POISON=true` explicitly (previously I\nonly set the `jobname`, expecting `ci/run-build-and-tests.sh` to pick up\non that.\n\nThanks,\nDscho\n\n>\n> Thanks.\n>\n> ---\n> t/t5514-fetch-multiple.sh | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh\n> index cce829b989..33f5220a53 100755\n> --- a/t/t5514-fetch-multiple.sh\n> +++ b/t/t5514-fetch-multiple.sh\n> @@ -190,8 +190,8 @@ test_expect_success 'parallel' '\n> \ttest_must_fail env GIT_TRACE=\"$PWD/trace\" \\\n> \t\tgit fetch --jobs=2 --multiple one two 2>err &&\n> \tgrep \"2 tasks\" trace &&\n> -\tgrep \"one.*128\" err &&\n> -\tgrep \"two.*128\" err\n> +\ttest_i18ngrep \"one.*128\" err &&\n> +\ttest_i18ngrep \"two.*128\" err\n> '\n>\n> test_done\n> --\n> 2.23.0-686-g3bf927a9c0\n>\n>\n"},{"id":"383471","messageId":"pull.369.v2.git.gitgitgadget@gmail.com","threadId":"51949","inReplyTo":"pull.369.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] fetch --multiple: respect --jobs=","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-05T18:46:39Z","receivedAt":"2019-10-05T18:46:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I saw with sadness that pd/fetch-jobs went nowhere, and read in the most\nrecent What's Cooking mail that it was even dropped.\n\nThis is my attempt to resurrect the idea (although without the overhead of\ntrying to support a first-class UI to control submodule and multiple-remote\nfetches independently, of which I was a rather outspoken opponent).\n\nTo make things a bit safer, this patch uses the --end-of-options marker, and\nis therefore based on top of jk/eoo.\n\nChanges since v1:\n\n * The regression test now passes even under GETTEXT_POISON.\n * The needles used in the regression test are now more indicative of the\n code producing them.\n\nJohannes Schindelin (1):\n fetch: let --jobs= parallelize --multiple, too\n\n Documentation/config/fetch.txt | 10 +++\n Documentation/fetch-options.txt | 13 ++--\n builtin/fetch.c | 124 +++++++++++++++++++++++++++-----\n t/t5514-fetch-multiple.sh | 11 +++\n 4 files changed, 137 insertions(+), 21 deletions(-)\n\n\nbase-commit: 67feca3b1c45a51b204253039139b46cc07e145f\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-369%2Fdscho%2Ffetch-jobs-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-369/dscho/fetch-jobs-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/369\n\nRange-diff vs v1:\n\n 1: 818936f1e0 ! 1: 93a155a000 fetch: let --jobs= parallelize --multiple, too\n @@ -267,9 +267,9 @@\n +\n +\ttest_must_fail env GIT_TRACE=\"$PWD/trace\" \\\n +\t\tgit fetch --jobs=2 --multiple one two 2>err &&\n -+\tgrep \"2 tasks\" trace &&\n -+\tgrep \"one.*128\" err &&\n -+\tgrep \"two.*128\" err\n ++\tgrep \"preparing to run up to 2 tasks\" trace &&\n ++\ttest_i18ngrep \"could not fetch .one.*128\" err &&\n ++\ttest_i18ngrep \"could not fetch .two.*128\" err\n +'\n +\n test_done\n\n-- \ngitgitgadget\n"},{"id":"383472","messageId":"93a155a000571df3f3f3a63cd8d886d68fbe8403.1570301198.git.gitgitgadget@gmail.com","threadId":"51949","inReplyTo":"pull.369.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] fetch: let --jobs= parallelize --multiple, too","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-05T18:46:40Z","receivedAt":"2019-10-05T18:46:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nSo far, `--jobs=` only parallelizes submodule fetches/clones, not\n`--multiple` fetches, which is unintuitive, given that the option's name\ndoes not say anything about submodules in particular.\n\nLet's change that. With this patch, also fetches from multiple remotes\nare parallelized.\n\nFor backwards-compatibility (and to prepare for a use case where\nsubmodule and multiple-remote fetches may need different parallelization\nlimits), the config setting `submodule.fetchJobs` still only controls\nthe submodule part of `git fetch`, while the newly-introduced setting\n`fetch.parallel` controls both (but can be overridden for submodules\nwith `submodule.fetchJobs`).\n\nSigned-off-by: Johannes Schindelin \n---\n Documentation/config/fetch.txt | 10 +++\n Documentation/fetch-options.txt | 13 ++--\n builtin/fetch.c | 124 +++++++++++++++++++++++++++-----\n t/t5514-fetch-multiple.sh | 11 +++\n 4 files changed, 137 insertions(+), 21 deletions(-)\n\ndiff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\nindex ba890b5884..b200634065 100644\n--- a/Documentation/config/fetch.txt\n+++ b/Documentation/config/fetch.txt\n@@ -68,3 +68,13 @@ fetch.showForcedUpdates::\n \tSet to false to enable `--no-show-forced-updates` in\n \tlinkgit:git-fetch[1] and linkgit:git-pull[1] commands.\n \tDefaults to true.\n+\n+fetch.parallel::\n+\tSpecifies the maximal number of fetch operations to be run in parallel\n+\tat a time (submodules, or remotes when the `--multiple` option of\n+\tlinkgit:git-fetch[1] is in effect).\n++\n+A value of 0 will give some reasonable default. If unset, it defaults to 1.\n++\n+For submodules, this setting can be overridden using the `submodule.fetchJobs`\n+config setting.\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 3c9b4f9e09..8f269d3baa 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -160,10 +160,15 @@ ifndef::git-pull[]\n \n -j::\n --jobs=::\n-\tNumber of parallel children to be used for fetching submodules.\n-\tEach will fetch from different submodules, such that fetching many\n-\tsubmodules will be faster. By default submodules will be fetched\n-\tone at a time.\n+\tNumber of parallel children to be used for all forms of fetching.\n++\n+If the `--multiple` option was specified, the different remotes will be fetched\n+in parallel. If multiple submodules are fetched, they will be fetched in\n+parallel. To control them independently, use the config settings\n+`fetch.parallel` and `submodule.fetchJobs` (see linkgit:git-config[1]).\n++\n+Typically, parallel recursive and multi-remote fetches will be faster. By\n+default fetches are performed sequentially, not in parallel.\n \n --no-recurse-submodules::\n \tDisable recursive fetching of submodules (this has the same effect as\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 53ce99d2bb..e2d374724d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -54,7 +54,8 @@ static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosit\n static int progress = -1;\n static int enable_auto_gc = 1;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;\n-static int max_children = 1;\n+static int max_jobs = -1, submodule_fetch_jobs_config = -1;\n+static int fetch_parallel_config = 1;\n static enum transport_family family;\n static const char *depth;\n static const char *deepen_since;\n@@ -96,13 +97,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n \t}\n \n \tif (!strcmp(k, \"submodule.fetchjobs\")) {\n-\t\tmax_children = parse_submodule_fetchjobs(k, v);\n+\t\tsubmodule_fetch_jobs_config = parse_submodule_fetchjobs(k, v);\n \t\treturn 0;\n \t} else if (!strcmp(k, \"fetch.recursesubmodules\")) {\n \t\trecurse_submodules = parse_fetch_recurse_submodules_arg(k, v);\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(k, \"fetch.parallel\")) {\n+\t\tfetch_parallel_config = git_config_int(k, v);\n+\t\tif (fetch_parallel_config < 0)\n+\t\t\tdie(_(\"fetch.parallel cannot be negative\"));\n+\t\treturn 0;\n+\t}\n+\n \treturn git_default_config(k, v, cb);\n }\n \n@@ -134,7 +142,7 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"fetch all tags and associated objects\"), TAGS_SET),\n \tOPT_SET_INT('n', NULL, &tags,\n \t\t N_(\"do not fetch all tags (--no-tags)\"), TAGS_UNSET),\n-\tOPT_INTEGER('j', \"jobs\", &max_children,\n+\tOPT_INTEGER('j', \"jobs\", &max_jobs,\n \t\t N_(\"number of submodules fetched in parallel\")),\n \tOPT_BOOL('p', \"prune\", &prune,\n \t\t N_(\"prune remote-tracking branches no longer on remote\")),\n@@ -1456,7 +1464,62 @@ static void add_options_to_argv(struct argv_array *argv)\n \n }\n \n-static int fetch_multiple(struct string_list *list)\n+/* Fetch multiple remotes in parallel */\n+\n+struct parallel_fetch_state {\n+\tconst char **argv;\n+\tstruct string_list *remotes;\n+\tint next, result;\n+};\n+\n+static int fetch_next_remote(struct child_process *cp, struct strbuf *out,\n+\t\t\t void *cb, void **task_cb)\n+{\n+\tstruct parallel_fetch_state *state = cb;\n+\tchar *remote;\n+\n+\tif (state->next < 0 || state->next >= state->remotes->nr)\n+\t\treturn 0;\n+\n+\tremote = state->remotes->items[state->next++].string;\n+\t*task_cb = remote;\n+\n+\targv_array_pushv(&cp->args, state->argv);\n+\targv_array_push(&cp->args, remote);\n+\tcp->git_cmd = 1;\n+\n+\tif (verbosity >= 0)\n+\t\tprintf(_(\"Fetching %s\\n\"), remote);\n+\n+\treturn 1;\n+}\n+\n+static int fetch_failed_to_start(struct strbuf *out, void *cb, void *task_cb)\n+{\n+\tstruct parallel_fetch_state *state = cb;\n+\tconst char *remote = task_cb;\n+\n+\tstate->result = error(_(\"Could not fetch %s\"), remote);\n+\n+\treturn 0;\n+}\n+\n+static int fetch_finished(int result, struct strbuf *out,\n+\t\t\t void *cb, void *task_cb)\n+{\n+\tstruct parallel_fetch_state *state = cb;\n+\tconst char *remote = task_cb;\n+\n+\tif (result) {\n+\t\tstrbuf_addf(out, _(\"could not fetch '%s' (exit code: %d)\\n\"),\n+\t\t\t remote, result);\n+\t\tstate->result = -1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int fetch_multiple(struct string_list *list, int max_children)\n {\n \tint i, result = 0;\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n@@ -1470,20 +1533,34 @@ static int fetch_multiple(struct string_list *list)\n \targv_array_pushl(&argv, \"fetch\", \"--append\", \"--no-auto-gc\", NULL);\n \tadd_options_to_argv(&argv);\n \n-\tfor (i = 0; i < list->nr; i++) {\n-\t\tconst char *name = list->items[i].string;\n-\t\targv_array_push(&argv, name);\n-\t\tif (verbosity >= 0)\n-\t\t\tprintf(_(\"Fetching %s\\n\"), name);\n-\t\tif (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {\n-\t\t\terror(_(\"Could not fetch %s\"), name);\n-\t\t\tresult = 1;\n+\tif (max_children != 1 && list->nr != 1) {\n+\t\tstruct parallel_fetch_state state = { argv.argv, list, 0, 0 };\n+\n+\t\targv_array_push(&argv, \"--end-of-options\");\n+\t\tresult = run_processes_parallel_tr2(max_children,\n+\t\t\t\t\t\t &fetch_next_remote,\n+\t\t\t\t\t\t &fetch_failed_to_start,\n+\t\t\t\t\t\t &fetch_finished,\n+\t\t\t\t\t\t &state,\n+\t\t\t\t\t\t \"fetch\", \"parallel/fetch\");\n+\n+\t\tif (!result)\n+\t\t\tresult = state.result;\n+\t} else\n+\t\tfor (i = 0; i < list->nr; i++) {\n+\t\t\tconst char *name = list->items[i].string;\n+\t\t\targv_array_push(&argv, name);\n+\t\t\tif (verbosity >= 0)\n+\t\t\t\tprintf(_(\"Fetching %s\\n\"), name);\n+\t\t\tif (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {\n+\t\t\t\terror(_(\"Could not fetch %s\"), name);\n+\t\t\t\tresult = 1;\n+\t\t\t}\n+\t\t\targv_array_pop(&argv);\n \t\t}\n-\t\targv_array_pop(&argv);\n-\t}\n \n \targv_array_clear(&argv);\n-\treturn result;\n+\treturn !!result;\n }\n \n /*\n@@ -1626,7 +1703,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tfor (i = 1; i < argc; i++)\n \t\tstrbuf_addf(&default_rla, \" %s\", argv[i]);\n \n-\tfetch_config_from_gitmodules(&max_children, &recurse_submodules);\n+\tfetch_config_from_gitmodules(&submodule_fetch_jobs_config,\n+\t\t\t\t &recurse_submodules);\n \tgit_config(git_fetch_config, NULL);\n \n \targc = parse_options(argc, argv, prefix,\n@@ -1692,15 +1770,27 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tfetch_one_setup_partial(remote);\n \t\tresult = fetch_one(remote, argc, argv, prune_tags_ok);\n \t} else {\n+\t\tint max_children = max_jobs;\n+\n \t\tif (filter_options.choice)\n \t\t\tdie(_(\"--filter can only be used with the remote \"\n \t\t\t \"configured in extensions.partialclone\"));\n+\n+\t\tif (max_children < 0)\n+\t\t\tmax_children = fetch_parallel_config;\n+\n \t\t/* TODO should this also die if we have a previous partial-clone? */\n-\t\tresult = fetch_multiple(&list);\n+\t\tresult = fetch_multiple(&list, max_children);\n \t}\n \n \tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n \t\tstruct argv_array options = ARGV_ARRAY_INIT;\n+\t\tint max_children = max_jobs;\n+\n+\t\tif (max_children < 0)\n+\t\t\tmax_children = submodule_fetch_jobs_config;\n+\t\tif (max_children < 0)\n+\t\t\tmax_children = fetch_parallel_config;\n \n \t\tadd_options_to_argv(&options);\n \t\tresult = fetch_populated_submodules(the_repository,\ndiff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh\nindex 5426d4b5ab..de8e2f1531 100755\n--- a/t/t5514-fetch-multiple.sh\n+++ b/t/t5514-fetch-multiple.sh\n@@ -183,4 +183,15 @@ test_expect_success 'git fetch --all --tags' '\n \ttest_cmp expect test8/output\n '\n \n+test_expect_success 'parallel' '\n+\tgit remote add one ./bogus1 &&\n+\tgit remote add two ./bogus2 &&\n+\n+\ttest_must_fail env GIT_TRACE=\"$PWD/trace\" \\\n+\t\tgit fetch --jobs=2 --multiple one two 2>err &&\n+\tgrep \"preparing to run up to 2 tasks\" trace &&\n+\ttest_i18ngrep \"could not fetch .one.*128\" err &&\n+\ttest_i18ngrep \"could not fetch .two.*128\" err\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"383514","messageId":"nycvar.QRO.7.76.6.1910061151440.46@tvgsbejvaqbjf.bet","threadId":"51949","inReplyTo":"xmqqftk67r6j.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] fetch: let --jobs= parallelize --multiple, too","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-06T09:53:54Z","receivedAt":"2019-10-06T09:54:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sun, 6 Oct 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > +test_expect_success 'parallel' '\n> > +\tgit remote add one ./bogus1 &&\n> > +\tgit remote add two ./bogus2 &&\n> > +\n> > +\ttest_must_fail env GIT_TRACE=\"$PWD/trace\" \\\n> > +\t\tgit fetch --jobs=2 --multiple one two 2>err &&\n> > +\tgrep \"preparing to run up to 2 tasks\" trace &&\n> > +\ttest_i18ngrep \"could not fetch .one.*128\" err &&\n> > +\ttest_i18ngrep \"could not fetch .two.*128\" err\n> > +'\n> > +\n> > test_done\n>\n> Thanks. I think it is much better to prepare these tests like this\n> patch does to be broken when phrasing changes---that would give\n> feedback and confidence to the person who is changing the message\n> and/or the logic to emit the message.\n>\n> Where does the constant 128 come from, by the way? If it is from errno.h\n> then we will soon hear breakage report from NonStop folks, I predict\n> ;-)\n\nIt comes from `die()`:\n\n-- snip --\nstatic NORETURN void die_builtin(const char *err, va_list params)\n{\n /*\n\t * We call this trace2 function first and expect it to va_copy 'params'\n\t * before using it (because an 'ap' can only be walked once).\n\t */\n\ttrace2_cmd_error_va(err, params);\n\n\tvreportf(\"fatal: \", err, params);\n\n\texit(128);\n}\n-- snap --\n\nThanks,\nDscho\n"},{"id":"383553","messageId":"xmqqwodh4ann.fsf@gitster-ct.c.googlers.com","threadId":"51949","inReplyTo":"nycvar.QRO.7.76.6.1910061151440.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/1] fetch: let --jobs= parallelize --multiple, too","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-07T01:17:00Z","receivedAt":"2019-10-07T01:17:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n> ...\n>> Thanks. I think it is much better to prepare these tests like this\n>> patch does to be broken when phrasing changes---that would give\n>> feedback and confidence to the person who is changing the message\n>> and/or the logic to emit the message.\n>>\n>> Where does the constant 128 come from, by the way? If it is from errno.h\n>> then we will soon hear breakage report from NonStop folks, I predict\n>> ;-)\n>\n> It comes from `die()`:\n> ...\n> \texit(128);\n\nOK, so hopefully we wouldn't see any platform specific variations.\nGood.\n\nThanks for digging.\n\n"},{"id":"383573","messageId":"nycvar.QRO.7.76.6.1910071212170.46@tvgsbejvaqbjf.bet","threadId":"51949","inReplyTo":"xmqqwodh4ann.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] fetch: let --jobs= parallelize --multiple, too","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-07T10:14:27Z","receivedAt":"2019-10-07T10:15:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 7 Oct 2019, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > Hi Junio,\n> > ...\n> >> Thanks. I think it is much better to prepare these tests like this\n> >> patch does to be broken when phrasing changes---that would give\n> >> feedback and confidence to the person who is changing the message\n> >> and/or the logic to emit the message.\n> >>\n> >> Where does the constant 128 come from, by the way? If it is from errno.h\n> >> then we will soon hear breakage report from NonStop folks, I predict\n> >> ;-)\n> >\n> > It comes from `die()`:\n> > ...\n> > \texit(128);\n>\n> OK, so hopefully we wouldn't see any platform specific variations.\n\nI am certain of it, as the matched `128` is not printed implicitly, it\nis printed by these two lines that I added in this patch (as part of\n`fetch_finished()`):\n\n+ strbuf_addf(out, _(\"could not fetch '%s' (exit code: %d)\\n\"),\n+ remote, result);\n\nThanks,\nDscho\n"},{"id":"383574","messageId":"nycvar.QRO.7.76.6.1910071215270.46@tvgsbejvaqbjf.bet","threadId":"51949","inReplyTo":"nycvar.QRO.7.76.6.1910071212170.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/1] fetch: let --jobs= parallelize --multiple, too","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-07T10:17:21Z","receivedAt":"2019-10-07T10:17:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 7 Oct 2019, Johannes Schindelin wrote:\n\n> On Mon, 7 Oct 2019, Junio C Hamano wrote:\n>\n> > Johannes Schindelin writes:\n> >\n> > > Hi Junio,\n> > > ...\n> > >> Thanks. I think it is much better to prepare these tests like this\n> > >> patch does to be broken when phrasing changes---that would give\n> > >> feedback and confidence to the person who is changing the message\n> > >> and/or the logic to emit the message.\n> > >>\n> > >> Where does the constant 128 come from, by the way? If it is from errno.h\n> > >> then we will soon hear breakage report from NonStop folks, I predict\n> > >> ;-)\n> > >\n> > > It comes from `die()`:\n> > > ...\n> > > \texit(128);\n> >\n> > OK, so hopefully we wouldn't see any platform specific variations.\n>\n> I am certain of it, as the matched `128` is not printed implicitly, it\n> is printed by these two lines that I added in this patch (as part of\n> `fetch_finished()`):\n>\n> + strbuf_addf(out, _(\"could not fetch '%s' (exit code: %d)\\n\"),\n> + remote, result);\n\nOh, and I forgot to say: the exit code is expected to be `128` because\nthe added regression test case uses a non-existing path to fetch from,\nessentially saying \"No, `git fetch`, I expect you to `die()`\".\n\nCiao,\nDscho\n"}]}