{"thread":{"id":"51633","subject":"[PATCH v2 0/5] fetch: Extend --jobs to multiple remotes","startedAt":"2019-08-12T21:35:44Z","lastAt":"2019-08-14T18:33:19Z","messageCount":14,"participants":["Palmer Dabbelt","Eric Wong","Johannes Schindelin","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"380343","messageId":"20190812213448.2649-1-palmer@sifive.com","threadId":"51633","inReplyTo":null,"subject":"[PATCH v2 0/5] fetch: Extend --jobs to multiple remotes","fromName":"Palmer Dabbelt","fromEmail":"palmer@sifive.com","sentAt":"2019-08-12T21:34:43Z","receivedAt":"2019-08-12T21:35:44Z","isPatch":true,"sender":{"key":"palmer@sifive.com","avatar":"https://gravatar.com/avatar/bf8be091de4e6d14300576518ac2a35e0871bd4948140539ce9ef6efade35c09?d=mp&s=160"},"body":"It seems like the feedback for v1 was fairly positive, so I managed to\nfind some time to go ahead finish it.  The result is a much cleaner\npatch set.  I think this could be merged in its current state, but there\nare a few outstanding concerns I have:\n\n* fetch.jobs isn't documented because I couldn't find any\n  documentation for submodule.fetchjobs so I didn't know where to start\n  writing.\n* I took the complicated approach and added --submodule-fetch-jobs and\n  --fetch-jobs before converting --jobs over to set both.  At the time I\n  thought it wouldn't add too much extra complexity.  I wasn't looking\n  closely enough and ended up with a custom parsing function which is a\n  bit ugly.\n\nI'm happy to fix either of these, but I wanted to send out the v2 before\ngoing any farther because I wasn't sure if --jobs would be converted\nover right away or if there was going to be a deprecation period.  I've\nwritten the patch set such that the final patch can easily be dropped to\navoid changing existing behavior.\n\nI'm also happy to re-spin this to just make --jobs control remotes and\ndrop the --fetch-jobs and --submodule-fetch-jobs arguments, which would\nmake the patch set somewhat simpler but preclude a deprecation period.\n\nChanges since v1 <20190717015903.4384-1-palmer@sifive.com>:\n\n* fetch_multiple() has been rewritten to use\n  run_processes_parallel_tr2() rather than manage children on its own.\n* The --fetch-jobs argument has been added.\n* The --jobs arugemnt has been used, instead of the --parallel argument.\n\n\n"},{"id":"380345","messageId":"20190812213448.2649-2-palmer@sifive.com","threadId":"51633","inReplyTo":"20190812213448.2649-1-palmer@sifive.com","subject":"[PATCH v2 1/5] fetch: Rename max_children to max_children_for_submodules","fromName":"Palmer Dabbelt","fromEmail":"palmer@sifive.com","sentAt":"2019-08-12T21:34:44Z","receivedAt":"2019-08-12T21:35:45Z","isPatch":true,"sender":{"key":"palmer@sifive.com","avatar":"https://gravatar.com/avatar/bf8be091de4e6d14300576518ac2a35e0871bd4948140539ce9ef6efade35c09?d=mp&s=160"},"body":"This does not change any functionality, but instead just prepares for\nthe upcoming \"--fetch-jobs=<n>\" support.  Essentially the \"max_children\"\nvariable is ambiguously named, which would complicate the diff in\nsquashed into the \"--fetch-jobs=<n>\" patch.\n\nSigned-off-by: Palmer Dabbelt <palmer@sifive.com>\n---\n builtin/fetch.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 717dd14e8961..8aa6a0caf1ab 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -54,7 +54,7 @@ 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_children_for_submodules = 1;\n static enum transport_family family;\n static const char *depth;\n static const char *deepen_since;\n@@ -96,7 +96,7 @@ 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\tmax_children_for_submodules = 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@@ -134,7 +134,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_children_for_submodules,\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@@ -1633,7 +1633,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(&max_children_for_submodules,\n+\t\t\t\t     &recurse_submodules);\n \tgit_config(git_fetch_config, NULL);\n \n \targc = parse_options(argc, argv, prefix,\n@@ -1716,7 +1717,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\t    recurse_submodules,\n \t\t\t\t\t\t    recurse_submodules_default,\n \t\t\t\t\t\t    verbosity < 0,\n-\t\t\t\t\t\t    max_children);\n+\t\t\t\t\t\t    max_children_for_submodules);\n \t\targv_array_clear(&options);\n \t}\n \n-- \n2.21.0\n\n"},{"id":"380344","messageId":"20190812213448.2649-3-palmer@sifive.com","threadId":"51633","inReplyTo":"20190812213448.2649-1-palmer@sifive.com","subject":"[PATCH v2 2/5] fetch: Add the \"--fetch-jobs\" option","fromName":"Palmer Dabbelt","fromEmail":"palmer@sifive.com","sentAt":"2019-08-12T21:34:45Z","receivedAt":"2019-08-12T21:35:47Z","isPatch":true,"sender":{"key":"palmer@sifive.com","avatar":"https://gravatar.com/avatar/bf8be091de4e6d14300576518ac2a35e0871bd4948140539ce9ef6efade35c09?d=mp&s=160"},"body":"This argument allows users to select the number of jobs that will run in\nparallel when fetching, with each job fetching from a different remote.\nThis is particularly useful when fetching from many remotes that change\nslowly on a high latency link, as the fetch time is dominated by\nhandshake latency -- probably not the most common use case, but one that\nends up frusturating me with some frequency.\n\nThis patch implements --fetch-jobs via run_processes_parallel_tr2(),\nwhich allows the user to restrict the amount of parallelism.  While this\nadds a lot of boilerplate to the implementation, the pattern is fairly\nstraight-forward and matches the existing submodule parallel fetch code.\nThe submodule version of this is a lot more complicated and does things\nlike retrying and scheduling I/O, but I don't get bit by those issues so\nit didn't seem worth the effort.\n\nThe speedup is quite noticable for me, even when I'm on a fast link --\nmy Linux fetches go from 6 seconds to 2 seconds, for example.\n\nI've given this a quick test locally, but haven't done anything\nexhaustive like attempting to fetch against broken remotes (where the\nintent is that the other remotes are fetched and the result is an\nerror).\n\nSigned-off-by: Palmer Dabbelt <palmer@sifive.com>\n---\n Documentation/fetch-options.txt |   5 ++\n builtin/fetch.c                 | 141 +++++++++++++++++++++++++++-----\n 2 files changed, 126 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 3c9b4f9e0951..dbd2add686dd 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -165,6 +165,11 @@ ifndef::git-pull[]\n \tsubmodules will be faster. By default submodules will be fetched\n \tone at a time.\n \n+--fetch-jobs=<n>::\n+\tNumber of parallel children to be used for fetching.  Each will fetch\n+\tfrom different remotes, such that fetching from many remotes will be\n+\tfaster.  By default remotes will be fetched from one at a time.\n+\n --no-recurse-submodules::\n \tDisable recursive fetching of submodules (this has the same effect as\n \tusing the `--recurse-submodules=no` option).\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 8aa6a0caf1ab..fa12ad44e7d9 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -55,6 +55,7 @@ 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_for_submodules = 1;\n+static int max_children_for_fetch = 1;\n static enum transport_family family;\n static const char *depth;\n static const char *deepen_since;\n@@ -136,6 +137,8 @@ static struct option builtin_fetch_options[] = {\n \t\t    N_(\"do not fetch all tags (--no-tags)\"), TAGS_UNSET),\n \tOPT_INTEGER('j', \"jobs\", &max_children_for_submodules,\n \t\t    N_(\"number of submodules fetched in parallel\")),\n+\tOPT_INTEGER(0, \"fetch-jobs\", &max_children_for_fetch,\n+\t\t    N_(\"number of remotes fetched in parallel\")),\n \tOPT_BOOL('p', \"prune\", &prune,\n \t\t N_(\"prune remote-tracking branches no longer on remote\")),\n \tOPT_BOOL('P', \"prune-tags\", &prune_tags,\n@@ -1463,10 +1466,100 @@ static void add_options_to_argv(struct argv_array *argv)\n \n }\n \n-static int fetch_multiple(struct string_list *list)\n+/*\n+ * Support for fetching multiple remotes in parallel.  This is spread over\n+ * multiple functions and structures, all of which are helpers for\n+ * fench_multiple().  The general idea is that there is a single struct\n+ * fetch_remote, which contains the entire state for a fetch_multiple()\n+ * instance.\n+ */\n+struct fetch_remote_task {\n+\tint in_use;\n+\tstruct argv_array argv;\n+};\n+\n+struct fetch_remote {\n+\tstruct string_list *all_remotes;\n+\tint next_remote_index;\n+\tstruct fetch_remote_task *all_tasks;\n+\tint task_count;\n+\tint result;\n+};\n+\n+static int next_remote_to_fetch(struct child_process *cp,\n+\t\t\t\tstruct strbuf *out,\n+\t\t\t\tvoid *state_uncast,\n+\t\t\t\tvoid **task_state_out)\n+{\n+\tint i;\n+\tstruct fetch_remote *state = state_uncast;\n+\tstruct fetch_remote_task *task_state = NULL;\n+\tconst char *remote_name;\n+\n+\tif (state->next_remote_index >= state->all_remotes->nr)\n+\t\treturn 0;\n+\n+\tremote_name = state->all_remotes->items[state->next_remote_index].string;\n+\tstate->next_remote_index++;\n+\n+\t/*\n+\t * Finds somewhere to store the state for a task.  This is guarnteed to\n+\t * succeed because there are always enough tasks allocated to cover the\n+\t * number that have been requested to run in parallel.  Rather than\n+\t * bothering with some sort of free list, this just brute force\n+\t * searches for a free task.  The assumption is that there aren't that\n+\t * many tasks to look through.\n+\t */\n+\tfor (i = 0; i < state->task_count; ++i) {\n+\t\tif (!state->all_tasks[i].in_use) {\n+\t\t\ttask_state = state->all_tasks + i;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\tassert(task_state != NULL);\n+\ttask_state->in_use = 1;\n+\n+\t*task_state_out = task_state;\n+\targv_array_push(&task_state->argv, remote_name);\n+\tcp->argv = task_state->argv.argv;\n+\tcp->git_cmd = 1;\n+\n+\tprintf(_(\"Fetching %s\\n\"), remote_name);\n+\n+\treturn 1;\n+}\n+\n+static int remote_fetch_failed_to_start(struct strbuf *out,\n+\t\t\t\t\tvoid *state_uncast,\n+\t\t\t\t\tvoid *task_state_uncast)\n+{\n+\tstruct fetch_remote *state = state_uncast;\n+\tstruct fetch_remote_task *task_state = task_state_uncast;\n+\tassert(task_state != NULL);\n+\targv_array_pop(&task_state->argv);\n+\ttask_state->in_use = 0;\n+\tstate->result |= -1;\n+\treturn 0;\n+}\n+\n+static int remote_fetch_finished(int result,\n+\t\t\t\t struct strbuf *out,\n+\t\t\t\t void *state_uncast,\n+\t\t\t\t void *task_state_uncast)\n+{\n+\tstruct fetch_remote *state = state_uncast;\n+\tstruct fetch_remote_task *task_state = task_state_uncast;\n+\tassert(task_state != NULL);\n+\targv_array_pop(&task_state->argv);\n+\ttask_state->in_use = 0;\n+\tstate->result |= result;\n+\treturn 0;\n+}\n+\n+static int fetch_multiple(struct string_list *all_remotes, int max_children)\n {\n \tint i, result = 0;\n-\tstruct argv_array argv = ARGV_ARRAY_INIT;\n+\tstruct fetch_remote state;\n \n \tif (!append && !dry_run) {\n \t\tint errcode = truncate_fetch_head();\n@@ -1474,23 +1567,31 @@ static int fetch_multiple(struct string_list *list)\n \t\t\treturn errcode;\n \t}\n \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-\t\t}\n-\t\targv_array_pop(&argv);\n-\t}\n-\n-\targv_array_clear(&argv);\n-\treturn result;\n+\tstate.all_remotes = all_remotes;\n+\tstate.next_remote_index = 0;\n+\tstate.all_tasks = xcalloc(sizeof(*state.all_tasks), max_children);\n+\tstate.task_count = max_children;\n+\tstate.result = 0;\n+\n+\tfor (i = 0; i < max_children; ++i) {\n+\t\tstruct argv_array *argv = &state.all_tasks[i].argv;\n+\t\tstate.all_tasks[i].in_use = 0;\n+\t\targv_array_init(argv);\n+\t\targv_array_pushl(argv, \"fetch\", \"--append\", \"--no-auto-gc\", NULL);\n+\t\tadd_options_to_argv(argv);\n+\t};\n+\n+\tresult = run_processes_parallel_tr2(max_children,\n+\t\t\t\t\t    &next_remote_to_fetch,\n+\t\t\t\t\t    &remote_fetch_failed_to_start,\n+\t\t\t\t\t    &remote_fetch_finished,\n+\t\t\t\t\t    &state,\n+\t\t\t\t\t    \"fetch\", \"parallel\");\n+\n+\tfor (i = 0; i < max_children; ++i)\n+\t\targv_array_clear(&state.all_tasks[i].argv);\n+\tfree(state.all_tasks);\n+\treturn result | state.result;\n }\n \n /*\n@@ -1704,7 +1805,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"--filter can only be used with the remote \"\n \t\t\t      \"configured in extensions.partialclone\"));\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_for_fetch);\n \t}\n \n \tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n-- \n2.21.0\n\n"},{"id":"380346","messageId":"20190812213448.2649-4-palmer@sifive.com","threadId":"51633","inReplyTo":"20190812213448.2649-1-palmer@sifive.com","subject":"[PATCH v2 3/5] fetch: Add the fetch.jobs config key","fromName":"Palmer Dabbelt","fromEmail":"palmer@sifive.com","sentAt":"2019-08-12T21:34:46Z","receivedAt":"2019-08-12T21:35:48Z","isPatch":true,"sender":{"key":"palmer@sifive.com","avatar":"https://gravatar.com/avatar/bf8be091de4e6d14300576518ac2a35e0871bd4948140539ce9ef6efade35c09?d=mp&s=160"},"body":"This allows users to default to parallel fetches, so they don't have to\npass --fetch-jobs=N on the command line every time.  The implementation\nmatches submodule.fetchjobs by die()ing on invalid job counts.\n\nI couldn't find any documentation for submodule.fetchjobs, so I didn't\nwrite any for this one.  I've tested this in my config and in\nconjunction with the command-line argument.\n\nSigned-off-by: Palmer Dabbelt <palmer@sifive.com>\n---\n builtin/fetch.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex fa12ad44e7d9..4c5f2ea3a931 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -104,6 +104,13 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(k, \"fetch.jobs\")) {\n+\t\tmax_children_for_fetch = git_config_int(k, v);\n+\t\tif (max_children_for_fetch < 0)\n+\t\t\tdie(_(\"negative values not allowed for fetch.jobs\"));\n+\t\treturn 0;\n+\t}\n+\n \treturn git_default_config(k, v, cb);\n }\n \n-- \n2.21.0\n\n"},{"id":"380347","messageId":"20190812213448.2649-5-palmer@sifive.com","threadId":"51633","inReplyTo":"20190812213448.2649-1-palmer@sifive.com","subject":"[PATCH v2 4/5] fetch: Add the --submodule-fetch-jobs option","fromName":"Palmer Dabbelt","fromEmail":"palmer@sifive.com","sentAt":"2019-08-12T21:34:47Z","receivedAt":"2019-08-12T21:35:50Z","isPatch":true,"sender":{"key":"palmer@sifive.com","avatar":"https://gravatar.com/avatar/bf8be091de4e6d14300576518ac2a35e0871bd4948140539ce9ef6efade35c09?d=mp&s=160"},"body":"This is exactly the same as --jobs, but the more explicit name will\nallow us to later change the behavior of --jobs to control both the\nnumber of jobs for remotes as well as submodules.\n\nSigned-off-by: Palmer Dabbelt <palmer@sifive.com>\n---\n Documentation/fetch-options.txt | 1 +\n builtin/fetch.c                 | 2 ++\n 2 files changed, 3 insertions(+)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex dbd2add686dd..5836024f1934 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -160,6 +160,7 @@ ifndef::git-pull[]\n \n -j::\n --jobs=<n>::\n+--submodule-fetch-jobs=<n>::\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\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 4c5f2ea3a931..67d001f3f78b 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -146,6 +146,8 @@ static struct option builtin_fetch_options[] = {\n \t\t    N_(\"number of submodules fetched in parallel\")),\n \tOPT_INTEGER(0, \"fetch-jobs\", &max_children_for_fetch,\n \t\t    N_(\"number of remotes fetched in parallel\")),\n+\tOPT_INTEGER(0, \"submodule-fetch-jobs\", &max_children_for_submodules,\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 \tOPT_BOOL('P', \"prune-tags\", &prune_tags,\n-- \n2.21.0\n\n"},{"id":"380348","messageId":"20190812213448.2649-6-palmer@sifive.com","threadId":"51633","inReplyTo":"20190812213448.2649-1-palmer@sifive.com","subject":"[PATCH v2 5/5] fetch: Make --jobs control submodules and remotes","fromName":"Palmer Dabbelt","fromEmail":"palmer@sifive.com","sentAt":"2019-08-12T21:34:48Z","receivedAt":"2019-08-12T21:35:50Z","isPatch":true,"sender":{"key":"palmer@sifive.com","avatar":"https://gravatar.com/avatar/bf8be091de4e6d14300576518ac2a35e0871bd4948140539ce9ef6efade35c09?d=mp&s=160"},"body":"The existing --jobs argument was defined to control the number of jobs\nused for submodule fetching, but it makes more sense to have this\nargument control the number of jobs to be used when fetching from\nmultiple remotes as well.\n\nThis patch simply changes the --jobs argument parsing code to set both\nmax_children_for_{submodules,fetch}, as well as noting this new behavior\nin the documentation.\n\nSigned-off-by: Palmer Dabbelt <palmer@sifive.com>\n---\n Documentation/fetch-options.txt |  4 ++++\n builtin/fetch.c                 | 21 ++++++++++++++++++---\n 2 files changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 5836024f1934..0915fd4ed6d5 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -160,6 +160,10 @@ ifndef::git-pull[]\n \n -j::\n --jobs=<n>::\n+\tNumber of parallel children to be used for all forms of fetching.\n+\tThis is the same as passing `--submodule-fetch-jobs=<n>` and\n+\t`--fetch-jobs=<n>`.\n+\n --submodule-fetch-jobs=<n>::\n \tNumber of parallel children to be used for fetching submodules.\n \tEach will fetch from different submodules, such that fetching many\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 67d001f3f78b..41498e9efb3b 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n \treturn git_default_config(k, v, cb);\n }\n \n+static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)\n+{\n+\tint jobs;\n+\n+\tjobs = atoi(arg);\n+\tif (jobs < 1)\n+\t\tdie(_(\"There must be a positive number of jobs\"));\n+\n+\tmax_children_for_submodules = jobs;\n+\tmax_children_for_fetch = jobs;\n+\n+\treturn 0;\n+}\n+\n static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)\n {\n \tBUG_ON_OPT_NEG(unset);\n@@ -142,12 +156,13 @@ 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_for_submodules,\n+\t{ OPTION_CALLBACK, 'j', \"jobs\", NULL, N_(\"jobs\"),\n+\t\t    N_(\"number of parallel tasks to run while fetching\"),\n+\t\t    PARSE_OPT_OPTARG, &parse_jobs_arg },\n+\tOPT_INTEGER(0, \"submodule-fetch-jobs\", &max_children_for_submodules,\n \t\t    N_(\"number of submodules fetched in parallel\")),\n \tOPT_INTEGER(0, \"fetch-jobs\", &max_children_for_fetch,\n \t\t    N_(\"number of remotes fetched in parallel\")),\n-\tOPT_INTEGER(0, \"submodule-fetch-jobs\", &max_children_for_submodules,\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 \tOPT_BOOL('P', \"prune-tags\", &prune_tags,\n-- \n2.21.0\n\n"},{"id":"380387","messageId":"20190813144441.qw5i3zbrvebz5o7z@dcvr","threadId":"51633","inReplyTo":"20190812213448.2649-3-palmer@sifive.com","subject":"Re: [PATCH v2 2/5] fetch: Add the \"--fetch-jobs\" option","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-08-13T14:44:41Z","receivedAt":"2019-08-13T14:45:06Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Palmer Dabbelt <palmer@sifive.com> wrote:\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 8aa6a0caf1ab..fa12ad44e7d9 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n\n<snip>\n\n> +static int next_remote_to_fetch(struct child_process *cp,\n> +\t\t\t\tstruct strbuf *out,\n> +\t\t\t\tvoid *state_uncast,\n> +\t\t\t\tvoid **task_state_out)\n> +{\n> +\tint i;\n> +\tstruct fetch_remote *state = state_uncast;\n> +\tstruct fetch_remote_task *task_state = NULL;\n> +\tconst char *remote_name;\n> +\n> +\tif (state->next_remote_index >= state->all_remotes->nr)\n> +\t\treturn 0;\n> +\n> +\tremote_name = state->all_remotes->items[state->next_remote_index].string;\n> +\tstate->next_remote_index++;\n> +\n> +\t/*\n> +\t * Finds somewhere to store the state for a task.  This is guarnteed to\n> +\t * succeed because there are always enough tasks allocated to cover the\n> +\t * number that have been requested to run in parallel.  Rather than\n> +\t * bothering with some sort of free list, this just brute force\n> +\t * searches for a free task.  The assumption is that there aren't that\n> +\t * many tasks to look through.\n> +\t */\n> +\tfor (i = 0; i < state->task_count; ++i) {\n> +\t\tif (!state->all_tasks[i].in_use) {\n> +\t\t\ttask_state = state->all_tasks + i;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n\nFwiw, I added list.h, the linked-list derived from the Linux\nkernel to simplify usage of free lists, queues, etc...\n\nI think it could improve readability, too; but I'm not really\na C programmer and prefer high-level scripting languages.\n"},{"id":"380415","messageId":"nycvar.QRO.7.76.6.1908132212420.656@tvgsbejvaqbjf.bet","threadId":"51633","inReplyTo":"20190812213448.2649-6-palmer@sifive.com","subject":"Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-08-13T20:16:11Z","receivedAt":"2019-08-13T20:16:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\n\nOn Mon, 12 Aug 2019, Palmer Dabbelt wrote:\n\n> The existing --jobs argument was defined to control the number of jobs\n> used for submodule fetching, but it makes more sense to have this\n> argument control the number of jobs to be used when fetching from\n> multiple remotes as well.\n>\n> This patch simply changes the --jobs argument parsing code to set both\n> max_children_for_{submodules,fetch}, as well as noting this new behavior\n> in the documentation.\n>\n> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>\n> ---\n\nI very much miss in this description a reflection of my analysis in\nhttps://public-inbox.org/git/nycvar.QRO.7.76.6.1907191507420.47@tvgsbejvaqbjf.bet/\n\nGiven that analysis, combined with the fact that the `--jobs` option\ntries to control both the `--multiple` and `--recursive-submodules` code\npaths in the end, anyway, I do doubt that it makes sense to even\nintroduce the `--fetch-jobs` and the `--submodule-fetch-jobs` options;\nThey are probably only confusing and do not add much benefit to the end\nuser.\n\nCiao,\nJohannes\n\n>  Documentation/fetch-options.txt |  4 ++++\n>  builtin/fetch.c                 | 21 ++++++++++++++++++---\n>  2 files changed, 22 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\n> index 5836024f1934..0915fd4ed6d5 100644\n> --- a/Documentation/fetch-options.txt\n> +++ b/Documentation/fetch-options.txt\n> @@ -160,6 +160,10 @@ ifndef::git-pull[]\n>\n>  -j::\n>  --jobs=<n>::\n> +\tNumber of parallel children to be used for all forms of fetching.\n> +\tThis is the same as passing `--submodule-fetch-jobs=<n>` and\n> +\t`--fetch-jobs=<n>`.\n> +\n>  --submodule-fetch-jobs=<n>::\n>  \tNumber of parallel children to be used for fetching submodules.\n>  \tEach will fetch from different submodules, such that fetching many\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 67d001f3f78b..41498e9efb3b 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n>  \treturn git_default_config(k, v, cb);\n>  }\n>\n> +static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tint jobs;\n> +\n> +\tjobs = atoi(arg);\n> +\tif (jobs < 1)\n> +\t\tdie(_(\"There must be a positive number of jobs\"));\n> +\n> +\tmax_children_for_submodules = jobs;\n> +\tmax_children_for_fetch = jobs;\n> +\n> +\treturn 0;\n> +}\n> +\n>  static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)\n>  {\n>  \tBUG_ON_OPT_NEG(unset);\n> @@ -142,12 +156,13 @@ 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_for_submodules,\n> +\t{ OPTION_CALLBACK, 'j', \"jobs\", NULL, N_(\"jobs\"),\n> +\t\t    N_(\"number of parallel tasks to run while fetching\"),\n> +\t\t    PARSE_OPT_OPTARG, &parse_jobs_arg },\n> +\tOPT_INTEGER(0, \"submodule-fetch-jobs\", &max_children_for_submodules,\n>  \t\t    N_(\"number of submodules fetched in parallel\")),\n>  \tOPT_INTEGER(0, \"fetch-jobs\", &max_children_for_fetch,\n>  \t\t    N_(\"number of remotes fetched in parallel\")),\n> -\tOPT_INTEGER(0, \"submodule-fetch-jobs\", &max_children_for_submodules,\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>  \tOPT_BOOL('P', \"prune-tags\", &prune_tags,\n> --\n> 2.21.0\n>\n>\n"},{"id":"380420","messageId":"xmqq7e7g7mky.fsf@gitster-ct.c.googlers.com","threadId":"51633","inReplyTo":"20190812213448.2649-6-palmer@sifive.com","subject":"Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-13T22:00:13Z","receivedAt":"2019-08-13T22:00:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Palmer Dabbelt <palmer@sifive.com> writes:\n\n> The existing --jobs argument was defined to control the number of jobs\n> used for submodule fetching, but it makes more sense to have this\n> argument control the number of jobs to be used when fetching from\n> multiple remotes as well.\n>\n> This patch simply changes the --jobs argument parsing code to set both\n> max_children_for_{submodules,fetch}, as well as noting this new behavior\n> in the documentation.\n\nThat's a sensible, if overly careful, transition plan.  This patch\ncannot be queued together with the other four, though, for the plan\nto be practical.  It probably needs to come a few releases after the\nother four hits a release.\n\nA less involved and much more careless transition plan may be to\njust declare that \"--jobs that only controls submodule fetches is a\nbug and it must also affect how fetches from multiple remote\nrepositories are done\" and come directly to this step, without\nnecessarily introducing options that control them independently.\n\nI have a suspicion that we can afford to go the careless route for\nthis particular one, if we wanted to, as not may people would care\nabout controlling these independently.\n\n"},{"id":"380421","messageId":"mhng-0255a7a4-dc9c-4b61-91d7-f06776c734af@palmer-si-x1e","threadId":"51633","inReplyTo":"nycvar.QRO.7.76.6.1908132212420.656@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes","fromName":"Palmer Dabbelt","fromEmail":"palmer@sifive.com","sentAt":"2019-08-13T22:06:35Z","receivedAt":"2019-08-13T22:06:38Z","isPatch":true,"sender":{"key":"palmer@sifive.com","avatar":"https://gravatar.com/avatar/bf8be091de4e6d14300576518ac2a35e0871bd4948140539ce9ef6efade35c09?d=mp&s=160"},"body":"On Tue, 13 Aug 2019 13:16:11 PDT (-0700), Johannes.Schindelin@gmx.de wrote:\n> Hi,\n>\n>\n> On Mon, 12 Aug 2019, Palmer Dabbelt wrote:\n>\n>> The existing --jobs argument was defined to control the number of jobs\n>> used for submodule fetching, but it makes more sense to have this\n>> argument control the number of jobs to be used when fetching from\n>> multiple remotes as well.\n>>\n>> This patch simply changes the --jobs argument parsing code to set both\n>> max_children_for_{submodules,fetch}, as well as noting this new behavior\n>> in the documentation.\n>>\n>> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>\n>> ---\n>\n> I very much miss in this description a reflection of my analysis in\n> https://public-inbox.org/git/nycvar.QRO.7.76.6.1907191507420.47@tvgsbejvaqbjf.bet/\n>\n> Given that analysis, combined with the fact that the `--jobs` option\n> tries to control both the `--multiple` and `--recursive-submodules` code\n> paths in the end, anyway, I do doubt that it makes sense to even\n> introduce the `--fetch-jobs` and the `--submodule-fetch-jobs` options;\n> They are probably only confusing and do not add much benefit to the end\n> user.\n\nThe cover letter at least attempts to describe this.  I figured I'd have to \npick one option for a v2, so I went with the more complicated one under the \nassumption it would be easy to re-spin a v3 that drops the extra arguments.  \nI'm happy to do so.\n\n>\n> Ciao,\n> Johannes\n>\n>>  Documentation/fetch-options.txt |  4 ++++\n>>  builtin/fetch.c                 | 21 ++++++++++++++++++---\n>>  2 files changed, 22 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\n>> index 5836024f1934..0915fd4ed6d5 100644\n>> --- a/Documentation/fetch-options.txt\n>> +++ b/Documentation/fetch-options.txt\n>> @@ -160,6 +160,10 @@ ifndef::git-pull[]\n>>\n>>  -j::\n>>  --jobs=<n>::\n>> +\tNumber of parallel children to be used for all forms of fetching.\n>> +\tThis is the same as passing `--submodule-fetch-jobs=<n>` and\n>> +\t`--fetch-jobs=<n>`.\n>> +\n>>  --submodule-fetch-jobs=<n>::\n>>  \tNumber of parallel children to be used for fetching submodules.\n>>  \tEach will fetch from different submodules, such that fetching many\n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index 67d001f3f78b..41498e9efb3b 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n>>  \treturn git_default_config(k, v, cb);\n>>  }\n>>\n>> +static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)\n>> +{\n>> +\tint jobs;\n>> +\n>> +\tjobs = atoi(arg);\n>> +\tif (jobs < 1)\n>> +\t\tdie(_(\"There must be a positive number of jobs\"));\n>> +\n>> +\tmax_children_for_submodules = jobs;\n>> +\tmax_children_for_fetch = jobs;\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>>  static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)\n>>  {\n>>  \tBUG_ON_OPT_NEG(unset);\n>> @@ -142,12 +156,13 @@ 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_for_submodules,\n>> +\t{ OPTION_CALLBACK, 'j', \"jobs\", NULL, N_(\"jobs\"),\n>> +\t\t    N_(\"number of parallel tasks to run while fetching\"),\n>> +\t\t    PARSE_OPT_OPTARG, &parse_jobs_arg },\n>> +\tOPT_INTEGER(0, \"submodule-fetch-jobs\", &max_children_for_submodules,\n>>  \t\t    N_(\"number of submodules fetched in parallel\")),\n>>  \tOPT_INTEGER(0, \"fetch-jobs\", &max_children_for_fetch,\n>>  \t\t    N_(\"number of remotes fetched in parallel\")),\n>> -\tOPT_INTEGER(0, \"submodule-fetch-jobs\", &max_children_for_submodules,\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>>  \tOPT_BOOL('P', \"prune-tags\", &prune_tags,\n>> --\n>> 2.21.0\n>>\n>>\n"},{"id":"380422","messageId":"mhng-f7e42958-08d9-4bbc-a745-4e40a67cb6f2@palmer-si-x1e","threadId":"51633","inReplyTo":"xmqq7e7g7mky.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes","fromName":"Palmer Dabbelt","fromEmail":"palmer@sifive.com","sentAt":"2019-08-13T22:06:36Z","receivedAt":"2019-08-13T22:06:41Z","isPatch":true,"sender":{"key":"palmer@sifive.com","avatar":"https://gravatar.com/avatar/bf8be091de4e6d14300576518ac2a35e0871bd4948140539ce9ef6efade35c09?d=mp&s=160"},"body":"On Tue, 13 Aug 2019 15:00:13 PDT (-0700), gitster@pobox.com wrote:\n> Palmer Dabbelt <palmer@sifive.com> writes:\n>\n>> The existing --jobs argument was defined to control the number of jobs\n>> used for submodule fetching, but it makes more sense to have this\n>> argument control the number of jobs to be used when fetching from\n>> multiple remotes as well.\n>>\n>> This patch simply changes the --jobs argument parsing code to set both\n>> max_children_for_{submodules,fetch}, as well as noting this new behavior\n>> in the documentation.\n>\n> That's a sensible, if overly careful, transition plan.  This patch\n> cannot be queued together with the other four, though, for the plan\n> to be practical.  It probably needs to come a few releases after the\n> other four hits a release.\n>\n> A less involved and much more careless transition plan may be to\n> just declare that \"--jobs that only controls submodule fetches is a\n> bug and it must also affect how fetches from multiple remote\n> repositories are done\" and come directly to this step, without\n> necessarily introducing options that control them independently.\n>\n> I have a suspicion that we can afford to go the careless route for\n> this particular one, if we wanted to, as not may people would care\n> about controlling these independently.\n\nThis was brought up as part of the v1, and the cover letter lays out a plan to \ndo so.  I'm happy to re-spin the patch set to just have --jobs control \neverything.\n"},{"id":"380428","messageId":"20190814083245.GP20404@szeder.dev","threadId":"51633","inReplyTo":"20190812213448.2649-6-palmer@sifive.com","subject":"Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-14T08:32:45Z","receivedAt":"2019-08-14T08:32:52Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Aug 12, 2019 at 02:34:48PM -0700, Palmer Dabbelt wrote:\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 67d001f3f78b..41498e9efb3b 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n>  \treturn git_default_config(k, v, cb);\n>  }\n>  \n> +static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tint jobs;\n> +\n> +\tjobs = atoi(arg);\n> +\tif (jobs < 1)\n> +\t\tdie(_(\"There must be a positive number of jobs\"));\n> +\n> +\tmax_children_for_submodules = jobs;\n> +\tmax_children_for_fetch = jobs;\n> +\n> +\treturn 0;\n> +}\n> +\n>  static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)\n>  {\n>  \tBUG_ON_OPT_NEG(unset);\n> @@ -142,12 +156,13 @@ 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_for_submodules,\n> +\t{ OPTION_CALLBACK, 'j', \"jobs\", NULL, N_(\"jobs\"),\n> +\t\t    N_(\"number of parallel tasks to run while fetching\"),\n> +\t\t    PARSE_OPT_OPTARG, &parse_jobs_arg },\n\nThese changes result segmentation faults in the tests '--quiet\npropagates to parallel submodules' and 'fetching submodules respects\nparallel settings' in 't5526-fetch-submodules.sh'.\n\nIf the number of jobs is specified as '-j 2' or '--jobs 7', i.e. as an\nunstuck argument of the option, as opposed to '-j2' or '--jobs=7',\nthen 'arg' in the parse_jobs_arg() callback is NULL, which then causes\nthe segfault somewhere inside that atoi() call.\n\n\n"},{"id":"380441","messageId":"xmqqy2zv68va.fsf@gitster-ct.c.googlers.com","threadId":"51633","inReplyTo":"20190814083245.GP20404@szeder.dev","subject":"Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-14T15:54:01Z","receivedAt":"2019-08-14T15:54:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> On Mon, Aug 12, 2019 at 02:34:48PM -0700, Palmer Dabbelt wrote:\n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index 67d001f3f78b..41498e9efb3b 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n>>  \treturn git_default_config(k, v, cb);\n>>  }\n>>  \n>> +static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)\n>> +{\n>> +\tint jobs;\n>> +\n>> +\tjobs = atoi(arg);\n>> +\tif (jobs < 1)\n>> +\t\tdie(_(\"There must be a positive number of jobs\"));\n>> +\n>> +\tmax_children_for_submodules = jobs;\n>> +\tmax_children_for_fetch = jobs;\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>>  static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)\n>>  {\n>>  \tBUG_ON_OPT_NEG(unset);\n>> @@ -142,12 +156,13 @@ 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_for_submodules,\n>> +\t{ OPTION_CALLBACK, 'j', \"jobs\", NULL, N_(\"jobs\"),\n>> +\t\t    N_(\"number of parallel tasks to run while fetching\"),\n>> +\t\t    PARSE_OPT_OPTARG, &parse_jobs_arg },\n>\n> These changes result segmentation faults in the tests '--quiet\n> propagates to parallel submodules' and 'fetching submodules respects\n> parallel settings' in 't5526-fetch-submodules.sh'.\n>\n> If the number of jobs is specified as '-j 2' or '--jobs 7', i.e. as an\n> unstuck argument of the option, as opposed to '-j2' or '--jobs=7',\n> then 'arg' in the parse_jobs_arg() callback is NULL, which then causes\n> the segfault somewhere inside that atoi() call.\n\nTrue.  \n\nAn easier and more readable way would be to set another \"nr-jobs\"\nvariable using plain vanilla OPT_INTEGER() and override the other\ntwo with it when they are not set after parse_options() returns, I\nguess.\n"},{"id":"380451","messageId":"mhng-0d288d1c-02fc-4280-bd8f-b7f611af3e8a@palmer-si-x1c4","threadId":"51633","inReplyTo":"20190814083245.GP20404@szeder.dev","subject":"Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes","fromName":"Palmer Dabbelt","fromEmail":"palmer@sifive.com","sentAt":"2019-08-14T18:33:15Z","receivedAt":"2019-08-14T18:33:19Z","isPatch":true,"sender":{"key":"palmer@sifive.com","avatar":"https://gravatar.com/avatar/bf8be091de4e6d14300576518ac2a35e0871bd4948140539ce9ef6efade35c09?d=mp&s=160"},"body":"On Wed, 14 Aug 2019 01:32:45 PDT (-0700), szeder.dev@gmail.com wrote:\n> On Mon, Aug 12, 2019 at 02:34:48PM -0700, Palmer Dabbelt wrote:\n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index 67d001f3f78b..41498e9efb3b 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n>>  \treturn git_default_config(k, v, cb);\n>>  }\n>>\n>> +static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)\n>> +{\n>> +\tint jobs;\n>> +\n>> +\tjobs = atoi(arg);\n>> +\tif (jobs < 1)\n>> +\t\tdie(_(\"There must be a positive number of jobs\"));\n>> +\n>> +\tmax_children_for_submodules = jobs;\n>> +\tmax_children_for_fetch = jobs;\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>>  static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)\n>>  {\n>>  \tBUG_ON_OPT_NEG(unset);\n>> @@ -142,12 +156,13 @@ 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_for_submodules,\n>> +\t{ OPTION_CALLBACK, 'j', \"jobs\", NULL, N_(\"jobs\"),\n>> +\t\t    N_(\"number of parallel tasks to run while fetching\"),\n>> +\t\t    PARSE_OPT_OPTARG, &parse_jobs_arg },\n>\n> These changes result segmentation faults in the tests '--quiet\n> propagates to parallel submodules' and 'fetching submodules respects\n> parallel settings' in 't5526-fetch-submodules.sh'.\n>\n> If the number of jobs is specified as '-j 2' or '--jobs 7', i.e. as an\n> unstuck argument of the option, as opposed to '-j2' or '--jobs=7',\n> then 'arg' in the parse_jobs_arg() callback is NULL, which then causes\n> the segfault somewhere inside that atoi() call.\n\nThanks, I'll fix that in the v3.\n"}]}