{"thread":{"id":"40655","subject":"[PATCH 6/9] clone: allow an explicit argument for parallel submodule clones","startedAt":"2015-10-27T18:15:44Z","lastAt":"2015-11-03T19:41:52Z","messageCount":48,"participants":["Stefan Beller","Junio C Hamano","Jonathan Nieder","Ramsay Jones","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"272329","messageId":"1445969753-418-1-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":null,"subject":"[PATCH 0/9] Expose the submodule parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-27T18:15:44Z","receivedAt":"2015-10-27T18:15:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Where does it apply?\n---\nThis applies on 376d400f4c (run-command: fix missing output from late callbacks,\nwhich is the latest commit in origin/sb/submodule-parallel-fetch which was\nmerged to origin/next)\nThe first patch is a duplicate of origin/sb/submodule-config-parse, so\nit may make sense to drop the first patch and apply this series on top of a\nmerge of 376d400f4c and origin/sb/submodule-config-parse.\n\nI realize sending refactorings in the area you'd be likely to touch as \na separate patch (series) is not necessarily a good idea as it leads to\nsituations like this.\n\nWhat does it do?\n---\nThis series should finish the on going efforts of parallelizing\nsubmodule network traffic. The patches contain tests for clone,\nfetch and submodule update to use the actual parallelism both via\ncommand line as well as a configured option. I decided to go with\n\"submodule.jobs\" for all three for now.\n\nDetailed breakdown of the patches\n---\n\nPatch 1 is a duplicate of origin/sb/submodule-config-parse and may make\nmerging with that easier.\n\nPatch 2 adds the update strategy to the struct submodule, which is required in\npatch 4.\n\nPatch 3 adds rudimentary tracing output to the parallel processing commands.\n\nPatch 4 rewrites parts of \"git submodule update\" in C, such that the cloning\nis done from within the parallel processing engine. \n\nPatch 5 however exposes the possible parallelism of patch 4 to the user.\n(doc + tests)\n\nPatch 6 adds the parallel feature to clone, which just invokes \"submodule update\"\ninternally.\n\nPatch 7 is a small refactoring preparing patch 8 to smoothly parse submodules.jobs.\n\nPatch 9 teaches fetch to respect the desired parallelism both from command line\nas well as the config option.\n\nThanks,\nStefan\n\nStefan Beller (9):\n submodule-config: \"goto\" removal in parse_config()\n submodule config: keep update strategy around\n run_processes_parallel: Add output to tracing messages\n git submodule update: have a dedicated helper for cloning\n submodule update: expose parallelism to the user\n clone: allow an explicit argument for parallel submodule clones\n submodule config: remove name_and_item_from_var\n submodule-config: parse_config\n fetching submodules: Respect `submodule.jobs` config option\n\n Documentation/config.txt | 7 ++\n Documentation/git-clone.txt | 5 +-\n Documentation/git-submodule.txt | 6 +-\n builtin/clone.c | 26 ++++-\n builtin/fetch.c | 2 +-\n builtin/submodule--helper.c | 243 ++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 54 ++++-----\n run-command.c | 4 +\n submodule-config.c | 166 ++++++++++++++-------------\n submodule-config.h | 3 +\n submodule.c | 5 +\n t/t5526-fetch-submodules.sh | 14 +++\n t/t7400-submodule-basic.sh | 4 +-\n t/t7406-submodule-update.sh | 27 +++++\n 14 files changed, 444 insertions(+), 122 deletions(-)\n\n-- \n2.5.0.283.g1a79c94.dirty\n"},{"id":"272328","messageId":"1445969753-418-2-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"1445969753-418-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/9] submodule-config: \"goto\" removal in parse_config()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-27T18:15:45Z","receivedAt":"2015-10-27T18:15:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Many components in if/else if/... cascade jumped to a shared\nclean-up with \"goto release_return\", but we can restructure the\nfunction a bit and make them disappear, which reduces the line count\nas well. Also reformat overlong lines and poorly indented ones\nwhile at it.\n\nThe order of rules to verify the value for \"ignore\" used to be to\ncomplain on multiple values first and then complain to boolean, but\nswap the order to match how the values for \"path\" and \"url\" are\nverified.\n\nSigned-off-by: Stefan Beller \n---\n submodule-config.c | 74 +++++++++++++++++++++---------------------------------\n 1 file changed, 29 insertions(+), 45 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 393de53..afe0ea8 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -257,78 +257,62 @@ static int parse_config(const char *var, const char *value, void *data)\n \tif (!name_and_item_from_var(var, &name, &item))\n \t\treturn 0;\n \n-\tsubmodule = lookup_or_create_by_name(me->cache, me->gitmodules_sha1,\n-\t\t\tname.buf);\n+\tsubmodule = lookup_or_create_by_name(me->cache,\n+\t\t\t\t\t me->gitmodules_sha1,\n+\t\t\t\t\t name.buf);\n \n \tif (!strcmp(item.buf, \"path\")) {\n-\t\tstruct strbuf path = STRBUF_INIT;\n-\t\tif (!value) {\n+\t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (!me->overwrite && submodule->path != NULL) {\n+\t\telse if (!me->overwrite && submodule->path != NULL)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"path\");\n-\t\t\tgoto release_return;\n+\t\telse {\n+\t\t\tif (submodule->path)\n+\t\t\t\tcache_remove_path(me->cache, submodule);\n+\t\t\tfree((void *) submodule->path);\n+\t\t\tsubmodule->path = xstrdup(value);\n+\t\t\tcache_put_path(me->cache, submodule);\n \t\t}\n-\n-\t\tif (submodule->path)\n-\t\t\tcache_remove_path(me->cache, submodule);\n-\t\tfree((void *) submodule->path);\n-\t\tstrbuf_addstr(&path, value);\n-\t\tsubmodule->path = strbuf_detach(&path, NULL);\n-\t\tcache_put_path(me->cache, submodule);\n \t} else if (!strcmp(item.buf, \"fetchrecursesubmodules\")) {\n \t\t/* when parsing worktree configurations we can die early */\n \t\tint die_on_error = is_null_sha1(me->gitmodules_sha1);\n \t\tif (!me->overwrite &&\n-\t\t submodule->fetch_recurse != RECURSE_SUBMODULES_NONE) {\n+\t\t submodule->fetch_recurse != RECURSE_SUBMODULES_NONE)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"fetchrecursesubmodules\");\n-\t\t\tgoto release_return;\n-\t\t}\n-\n-\t\tsubmodule->fetch_recurse = parse_fetch_recurse(var, value,\n+\t\telse\n+\t\t\tsubmodule->fetch_recurse = parse_fetch_recurse(\n+\t\t\t\t\t\t\t\tvar, value,\n \t\t\t\t\t\t\t\tdie_on_error);\n \t} else if (!strcmp(item.buf, \"ignore\")) {\n-\t\tstruct strbuf ignore = STRBUF_INIT;\n-\t\tif (!me->overwrite && submodule->ignore != NULL) {\n+\t\tif (!value)\n+\t\t\tret = config_error_nonbool(var);\n+\t\telse if (!me->overwrite && submodule->ignore != NULL)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"ignore\");\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (!value) {\n-\t\t\tret = config_error_nonbool(var);\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (strcmp(value, \"untracked\") && strcmp(value, \"dirty\") &&\n-\t\t strcmp(value, \"all\") && strcmp(value, \"none\")) {\n+\t\telse if (strcmp(value, \"untracked\") &&\n+\t\t\t strcmp(value, \"dirty\") &&\n+\t\t\t strcmp(value, \"all\") &&\n+\t\t\t strcmp(value, \"none\"))\n \t\t\twarning(\"Invalid parameter '%s' for config option \"\n \t\t\t\t\t\"'submodule.%s.ignore'\", value, var);\n-\t\t\tgoto release_return;\n+\t\telse {\n+\t\t\tfree((void *) submodule->ignore);\n+\t\t\tsubmodule->ignore = xstrdup(value);\n \t\t}\n-\n-\t\tfree((void *) submodule->ignore);\n-\t\tstrbuf_addstr(&ignore, value);\n-\t\tsubmodule->ignore = strbuf_detach(&ignore, NULL);\n \t} else if (!strcmp(item.buf, \"url\")) {\n-\t\tstruct strbuf url = STRBUF_INIT;\n \t\tif (!value) {\n \t\t\tret = config_error_nonbool(var);\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (!me->overwrite && submodule->url != NULL) {\n+\t\t} else if (!me->overwrite && submodule->url != NULL) {\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"url\");\n-\t\t\tgoto release_return;\n+\t\t} else {\n+\t\t\tfree((void *) submodule->url);\n+\t\t\tsubmodule->url = xstrdup(value);\n \t\t}\n-\n-\t\tfree((void *) submodule->url);\n-\t\tstrbuf_addstr(&url, value);\n-\t\tsubmodule->url = strbuf_detach(&url, NULL);\n \t}\n \n-release_return:\n \tstrbuf_release(&name);\n \tstrbuf_release(&item);\n \n-- \n2.5.0.283.g1a79c94.dirty\n"},{"id":"272323","messageId":"1445969753-418-3-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"1445969753-418-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/9] submodule config: keep update strategy around","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-27T18:15:46Z","receivedAt":"2015-10-27T18:15:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We need the submodule update strategies in a later patch.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n submodule-config.c | 11 +++++++++++\n submodule-config.h | 1 +\n 2 files changed, 12 insertions(+)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex afe0ea8..8b8c7d1 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -194,6 +194,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,\n \n \tsubmodule->path = NULL;\n \tsubmodule->url = NULL;\n+\tsubmodule->update = NULL;\n \tsubmodule->fetch_recurse = RECURSE_SUBMODULES_NONE;\n \tsubmodule->ignore = NULL;\n \n@@ -311,6 +312,16 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tfree((void *) submodule->url);\n \t\t\tsubmodule->url = xstrdup(value);\n \t\t}\n+\t} else if (!strcmp(item.buf, \"update\")) {\n+\t\tif (!value)\n+\t\t\tret = config_error_nonbool(var);\n+\t\telse if (!me->overwrite && submodule->update != NULL)\n+\t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n+\t\t\t\t\t \"update\");\n+\t\telse {\n+\t\t\tfree((void *)submodule->update);\n+\t\t\tsubmodule->update = xstrdup(value);\n+\t\t}\n \t}\n \n \tstrbuf_release(&name);\ndiff --git a/submodule-config.h b/submodule-config.h\nindex 9061e4e..f9e2a29 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -14,6 +14,7 @@ struct submodule {\n \tconst char *url;\n \tint fetch_recurse;\n \tconst char *ignore;\n+\tconst char *update;\n \t/* the sha1 blob id of the responsible .gitmodules file */\n \tunsigned char gitmodules_sha1[20];\n };\n-- \n2.5.0.283.g1a79c94.dirty\n"},{"id":"272324","messageId":"1445969753-418-4-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"1445969753-418-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/9] run_processes_parallel: Add output to tracing messages","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-27T18:15:47Z","receivedAt":"2015-10-27T18:15:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This commit serves 2 purposes. First this may help the user who\ntries to diagnose intermixed process calls. Second this may be used\nin a later patch for testing. As the output of a command should not\nchange visibly except for going faster, grepping for the trace output\nseems like a viable testing strategy.\n\nSigned-off-by: Stefan Beller \n---\n run-command.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 1fbd286..9ac2df5 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -949,6 +949,9 @@ static struct parallel_processes *pp_init(int n,\n \t\tn = online_cpus();\n \n \tpp->max_processes = n;\n+\n+\ttrace_printf(\"run_processes_parallel: preparing to run up to %d children in parallel\", n);\n+\n \tpp->data = data;\n \tif (!get_next_task)\n \t\tdie(\"BUG: you need to specify a get_next_task function\");\n@@ -978,6 +981,7 @@ static void pp_cleanup(struct parallel_processes *pp)\n {\n \tint i;\n \n+\ttrace_printf(\"run_processes_parallel: parallel processing done\");\n \tfor (i = 0; i < pp->max_processes; i++) {\n \t\tstrbuf_release(&pp->children[i].err);\n \t\tchild_process_deinit(&pp->children[i].process);\n-- \n2.5.0.283.g1a79c94.dirty\n"},{"id":"272321","messageId":"1445969753-418-5-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"1445969753-418-1-git-send-email-sbeller@google.com","subject":"[PATCH 4/9] git submodule update: have a dedicated helper for cloning","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-27T18:15:48Z","receivedAt":"2015-10-27T18:15:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This introduces a new helper function in git submodule--helper\nwhich takes care of cloning all submodules, which we want to\nparallelize eventually.\n\nSome tests (such as empty URL, update_mode=none) are required in the\nhelper to make the decision for cloning. These checks have been\nmoved into the C function as well (no need to repeat them in the\nshell script).\n\nAs we can only access the stderr channel from within the parallel\nprocessing engine, we need to reroute the error message for\nspecified but initialized submodules to stderr. As it is an error\nmessage, this should have gone to stderr in the first place, so it\nis a bug fix along the way.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n builtin/submodule--helper.c | 234 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 45 +++------\n t/t7400-submodule-basic.sh | 4 +-\n 3 files changed, 247 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff..1ec1b85 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -255,6 +255,239 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int git_submodule_config(const char *var, const char *value, void *cb)\n+{\n+\treturn parse_submodule_config_option(var, value);\n+}\n+\n+struct submodule_update_clone {\n+\tint count;\n+\tint quiet;\n+\tint print_unmatched;\n+\tchar *reference;\n+\tchar *depth;\n+\tchar *update;\n+\tconst char *recursive_prefix;\n+\tconst char *prefix;\n+\tstruct module_list list;\n+\tstruct string_list projectlines;\n+\tstruct pathspec pathspec;\n+};\n+#define SUBMODULE_UPDATE_CLONE_INIT {0, 0, 0, NULL, NULL, NULL, NULL, NULL, MODULE_LIST_INIT, STRING_LIST_INIT_DUP}\n+\n+static void fill_clone_command(struct child_process *cp, int quiet,\n+\t\t\t const char *prefix, const char *path,\n+\t\t\t const char *name, const char *url,\n+\t\t\t const char *reference, const char *depth)\n+{\n+\tcp->git_cmd = 1;\n+\tcp->no_stdin = 1;\n+\tcp->stdout_to_stderr = 1;\n+\tcp->err = -1;\n+\targv_array_push(&cp->args, \"submodule--helper\");\n+\targv_array_push(&cp->args, \"clone\");\n+\tif (quiet)\n+\t\targv_array_push(&cp->args, \"--quiet\");\n+\n+\tif (prefix) {\n+\t\targv_array_push(&cp->args, \"--prefix\");\n+\t\targv_array_push(&cp->args, prefix);\n+\t}\n+\targv_array_push(&cp->args, \"--path\");\n+\targv_array_push(&cp->args, path);\n+\n+\targv_array_push(&cp->args, \"--name\");\n+\targv_array_push(&cp->args, name);\n+\n+\targv_array_push(&cp->args, \"--url\");\n+\targv_array_push(&cp->args, url);\n+\tif (reference)\n+\t\targv_array_push(&cp->args, reference);\n+\tif (depth)\n+\t\targv_array_push(&cp->args, depth);\n+}\n+\n+static int update_clone_get_next_task(void **pp_task_cb,\n+\t\t\t\t struct child_process *cp,\n+\t\t\t\t struct strbuf *err,\n+\t\t\t\t void *pp_cb)\n+{\n+\tstruct submodule_update_clone *pp = pp_cb;\n+\n+\tfor (; pp->count < pp->list.nr; pp->count++) {\n+\t\tconst struct submodule *sub = NULL;\n+\t\tconst char *displaypath = NULL;\n+\t\tconst struct cache_entry *ce = pp->list.entries[pp->count];\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tconst char *update_module = NULL;\n+\t\tchar *url = NULL;\n+\t\tint just_cloned = 0;\n+\n+\t\tif (ce_stage(ce)) {\n+\t\t\tif (pp->recursive_prefix)\n+\t\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s/%s\\n\",\n+\t\t\t\t\tpp->recursive_prefix, ce->name);\n+\t\t\telse\n+\t\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s\\n\",\n+\t\t\t\t\tce->name);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tsub = submodule_from_path(null_sha1, ce->name);\n+\t\tif (!sub) {\n+\t\t\tstrbuf_addf(err, \"BUG: internal error managing submodules. \"\n+\t\t\t\t \"The cache could not locate '%s'\", ce->name);\n+\t\t\tpp->print_unmatched = 1;\n+\t\t\treturn 0;\n+\t\t}\n+\n+\t\tif (pp->recursive_prefix)\n+\t\t\tdisplaypath = relative_path(pp->recursive_prefix, ce->name, &sb);\n+\t\telse\n+\t\t\tdisplaypath = ce->name;\n+\n+\t\tif (pp->update)\n+\t\t\tupdate_module = pp->update;\n+\t\tif (!update_module)\n+\t\t\tupdate_module = sub->update;\n+\t\tif (!update_module)\n+\t\t\tupdate_module = \"checkout\";\n+\t\tif (!strcmp(update_module, \"none\")) {\n+\t\t\tstrbuf_addf(err, \"Skipping submodule '%s'\\n\", displaypath);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/*\n+\t\t * Looking up the url in .git/config.\n+\t\t * We cannot fall back to .gitmodules as we only want to process\n+\t\t * configured submodules. This renders the submodule lookup API\n+\t\t * useless, as it cannot lookup without fallback.\n+\t\t */\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n+\t\tgit_config_get_string(sb.buf, &url);\n+\t\tif (!url) {\n+\t\t\t/*\n+\t\t\t * Only mention uninitialized submodules when its\n+\t\t\t * path have been specified\n+\t\t\t */\n+\t\t\tif (pp->pathspec.nr)\n+\t\t\t\tstrbuf_addf(err, _(\"Submodule path '%s' not initialized\\n\"\n+\t\t\t\t\t\"Maybe you want to use 'update --init'?\"), displaypath);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n+\t\tjust_cloned = !file_exists(sb.buf);\n+\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_addf(&sb, \"%06o %s %d %d\\t%s\\n\", ce->ce_mode,\n+\t\t\t\tsha1_to_hex(ce->sha1), ce_stage(ce),\n+\t\t\t\tjust_cloned, ce->name);\n+\t\tstring_list_append(&pp->projectlines, sb.buf);\n+\n+\t\tif (just_cloned) {\n+\t\t\tfill_clone_command(cp, pp->quiet, pp->prefix, ce->name,\n+\t\t\t\t\t sub->name, url, pp->reference, pp->depth);\n+\t\t\tpp->count++;\n+\t\t\tfree(url);\n+\t\t\treturn 1;\n+\t\t} else\n+\t\t\tfree(url);\n+\t}\n+\treturn 0;\n+}\n+\n+static int update_clone_start_failure(struct child_process *cp,\n+\t\t\t\t struct strbuf *err,\n+\t\t\t\t void *pp_cb,\n+\t\t\t\t void *pp_task_cb)\n+{\n+\tstruct submodule_update_clone *pp = pp_cb;\n+\n+\tstrbuf_addf(err, \"error when starting a child process\");\n+\tpp->print_unmatched = 1;\n+\n+\treturn 1;\n+}\n+\n+static int update_clone_task_finished(int result,\n+\t\t\t\t struct child_process *cp,\n+\t\t\t\t struct strbuf *err,\n+\t\t\t\t void *pp_cb,\n+\t\t\t\t void *pp_task_cb)\n+{\n+\tstruct submodule_update_clone *pp = pp_cb;\n+\n+\tif (!result) {\n+\t\treturn 0;\n+\t} else {\n+\t\tstrbuf_addf(err, \"error in one child process\");\n+\t\tpp->print_unmatched = 1;\n+\t\treturn 1;\n+\t}\n+}\n+\n+static int update_clone(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct string_list_item *item;\n+\tstruct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;\n+\n+\tstruct option module_list_options[] = {\n+\t\tOPT_STRING(0, \"prefix\", &prefix,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"path into the working tree\")),\n+\t\tOPT_STRING(0, \"recursive_prefix\", &pp.recursive_prefix,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"path into the working tree, across nested \"\n+\t\t\t \"submodule boundaries\")),\n+\t\tOPT_STRING(0, \"update\", &pp.update,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"update command for submodules\")),\n+\t\tOPT_STRING(0, \"reference\", &pp.reference, \"\",\n+\t\t\t N_(\"Use the local reference repository \"\n+\t\t\t \"instead of a full clone\")),\n+\t\tOPT_STRING(0, \"depth\", &pp.depth, \"\",\n+\t\t\t N_(\"Create a shallow clone truncated to the \"\n+\t\t\t \"specified number of revisions\")),\n+\t\tOPT__QUIET(&pp.quiet, N_(\"do't print cloning progress\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper list [--prefix=] [...]\"),\n+\t\tNULL\n+\t};\n+\tpp.prefix = prefix;\n+\n+\targc = parse_options(argc, argv, prefix, module_list_options,\n+\t\t\t git_submodule_helper_usage, 0);\n+\n+\tif (module_list_compute(argc, argv, prefix, &pp.pathspec, &pp.list) < 0) {\n+\t\tprintf(\"#unmatched\\n\");\n+\t\treturn 1;\n+\t}\n+\n+\tgitmodules_config();\n+\t/* Overlay the parsed .gitmodules file with .git/config */\n+\tgit_config(git_submodule_config, NULL);\n+\trun_processes_parallel(1, update_clone_get_next_task,\n+\t\t\t\t update_clone_start_failure,\n+\t\t\t\t update_clone_task_finished,\n+\t\t\t\t &pp);\n+\n+\tif (pp.print_unmatched) {\n+\t\tprintf(\"#unmatched\\n\");\n+\t\treturn 1;\n+\t}\n+\n+\tfor_each_string_list_item(item, &pp.projectlines) {\n+\t\tutf8_fprintf(stdout, \"%s\", item->string);\n+\t}\n+\treturn 0;\n+}\n+\n struct cmd_struct {\n \tconst char *cmd;\n \tint (*fn)(int, const char **, const char *);\n@@ -264,6 +497,7 @@ static struct cmd_struct commands[] = {\n \t{\"list\", module_list},\n \t{\"name\", module_name},\n \t{\"clone\", module_clone},\n+\t{\"update-clone\", update_clone}\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 8b0eb9a..ea883b9 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -655,17 +655,18 @@ cmd_update()\n \t\tcmd_init \"--\" \"$@\" || return\n \tfi\n \n-\tcloned_modules=\n-\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n+\tgit submodule--helper update-clone ${GIT_QUIET:+--quiet} \\\n+\t\t${wt_prefix:+--prefix \"$wt_prefix\"} \\\n+\t\t${prefix:+--recursive_prefix \"$prefix\"} \\\n+\t\t${update:+--update \"$update\"} \\\n+\t\t${reference:+--reference \"$reference\"} \\\n+\t\t${depth:+--depth \"$depth\"} \\\n+\t\t\"$@\" | {\n \terr=\n-\twhile read mode sha1 stage sm_path\n+\twhile read mode sha1 stage just_cloned sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tif test \"$stage\" = U\n-\t\tthen\n-\t\t\techo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n-\t\t\tcontinue\n-\t\tfi\n+\n \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tbranch=$(get_submodule_config \"$name\" branch master)\n@@ -682,27 +683,10 @@ cmd_update()\n \n \t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n \n-\t\tif test \"$update_module\" = \"none\"\n-\t\tthen\n-\t\t\techo \"Skipping submodule '$displaypath'\"\n-\t\t\tcontinue\n-\t\tfi\n-\n-\t\tif test -z \"$url\"\n-\t\tthen\n-\t\t\t# Only mention uninitialized submodules when its\n-\t\t\t# path have been specified\n-\t\t\ttest \"$#\" != \"0\" &&\n-\t\t\tsay \"$(eval_gettext \"Submodule path '\\$displaypath' not initialized\n-Maybe you want to use 'update --init'?\")\"\n-\t\t\tcontinue\n-\t\tfi\n-\n-\t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n+\t\tif test $just_cloned -eq 1\n \t\tthen\n-\t\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$prefix\" --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n-\t\t\tcloned_modules=\"$cloned_modules;$name\"\n \t\t\tsubsha1=\n+\t\t\tupdate_module=checkout\n \t\telse\n \t\t\tsubsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n@@ -742,13 +726,6 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n \t\t\tfi\n \n-\t\t\t# Is this something we just cloned?\n-\t\t\tcase \";$cloned_modules;\" in\n-\t\t\t*\";$name;\"*)\n-\t\t\t\t# then there is no local change to integrate\n-\t\t\t\tupdate_module=checkout ;;\n-\t\t\tesac\n-\n \t\t\tmust_die_on_failure=\n \t\t\tcase \"$update_module\" in\n \t\t\tcheckout)\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 540771c..5991e3c 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -462,7 +462,7 @@ test_expect_success 'update --init' '\n \tgit config --remove-section submodule.example &&\n \ttest_must_fail git config submodule.example.url &&\n \n-\tgit submodule update init > update.out &&\n+\tgit submodule update init 2> update.out &&\n \tcat update.out &&\n \ttest_i18ngrep \"not initialized\" update.out &&\n \ttest_must_fail git rev-parse --resolve-git-dir init/.git &&\n@@ -480,7 +480,7 @@ test_expect_success 'update --init from subdirectory' '\n \tmkdir -p sub &&\n \t(\n \t\tcd sub &&\n-\t\tgit submodule update ../init >update.out &&\n+\t\tgit submodule update ../init 2>update.out &&\n \t\tcat update.out &&\n \t\ttest_i18ngrep \"not initialized\" update.out &&\n \t\ttest_must_fail git rev-parse --resolve-git-dir ../init/.git &&\n-- \n2.5.0.283.g1a79c94.dirty\n"},{"id":"272325","messageId":"1445969753-418-6-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"1445969753-418-1-git-send-email-sbeller@google.com","subject":"[PATCH 5/9] submodule update: expose parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-27T18:15:49Z","receivedAt":"2015-10-27T18:15:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Expose possible parallelism either via the \"--jobs\" CLI parameter or\nthe \"submodule.jobs\" setting.\n\nBy having the variable initialized to -1, we make sure 0 can be passed\ninto the parallel processing machine, which will then pick as many parallel\nworkers as there are CPUs.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/git-submodule.txt | 6 +++++-\n builtin/submodule--helper.c | 17 +++++++++++++----\n git-submodule.sh | 9 +++++++++\n t/t7406-submodule-update.sh | 12 ++++++++++++\n 4 files changed, 39 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex f17687e..f5429fa 100644\n--- a/Documentation/git-submodule.txt\n+++ b/Documentation/git-submodule.txt\n@@ -16,7 +16,7 @@ SYNOPSIS\n 'git submodule' [--quiet] deinit [-f|--force] [--] ...\n 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]\n \t [-f|--force] [--rebase|--merge] [--reference ]\n-\t [--depth ] [--recursive] [--] [...]\n+\t [--depth ] [--recursive] [--jobs ] [--] [...]\n 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]\n \t [commit] [--] [...]\n 'git submodule' [--quiet] foreach [--recursive] \n@@ -374,6 +374,10 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` options carefully.\n \tclone with a history truncated to the specified number of revisions.\n \tSee linkgit:git-clone[1]\n \n+-j::\n+--jobs::\n+\tThis option is only valid for the update command.\n+\tClone new submodules in parallel with as many jobs.\n \n ...::\n \tPaths to submodule(s). When specified this will restrict the command\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1ec1b85..c3d438a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -431,6 +431,7 @@ static int update_clone_task_finished(int result,\n \n static int update_clone(int argc, const char **argv, const char *prefix)\n {\n+\tint max_jobs = -1;\n \tstruct string_list_item *item;\n \tstruct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;\n \n@@ -451,6 +452,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"depth\", &pp.depth, \"\",\n \t\t\t N_(\"Create a shallow clone truncated to the \"\n \t\t\t \"specified number of revisions\")),\n+\t\tOPT_INTEGER('j', \"jobs\", &max_jobs,\n+\t\t\t N_(\"parallel jobs\")),\n \t\tOPT__QUIET(&pp.quiet, N_(\"do't print cloning progress\")),\n \t\tOPT_END()\n \t};\n@@ -472,10 +475,16 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \tgitmodules_config();\n \t/* Overlay the parsed .gitmodules file with .git/config */\n \tgit_config(git_submodule_config, NULL);\n-\trun_processes_parallel(1, update_clone_get_next_task,\n-\t\t\t\t update_clone_start_failure,\n-\t\t\t\t update_clone_task_finished,\n-\t\t\t\t &pp);\n+\n+\tif (max_jobs == -1)\n+\t\tif (git_config_get_int(\"submodule.jobs\", &max_jobs))\n+\t\t\tmax_jobs = 1;\n+\n+\trun_processes_parallel(max_jobs,\n+\t\t\t update_clone_get_next_task,\n+\t\t\t update_clone_start_failure,\n+\t\t\t update_clone_task_finished,\n+\t\t\t &pp);\n \n \tif (pp.print_unmatched) {\n \t\tprintf(\"#unmatched\\n\");\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex ea883b9..c2dfb16 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -636,6 +636,14 @@ cmd_update()\n \t\t--depth=*)\n \t\t\tdepth=$1\n \t\t\t;;\n+\t\t-j|--jobs)\n+\t\t\tcase \"$2\" in '') usage ;; esac\n+\t\t\tjobs=\"--jobs=$2\"\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--jobs=*)\n+\t\t\tjobs=$1\n+\t\t\t;;\n \t\t--)\n \t\t\tshift\n \t\t\tbreak\n@@ -661,6 +669,7 @@ cmd_update()\n \t\t${update:+--update \"$update\"} \\\n \t\t${reference:+--reference \"$reference\"} \\\n \t\t${depth:+--depth \"$depth\"} \\\n+\t\t${jobs:+$jobs} \\\n \t\t\"$@\" | {\n \terr=\n \twhile read mode sha1 stage just_cloned sm_path\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex dda3929..05ea66f 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops module name before recur\n \t test_i18ngrep \"Submodule path .deeper/submodule/subsubmodule.: checked out\" actual\n \t)\n '\n+\n+test_expect_success 'submodule update can be run in parallel' '\n+\t(cd super2 &&\n+\t GIT_TRACE=$(pwd)/trace.out git submodule update --jobs 7 &&\n+\t grep \"7 children\" trace.out &&\n+\t git config submodule.jobs 8 &&\n+\t GIT_TRACE=$(pwd)/trace.out git submodule update &&\n+\t grep \"8 children\" trace.out &&\n+\t GIT_TRACE=$(pwd)/trace.out git submodule update --jobs 9 &&\n+\t grep \"9 children\" trace.out\n+\t)\n+'\n test_done\n-- \n2.5.0.283.g1a79c94.dirty\n"},{"id":"272320","messageId":"1445969753-418-7-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"1445969753-418-1-git-send-email-sbeller@google.com","subject":"[PATCH 6/9] clone: allow an explicit argument for parallel submodule clones","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-27T18:15:50Z","receivedAt":"2015-10-27T18:15:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Just pass it along to \"git submodule update\", which may pick reasonable\ndefaults if you don't specify an explicit number.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/git-clone.txt | 5 ++++-\n builtin/clone.c | 26 ++++++++++++++++++++------\n t/t7406-submodule-update.sh | 15 +++++++++++++++\n 3 files changed, 39 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex f1f2a3f..affa52e 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -14,7 +14,7 @@ SYNOPSIS\n \t [-o ] [-b ] [-u ] [--reference ]\n \t [--dissociate] [--separate-git-dir ]\n \t [--depth ] [--[no-]single-branch]\n-\t [--recursive | --recurse-submodules] [--] \n+\t [--recursive | --recurse-submodules] [--jobs ] [--] \n \t []\n \n DESCRIPTION\n@@ -216,6 +216,9 @@ objects from the source repository into a pack in the cloned repository.\n \tThe result is Git repository can be separated from working\n \ttree.\n \n+-j::\n+--jobs::\n+\tThe number of submodules fetched at the same time.\n \n ::\n \tThe (possibly remote) repository to clone from. See the\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 5864ad1..b8b1d4c 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -50,6 +50,7 @@ static int option_progress = -1;\n static struct string_list option_config;\n static struct string_list option_reference;\n static int option_dissociate;\n+static int max_jobs = -1;\n \n static struct option builtin_clone_options[] = {\n \tOPT__VERBOSITY(&option_verbosity),\n@@ -72,6 +73,8 @@ static struct option builtin_clone_options[] = {\n \t\t N_(\"initialize submodules in the clone\")),\n \tOPT_BOOL(0, \"recurse-submodules\", &option_recursive,\n \t\t N_(\"initialize submodules in the clone\")),\n+\tOPT_INTEGER('j', \"jobs\", &max_jobs,\n+\t\t N_(\"number of submodules cloned in parallel\")),\n \tOPT_STRING(0, \"template\", &option_template, N_(\"template-directory\"),\n \t\t N_(\"directory from which templates will be used\")),\n \tOPT_STRING_LIST(0, \"reference\", &option_reference, N_(\"repo\"),\n@@ -95,10 +98,6 @@ static struct option builtin_clone_options[] = {\n \tOPT_END()\n };\n \n-static const char *argv_submodule[] = {\n-\t\"submodule\", \"update\", \"--init\", \"--recursive\", NULL\n-};\n-\n static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)\n {\n \tstatic char *suffix[] = { \"/.git\", \"\", \".git/.git\", \".git\" };\n@@ -674,8 +673,23 @@ static int checkout(void)\n \terr |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n \t\t\t sha1_to_hex(sha1), \"1\", NULL);\n \n-\tif (!err && option_recursive)\n-\t\terr = run_command_v_opt(argv_submodule, RUN_GIT_CMD);\n+\tif (!err && option_recursive) {\n+\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\t\targv_array_pushl(&args, \"submodule\", \"update\", \"--init\", \"--recursive\", NULL);\n+\n+\t\tif (max_jobs == -1)\n+\t\t\tif (git_config_get_int(\"submodule.jobs\", &max_jobs))\n+\t\t\t\tmax_jobs = 1;\n+\t\tif (max_jobs != 1) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_addf(&sb, \"--jobs=%d\", max_jobs);\n+\t\t\targv_array_push(&args, sb.buf);\n+\t\t\tstrbuf_release(&sb);\n+\t\t}\n+\n+\t\terr = run_command_v_opt(args.argv, RUN_GIT_CMD);\n+\t\targv_array_clear(&args);\n+\t}\n \n \treturn err;\n }\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 05ea66f..ade0524 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -786,4 +786,19 @@ test_expect_success 'submodule update can be run in parallel' '\n \t grep \"9 children\" trace.out\n \t)\n '\n+\n+test_expect_success 'git clone passes the parallel jobs config on to submodules' '\n+\ttest_when_finished \"rm -rf super4\" &&\n+\tGIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 . super4 &&\n+\tgrep \"7 children\" trace.out &&\n+\trm -rf super4 &&\n+\tgit config --global submodule.jobs 8 &&\n+\tGIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules . super4 &&\n+\tgrep \"8 children\" trace.out &&\n+\trm -rf super4 &&\n+\tGIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 9 . super4 &&\n+\tgrep \"9 children\" trace.out &&\n+\trm -rf super4\n+'\n+\n test_done\n-- \n2.5.0.283.g1a79c94.dirty\n"},{"id":"272322","messageId":"1445969753-418-8-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"1445969753-418-1-git-send-email-sbeller@google.com","subject":"[PATCH 7/9] submodule config: remove name_and_item_from_var","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-27T18:15:51Z","receivedAt":"2015-10-27T18:15:51Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"By inlining `name_and_item_from_var` it is easy to add later options\nwhich are not required to have a submodule name.\n\nSigned-off-by: Stefan Beller \n---\n submodule-config.c | 46 +++++++++++++++++-----------------------------\n 1 file changed, 17 insertions(+), 29 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 8b8c7d1..4d0563c 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -161,22 +161,6 @@ static struct submodule *cache_lookup_name(struct submodule_cache *cache,\n \treturn NULL;\n }\n \n-static int name_and_item_from_var(const char *var, struct strbuf *name,\n-\t\t\t\t struct strbuf *item)\n-{\n-\tconst char *subsection, *key;\n-\tint subsection_len, parse;\n-\tparse = parse_config_key(var, \"submodule\", &subsection,\n-\t\t\t&subsection_len, &key);\n-\tif (parse < 0 || !subsection)\n-\t\treturn 0;\n-\n-\tstrbuf_add(name, subsection, subsection_len);\n-\tstrbuf_addstr(item, key);\n-\n-\treturn 1;\n-}\n-\n static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,\n \t\tconst unsigned char *gitmodules_sha1, const char *name)\n {\n@@ -251,18 +235,25 @@ static int parse_config(const char *var, const char *value, void *data)\n {\n \tstruct parse_config_parameter *me = data;\n \tstruct submodule *submodule;\n-\tstruct strbuf name = STRBUF_INIT, item = STRBUF_INIT;\n-\tint ret = 0;\n+\tint subsection_len, ret = 0;\n+\tconst char *subsection, *key;\n+\tchar *name;\n \n-\t/* this also ensures that we only parse submodule entries */\n-\tif (!name_and_item_from_var(var, &name, &item))\n+\tif (parse_config_key(var, \"submodule\", &subsection,\n+\t\t\t &subsection_len, &key) < 0)\n \t\treturn 0;\n \n+\tif (!subsection_len)\n+\t\treturn 0;\n+\n+\t/* subsection is not null terminated */\n+\tname = xmemdupz(subsection, subsection_len);\n \tsubmodule = lookup_or_create_by_name(me->cache,\n \t\t\t\t\t me->gitmodules_sha1,\n-\t\t\t\t\t name.buf);\n+\t\t\t\t\t name);\n+\tfree(name);\n \n-\tif (!strcmp(item.buf, \"path\")) {\n+\tif (!strcmp(key, \"path\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->path != NULL)\n@@ -275,7 +266,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tsubmodule->path = xstrdup(value);\n \t\t\tcache_put_path(me->cache, submodule);\n \t\t}\n-\t} else if (!strcmp(item.buf, \"fetchrecursesubmodules\")) {\n+\t} else if (!strcmp(key, \"fetchrecursesubmodules\")) {\n \t\t/* when parsing worktree configurations we can die early */\n \t\tint die_on_error = is_null_sha1(me->gitmodules_sha1);\n \t\tif (!me->overwrite &&\n@@ -286,7 +277,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tsubmodule->fetch_recurse = parse_fetch_recurse(\n \t\t\t\t\t\t\t\tvar, value,\n \t\t\t\t\t\t\t\tdie_on_error);\n-\t} else if (!strcmp(item.buf, \"ignore\")) {\n+\t} else if (!strcmp(key, \"ignore\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->ignore != NULL)\n@@ -302,7 +293,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tfree((void *) submodule->ignore);\n \t\t\tsubmodule->ignore = xstrdup(value);\n \t\t}\n-\t} else if (!strcmp(item.buf, \"url\")) {\n+\t} else if (!strcmp(key, \"url\")) {\n \t\tif (!value) {\n \t\t\tret = config_error_nonbool(var);\n \t\t} else if (!me->overwrite && submodule->url != NULL) {\n@@ -312,7 +303,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tfree((void *) submodule->url);\n \t\t\tsubmodule->url = xstrdup(value);\n \t\t}\n-\t} else if (!strcmp(item.buf, \"update\")) {\n+\t} else if (!strcmp(key, \"update\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->update != NULL)\n@@ -324,9 +315,6 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t}\n \t}\n \n-\tstrbuf_release(&name);\n-\tstrbuf_release(&item);\n-\n \treturn ret;\n }\n \n-- \n2.5.0.283.g1a79c94.dirty\n"},{"id":"272327","messageId":"1445969753-418-9-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"1445969753-418-1-git-send-email-sbeller@google.com","subject":"[PATCH 8/9] submodule-config: parse_config","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-27T18:15:52Z","receivedAt":"2015-10-27T18:15:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This rewrites parse_config to distinguish between configs specific to\none submodule and configs which apply generically to all submodules.\nWe do not have generic submodule configs yet, but the next patch will\nintroduce \"submodule.jobs\".\n\nSigned-off-by: Stefan Beller \n\n# Conflicts:\n#\tsubmodule-config.c\n\nSigned-off-by: Stefan Beller \n---\n submodule-config.c | 58 ++++++++++++++++++++++++++++++++++++------------------\n 1 file changed, 39 insertions(+), 19 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 4d0563c..1cea404 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -231,27 +231,23 @@ struct parse_config_parameter {\n \tint overwrite;\n };\n \n-static int parse_config(const char *var, const char *value, void *data)\n+static int parse_generic_submodule_config(const char *var,\n+\t\t\t\t\t const char *key,\n+\t\t\t\t\t const char *value)\n {\n-\tstruct parse_config_parameter *me = data;\n-\tstruct submodule *submodule;\n-\tint subsection_len, ret = 0;\n-\tconst char *subsection, *key;\n-\tchar *name;\n-\n-\tif (parse_config_key(var, \"submodule\", &subsection,\n-\t\t\t &subsection_len, &key) < 0)\n-\t\treturn 0;\n-\n-\tif (!subsection_len)\n-\t\treturn 0;\n+\treturn 0;\n+}\n \n-\t/* subsection is not null terminated */\n-\tname = xmemdupz(subsection, subsection_len);\n-\tsubmodule = lookup_or_create_by_name(me->cache,\n-\t\t\t\t\t me->gitmodules_sha1,\n-\t\t\t\t\t name);\n-\tfree(name);\n+static int parse_specific_submodule_config(struct parse_config_parameter *me,\n+\t\t\t\t\t const char *name,\n+\t\t\t\t\t const char *key,\n+\t\t\t\t\t const char *value,\n+\t\t\t\t\t const char *var)\n+{\n+\tint ret = 0;\n+\tstruct submodule *submodule = lookup_or_create_by_name(me->cache,\n+\t\t\t\t\t\t\t me->gitmodules_sha1,\n+\t\t\t\t\t\t\t name);\n \n \tif (!strcmp(key, \"path\")) {\n \t\tif (!value)\n@@ -318,6 +314,30 @@ static int parse_config(const char *var, const char *value, void *data)\n \treturn ret;\n }\n \n+static int parse_config(const char *var, const char *value, void *data)\n+{\n+\tstruct parse_config_parameter *me = data;\n+\n+\tint subsection_len;\n+\tconst char *subsection, *key;\n+\tchar *name;\n+\n+\tif (parse_config_key(var, \"submodule\", &subsection,\n+\t\t\t &subsection_len, &key) < 0)\n+\t\treturn 0;\n+\n+\tif (!subsection_len)\n+\t\treturn parse_generic_submodule_config(var, key, value);\n+\telse {\n+\t\tint ret;\n+\t\t/* subsection is not null terminated */\n+\t\tname = xmemdupz(subsection, subsection_len);\n+\t\tret = parse_specific_submodule_config(me, name, key, value, var);\n+\t\tfree(name);\n+\t\treturn ret;\n+\t}\n+}\n+\n static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,\n \t\t\t\t unsigned char *gitmodules_sha1)\n {\n-- \n2.5.0.283.g1a79c94.dirty\n"},{"id":"272326","messageId":"1445969753-418-10-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"1445969753-418-1-git-send-email-sbeller@google.com","subject":"[PATCH 9/9] fetching submodules: Respect `submodule.jobs` config option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-27T18:15:53Z","receivedAt":"2015-10-27T18:15:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This allows to configure fetching and updating in parallel\nwithout having the command line option.\n\nThis moved the responsibility to determine how many parallel processes\nto start from builtin/fetch to submodule.c as we need a way to communicate\n\"The user did not specify the number of parallel processes in the command\nline options\" in the builtin fetch. The submodule code takes care of\nthe precedence (CLI > config > default)\n\nSigned-off-by: Stefan Beller \n---\n Documentation/config.txt | 7 +++++++\n builtin/fetch.c | 2 +-\n submodule-config.c | 9 +++++++++\n submodule-config.h | 2 ++\n submodule.c | 5 +++++\n t/t5526-fetch-submodules.sh | 14 ++++++++++++++\n 6 files changed, 38 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 315f271..0b733d7 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2575,6 +2575,13 @@ submodule..ignore::\n \t\"--ignore-submodules\" option. The 'git submodule' commands are not\n \taffected by this setting.\n \n+submodule::jobs\n+\tThis is used to determine how many submodules can be operated on in\n+\tparallel. Specifying a positive integer allows up to that number\n+\tof submodules being fetched in parallel. Specifying 0 the number\n+\tof cpus will be taken as the maximum number. Currently this is\n+\tused in fetch and clone operations only.\n+\n tag.sort::\n \tThis variable controls the sort ordering of tags when displayed by\n \tlinkgit:git-tag[1]. Without the \"--sort=\" option provided, the\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f28eac6..b1399dc 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -37,7 +37,7 @@ static int prune = -1; /* unspecified */\n static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity;\n static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow;\n-static int max_children = 1;\n+static int max_children = -1;\n static const char *depth;\n static const char *upload_pack;\n static struct strbuf default_rla = STRBUF_INIT;\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 1cea404..07bdcdf 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -32,6 +32,7 @@ enum lookup_type {\n \n static struct submodule_cache cache;\n static int is_cache_init;\n+static int parallel_jobs = -1;\n \n static int config_path_cmp(const struct submodule_entry *a,\n \t\t\t const struct submodule_entry *b,\n@@ -235,6 +236,9 @@ static int parse_generic_submodule_config(const char *var,\n \t\t\t\t\t const char *key,\n \t\t\t\t\t const char *value)\n {\n+\tif (!strcmp(key, \"jobs\")) {\n+\t\tparallel_jobs = strtol(value, NULL, 10);\n+\t}\n \treturn 0;\n }\n \n@@ -483,3 +487,8 @@ void submodule_free(void)\n \tcache_free(&cache);\n \tis_cache_init = 0;\n }\n+\n+int config_parallel_submodules(void)\n+{\n+\treturn parallel_jobs;\n+}\ndiff --git a/submodule-config.h b/submodule-config.h\nindex f9e2a29..d9bbf9a 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -27,4 +27,6 @@ const struct submodule *submodule_from_path(const unsigned char *commit_sha1,\n \t\tconst char *path);\n void submodule_free(void);\n \n+int config_parallel_submodules(void);\n+\n #endif /* SUBMODULE_CONFIG_H */\ndiff --git a/submodule.c b/submodule.c\nindex c21b265..4822605 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -759,6 +759,11 @@ int fetch_populated_submodules(const struct argv_array *options,\n \targv_array_push(&spf.args, \"--recurse-submodules-default\");\n \t/* default value, \"--submodule-prefix\" and its value are added later */\n \n+\tif (max_parallel_jobs < 0)\n+\t\tmax_parallel_jobs = config_parallel_submodules();\n+\tif (max_parallel_jobs < 0)\n+\t\tmax_parallel_jobs = 1;\n+\n \tcalculate_changed_submodule_paths();\n \trun_processes_parallel(max_parallel_jobs,\n \t\t\t get_next_submodule,\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 1b4ce69..5c3579c 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -470,4 +470,18 @@ test_expect_success \"don't fetch submodule when newly recorded commits are alrea\n \ttest_i18ncmp expect.err actual.err\n '\n \n+test_expect_success 'fetching submodules respects parallel settings' '\n+\tgit config fetch.recurseSubmodules true &&\n+\t(\n+\t\tcd downstream &&\n+\t\tGIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 &&\n+\t\tgrep \"7 children\" trace.out &&\n+\t\tgit config submodule.jobs 8 &&\n+\t\tGIT_TRACE=$(pwd)/trace.out git fetch &&\n+\t\tgrep \"8 children\" trace.out &&\n+\t\tGIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&\n+\t\tgrep \"9 children\" trace.out\n+\t)\n+'\n+\n test_done\n-- \n2.5.0.283.g1a79c94.dirty\n"},{"id":"272336","messageId":"xmqqfv0wp1l1.fsf@gitster.mtv.corp.google.com","threadId":"40655","inReplyTo":"1445969753-418-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH 0/9] Expose the submodule parallelism to the user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-27T19:12:58Z","receivedAt":"2015-10-27T19:12:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Where does it apply?\n> ---\n> This applies on 376d400f4c (run-command: fix missing output from late callbacks,\n> which is the latest commit in origin/sb/submodule-parallel-fetch which was\n> merged to origin/next)\n\nThanks for a detailed description. I'd do this:\n\n $ git checkout -b sb/submodule-parallel-update 8b70042\n $ git merge sb/submodule-parallel-fetch~4 ;# 376d400f4c\n\napply 2-9 there (the fork point is the merge of config-parse topic\nto 'master'), and drop the four patches near the top of the other\nbranch.\n\n> I realize sending refactorings in the area you'd be likely to touch as \n> a separate patch (series) is not necessarily a good idea as it leads to\n> situations like this.\n\nDon't worry too much about it. When you tackle a large area with a\nlot of existing code, these things are bound to happen.\n\n> What does it do?\n> ---\n> This series should finish the on going efforts of parallelizing\n> submodule network traffic. The patches contain tests for clone,\n> fetch and submodule update to use the actual parallelism both via\n> command line as well as a configured option.\n\n;-)\n"},{"id":"272339","messageId":"xmqqlhaoni6q.fsf@gitster.mtv.corp.google.com","threadId":"40655","inReplyTo":"1445969753-418-7-git-send-email-sbeller@google.com","subject":"Re: [PATCH 6/9] clone: allow an explicit argument for parallel submodule clones","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-27T20:57:17Z","receivedAt":"2015-10-27T20:57:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Just pass it along to \"git submodule update\", which may pick reasonable\n> defaults if you don't specify an explicit number.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> Documentation/git-clone.txt | 5 ++++-\n> builtin/clone.c | 26 ++++++++++++++++++++------\n> t/t7406-submodule-update.sh | 15 +++++++++++++++\n> 3 files changed, 39 insertions(+), 7 deletions(-)\n>\n> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\n> index f1f2a3f..affa52e 100644\n> --- a/Documentation/git-clone.txt\n> +++ b/Documentation/git-clone.txt\n> @@ -14,7 +14,7 @@ SYNOPSIS\n> \t [-o ] [-b ] [-u ] [--reference ]\n> \t [--dissociate] [--separate-git-dir ]\n> \t [--depth ] [--[no-]single-branch]\n> -\t [--recursive | --recurse-submodules] [--] \n> +\t [--recursive | --recurse-submodules] [--jobs ] [--] \n> \t []\n> \n> DESCRIPTION\n> @@ -216,6 +216,9 @@ objects from the source repository into a pack in the cloned repository.\n> \tThe result is Git repository can be separated from working\n> \ttree.\n> \n> +-j::\n> +--jobs::\n\nJudging from the way how \"--depth \" and other options with\nparameter are described, I think this should be:\n\n -j ::\n --jobs ::\n\n> +\tThe number of submodules fetched at the same time.\n\nDo we want to say \"Defaults to submodule.jobs\" somewhere?\n\n> \n> ::\n> \tThe (possibly remote) repository to clone from. See the\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 5864ad1..b8b1d4c 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -50,6 +50,7 @@ static int option_progress = -1;\n> static struct string_list option_config;\n> static struct string_list option_reference;\n> static int option_dissociate;\n> +static int max_jobs = -1;\n> \n> static struct option builtin_clone_options[] = {\n> \tOPT__VERBOSITY(&option_verbosity),\n> @@ -72,6 +73,8 @@ static struct option builtin_clone_options[] = {\n> \t\t N_(\"initialize submodules in the clone\")),\n> \tOPT_BOOL(0, \"recurse-submodules\", &option_recursive,\n> \t\t N_(\"initialize submodules in the clone\")),\n> +\tOPT_INTEGER('j', \"jobs\", &max_jobs,\n> +\t\t N_(\"number of submodules cloned in parallel\")),\n> \tOPT_STRING(0, \"template\", &option_template, N_(\"template-directory\"),\n> \t\t N_(\"directory from which templates will be used\")),\n> \tOPT_STRING_LIST(0, \"reference\", &option_reference, N_(\"repo\"),\n> @@ -95,10 +98,6 @@ static struct option builtin_clone_options[] = {\n> \tOPT_END()\n> };\n> \n> -static const char *argv_submodule[] = {\n> -\t\"submodule\", \"update\", \"--init\", \"--recursive\", NULL\n> -};\n> -\n> static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)\n> {\n> \tstatic char *suffix[] = { \"/.git\", \"\", \".git/.git\", \".git\" };\n> @@ -674,8 +673,23 @@ static int checkout(void)\n> \terr |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n> \t\t\t sha1_to_hex(sha1), \"1\", NULL);\n> \n> -\tif (!err && option_recursive)\n> -\t\terr = run_command_v_opt(argv_submodule, RUN_GIT_CMD);\n> +\tif (!err && option_recursive) {\n> +\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> +\t\targv_array_pushl(&args, \"submodule\", \"update\", \"--init\", \"--recursive\", NULL);\n> +\n> +\t\tif (max_jobs == -1)\n> +\t\t\tif (git_config_get_int(\"submodule.jobs\", &max_jobs))\n> +\t\t\t\tmax_jobs = 1;\n\nThis is somewhat an irregular way to handle a configuration\nvariable. Usually we instead do:\n\n\t* initialize a variable to \"unspecified\" (e.g. -1);\n * let git_config() callback to overwrite the variable;\n * let parse_options() to overwrite the variable.\n\nso that you can just use the variable at the use site like this\nfunction, knowing that the variable is already set with the correct\nprecedence order.\n\nBesides, if you really cared what the value of submodule.jobs is,\nshouldn't you be calling config_parallel_submodules()? I'd also\nthink that you do not want to read that variable here in the first\nplace (see below)...\n\n> +\t\tif (max_jobs != 1) {\n> +\t\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\t\tstrbuf_addf(&sb, \"--jobs=%d\", max_jobs);\n> +\t\t\targv_array_push(&args, sb.buf);\n> +\t\t\tstrbuf_release(&sb);\n> +\t\t}\n\nI am tempted to suggest that you should not pay attention to\n\"submodule.jobs\" in this command at all and just pass through\n\"--jobs=$max_jobs\" that was specified from the command line, as the\nspawned \"submodule update --init --recursive\" would handle\n\"submodule.jobs\" itself.\n\nOnce you start allowing \"clone.jobs\" as a more specific version of\n\"submodule.jobs\", then reading max_jobs first from \"clone.jobs\" and\nthen from the command line starts to make sense. When neither is\nspecified, you would spawn \"submodule update --init --recursive\"\nwithout any explicit \"-j N\" and let it honor its more generic\n\"submodule.jobs\" setting; otherwise, you would run it with \"-j N\" to\noverride that more generic \"submodule.jobs\" setting with either the\nvalue the command line -j given to \"clone\" or specified by a more\nspecific \"clone.jobs\".\n\n> +\t\terr = run_command_v_opt(args.argv, RUN_GIT_CMD);\n> +\t\targv_array_clear(&args);\n> +\t}\n\nThanks.\n"},{"id":"272340","messageId":"xmqqk2q8ni2i.fsf@gitster.mtv.corp.google.com","threadId":"40655","inReplyTo":"1445969753-418-6-git-send-email-sbeller@google.com","subject":"Re: [PATCH 5/9] submodule update: expose parallelism to the user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-27T20:59:49Z","receivedAt":"2015-10-27T20:59:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> @@ -374,6 +374,10 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` options carefully.\n> \tclone with a history truncated to the specified number of revisions.\n> \tSee linkgit:git-clone[1]\n> \n> +-j::\n> +--jobs::\n\nThis probably should be \n\n -j ::\n --jobs ::\n\n(see comments on [6/9]). I know the option description in this file\nis sloppy and does not say \"--name \" etc., as it should (but\nit does say \"--reference \"), and fixing them may not be\nwithin the scope of this series, but we do not need to add more to\nthe existing problems.\n\n> +\tThis option is only valid for the update command.\n> +\tClone new submodules in parallel with as many jobs.\n\nAnd when 0 starts to meaning something special, we would need to\ndescribe that here (and/or submodule.jobs entry in config.txt).\nAs I already said, I do not think \"0 means num_cpus\" is a useful\ndefault, and I would prefer if we reserved 0 to mean something more\nuseful we would figure out later.\n\nThanks.\n"},{"id":"272341","messageId":"xmqqio5sni1j.fsf@gitster.mtv.corp.google.com","threadId":"40655","inReplyTo":"1445969753-418-10-git-send-email-sbeller@google.com","subject":"Re: [PATCH 9/9] fetching submodules: Respect `submodule.jobs` config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-27T21:00:24Z","receivedAt":"2015-10-27T21:00:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 315f271..0b733d7 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2575,6 +2575,13 @@ submodule..ignore::\n> \t\"--ignore-submodules\" option. The 'git submodule' commands are not\n> \taffected by this setting.\n> \n> +submodule::jobs\n\nDid you mean this?\n\n submodule.jobs::\n\n> +\tThis is used to determine how many submodules can be operated on in\n> +\tparallel. Specifying a positive integer allows up to that number\n> +\tof submodules being fetched in parallel. Specifying 0 the number\n> +\tof cpus will be taken as the maximum number. Currently this is\n> +\tused in fetch and clone operations only.\n> +\n\nYou probably do not want to say \"Currently this is\" (you may still\nwant \"only\", though). Whoever teaches other codepaths to pay\nattention to the variable would update this as long as the\ndocumentation stays current.\n\nBy the way, I doubt that \"0 means num-CPUs\" is a useful default for\nparallelism that is used to help anything that is not CPU bound;\n\"clone\", \"submodule update\", etc. are dominantly network bound, and\nthen disk I/O bound (especially if you are cloning from local disk).\nI'd rather see \"-j 0\" to error out as \"reserved for future use\",\nuntil we figure out what the useful default is, and then \"-j 0\" can\nstart using that default that is more useful than num_cpu.\n"},{"id":"272345","messageId":"20151027212645.GF7881@google.com","threadId":"40655","inReplyTo":"1445969753-418-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/9] submodule-config: \"goto\" removal in parse_config()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2015-10-27T21:26:45Z","receivedAt":"2015-10-27T21:26:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> Subject: submodule-config: \"goto\" removal in parse_config()\n>\n> Many components in if/else if/... cascade jumped to a shared\n> clean-up with \"goto release_return\", but we can restructure the\n> function a bit and make them disappear,\n\nNot having read the patch yet, the above makes me suspect this is\ngoing to make the code worse. A 'goto' for exception handling can\nbe a clean way to ensure everything allocated gets released, and\nrestructuring to avoid that can end up making the code more error\nprone and harder to read.\n\nIn other words, the \"goto\" removal should be a side effect and not\nthe motivation.\n\n> which reduces the line count\n> as well. Also reformat overlong lines and poorly indented ones\n> while at it.\n\nThese sound like good things. Hopefully this will make the code\nstructure easier to understand, too.\n\n> The order of rules to verify the value for \"ignore\" used to be to\n> complain on multiple values first and then complain to boolean, but\n> swap the order to match how the values for \"path\" and \"url\" are\n> verified.\n\nI don't understand this. Hopefully the patch will make it clearer.\n\n> Signed-off-by: Stefan Beller \n> ---\n> submodule-config.c | 74 +++++++++++++++++++++---------------------------------\n> 1 file changed, 29 insertions(+), 45 deletions(-)\n\nWhat patch does this apply against? A similar patch appears to\nalready be part of \"master\".\n\n[...]\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -257,78 +257,62 @@ static int parse_config(const char *var, const char *value, void *data)\n> \tif (!name_and_item_from_var(var, &name, &item))\n> \t\treturn 0;\n> \n> -\tsubmodule = lookup_or_create_by_name(me->cache, me->gitmodules_sha1,\n> -\t\t\tname.buf);\n> +\tsubmodule = lookup_or_create_by_name(me->cache,\n> +\t\t\t\t\t me->gitmodules_sha1,\n> +\t\t\t\t\t name.buf);\n\nOk.\n\n> \tif (!strcmp(item.buf, \"path\")) {\n> -\t\tstruct strbuf path = STRBUF_INIT;\n> -\t\tif (!value) {\n> +\t\tif (!value)\n> \t\t\tret = config_error_nonbool(var);\n> -\t\t\tgoto release_return;\n> -\t\t}\n\nIn the preimage, I can see at this line already that nothing more is going to\nhappen in this case. In the postimage, I need to scroll down to find that\neverything else is \"else\"s.\n\nMore generally, the patch seems to be about changing from a code structure\nof\n\n\tif (condition) {\n\t\thandle it;\n\t\tgoto done;\n\t}\n\tif (other condition) {\n\t\thandle it;\n\t\tgoto done;\n\t}\n\thandle misc;\n\tgoto done;\n\nto\n\n\tif (condition) {\n\t\thandle it;\n\t} else if (other condition) {\n\t\thandle it;\n\t} else {\n\t\thandle misc;\n\t}\n\nIn this example the postimage is concise and simple enough that it's\nprobably worth it, but it is not obvious in the general case that this\nis always a good thing to do.\n\nNow that I see the patch is already merged, I don't think it needs\ntweaks. Just a little concerned about the possibility of people\njudging from the commit message and emulating the pattern in the rest\nof git.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"272349","messageId":"xmqq611sng86.fsf@gitster.mtv.corp.google.com","threadId":"40655","inReplyTo":"20151027212645.GF7881@google.com","subject":"Re: [PATCH 1/9] submodule-config: \"goto\" removal in parse_config()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-27T21:39:37Z","receivedAt":"2015-10-27T21:39:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Not having read the patch yet, the above makes me suspect this is\n> going to make the code worse. A 'goto' for exception handling can\n> be a clean way to ensure everything allocated gets released, and\n> restructuring to avoid that can end up making the code more error\n> prone and harder to read.\n>\n> In other words, the \"goto\" removal should be a side effect and not\n> the motivation.\n\nYes, I shared the same general feeling (cf. $gmane/279405).\n\n> More generally, the patch seems to be about changing from a code structure\n> of\n>\n> \tif (condition) {\n> \t\thandle it;\n> \t\tgoto done;\n> \t}\n> \tif (other condition) {\n> \t\thandle it;\n> \t\tgoto done;\n> \t}\n> \thandle misc;\n> \tgoto done;\n>\n> to\n>\n> \tif (condition) {\n> \t\thandle it;\n> \t} else if (other condition) {\n> \t\thandle it;\n> \t} else {\n> \t\thandle misc;\n> \t}\n>\n> In this example the postimage is concise and simple enough that it's\n> probably worth it, but it is not obvious in the general case that this\n> is always a good thing to do.\n\nGenerally, a large piece of code is _easier_ to read with forward\n\"goto\"s that jump to the shared clean-up code, as they serve as\nvisual cues that tell the reader \"you can stop reading here and\nignore the remainder of this if/else if/... cascade\".\n\n> Now that I see the patch is already merged, I don't think it needs\n> tweaks. Just a little concerned about the possibility of people\n> judging from the commit message and emulating the pattern in the rest\n> of git.\n\nYes, we shouldn't let people blindly imitate this change. I merged\nit primarily because I wanted the change get out of my hair, as\nother changes in flight started conflicting with it.\n\nThis kind of change can be good one only in a narrowly defined case\n(like this one) but I agree that in general, as you said at the\nbeginning, it is an easy way to make the resulting code less\nmaintainable and harder to read.\n\nThanks.\n"},{"id":"272435","messageId":"CAGZ79kauFzSKHnUyUPB29Lu59FODVXG5wxWfGK+v7UCAUuESJQ@mail.gmail.com","threadId":"40655","inReplyTo":"xmqqlhaoni6q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 6/9] clone: allow an explicit argument for parallel submodule clones","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-28T20:50:43Z","receivedAt":"2015-10-28T20:50:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 27, 2015 at 1:57 PM, Junio C Hamano wrote:\n>> + The number of submodules fetched at the same time.\n>\n> Do we want to say \"Defaults to submodule.jobs\" somewhere?\n\nYes. :)\n\n> I am tempted to suggest that you should not pay attention to\n> \"submodule.jobs\" in this command at all and just pass through\n> \"--jobs=$max_jobs\" that was specified from the command line, as the\n> spawned \"submodule update --init --recursive\" would handle\n> \"submodule.jobs\" itself.\n\nmakes sense.\n\n>\n> Once you start allowing \"clone.jobs\" as a more specific version of\n> \"submodule.jobs\", then reading max_jobs first from \"clone.jobs\" and\n> then from the command line starts to make sense. When neither is\n> specified, you would spawn \"submodule update --init --recursive\"\n> without any explicit \"-j N\" and let it honor its more generic\n> \"submodule.jobs\" setting; otherwise, you would run it with \"-j N\" to\n> override that more generic \"submodule.jobs\" setting with either the\n> value the command line -j given to \"clone\" or specified by a more\n> specific \"clone.jobs\".\n\nI see. Though I do not plan adding clone.jobs in the near future.\n"},{"id":"272438","messageId":"CAGZ79kbm_aucoEADLFt3VjShP5Kgi0Wwyb6m1dRJtQWu9_ZtBA@mail.gmail.com","threadId":"40655","inReplyTo":"xmqqk2q8ni2i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/9] submodule update: expose parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-28T21:40:09Z","receivedAt":"2015-10-28T21:40:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 27, 2015 at 1:59 PM, Junio C Hamano wrote:\n> And when 0 starts to meaning something special, we would need to\n> describe that here (and/or submodule.jobs entry in config.txt).\n> As I already said, I do not think \"0 means num_cpus\" is a useful\n> default, and I would prefer if we reserved 0 to mean something more\n> useful we would figure out later.\n\nOk I'll add that, too.\n\nI am just debating with myself where the best place is.\nIn run-command.c in pp_init we have:\n\n if (n < 1)\n n = online_cpus();\n pp->max_processes = n;\n\nwe would need to change only that one place to insert an\n\n die(\"We haven't found the right default yet for 0\");\n\nHowever I think for most loads online_cpus makes sense as that\nis ususally the bottleneck for local operations (if being excessive\nmemory may become an issue, but unlikely IMHO).\nSo instead I think it makes more sense to add it in the fetch/clone/update\nto come up with a treatment for 0.\n\nMaybe we want to make the explicit decision for the default value\nfor any user of the parallel processing, such that this code above\nis misguided as it leads to bad defaults if reviewers are inattentive.\n\nSo having spelled out that, we may just want to bark in the pp_init\nfor having a number n < 1.\n"},{"id":"272440","messageId":"xmqqtwpaskhn.fsf@gitster.mtv.corp.google.com","threadId":"40655","inReplyTo":"CAGZ79kbm_aucoEADLFt3VjShP5Kgi0Wwyb6m1dRJtQWu9_ZtBA@mail.gmail.com","subject":"Re: [PATCH 5/9] submodule update: expose parallelism to the user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-28T22:20:52Z","receivedAt":"2015-10-28T22:20:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Tue, Oct 27, 2015 at 1:59 PM, Junio C Hamano wrote:\n>> And when 0 starts to meaning something special, we would need to\n>> describe that here (and/or submodule.jobs entry in config.txt).\n>> As I already said, I do not think \"0 means num_cpus\" is a useful\n>> default, and I would prefer if we reserved 0 to mean something more\n>> useful we would figure out later.\n>\n> Ok I'll add that, too.\n\nSorry, but I take it back. We just can document that (1) \"-j 0\"\nwill give you some default, (2) we do not promise that the default\nwill be optimal for you from day one, (3) we reserve the right to\n\"improve\" it over time, and (4) we promise that we won't make it an\ninsanely wrong value. And let's keep \"0 currently means num_cpu\",\nwhich may or may not be optimal but it cannot be an \"insanely wrong\"\nvalue.\n"},{"id":"272464","messageId":"1446074504-6014-1-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"xmqqfv0wp1l1.fsf@gitster.mtv.corp.google.com","subject":"[PATCHv2 0/8] Expose the submodule parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-28T23:21:36Z","receivedAt":"2015-10-28T23:21:36Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This replaces origin/sb/submodule-parallel-update\n(anchoring at 74367d8938, Merge branch 'sb/submodule-parallel-fetch'\ninto sb/submodule-parallel-update)\n\nWhat does it do?\n---\nThis series should finish the on going efforts of parallelizing\nsubmodule network traffic. The patches contain tests for clone,\nfetch and submodule update to use the actual parallelism both via\ncommand line as well as a configured option. I decided to go with\n\"submodule.jobs\" for all three for now.\n\nWhat is new in v2?\n---\n* The patches got reordered slightly\n* Documentation was adapted\n\nInterdiff below\n\nStefan Beller (8):\n run_processes_parallel: Add output to tracing messages\n submodule config: keep update strategy around\n submodule config: remove name_and_item_from_var\n submodule-config: parse_config\n fetching submodules: Respect `submodule.jobs` config option\n git submodule update: have a dedicated helper for cloning\n submodule update: expose parallelism to the user\n clone: allow an explicit argument for parallel submodule clones\n\n Documentation/config.txt | 7 ++\n Documentation/git-clone.txt | 6 +-\n Documentation/git-submodule.txt | 7 +-\n builtin/clone.c | 23 +++-\n builtin/fetch.c | 2 +-\n builtin/submodule--helper.c | 244 ++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 54 ++++-----\n run-command.c | 4 +\n submodule-config.c | 98 ++++++++++------\n submodule-config.h | 3 +\n submodule.c | 5 +\n t/t5526-fetch-submodules.sh | 14 +++\n t/t7400-submodule-basic.sh | 4 +-\n t/t7406-submodule-update.sh | 27 +++++\n 14 files changed, 418 insertions(+), 80 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0de0138..785721a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2643,12 +2643,12 @@ submodule..ignore::\n \t\"--ignore-submodules\" option. The 'git submodule' commands are not\n \taffected by this setting.\n \n-submodule::jobs\n+submodule.jobs::\n \tThis is used to determine how many submodules can be operated on in\n \tparallel. Specifying a positive integer allows up to that number\n-\tof submodules being fetched in parallel. Specifying 0 the number\n-\tof cpus will be taken as the maximum number. Currently this is\n-\tused in fetch and clone operations only.\n+\tof submodules being fetched in parallel. This is used in fetch\n+\tand clone operations only. A value of 0 will give some reasonable\n+\tdefault. The defaults may change with different versions of Git.\n \n tag.sort::\n \tThis variable controls the sort ordering of tags when displayed by\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex affa52e..01bd6b7 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -216,9 +216,10 @@ objects from the source repository into a pack in the cloned repository.\n \tThe result is Git repository can be separated from working\n \ttree.\n \n--j::\n---jobs::\n+-j ::\n+--jobs ::\n \tThe number of submodules fetched at the same time.\n+\tDefaults to the `submodule.jobs` option.\n \n ::\n \tThe (possibly remote) repository to clone from. See the\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex f5429fa..c70fafd 100644\n--- a/Documentation/git-submodule.txt\n+++ b/Documentation/git-submodule.txt\n@@ -374,10 +374,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` options carefully.\n \tclone with a history truncated to the specified number of revisions.\n \tSee linkgit:git-clone[1]\n \n--j::\n---jobs::\n+-j ::\n+--jobs ::\n \tThis option is only valid for the update command.\n \tClone new submodules in parallel with as many jobs.\n+\tDefaults to the `submodule.jobs` option.\n \n ...::\n \tPaths to submodule(s). When specified this will restrict the command\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 5ac2d89..22b9924 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -727,10 +727,7 @@ static int checkout(void)\n \t\tstruct argv_array args = ARGV_ARRAY_INIT;\n \t\targv_array_pushl(&args, \"submodule\", \"update\", \"--init\", \"--recursive\", NULL);\n \n-\t\tif (max_jobs == -1)\n-\t\t\tif (git_config_get_int(\"submodule.jobs\", &max_jobs))\n-\t\t\t\tmax_jobs = 1;\n-\t\tif (max_jobs != 1) {\n+\t\tif (max_jobs != -1) {\n \t\t\tstruct strbuf sb = STRBUF_INIT;\n \t\t\tstrbuf_addf(&sb, \"--jobs=%d\", max_jobs);\n \t\t\targv_array_push(&args, sb.buf);\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c3d438a..67dba1c 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -476,9 +476,10 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \t/* Overlay the parsed .gitmodules file with .git/config */\n \tgit_config(git_submodule_config, NULL);\n \n-\tif (max_jobs == -1)\n-\t\tif (git_config_get_int(\"submodule.jobs\", &max_jobs))\n-\t\t\tmax_jobs = 1;\n+\tif (max_jobs < 0)\n+\t\tmax_jobs = config_parallel_submodules();\n+\tif (max_jobs < 0)\n+\t\tmax_jobs = 1;\n \n \trun_processes_parallel(max_jobs,\n \t\t\t update_clone_get_next_task,\n\n-- \n2.5.0.281.g4ed9cdb\n"},{"id":"272463","messageId":"1446074504-6014-2-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"1446074504-6014-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 1/8] run_processes_parallel: Add output to tracing messages","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-28T23:21:37Z","receivedAt":"2015-10-28T23:21:37Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This commit serves 2 purposes. First this may help the user who\ntries to diagnose intermixed process calls. Second this may be used\nin a later patch for testing. As the output of a command should not\nchange visibly except for going faster, grepping for the trace output\nseems like a viable testing strategy.\n\nSigned-off-by: Stefan Beller \n---\n run-command.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 82cc238..49dec74 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -959,6 +959,9 @@ static struct parallel_processes *pp_init(int n,\n \t\tn = online_cpus();\n \n \tpp->max_processes = n;\n+\n+\ttrace_printf(\"run_processes_parallel: preparing to run up to %d children in parallel\", n);\n+\n \tpp->data = data;\n \tif (!get_next_task)\n \t\tdie(\"BUG: you need to specify a get_next_task function\");\n@@ -988,6 +991,7 @@ static void pp_cleanup(struct parallel_processes *pp)\n {\n \tint i;\n \n+\ttrace_printf(\"run_processes_parallel: parallel processing done\");\n \tfor (i = 0; i < pp->max_processes; i++) {\n \t\tstrbuf_release(&pp->children[i].err);\n \t\tchild_process_deinit(&pp->children[i].process);\n-- \n2.5.0.281.g4ed9cdb\n"},{"id":"272471","messageId":"1446074504-6014-3-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"1446074504-6014-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 2/8] submodule config: keep update strategy around","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-28T23:21:38Z","receivedAt":"2015-10-28T23:21:38Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We need the submodule update strategies in a later patch.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n submodule-config.c | 11 +++++++++++\n submodule-config.h | 1 +\n 2 files changed, 12 insertions(+)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex afe0ea8..8b8c7d1 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -194,6 +194,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,\n \n \tsubmodule->path = NULL;\n \tsubmodule->url = NULL;\n+\tsubmodule->update = NULL;\n \tsubmodule->fetch_recurse = RECURSE_SUBMODULES_NONE;\n \tsubmodule->ignore = NULL;\n \n@@ -311,6 +312,16 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tfree((void *) submodule->url);\n \t\t\tsubmodule->url = xstrdup(value);\n \t\t}\n+\t} else if (!strcmp(item.buf, \"update\")) {\n+\t\tif (!value)\n+\t\t\tret = config_error_nonbool(var);\n+\t\telse if (!me->overwrite && submodule->update != NULL)\n+\t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n+\t\t\t\t\t \"update\");\n+\t\telse {\n+\t\t\tfree((void *)submodule->update);\n+\t\t\tsubmodule->update = xstrdup(value);\n+\t\t}\n \t}\n \n \tstrbuf_release(&name);\ndiff --git a/submodule-config.h b/submodule-config.h\nindex 9061e4e..f9e2a29 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -14,6 +14,7 @@ struct submodule {\n \tconst char *url;\n \tint fetch_recurse;\n \tconst char *ignore;\n+\tconst char *update;\n \t/* the sha1 blob id of the responsible .gitmodules file */\n \tunsigned char gitmodules_sha1[20];\n };\n-- \n2.5.0.281.g4ed9cdb\n"},{"id":"272467","messageId":"1446074504-6014-4-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"1446074504-6014-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 3/8] submodule config: remove name_and_item_from_var","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-28T23:21:39Z","receivedAt":"2015-10-28T23:21:39Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"By inlining `name_and_item_from_var` it is easy to add later options\nwhich are not required to have a submodule name.\n\nSigned-off-by: Stefan Beller \n---\n submodule-config.c | 46 +++++++++++++++++-----------------------------\n 1 file changed, 17 insertions(+), 29 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 8b8c7d1..4d0563c 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -161,22 +161,6 @@ static struct submodule *cache_lookup_name(struct submodule_cache *cache,\n \treturn NULL;\n }\n \n-static int name_and_item_from_var(const char *var, struct strbuf *name,\n-\t\t\t\t struct strbuf *item)\n-{\n-\tconst char *subsection, *key;\n-\tint subsection_len, parse;\n-\tparse = parse_config_key(var, \"submodule\", &subsection,\n-\t\t\t&subsection_len, &key);\n-\tif (parse < 0 || !subsection)\n-\t\treturn 0;\n-\n-\tstrbuf_add(name, subsection, subsection_len);\n-\tstrbuf_addstr(item, key);\n-\n-\treturn 1;\n-}\n-\n static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,\n \t\tconst unsigned char *gitmodules_sha1, const char *name)\n {\n@@ -251,18 +235,25 @@ static int parse_config(const char *var, const char *value, void *data)\n {\n \tstruct parse_config_parameter *me = data;\n \tstruct submodule *submodule;\n-\tstruct strbuf name = STRBUF_INIT, item = STRBUF_INIT;\n-\tint ret = 0;\n+\tint subsection_len, ret = 0;\n+\tconst char *subsection, *key;\n+\tchar *name;\n \n-\t/* this also ensures that we only parse submodule entries */\n-\tif (!name_and_item_from_var(var, &name, &item))\n+\tif (parse_config_key(var, \"submodule\", &subsection,\n+\t\t\t &subsection_len, &key) < 0)\n \t\treturn 0;\n \n+\tif (!subsection_len)\n+\t\treturn 0;\n+\n+\t/* subsection is not null terminated */\n+\tname = xmemdupz(subsection, subsection_len);\n \tsubmodule = lookup_or_create_by_name(me->cache,\n \t\t\t\t\t me->gitmodules_sha1,\n-\t\t\t\t\t name.buf);\n+\t\t\t\t\t name);\n+\tfree(name);\n \n-\tif (!strcmp(item.buf, \"path\")) {\n+\tif (!strcmp(key, \"path\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->path != NULL)\n@@ -275,7 +266,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tsubmodule->path = xstrdup(value);\n \t\t\tcache_put_path(me->cache, submodule);\n \t\t}\n-\t} else if (!strcmp(item.buf, \"fetchrecursesubmodules\")) {\n+\t} else if (!strcmp(key, \"fetchrecursesubmodules\")) {\n \t\t/* when parsing worktree configurations we can die early */\n \t\tint die_on_error = is_null_sha1(me->gitmodules_sha1);\n \t\tif (!me->overwrite &&\n@@ -286,7 +277,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tsubmodule->fetch_recurse = parse_fetch_recurse(\n \t\t\t\t\t\t\t\tvar, value,\n \t\t\t\t\t\t\t\tdie_on_error);\n-\t} else if (!strcmp(item.buf, \"ignore\")) {\n+\t} else if (!strcmp(key, \"ignore\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->ignore != NULL)\n@@ -302,7 +293,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tfree((void *) submodule->ignore);\n \t\t\tsubmodule->ignore = xstrdup(value);\n \t\t}\n-\t} else if (!strcmp(item.buf, \"url\")) {\n+\t} else if (!strcmp(key, \"url\")) {\n \t\tif (!value) {\n \t\t\tret = config_error_nonbool(var);\n \t\t} else if (!me->overwrite && submodule->url != NULL) {\n@@ -312,7 +303,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tfree((void *) submodule->url);\n \t\t\tsubmodule->url = xstrdup(value);\n \t\t}\n-\t} else if (!strcmp(item.buf, \"update\")) {\n+\t} else if (!strcmp(key, \"update\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->update != NULL)\n@@ -324,9 +315,6 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t}\n \t}\n \n-\tstrbuf_release(&name);\n-\tstrbuf_release(&item);\n-\n \treturn ret;\n }\n \n-- \n2.5.0.281.g4ed9cdb\n"},{"id":"272469","messageId":"1446074504-6014-5-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"1446074504-6014-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 4/8] submodule-config: parse_config","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-28T23:21:40Z","receivedAt":"2015-10-28T23:21:40Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This rewrites parse_config to distinguish between configs specific to\none submodule and configs which apply generically to all submodules.\nWe do not have generic submodule configs yet, but the next patch will\nintroduce \"submodule.jobs\".\n\nSigned-off-by: Stefan Beller \n\n# Conflicts:\n#\tsubmodule-config.c\n\nSigned-off-by: Stefan Beller \n---\n submodule-config.c | 58 ++++++++++++++++++++++++++++++++++++------------------\n 1 file changed, 39 insertions(+), 19 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 4d0563c..1cea404 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -231,27 +231,23 @@ struct parse_config_parameter {\n \tint overwrite;\n };\n \n-static int parse_config(const char *var, const char *value, void *data)\n+static int parse_generic_submodule_config(const char *var,\n+\t\t\t\t\t const char *key,\n+\t\t\t\t\t const char *value)\n {\n-\tstruct parse_config_parameter *me = data;\n-\tstruct submodule *submodule;\n-\tint subsection_len, ret = 0;\n-\tconst char *subsection, *key;\n-\tchar *name;\n-\n-\tif (parse_config_key(var, \"submodule\", &subsection,\n-\t\t\t &subsection_len, &key) < 0)\n-\t\treturn 0;\n-\n-\tif (!subsection_len)\n-\t\treturn 0;\n+\treturn 0;\n+}\n \n-\t/* subsection is not null terminated */\n-\tname = xmemdupz(subsection, subsection_len);\n-\tsubmodule = lookup_or_create_by_name(me->cache,\n-\t\t\t\t\t me->gitmodules_sha1,\n-\t\t\t\t\t name);\n-\tfree(name);\n+static int parse_specific_submodule_config(struct parse_config_parameter *me,\n+\t\t\t\t\t const char *name,\n+\t\t\t\t\t const char *key,\n+\t\t\t\t\t const char *value,\n+\t\t\t\t\t const char *var)\n+{\n+\tint ret = 0;\n+\tstruct submodule *submodule = lookup_or_create_by_name(me->cache,\n+\t\t\t\t\t\t\t me->gitmodules_sha1,\n+\t\t\t\t\t\t\t name);\n \n \tif (!strcmp(key, \"path\")) {\n \t\tif (!value)\n@@ -318,6 +314,30 @@ static int parse_config(const char *var, const char *value, void *data)\n \treturn ret;\n }\n \n+static int parse_config(const char *var, const char *value, void *data)\n+{\n+\tstruct parse_config_parameter *me = data;\n+\n+\tint subsection_len;\n+\tconst char *subsection, *key;\n+\tchar *name;\n+\n+\tif (parse_config_key(var, \"submodule\", &subsection,\n+\t\t\t &subsection_len, &key) < 0)\n+\t\treturn 0;\n+\n+\tif (!subsection_len)\n+\t\treturn parse_generic_submodule_config(var, key, value);\n+\telse {\n+\t\tint ret;\n+\t\t/* subsection is not null terminated */\n+\t\tname = xmemdupz(subsection, subsection_len);\n+\t\tret = parse_specific_submodule_config(me, name, key, value, var);\n+\t\tfree(name);\n+\t\treturn ret;\n+\t}\n+}\n+\n static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,\n \t\t\t\t unsigned char *gitmodules_sha1)\n {\n-- \n2.5.0.281.g4ed9cdb\n"},{"id":"272465","messageId":"1446074504-6014-6-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"1446074504-6014-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 5/8] fetching submodules: Respect `submodule.jobs` config option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-28T23:21:41Z","receivedAt":"2015-10-28T23:21:41Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This allows to configure fetching and updating in parallel\nwithout having the command line option.\n\nThis moved the responsibility to determine how many parallel processes\nto start from builtin/fetch to submodule.c as we need a way to communicate\n\"The user did not specify the number of parallel processes in the command\nline options\" in the builtin fetch. The submodule code takes care of\nthe precedence (CLI > config > default)\n\nSigned-off-by: Stefan Beller \n---\n Documentation/config.txt | 7 +++++++\n builtin/fetch.c | 2 +-\n submodule-config.c | 9 +++++++++\n submodule-config.h | 2 ++\n submodule.c | 5 +++++\n t/t5526-fetch-submodules.sh | 14 ++++++++++++++\n 6 files changed, 38 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 391a0c3..785721a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2643,6 +2643,13 @@ submodule..ignore::\n \t\"--ignore-submodules\" option. The 'git submodule' commands are not\n \taffected by this setting.\n \n+submodule.jobs::\n+\tThis is used to determine how many submodules can be operated on in\n+\tparallel. Specifying a positive integer allows up to that number\n+\tof submodules being fetched in parallel. This is used in fetch\n+\tand clone operations only. A value of 0 will give some reasonable\n+\tdefault. The defaults may change with different versions of Git.\n+\n tag.sort::\n \tThis variable controls the sort ordering of tags when displayed by\n \tlinkgit:git-tag[1]. Without the \"--sort=\" option provided, the\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 9cc1c9d..60e6797 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -37,7 +37,7 @@ static int prune = -1; /* unspecified */\n static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity;\n static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow;\n-static int max_children = 1;\n+static int max_children = -1;\n static const char *depth;\n static const char *upload_pack;\n static struct strbuf default_rla = STRBUF_INIT;\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 1cea404..07bdcdf 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -32,6 +32,7 @@ enum lookup_type {\n \n static struct submodule_cache cache;\n static int is_cache_init;\n+static int parallel_jobs = -1;\n \n static int config_path_cmp(const struct submodule_entry *a,\n \t\t\t const struct submodule_entry *b,\n@@ -235,6 +236,9 @@ static int parse_generic_submodule_config(const char *var,\n \t\t\t\t\t const char *key,\n \t\t\t\t\t const char *value)\n {\n+\tif (!strcmp(key, \"jobs\")) {\n+\t\tparallel_jobs = strtol(value, NULL, 10);\n+\t}\n \treturn 0;\n }\n \n@@ -483,3 +487,8 @@ void submodule_free(void)\n \tcache_free(&cache);\n \tis_cache_init = 0;\n }\n+\n+int config_parallel_submodules(void)\n+{\n+\treturn parallel_jobs;\n+}\ndiff --git a/submodule-config.h b/submodule-config.h\nindex f9e2a29..d9bbf9a 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -27,4 +27,6 @@ const struct submodule *submodule_from_path(const unsigned char *commit_sha1,\n \t\tconst char *path);\n void submodule_free(void);\n \n+int config_parallel_submodules(void);\n+\n #endif /* SUBMODULE_CONFIG_H */\ndiff --git a/submodule.c b/submodule.c\nindex 0257ea3..188ba02 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -752,6 +752,11 @@ int fetch_populated_submodules(const struct argv_array *options,\n \targv_array_push(&spf.args, \"--recurse-submodules-default\");\n \t/* default value, \"--submodule-prefix\" and its value are added later */\n \n+\tif (max_parallel_jobs < 0)\n+\t\tmax_parallel_jobs = config_parallel_submodules();\n+\tif (max_parallel_jobs < 0)\n+\t\tmax_parallel_jobs = 1;\n+\n \tcalculate_changed_submodule_paths();\n \trun_processes_parallel(max_parallel_jobs,\n \t\t\t get_next_submodule,\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 1b4ce69..5c3579c 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -470,4 +470,18 @@ test_expect_success \"don't fetch submodule when newly recorded commits are alrea\n \ttest_i18ncmp expect.err actual.err\n '\n \n+test_expect_success 'fetching submodules respects parallel settings' '\n+\tgit config fetch.recurseSubmodules true &&\n+\t(\n+\t\tcd downstream &&\n+\t\tGIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 &&\n+\t\tgrep \"7 children\" trace.out &&\n+\t\tgit config submodule.jobs 8 &&\n+\t\tGIT_TRACE=$(pwd)/trace.out git fetch &&\n+\t\tgrep \"8 children\" trace.out &&\n+\t\tGIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&\n+\t\tgrep \"9 children\" trace.out\n+\t)\n+'\n+\n test_done\n-- \n2.5.0.281.g4ed9cdb\n"},{"id":"272470","messageId":"1446074504-6014-7-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"1446074504-6014-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 6/8] git submodule update: have a dedicated helper for cloning","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-28T23:21:42Z","receivedAt":"2015-10-28T23:21:42Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This introduces a new helper function in git submodule--helper\nwhich takes care of cloning all submodules, which we want to\nparallelize eventually.\n\nSome tests (such as empty URL, update_mode=none) are required in the\nhelper to make the decision for cloning. These checks have been\nmoved into the C function as well (no need to repeat them in the\nshell script).\n\nAs we can only access the stderr channel from within the parallel\nprocessing engine, we need to reroute the error message for\nspecified but initialized submodules to stderr. As it is an error\nmessage, this should have gone to stderr in the first place, so it\nis a bug fix along the way.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n builtin/submodule--helper.c | 234 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 45 +++------\n t/t7400-submodule-basic.sh | 4 +-\n 3 files changed, 247 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff..1ec1b85 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -255,6 +255,239 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int git_submodule_config(const char *var, const char *value, void *cb)\n+{\n+\treturn parse_submodule_config_option(var, value);\n+}\n+\n+struct submodule_update_clone {\n+\tint count;\n+\tint quiet;\n+\tint print_unmatched;\n+\tchar *reference;\n+\tchar *depth;\n+\tchar *update;\n+\tconst char *recursive_prefix;\n+\tconst char *prefix;\n+\tstruct module_list list;\n+\tstruct string_list projectlines;\n+\tstruct pathspec pathspec;\n+};\n+#define SUBMODULE_UPDATE_CLONE_INIT {0, 0, 0, NULL, NULL, NULL, NULL, NULL, MODULE_LIST_INIT, STRING_LIST_INIT_DUP}\n+\n+static void fill_clone_command(struct child_process *cp, int quiet,\n+\t\t\t const char *prefix, const char *path,\n+\t\t\t const char *name, const char *url,\n+\t\t\t const char *reference, const char *depth)\n+{\n+\tcp->git_cmd = 1;\n+\tcp->no_stdin = 1;\n+\tcp->stdout_to_stderr = 1;\n+\tcp->err = -1;\n+\targv_array_push(&cp->args, \"submodule--helper\");\n+\targv_array_push(&cp->args, \"clone\");\n+\tif (quiet)\n+\t\targv_array_push(&cp->args, \"--quiet\");\n+\n+\tif (prefix) {\n+\t\targv_array_push(&cp->args, \"--prefix\");\n+\t\targv_array_push(&cp->args, prefix);\n+\t}\n+\targv_array_push(&cp->args, \"--path\");\n+\targv_array_push(&cp->args, path);\n+\n+\targv_array_push(&cp->args, \"--name\");\n+\targv_array_push(&cp->args, name);\n+\n+\targv_array_push(&cp->args, \"--url\");\n+\targv_array_push(&cp->args, url);\n+\tif (reference)\n+\t\targv_array_push(&cp->args, reference);\n+\tif (depth)\n+\t\targv_array_push(&cp->args, depth);\n+}\n+\n+static int update_clone_get_next_task(void **pp_task_cb,\n+\t\t\t\t struct child_process *cp,\n+\t\t\t\t struct strbuf *err,\n+\t\t\t\t void *pp_cb)\n+{\n+\tstruct submodule_update_clone *pp = pp_cb;\n+\n+\tfor (; pp->count < pp->list.nr; pp->count++) {\n+\t\tconst struct submodule *sub = NULL;\n+\t\tconst char *displaypath = NULL;\n+\t\tconst struct cache_entry *ce = pp->list.entries[pp->count];\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tconst char *update_module = NULL;\n+\t\tchar *url = NULL;\n+\t\tint just_cloned = 0;\n+\n+\t\tif (ce_stage(ce)) {\n+\t\t\tif (pp->recursive_prefix)\n+\t\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s/%s\\n\",\n+\t\t\t\t\tpp->recursive_prefix, ce->name);\n+\t\t\telse\n+\t\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s\\n\",\n+\t\t\t\t\tce->name);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tsub = submodule_from_path(null_sha1, ce->name);\n+\t\tif (!sub) {\n+\t\t\tstrbuf_addf(err, \"BUG: internal error managing submodules. \"\n+\t\t\t\t \"The cache could not locate '%s'\", ce->name);\n+\t\t\tpp->print_unmatched = 1;\n+\t\t\treturn 0;\n+\t\t}\n+\n+\t\tif (pp->recursive_prefix)\n+\t\t\tdisplaypath = relative_path(pp->recursive_prefix, ce->name, &sb);\n+\t\telse\n+\t\t\tdisplaypath = ce->name;\n+\n+\t\tif (pp->update)\n+\t\t\tupdate_module = pp->update;\n+\t\tif (!update_module)\n+\t\t\tupdate_module = sub->update;\n+\t\tif (!update_module)\n+\t\t\tupdate_module = \"checkout\";\n+\t\tif (!strcmp(update_module, \"none\")) {\n+\t\t\tstrbuf_addf(err, \"Skipping submodule '%s'\\n\", displaypath);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/*\n+\t\t * Looking up the url in .git/config.\n+\t\t * We cannot fall back to .gitmodules as we only want to process\n+\t\t * configured submodules. This renders the submodule lookup API\n+\t\t * useless, as it cannot lookup without fallback.\n+\t\t */\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n+\t\tgit_config_get_string(sb.buf, &url);\n+\t\tif (!url) {\n+\t\t\t/*\n+\t\t\t * Only mention uninitialized submodules when its\n+\t\t\t * path have been specified\n+\t\t\t */\n+\t\t\tif (pp->pathspec.nr)\n+\t\t\t\tstrbuf_addf(err, _(\"Submodule path '%s' not initialized\\n\"\n+\t\t\t\t\t\"Maybe you want to use 'update --init'?\"), displaypath);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n+\t\tjust_cloned = !file_exists(sb.buf);\n+\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_addf(&sb, \"%06o %s %d %d\\t%s\\n\", ce->ce_mode,\n+\t\t\t\tsha1_to_hex(ce->sha1), ce_stage(ce),\n+\t\t\t\tjust_cloned, ce->name);\n+\t\tstring_list_append(&pp->projectlines, sb.buf);\n+\n+\t\tif (just_cloned) {\n+\t\t\tfill_clone_command(cp, pp->quiet, pp->prefix, ce->name,\n+\t\t\t\t\t sub->name, url, pp->reference, pp->depth);\n+\t\t\tpp->count++;\n+\t\t\tfree(url);\n+\t\t\treturn 1;\n+\t\t} else\n+\t\t\tfree(url);\n+\t}\n+\treturn 0;\n+}\n+\n+static int update_clone_start_failure(struct child_process *cp,\n+\t\t\t\t struct strbuf *err,\n+\t\t\t\t void *pp_cb,\n+\t\t\t\t void *pp_task_cb)\n+{\n+\tstruct submodule_update_clone *pp = pp_cb;\n+\n+\tstrbuf_addf(err, \"error when starting a child process\");\n+\tpp->print_unmatched = 1;\n+\n+\treturn 1;\n+}\n+\n+static int update_clone_task_finished(int result,\n+\t\t\t\t struct child_process *cp,\n+\t\t\t\t struct strbuf *err,\n+\t\t\t\t void *pp_cb,\n+\t\t\t\t void *pp_task_cb)\n+{\n+\tstruct submodule_update_clone *pp = pp_cb;\n+\n+\tif (!result) {\n+\t\treturn 0;\n+\t} else {\n+\t\tstrbuf_addf(err, \"error in one child process\");\n+\t\tpp->print_unmatched = 1;\n+\t\treturn 1;\n+\t}\n+}\n+\n+static int update_clone(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct string_list_item *item;\n+\tstruct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;\n+\n+\tstruct option module_list_options[] = {\n+\t\tOPT_STRING(0, \"prefix\", &prefix,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"path into the working tree\")),\n+\t\tOPT_STRING(0, \"recursive_prefix\", &pp.recursive_prefix,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"path into the working tree, across nested \"\n+\t\t\t \"submodule boundaries\")),\n+\t\tOPT_STRING(0, \"update\", &pp.update,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"update command for submodules\")),\n+\t\tOPT_STRING(0, \"reference\", &pp.reference, \"\",\n+\t\t\t N_(\"Use the local reference repository \"\n+\t\t\t \"instead of a full clone\")),\n+\t\tOPT_STRING(0, \"depth\", &pp.depth, \"\",\n+\t\t\t N_(\"Create a shallow clone truncated to the \"\n+\t\t\t \"specified number of revisions\")),\n+\t\tOPT__QUIET(&pp.quiet, N_(\"do't print cloning progress\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper list [--prefix=] [...]\"),\n+\t\tNULL\n+\t};\n+\tpp.prefix = prefix;\n+\n+\targc = parse_options(argc, argv, prefix, module_list_options,\n+\t\t\t git_submodule_helper_usage, 0);\n+\n+\tif (module_list_compute(argc, argv, prefix, &pp.pathspec, &pp.list) < 0) {\n+\t\tprintf(\"#unmatched\\n\");\n+\t\treturn 1;\n+\t}\n+\n+\tgitmodules_config();\n+\t/* Overlay the parsed .gitmodules file with .git/config */\n+\tgit_config(git_submodule_config, NULL);\n+\trun_processes_parallel(1, update_clone_get_next_task,\n+\t\t\t\t update_clone_start_failure,\n+\t\t\t\t update_clone_task_finished,\n+\t\t\t\t &pp);\n+\n+\tif (pp.print_unmatched) {\n+\t\tprintf(\"#unmatched\\n\");\n+\t\treturn 1;\n+\t}\n+\n+\tfor_each_string_list_item(item, &pp.projectlines) {\n+\t\tutf8_fprintf(stdout, \"%s\", item->string);\n+\t}\n+\treturn 0;\n+}\n+\n struct cmd_struct {\n \tconst char *cmd;\n \tint (*fn)(int, const char **, const char *);\n@@ -264,6 +497,7 @@ static struct cmd_struct commands[] = {\n \t{\"list\", module_list},\n \t{\"name\", module_name},\n \t{\"clone\", module_clone},\n+\t{\"update-clone\", update_clone}\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 9bc5c5f..9f554fb 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -664,17 +664,18 @@ cmd_update()\n \t\tcmd_init \"--\" \"$@\" || return\n \tfi\n \n-\tcloned_modules=\n-\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n+\tgit submodule--helper update-clone ${GIT_QUIET:+--quiet} \\\n+\t\t${wt_prefix:+--prefix \"$wt_prefix\"} \\\n+\t\t${prefix:+--recursive_prefix \"$prefix\"} \\\n+\t\t${update:+--update \"$update\"} \\\n+\t\t${reference:+--reference \"$reference\"} \\\n+\t\t${depth:+--depth \"$depth\"} \\\n+\t\t\"$@\" | {\n \terr=\n-\twhile read mode sha1 stage sm_path\n+\twhile read mode sha1 stage just_cloned sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tif test \"$stage\" = U\n-\t\tthen\n-\t\t\techo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n-\t\t\tcontinue\n-\t\tfi\n+\n \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tbranch=$(get_submodule_config \"$name\" branch master)\n@@ -691,27 +692,10 @@ cmd_update()\n \n \t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n \n-\t\tif test \"$update_module\" = \"none\"\n-\t\tthen\n-\t\t\techo \"Skipping submodule '$displaypath'\"\n-\t\t\tcontinue\n-\t\tfi\n-\n-\t\tif test -z \"$url\"\n-\t\tthen\n-\t\t\t# Only mention uninitialized submodules when its\n-\t\t\t# path have been specified\n-\t\t\ttest \"$#\" != \"0\" &&\n-\t\t\tsay \"$(eval_gettext \"Submodule path '\\$displaypath' not initialized\n-Maybe you want to use 'update --init'?\")\"\n-\t\t\tcontinue\n-\t\tfi\n-\n-\t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n+\t\tif test $just_cloned -eq 1\n \t\tthen\n-\t\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$prefix\" --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n-\t\t\tcloned_modules=\"$cloned_modules;$name\"\n \t\t\tsubsha1=\n+\t\t\tupdate_module=checkout\n \t\telse\n \t\t\tsubsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n@@ -751,13 +735,6 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n \t\t\tfi\n \n-\t\t\t# Is this something we just cloned?\n-\t\t\tcase \";$cloned_modules;\" in\n-\t\t\t*\";$name;\"*)\n-\t\t\t\t# then there is no local change to integrate\n-\t\t\t\tupdate_module=checkout ;;\n-\t\t\tesac\n-\n \t\t\tmust_die_on_failure=\n \t\t\tcase \"$update_module\" in\n \t\t\tcheckout)\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 540771c..5991e3c 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -462,7 +462,7 @@ test_expect_success 'update --init' '\n \tgit config --remove-section submodule.example &&\n \ttest_must_fail git config submodule.example.url &&\n \n-\tgit submodule update init > update.out &&\n+\tgit submodule update init 2> update.out &&\n \tcat update.out &&\n \ttest_i18ngrep \"not initialized\" update.out &&\n \ttest_must_fail git rev-parse --resolve-git-dir init/.git &&\n@@ -480,7 +480,7 @@ test_expect_success 'update --init from subdirectory' '\n \tmkdir -p sub &&\n \t(\n \t\tcd sub &&\n-\t\tgit submodule update ../init >update.out &&\n+\t\tgit submodule update ../init 2>update.out &&\n \t\tcat update.out &&\n \t\ttest_i18ngrep \"not initialized\" update.out &&\n \t\ttest_must_fail git rev-parse --resolve-git-dir ../init/.git &&\n-- \n2.5.0.281.g4ed9cdb\n"},{"id":"272466","messageId":"1446074504-6014-8-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"1446074504-6014-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 7/8] submodule update: expose parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-28T23:21:43Z","receivedAt":"2015-10-28T23:21:43Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Expose possible parallelism either via the \"--jobs\" CLI parameter or\nthe \"submodule.jobs\" setting.\n\nBy having the variable initialized to -1, we make sure 0 can be passed\ninto the parallel processing machine, which will then pick as many parallel\nworkers as there are CPUs.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/git-submodule.txt | 7 ++++++-\n builtin/submodule--helper.c | 18 ++++++++++++++----\n git-submodule.sh | 9 +++++++++\n t/t7406-submodule-update.sh | 12 ++++++++++++\n 4 files changed, 41 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex f17687e..c70fafd 100644\n--- a/Documentation/git-submodule.txt\n+++ b/Documentation/git-submodule.txt\n@@ -16,7 +16,7 @@ SYNOPSIS\n 'git submodule' [--quiet] deinit [-f|--force] [--] ...\n 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]\n \t [-f|--force] [--rebase|--merge] [--reference ]\n-\t [--depth ] [--recursive] [--] [...]\n+\t [--depth ] [--recursive] [--jobs ] [--] [...]\n 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]\n \t [commit] [--] [...]\n 'git submodule' [--quiet] foreach [--recursive] \n@@ -374,6 +374,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` options carefully.\n \tclone with a history truncated to the specified number of revisions.\n \tSee linkgit:git-clone[1]\n \n+-j ::\n+--jobs ::\n+\tThis option is only valid for the update command.\n+\tClone new submodules in parallel with as many jobs.\n+\tDefaults to the `submodule.jobs` option.\n \n ...::\n \tPaths to submodule(s). When specified this will restrict the command\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1ec1b85..67dba1c 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -431,6 +431,7 @@ static int update_clone_task_finished(int result,\n \n static int update_clone(int argc, const char **argv, const char *prefix)\n {\n+\tint max_jobs = -1;\n \tstruct string_list_item *item;\n \tstruct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;\n \n@@ -451,6 +452,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"depth\", &pp.depth, \"\",\n \t\t\t N_(\"Create a shallow clone truncated to the \"\n \t\t\t \"specified number of revisions\")),\n+\t\tOPT_INTEGER('j', \"jobs\", &max_jobs,\n+\t\t\t N_(\"parallel jobs\")),\n \t\tOPT__QUIET(&pp.quiet, N_(\"do't print cloning progress\")),\n \t\tOPT_END()\n \t};\n@@ -472,10 +475,17 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \tgitmodules_config();\n \t/* Overlay the parsed .gitmodules file with .git/config */\n \tgit_config(git_submodule_config, NULL);\n-\trun_processes_parallel(1, update_clone_get_next_task,\n-\t\t\t\t update_clone_start_failure,\n-\t\t\t\t update_clone_task_finished,\n-\t\t\t\t &pp);\n+\n+\tif (max_jobs < 0)\n+\t\tmax_jobs = config_parallel_submodules();\n+\tif (max_jobs < 0)\n+\t\tmax_jobs = 1;\n+\n+\trun_processes_parallel(max_jobs,\n+\t\t\t update_clone_get_next_task,\n+\t\t\t update_clone_start_failure,\n+\t\t\t update_clone_task_finished,\n+\t\t\t &pp);\n \n \tif (pp.print_unmatched) {\n \t\tprintf(\"#unmatched\\n\");\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 9f554fb..10c5af9 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -645,6 +645,14 @@ cmd_update()\n \t\t--depth=*)\n \t\t\tdepth=$1\n \t\t\t;;\n+\t\t-j|--jobs)\n+\t\t\tcase \"$2\" in '') usage ;; esac\n+\t\t\tjobs=\"--jobs=$2\"\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--jobs=*)\n+\t\t\tjobs=$1\n+\t\t\t;;\n \t\t--)\n \t\t\tshift\n \t\t\tbreak\n@@ -670,6 +678,7 @@ cmd_update()\n \t\t${update:+--update \"$update\"} \\\n \t\t${reference:+--reference \"$reference\"} \\\n \t\t${depth:+--depth \"$depth\"} \\\n+\t\t${jobs:+$jobs} \\\n \t\t\"$@\" | {\n \terr=\n \twhile read mode sha1 stage just_cloned sm_path\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex dda3929..05ea66f 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops module name before recur\n \t test_i18ngrep \"Submodule path .deeper/submodule/subsubmodule.: checked out\" actual\n \t)\n '\n+\n+test_expect_success 'submodule update can be run in parallel' '\n+\t(cd super2 &&\n+\t GIT_TRACE=$(pwd)/trace.out git submodule update --jobs 7 &&\n+\t grep \"7 children\" trace.out &&\n+\t git config submodule.jobs 8 &&\n+\t GIT_TRACE=$(pwd)/trace.out git submodule update &&\n+\t grep \"8 children\" trace.out &&\n+\t GIT_TRACE=$(pwd)/trace.out git submodule update --jobs 9 &&\n+\t grep \"9 children\" trace.out\n+\t)\n+'\n test_done\n-- \n2.5.0.281.g4ed9cdb\n"},{"id":"272468","messageId":"1446074504-6014-9-git-send-email-sbeller@google.com","threadId":"40655","inReplyTo":"1446074504-6014-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 8/8] clone: allow an explicit argument for parallel submodule clones","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-28T23:21:44Z","receivedAt":"2015-10-28T23:21:44Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Just pass it along to \"git submodule update\", which may pick reasonable\ndefaults if you don't specify an explicit number.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/git-clone.txt | 6 +++++-\n builtin/clone.c | 23 +++++++++++++++++------\n t/t7406-submodule-update.sh | 15 +++++++++++++++\n 3 files changed, 37 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex f1f2a3f..01bd6b7 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -14,7 +14,7 @@ SYNOPSIS\n \t [-o ] [-b ] [-u ] [--reference ]\n \t [--dissociate] [--separate-git-dir ]\n \t [--depth ] [--[no-]single-branch]\n-\t [--recursive | --recurse-submodules] [--] \n+\t [--recursive | --recurse-submodules] [--jobs ] [--] \n \t []\n \n DESCRIPTION\n@@ -216,6 +216,10 @@ objects from the source repository into a pack in the cloned repository.\n \tThe result is Git repository can be separated from working\n \ttree.\n \n+-j ::\n+--jobs ::\n+\tThe number of submodules fetched at the same time.\n+\tDefaults to the `submodule.jobs` option.\n \n ::\n \tThe (possibly remote) repository to clone from. See the\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 9eaecd9..22b9924 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -50,6 +50,7 @@ static int option_progress = -1;\n static struct string_list option_config;\n static struct string_list option_reference;\n static int option_dissociate;\n+static int max_jobs = -1;\n \n static struct option builtin_clone_options[] = {\n \tOPT__VERBOSITY(&option_verbosity),\n@@ -72,6 +73,8 @@ static struct option builtin_clone_options[] = {\n \t\t N_(\"initialize submodules in the clone\")),\n \tOPT_BOOL(0, \"recurse-submodules\", &option_recursive,\n \t\t N_(\"initialize submodules in the clone\")),\n+\tOPT_INTEGER('j', \"jobs\", &max_jobs,\n+\t\t N_(\"number of submodules cloned in parallel\")),\n \tOPT_STRING(0, \"template\", &option_template, N_(\"template-directory\"),\n \t\t N_(\"directory from which templates will be used\")),\n \tOPT_STRING_LIST(0, \"reference\", &option_reference, N_(\"repo\"),\n@@ -95,10 +98,6 @@ static struct option builtin_clone_options[] = {\n \tOPT_END()\n };\n \n-static const char *argv_submodule[] = {\n-\t\"submodule\", \"update\", \"--init\", \"--recursive\", NULL\n-};\n-\n static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)\n {\n \tstatic char *suffix[] = { \"/.git\", \"\", \".git/.git\", \".git\" };\n@@ -724,8 +723,20 @@ static int checkout(void)\n \terr |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n \t\t\t sha1_to_hex(sha1), \"1\", NULL);\n \n-\tif (!err && option_recursive)\n-\t\terr = run_command_v_opt(argv_submodule, RUN_GIT_CMD);\n+\tif (!err && option_recursive) {\n+\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\t\targv_array_pushl(&args, \"submodule\", \"update\", \"--init\", \"--recursive\", NULL);\n+\n+\t\tif (max_jobs != -1) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_addf(&sb, \"--jobs=%d\", max_jobs);\n+\t\t\targv_array_push(&args, sb.buf);\n+\t\t\tstrbuf_release(&sb);\n+\t\t}\n+\n+\t\terr = run_command_v_opt(args.argv, RUN_GIT_CMD);\n+\t\targv_array_clear(&args);\n+\t}\n \n \treturn err;\n }\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 05ea66f..ade0524 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -786,4 +786,19 @@ test_expect_success 'submodule update can be run in parallel' '\n \t grep \"9 children\" trace.out\n \t)\n '\n+\n+test_expect_success 'git clone passes the parallel jobs config on to submodules' '\n+\ttest_when_finished \"rm -rf super4\" &&\n+\tGIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 . super4 &&\n+\tgrep \"7 children\" trace.out &&\n+\trm -rf super4 &&\n+\tgit config --global submodule.jobs 8 &&\n+\tGIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules . super4 &&\n+\tgrep \"8 children\" trace.out &&\n+\trm -rf super4 &&\n+\tGIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 9 . super4 &&\n+\tgrep \"9 children\" trace.out &&\n+\trm -rf super4\n+'\n+\n test_done\n-- \n2.5.0.281.g4ed9cdb\n"},{"id":"272478","messageId":"56321CF4.60807@ramsayjones.plus.com","threadId":"40655","inReplyTo":"1446074504-6014-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 0/8] Expose the submodule parallelism to the user","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2015-10-29T13:19:48Z","receivedAt":"2015-10-29T13:19:48Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 28/10/15 23:21, Stefan Beller wrote:\n> This replaces origin/sb/submodule-parallel-update\n> (anchoring at 74367d8938, Merge branch 'sb/submodule-parallel-fetch'\n> into sb/submodule-parallel-update)\n> \n> What does it do?\n> ---\n> This series should finish the on going efforts of parallelizing\n> submodule network traffic. The patches contain tests for clone,\n> fetch and submodule update to use the actual parallelism both via\n> command line as well as a configured option. I decided to go with\n> \"submodule.jobs\" for all three for now.\n> \n> What is new in v2?\n> ---\n> * The patches got reordered slightly\n> * Documentation was adapted\n> \n> Interdiff below\n> \n> Stefan Beller (8):\n> run_processes_parallel: Add output to tracing messages\n> submodule config: keep update strategy around\n> submodule config: remove name_and_item_from_var\n> submodule-config: parse_config\n> fetching submodules: Respect `submodule.jobs` config option\n> git submodule update: have a dedicated helper for cloning\n> submodule update: expose parallelism to the user\n> clone: allow an explicit argument for parallel submodule clones\n> \n> Documentation/config.txt | 7 ++\n> Documentation/git-clone.txt | 6 +-\n> Documentation/git-submodule.txt | 7 +-\n> builtin/clone.c | 23 +++-\n> builtin/fetch.c | 2 +-\n> builtin/submodule--helper.c | 244 ++++++++++++++++++++++++++++++++++++++++\n> git-submodule.sh | 54 ++++-----\n> run-command.c | 4 +\n> submodule-config.c | 98 ++++++++++------\n> submodule-config.h | 3 +\n> submodule.c | 5 +\n> t/t5526-fetch-submodules.sh | 14 +++\n> t/t7400-submodule-basic.sh | 4 +-\n> t/t7406-submodule-update.sh | 27 +++++\n> 14 files changed, 418 insertions(+), 80 deletions(-)\n> \n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 0de0138..785721a 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2643,12 +2643,12 @@ submodule..ignore::\n> \t\"--ignore-submodules\" option. The 'git submodule' commands are not\n> \taffected by this setting.\n> \n> -submodule::jobs\n> +submodule.jobs::\n> \tThis is used to determine how many submodules can be operated on in\n> \tparallel. Specifying a positive integer allows up to that number\n> -\tof submodules being fetched in parallel. Specifying 0 the number\n> -\tof cpus will be taken as the maximum number. Currently this is\n> -\tused in fetch and clone operations only.\n> +\tof submodules being fetched in parallel. This is used in fetch\n> +\tand clone operations only. A value of 0 will give some reasonable\n> +\tdefault. The defaults may change with different versions of Git.\n> \n> tag.sort::\n> \tThis variable controls the sort ordering of tags when displayed by\n> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\n> index affa52e..01bd6b7 100644\n> --- a/Documentation/git-clone.txt\n> +++ b/Documentation/git-clone.txt\n> @@ -216,9 +216,10 @@ objects from the source repository into a pack in the cloned repository.\n> \tThe result is Git repository can be separated from working\n> \ttree.\n> \n> --j::\n> ---jobs::\n> +-j ::\n> +--jobs ::\n> \tThe number of submodules fetched at the same time.\n> +\tDefaults to the `submodule.jobs` option.\n\nHmm, is there a way to _not_ fetch in parallel (override the\nconfig) from the command line for a given command?\n\nATB,\nRamsay Jones\n\n> \n> ::\n> \tThe (possibly remote) repository to clone from. See the\n> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\n> index f5429fa..c70fafd 100644\n> --- a/Documentation/git-submodule.txt\n> +++ b/Documentation/git-submodule.txt\n> @@ -374,10 +374,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` options carefully.\n> \tclone with a history truncated to the specified number of revisions.\n> \tSee linkgit:git-clone[1]\n> \n> --j::\n> ---jobs::\n> +-j ::\n> +--jobs ::\n> \tThis option is only valid for the update command.\n> \tClone new submodules in parallel with as many jobs.\n> +\tDefaults to the `submodule.jobs` option.\n> \n> ...::\n> \tPaths to submodule(s). When specified this will restrict the command\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 5ac2d89..22b9924 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -727,10 +727,7 @@ static int checkout(void)\n> \t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> \t\targv_array_pushl(&args, \"submodule\", \"update\", \"--init\", \"--recursive\", NULL);\n> \n> -\t\tif (max_jobs == -1)\n> -\t\t\tif (git_config_get_int(\"submodule.jobs\", &max_jobs))\n> -\t\t\t\tmax_jobs = 1;\n> -\t\tif (max_jobs != 1) {\n> +\t\tif (max_jobs != -1) {\n> \t\t\tstruct strbuf sb = STRBUF_INIT;\n> \t\t\tstrbuf_addf(&sb, \"--jobs=%d\", max_jobs);\n> \t\t\targv_array_push(&args, sb.buf);\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index c3d438a..67dba1c 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -476,9 +476,10 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n> \t/* Overlay the parsed .gitmodules file with .git/config */\n> \tgit_config(git_submodule_config, NULL);\n> \n> -\tif (max_jobs == -1)\n> -\t\tif (git_config_get_int(\"submodule.jobs\", &max_jobs))\n> -\t\t\tmax_jobs = 1;\n> +\tif (max_jobs < 0)\n> +\t\tmax_jobs = config_parallel_submodules();\n> +\tif (max_jobs < 0)\n> +\t\tmax_jobs = 1;\n> \n> \trun_processes_parallel(max_jobs,\n> \t\t\t update_clone_get_next_task,\n> \n"},{"id":"272482","messageId":"CAGZ79kYXrOFDqs5c-OYG2vRO9GY_aoD_GU1=TkRtOMaGC_GowA@mail.gmail.com","threadId":"40655","inReplyTo":"56321CF4.60807@ramsayjones.plus.com","subject":"Re: [PATCHv2 0/8] Expose the submodule parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-29T15:51:27Z","receivedAt":"2015-10-29T15:51:27Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 29, 2015 at 6:19 AM, Ramsay Jones\n wrote:\n\n> Hmm, is there a way to _not_ fetch in parallel (override the\n> config) from the command line for a given command?\n>\n> ATB,\n> Ramsay Jones\n\ngit config submodule.jobs 42\ngit --jobs 1 # should run just one task, despite having 42 configured\n\nIt does use the parallel processing machinery though, but with a maximum of\none subcommand being spawned. Is that what you're asking?\n\nThanks,\nStefan\n"},{"id":"272486","messageId":"xmqqh9l9si57.fsf@gitster.mtv.corp.google.com","threadId":"40655","inReplyTo":"CAGZ79kYXrOFDqs5c-OYG2vRO9GY_aoD_GU1=TkRtOMaGC_GowA@mail.gmail.com","subject":"Re: [PATCHv2 0/8] Expose the submodule parallelism to the user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-29T17:23:48Z","receivedAt":"2015-10-29T17:23:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Thu, Oct 29, 2015 at 6:19 AM, Ramsay Jones\n> wrote:\n>\n>> Hmm, is there a way to _not_ fetch in parallel (override the\n>> config) from the command line for a given command?\n>>\n>> ATB,\n>> Ramsay Jones\n>\n> git config submodule.jobs 42\n> git --jobs 1 # should run just one task, despite having 42 configured\n>\n> It does use the parallel processing machinery though, but with a maximum of\n> one subcommand being spawned. Is that what you're asking?\n\nWith this patch, do we still keep a separate machinery that bypasses\nthe parallel thing altogether in the first place?\n\nI was hoping that the underlying parallel machinery is polished\nenough that using it with max=1 parallelism would be equivalent to\nserial execution. At least, that was my understanding of our goal,\nand back when we reviewed the previous \"fetch --recurse-sub\" series,\nmy impression was we were already there.\n\nAnd in that ideal endgame world, your \"Give '-j1' from the command\nline\" would be perfectly an acceptable answer ;-).\n\nThanks.\n \n"},{"id":"272487","messageId":"CAGZ79kbUQUpCgHP8rKnijog77AwoMn4GunzfV25sgLwpTC-4ag@mail.gmail.com","threadId":"40655","inReplyTo":"xmqqh9l9si57.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2 0/8] Expose the submodule parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-29T17:30:49Z","receivedAt":"2015-10-29T17:30:49Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 29, 2015 at 10:23 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> On Thu, Oct 29, 2015 at 6:19 AM, Ramsay Jones\n>> wrote:\n>>\n>>> Hmm, is there a way to _not_ fetch in parallel (override the\n>>> config) from the command line for a given command?\n>>>\n>>> ATB,\n>>> Ramsay Jones\n>>\n>> git config submodule.jobs 42\n>> git --jobs 1 # should run just one task, despite having 42 configured\n>>\n>> It does use the parallel processing machinery though, but with a maximum of\n>> one subcommand being spawned. Is that what you're asking?\n>\n> With this patch, do we still keep a separate machinery that bypasses\n> the parallel thing altogether in the first place?\n\nNo.\n\n>\n> I was hoping that the underlying parallel machinery is polished\n> enough that using it with max=1 parallelism would be equivalent to\n> serial execution.\n\nThere is no special code path for jobs=1.\n\nIt should be pretty close, just with the overhead of the parallel engine\nspawning it one after the other and being an intermediate for output piping.\nThe one subcommand would still output via a pipe to the parallel engine,\nwhich then outputs it immediately.\n\n> At least, that was my understanding of our goal,\n> and back when we reviewed the previous \"fetch --recurse-sub\" series,\n> my impression was we were already there.\n>\n> And in that ideal endgame world, your \"Give '-j1' from the command\n> line\" would be perfectly an acceptable answer ;-).\n\nok. :)\n\n>\n> Thanks.\n>\n"},{"id":"272497","messageId":"xmqqsi4tqvs1.fsf@gitster.mtv.corp.google.com","threadId":"40655","inReplyTo":"1446074504-6014-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 0/8] Expose the submodule parallelism to the user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-29T20:12:14Z","receivedAt":"2015-10-29T20:12:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This replaces origin/sb/submodule-parallel-update\n> (anchoring at 74367d8938, Merge branch 'sb/submodule-parallel-fetch'\n> into sb/submodule-parallel-update)\n>\n> What does it do?\n> ---\n> This series should finish the on going efforts of parallelizing\n> submodule network traffic. The patches contain tests for clone,\n> fetch and submodule update to use the actual parallelism both via\n> command line as well as a configured option. I decided to go with\n> \"submodule.jobs\" for all three for now.\n>\n> What is new in v2?\n> ---\n> * The patches got reordered slightly\n> * Documentation was adapted\n\nA couple of things I noticed (other than \"many issues pointed out in\nv1 have been updated\") are:\n\n - The way 7/8 and 8/8 checks for uninitialized max_jobs are\n inconsistently written. The way 7/8 does, i.e. (max_jobs < 0),\n looks more conventional.\n\n - \"Defaults to the `submodule.jobs` option\" should say\n \"configuration variable\" instead.\n\nI haven't formed an opinion on 6/8 yet.\n"},{"id":"272500","messageId":"xmqqfv0tqp6u.fsf@gitster.mtv.corp.google.com","threadId":"40655","inReplyTo":"1446074504-6014-7-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 6/8] git submodule update: have a dedicated helper for cloning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-29T22:34:33Z","receivedAt":"2015-10-29T22:34:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> +struct submodule_update_clone {\n> +\tint count;\n> +\tint quiet;\n> +\tint print_unmatched;\n> +\tchar *reference;\n> +\tchar *depth;\n> +\tchar *update;\n> +\tconst char *recursive_prefix;\n> +\tconst char *prefix;\n> +\tstruct module_list list;\n> +\tstruct string_list projectlines;\n> +\tstruct pathspec pathspec;\n> +};\n\nThese fields should be split into at least two classes, the ones\nthat are primarily the \"configuration\", and the others that are\n\"states\". I am guessing 'quiet' is what the caller prepares and\ntells the pp callbacks that they must work with reduced verbosity,\nand 'print_unmatched' is also in the same boat. From the above\nstructure definition, nobody can guess what 'count' represents. Is\nthat the number of modules you have in the top-level superproject?\nIs that the number of modules updated so far? Some other number?\n\nWe can guess \"list\" is probably the list of modules to be cloned or\nupdated, but we have no idea what \"projectlines\" mean and what it\nwill be used for. The only word with 'project' we would use in the\ncontext of discussing submodules is the \"top level superproject\",\nbut then that will not need a \"list\", so that is not it. Perhaps\nthis refers to a list of projects bound to our tree as submodules,\nand perhaps each such submodule gives some kind of \"lines\", but it\nis totally unclear what kind of lines they use.\n\n> +static void fill_clone_command(struct child_process *cp, int quiet,\n> +\t\t\t const char *prefix, const char *path,\n> +\t\t\t const char *name, const char *url,\n> +\t\t\t const char *reference, const char *depth)\n> +{\n> +\tcp->git_cmd = 1;\n> +\tcp->no_stdin = 1;\n> +\tcp->stdout_to_stderr = 1;\n> +\tcp->err = -1;\n> +\targv_array_push(&cp->args, \"submodule--helper\");\n> +\targv_array_push(&cp->args, \"clone\");\n> +\tif (quiet)\n> +\t\targv_array_push(&cp->args, \"--quiet\");\n> +\n> +\tif (prefix) {\n> +\t\targv_array_push(&cp->args, \"--prefix\");\n> +\t\targv_array_push(&cp->args, prefix);\n> +\t}\n> +\targv_array_push(&cp->args, \"--path\");\n> +\targv_array_push(&cp->args, path);\n\nThe pattern makes readers wish if there were a way to make these\npair of pushes easier to read. The best I can come up with is\n\n argv_array_pushl(&cp->args, \"--path\", path, NULL);\n\nWhile that would be already a vast improvement, when we know there\nare many \"I want to push two\", it makes me wonder if I am entitled\nto find the repeated \", NULL\" irritating.\n\n argv_array_push2(&cp->args, \"--path\", path);\n\non the hand feels slightly too specific. I dunno.\n\n> +static int update_clone_get_next_task(void **pp_task_cb,\n> +\t\t\t\t struct child_process *cp,\n> +\t\t\t\t struct strbuf *err,\n> +\t\t\t\t void *pp_cb)\n> +{\n> +\tstruct submodule_update_clone *pp = pp_cb;\n> +\n> +\tfor (; pp->count < pp->list.nr; pp->count++) {\n> +\t\tconst struct submodule *sub = NULL;\n> +\t\tconst char *displaypath = NULL;\n> +\t\tconst struct cache_entry *ce = pp->list.entries[pp->count];\n> +\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\tconst char *update_module = NULL;\n> +\t\tchar *url = NULL;\n> +\t\tint just_cloned = 0;\n> +\n> +\t\tif (ce_stage(ce)) {\n> +\t\t\tif (pp->recursive_prefix)\n> +\t\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s/%s\\n\",\n> +\t\t\t\t\tpp->recursive_prefix, ce->name);\n> +\t\t\telse\n> +\t\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s\\n\",\n> +\t\t\t\t\tce->name);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tsub = submodule_from_path(null_sha1, ce->name);\n> +\t\tif (!sub) {\n> +\t\t\tstrbuf_addf(err, \"BUG: internal error managing submodules. \"\n> +\t\t\t\t \"The cache could not locate '%s'\", ce->name);\n> +\t\t\tpp->print_unmatched = 1;\n> +\t\t\treturn 0;\n\nThis feels a bit inconsistent. When the pp->count'th submodule is\nset not to update (i.e. \"none\" below), you let this loop to ignore\nthat submodule and continue on to process pp->count+1'th one without\nreturning to the caller. Is there a reason why this case should be\nprocessed differently? If the rest of the code treats this\ncondition as a \"grave error\" that tells the caller to never call\nget-next again (i.e. the \"emergency abort\" condition), that sort of\nmakes sense, but I cannot offhand see if that is being done in this\npatch.\n\n> +\t\t}\n> +\n> +\t\tif (pp->recursive_prefix)\n> +\t\t\tdisplaypath = relative_path(pp->recursive_prefix, ce->name, &sb);\n> +\t\telse\n> +\t\t\tdisplaypath = ce->name;\n> +\n> +\t\tif (pp->update)\n> +\t\t\tupdate_module = pp->update;\n> +\t\tif (!update_module)\n> +\t\t\tupdate_module = sub->update;\n> +\t\tif (!update_module)\n> +\t\t\tupdate_module = \"checkout\";\n> +\t\tif (!strcmp(update_module, \"none\")) {\n> +\t\t\tstrbuf_addf(err, \"Skipping submodule '%s'\\n\", displaypath);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Looking up the url in .git/config.\n> +\t\t * We cannot fall back to .gitmodules as we only want to process\n\ns/cannot/must not/, right?\n\n> +\t\t * configured submodules. This renders the submodule lookup API\n> +\t\t * useless, as it cannot lookup without fallback.\n> +\t\t */\n\nI doubt the value of the last sentence, especially the \"useless\"\npart.\n\nEither \"We do not want to read .gitmodules and that is why we do not\nuse submodule config API, period\" (which does not make it \"useless\",\nit is just not meant to be used here at all), or \"We do not want to\nread .gitmodules in this codepath, and submodule config API cannot\nbe used here before we teach it an option to only check the config\nwithout falling back\" (which does not make it \"useless\", it is just\nthat you haven't made it ready to be used here yet).\n\n> +\t\tstrbuf_reset(&sb);\n> +\t\tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n> +\t\tgit_config_get_string(sb.buf, &url);\n> +\t\tif (!url) {\n> +\t\t\t/*\n> +\t\t\t * Only mention uninitialized submodules when its\n> +\t\t\t * path have been specified\n> +\t\t\t */\n> +\t\t\tif (pp->pathspec.nr)\n> +\t\t\t\tstrbuf_addf(err, _(\"Submodule path '%s' not initialized\\n\"\n> +\t\t\t\t\t\"Maybe you want to use 'update --init'?\"), displaypath);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tstrbuf_reset(&sb);\n> +\t\tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n> +\t\tjust_cloned = !file_exists(sb.buf);\n\nThat name was misleading and had me scratch my head for a while.\nThis module is in the \"needs cloning\" state, and you haven't even\nstarted cloning it yet.\n\n> +\t\tstrbuf_reset(&sb);\n> +\t\tstrbuf_addf(&sb, \"%06o %s %d %d\\t%s\\n\", ce->ce_mode,\n> +\t\t\t\tsha1_to_hex(ce->sha1), ce_stage(ce),\n> +\t\t\t\tjust_cloned, ce->name);\n> +\t\tstring_list_append(&pp->projectlines, sb.buf);\n> +\n> +\t\tif (just_cloned) {\n> +\t\t\tfill_clone_command(cp, pp->quiet, pp->prefix, ce->name,\n> +\t\t\t\t\t sub->name, url, pp->reference, pp->depth);\n> +\t\t\tpp->count++;\n> +\t\t\tfree(url);\n> +\t\t\treturn 1;\n> +\t\t} else\n> +\t\t\tfree(url);\n> +\t}\n> +\treturn 0;\n> +}\n\nThat's it for today. I'll take a look at the remainder another day.\n\nThanks.\n"},{"id":"272501","messageId":"5632B0E1.8040309@ramsayjones.plus.com","threadId":"40655","inReplyTo":"CAGZ79kYXrOFDqs5c-OYG2vRO9GY_aoD_GU1=TkRtOMaGC_GowA@mail.gmail.com","subject":"Re: [PATCHv2 0/8] Expose the submodule parallelism to the user","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2015-10-29T23:50:57Z","receivedAt":"2015-10-29T23:50:57Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 29/10/15 15:51, Stefan Beller wrote:\n> On Thu, Oct 29, 2015 at 6:19 AM, Ramsay Jones\n> wrote:\n> \n>> Hmm, is there a way to _not_ fetch in parallel (override the\n>> config) from the command line for a given command?\n>>\n>> ATB,\n>> Ramsay Jones\n> \n> git config submodule.jobs 42\n> git --jobs 1 # should run just one task, despite having 42 configured\n\nHeh, yes ... I didn't pose the question quite right ...\n> \n> It does use the parallel processing machinery though, but with a maximum of\n> one subcommand being spawned. Is that what you're asking?\n\n... but, despite that, you correctly inferred what I was really\nasking about! :)\n\nI was just wondering what overhead the parallel processing machinery\nadds to the original 'non-parallel' code path (for the j=1 case).\nI suspect the answer is 'not much', but that's just a guess.\nHave you measured it? What happens if there is only a single\nsubmodule to fetch?\n\nATB,\nRamsay Jones\n"},{"id":"272507","messageId":"CAPig+cToAFAPhhFhOd_MF+EUcvRUjWOooeZH4uDy3-d9GEq73g@mail.gmail.com","threadId":"40655","inReplyTo":"1446074504-6014-2-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 1/8] run_processes_parallel: Add output to tracing messages","fromName":"Eric Sunshine","fromEmail":"ericsunshine@gmail.com","sentAt":"2015-10-30T01:10:16Z","receivedAt":"2015-10-30T01:10:16Z","isPatch":false,"sender":{"key":"ericsunshine@gmail.com","avatar":null},"body":"On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller wrote:\n> run_processes_parallel: Add output to tracing messages\n\nThis doesn't really say much. I guess you mean that the intention is\nto delimit a section in which output from various tasks may be\nintermixed. Perhaps:\n\n run_processes_parallel: delimit intermixed task output\n\nor something.\n\n> This commit serves 2 purposes. First this may help the user who\n> tries to diagnose intermixed process calls. Second this may be used\n> in a later patch for testing. As the output of a command should not\n> change visibly except for going faster, grepping for the trace output\n> seems like a viable testing strategy.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/run-command.c b/run-command.c\n> index 82cc238..49dec74 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -959,6 +959,9 @@ static struct parallel_processes *pp_init(int n,\n> n = online_cpus();\n>\n> pp->max_processes = n;\n> +\n> + trace_printf(\"run_processes_parallel: preparing to run up to %d children in parallel\", n);\n\ns/children/tasks/ maybe?\n\nMinor: Perhaps drop \"in parallel\" since the parallelism is already\nimplied by the \"run_processes_parallel\" prefix.\n\n> +\n> pp->data = data;\n> if (!get_next_task)\n> die(\"BUG: you need to specify a get_next_task function\");\n> @@ -988,6 +991,7 @@ static void pp_cleanup(struct parallel_processes *pp)\n> {\n> int i;\n>\n> + trace_printf(\"run_processes_parallel: parallel processing done\");\n\nMinor: Likewise, perhaps just \"done\" rather than \"parallel processing\ndone\" since the \"run_processes_parallel\" prefix already implies\nparallelism.\n\n> for (i = 0; i < pp->max_processes; i++) {\n> strbuf_release(&pp->children[i].err);\n> child_process_deinit(&pp->children[i].process);\n> --\n> 2.5.0.281.g4ed9cdb\n"},{"id":"272508","messageId":"CAPig+cRTa35B5aHcopaWOtCLxN6BhGJKTcVeDUf0hrZE_nfCKQ@mail.gmail.com","threadId":"40655","inReplyTo":"1446074504-6014-3-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 2/8] submodule config: keep update strategy around","fromName":"Eric Sunshine","fromEmail":"ericsunshine@gmail.com","sentAt":"2015-10-30T01:14:22Z","receivedAt":"2015-10-30T01:14:22Z","isPatch":false,"sender":{"key":"ericsunshine@gmail.com","avatar":null},"body":"On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller wrote:\n> We need the submodule update strategies in a later patch.\n>\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> ---\n> diff --git a/submodule-config.c b/submodule-config.c\n> index afe0ea8..8b8c7d1 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -311,6 +312,16 @@ static int parse_config(const char *var, const char *value, void *data)\n> free((void *) submodule->url);\n> submodule->url = xstrdup(value);\n> }\n> + } else if (!strcmp(item.buf, \"update\")) {\n> + if (!value)\n> + ret = config_error_nonbool(var);\n> + else if (!me->overwrite && submodule->update != NULL)\n\nAlthough \"foo != NULL\" is unusual in this code-base, it is used\nelsewhere in this file, including just outside the context seen above.\nOkay.\n\n> + warn_multiple_config(me->commit_sha1, submodule->name,\n> + \"update\");\n> + else {\n> + free((void *)submodule->update);\n\nMinor: Every other 'free((void *) foo)' in this file has a space after\n\"(void *)\", one of which can be seen in the context just above.\n\n> + submodule->update = xstrdup(value);\n> + }\n> }\n>\n> strbuf_release(&name);\n"},{"id":"272511","messageId":"CAPig+cSpnGE5Acgvd+b0arcFx8oStuRRKR4fcSwZG2fbEZ6wSQ@mail.gmail.com","threadId":"40655","inReplyTo":"1446074504-6014-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 3/8] submodule config: remove name_and_item_from_var","fromName":"Eric Sunshine","fromEmail":"ericsunshine@gmail.com","sentAt":"2015-10-30T01:23:49Z","receivedAt":"2015-10-30T01:23:49Z","isPatch":false,"sender":{"key":"ericsunshine@gmail.com","avatar":null},"body":"On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller wrote:\n> submodule config: remove name_and_item_from_var\n>\n> By inlining `name_and_item_from_var` it is easy to add later options\n> which are not required to have a submodule name.\n\nI guess you're trying to say that name_and_item_from_var() didn't\nprovide a proper abstraction, thus wasn't as useful as expected.\nPerhaps that commit message could make this shortcoming clearer.\n\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/submodule-config.c b/submodule-config.c\n> index 8b8c7d1..4d0563c 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -251,18 +235,25 @@ static int parse_config(const char *var, const char *value, void *data)\n> {\n> struct parse_config_parameter *me = data;\n> struct submodule *submodule;\n> - struct strbuf name = STRBUF_INIT, item = STRBUF_INIT;\n> - int ret = 0;\n> + int subsection_len, ret = 0;\n> + const char *subsection, *key;\n> + char *name;\n>\n> - /* this also ensures that we only parse submodule entries */\n> - if (!name_and_item_from_var(var, &name, &item))\n> + if (parse_config_key(var, \"submodule\", &subsection,\n> + &subsection_len, &key) < 0)\n> return 0;\n>\n> + if (!subsection_len)\n> + return 0;\n\nAlternately:\n\n if (parse_config_key(var, \"submodule\", &subsection,\n &subsection_len, &key) < 0 || !subsection_len)\n return 0;\n\n> +\n> + /* subsection is not null terminated */\n> + name = xmemdupz(subsection, subsection_len);\n> submodule = lookup_or_create_by_name(me->cache,\n> me->gitmodules_sha1,\n> - name.buf);\n> + name);\n> + free(name);\n\nSince this is all private to submodule-config.c, I wonder if it would\nbe cleaner to change lookup_or_create_by_name() to accept a\nname_length argument?\n\n> - if (!strcmp(item.buf, \"path\")) {\n> + if (!strcmp(key, \"path\")) {\n> if (!value)\n> ret = config_error_nonbool(var);\n> else if (!me->overwrite && submodule->path != NULL)\n"},{"id":"272512","messageId":"CAPig+cRHy5iT940scnKyMNDx8zgXt50ZsFqF0tALVRpueKdo-A@mail.gmail.com","threadId":"40655","inReplyTo":"1446074504-6014-5-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 4/8] submodule-config: parse_config","fromName":"Eric Sunshine","fromEmail":"ericsunshine@gmail.com","sentAt":"2015-10-30T01:53:44Z","receivedAt":"2015-10-30T01:53:44Z","isPatch":false,"sender":{"key":"ericsunshine@gmail.com","avatar":null},"body":"On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller wrote:\n> submodule-config: parse_config\n\nUm, what?\n\n> This rewrites parse_config to distinguish between configs specific to\n> one submodule and configs which apply generically to all submodules.\n> We do not have generic submodule configs yet, but the next patch will\n> introduce \"submodule.jobs\".\n>\n> Signed-off-by: Stefan Beller \n>\n> # Conflicts:\n> # submodule-config.c\n\nInteresting.\n\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/submodule-config.c b/submodule-config.c\n> index 4d0563c..1cea404 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -231,27 +231,23 @@ struct parse_config_parameter {\n> int overwrite;\n> };\n>\n> -static int parse_config(const char *var, const char *value, void *data)\n> +static int parse_generic_submodule_config(const char *var,\n> + const char *key,\n> + const char *value)\n> {\n> - struct parse_config_parameter *me = data;\n> - struct submodule *submodule;\n> - int subsection_len, ret = 0;\n> - const char *subsection, *key;\n> - char *name;\n> -\n> - if (parse_config_key(var, \"submodule\", &subsection,\n> - &subsection_len, &key) < 0)\n> - return 0;\n> -\n> - if (!subsection_len)\n> - return 0;\n> + return 0;\n> +}\n>\n> - /* subsection is not null terminated */\n> - name = xmemdupz(subsection, subsection_len);\n> - submodule = lookup_or_create_by_name(me->cache,\n> - me->gitmodules_sha1,\n> - name);\n> - free(name);\n> +static int parse_specific_submodule_config(struct parse_config_parameter *me,\n> + const char *name,\n> + const char *key,\n> + const char *value,\n> + const char *var)\n\nMinor: Are these 'key', 'value', 'var' arguments analogous to the\nlike-named arguments of parse_generic_submodule_config()? If so, why\nis the order of arguments different?\n\n> +{\n> + int ret = 0;\n> + struct submodule *submodule = lookup_or_create_by_name(me->cache,\n> + me->gitmodules_sha1,\n> + name);\n>\n> if (!strcmp(key, \"path\")) {\n> if (!value)\n> @@ -318,6 +314,30 @@ static int parse_config(const char *var, const char *value, void *data)\n> return ret;\n> }\n>\n> +static int parse_config(const char *var, const char *value, void *data)\n> +{\n> + struct parse_config_parameter *me = data;\n> +\n> + int subsection_len;\n> + const char *subsection, *key;\n> + char *name;\n> +\n> + if (parse_config_key(var, \"submodule\", &subsection,\n> + &subsection_len, &key) < 0)\n> + return 0;\n> +\n> + if (!subsection_len)\n> + return parse_generic_submodule_config(var, key, value);\n> + else {\n> + int ret;\n> + /* subsection is not null terminated */\n> + name = xmemdupz(subsection, subsection_len);\n> + ret = parse_specific_submodule_config(me, name, key, value, var);\n> + free(name);\n> + return ret;\n> + }\n> +}\n\nMinor: You could drop the 'else' and outdent its body, thus losing one\nindentation level.\n\n if (!subsection_len)\n return parse_generic_submodule_config(...);\n\n int ret;\n ...\n return ret;\n\nThis might give you a less noisy diff and would be a bit more\nconsistent with the early part of the function where you don't bother\ngiving the if (parse_config_key(...)) an 'else' body.\n"},{"id":"272514","messageId":"CAPig+cS5yCzcz6xNyaMLTBFUzPqmbbE8x2_toFxAvXELcc786A@mail.gmail.com","threadId":"40655","inReplyTo":"1446074504-6014-6-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 5/8] fetching submodules: Respect `submodule.jobs` config option","fromName":"Eric Sunshine","fromEmail":"ericsunshine@gmail.com","sentAt":"2015-10-30T02:17:12Z","receivedAt":"2015-10-30T02:17:12Z","isPatch":false,"sender":{"key":"ericsunshine@gmail.com","avatar":null},"body":"On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller wrote:\n> This allows to configure fetching and updating in parallel\n> without having the command line option.\n>\n> This moved the responsibility to determine how many parallel processes\n> to start from builtin/fetch to submodule.c as we need a way to communicate\n> \"The user did not specify the number of parallel processes in the command\n> line options\" in the builtin fetch. The submodule code takes care of\n> the precedence (CLI > config > default)\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 391a0c3..785721a 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2643,6 +2643,13 @@ submodule..ignore::\n> \"--ignore-submodules\" option. The 'git submodule' commands are not\n> affected by this setting.\n>\n> +submodule.jobs::\n> + This is used to determine how many submodules can be operated on in\n> + parallel. Specifying a positive integer allows up to that number\n> + of submodules being fetched in parallel. This is used in fetch\n> + and clone operations only. A value of 0 will give some reasonable\n> + default. The defaults may change with different versions of Git.\n\nI'm not sure that \"default\" is the correct word here. When you talk\nabout a \"default\", you're normally explaining what happens when the\nconfiguration is not provided. (In fact, the default number of jobs is\n1, which you may want to document here).\n\n> tag.sort::\n> This variable controls the sort ordering of tags when displayed by\n> linkgit:git-tag[1]. Without the \"--sort=\" option provided, the\n> diff --git a/submodule-config.c b/submodule-config.c\n> index 1cea404..07bdcdf 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -32,6 +32,7 @@ enum lookup_type {\n>\n> static struct submodule_cache cache;\n> static int is_cache_init;\n> +static int parallel_jobs = -1;\n>\n> static int config_path_cmp(const struct submodule_entry *a,\n> const struct submodule_entry *b,\n> @@ -235,6 +236,9 @@ static int parse_generic_submodule_config(const char *var,\n> const char *key,\n> const char *value)\n> {\n> + if (!strcmp(key, \"jobs\")) {\n> + parallel_jobs = strtol(value, NULL, 10);\n> + }\n\nStyle: unnecessary braces\n\nWhy does this allow a negative value? The documentation doesn't\nmention anything about it.\n\n> return 0;\n> }\n>\n> diff --git a/submodule.c b/submodule.c\n> index 0257ea3..188ba02 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -752,6 +752,11 @@ int fetch_populated_submodules(const struct argv_array *options,\n> argv_array_push(&spf.args, \"--recurse-submodules-default\");\n> /* default value, \"--submodule-prefix\" and its value are added later */\n>\n> + if (max_parallel_jobs < 0)\n> + max_parallel_jobs = config_parallel_submodules();\n> + if (max_parallel_jobs < 0)\n> + max_parallel_jobs = 1;\n\nrun_process_parallel() itself specially handles max_parallel_jobs==0,\nso you don't need to consider it here. Okay.\n\n> +\n> calculate_changed_submodule_paths();\n> run_processes_parallel(max_parallel_jobs,\n> get_next_submodule,\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index 1b4ce69..5c3579c 100755\n> --- a/t/t5526-fetch-submodules.sh\n> +++ b/t/t5526-fetch-submodules.sh\n> @@ -470,4 +470,18 @@ test_expect_success \"don't fetch submodule when newly recorded commits are alrea\n> test_i18ncmp expect.err actual.err\n> '\n>\n> +test_expect_success 'fetching submodules respects parallel settings' '\n> + git config fetch.recurseSubmodules true &&\n> + (\n> + cd downstream &&\n> + GIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 &&\n> + grep \"7 children\" trace.out &&\n> + git config submodule.jobs 8 &&\n> + GIT_TRACE=$(pwd)/trace.out git fetch &&\n> + grep \"8 children\" trace.out &&\n> + GIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&\n> + grep \"9 children\" trace.out\n> + )\n> +'\n\nNot specifically related to this test, but maybe add tests to check\ncases when --jobs is not specified, and --jobs=1?\n\n> +\n> test_done\n> --\n> 2.5.0.281.g4ed9cdb\n>\n"},{"id":"272536","messageId":"CAGZ79kbmkwiQYSqtvn0kTCqh6XfkkcfxN1exTXzr8FOz4pWDQw@mail.gmail.com","threadId":"40655","inReplyTo":"CAPig+cToAFAPhhFhOd_MF+EUcvRUjWOooeZH4uDy3-d9GEq73g@mail.gmail.com","subject":"Re: [PATCHv2 1/8] run_processes_parallel: Add output to tracing messages","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-30T17:32:55Z","receivedAt":"2015-10-30T17:32:55Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 29, 2015 at 6:10 PM, Eric Sunshine wrote:\n> On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller wrote:\n>> run_processes_parallel: Add output to tracing messages\n>\n> This doesn't really say much. I guess you mean that the intention is\n> to delimit a section in which output from various tasks may be\n> intermixed.\n\nMy original intention is to have it there for testing in later patches,\nso I am not so much interested in the delimiting but the raw number\n%d here.\n\n> run_processes_parallel: delimit intermixed task output\n\nSounds good to me, better than my subject.\n\n> s/children/tasks/ maybe?\n>\n> Minor: Perhaps drop \"in parallel\" since the parallelism is already\n> implied by the \"run_processes_parallel\" prefix.\n\ndone\n\n>> + trace_printf(\"run_processes_parallel: parallel processing done\");\n>\n> Minor: Likewise, perhaps just \"done\" rather than \"parallel processing\n> done\" since the \"run_processes_parallel\" prefix already implies\n> parallelism.\n\ndone\n"},{"id":"272538","messageId":"CAGZ79kZ1usWVutWwyFQKeujyyTPVRtSQM6dvkU9gWUDSTNpB6w@mail.gmail.com","threadId":"40655","inReplyTo":"CAPig+cRTa35B5aHcopaWOtCLxN6BhGJKTcVeDUf0hrZE_nfCKQ@mail.gmail.com","subject":"Re: [PATCHv2 2/8] submodule config: keep update strategy around","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-30T17:38:38Z","receivedAt":"2015-10-30T17:38:38Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 29, 2015 at 6:14 PM, Eric Sunshine wrote:\n>> + else if (!me->overwrite && submodule->update != NULL)\n>\n> Although \"foo != NULL\" is unusual in this code-base, it is used\n> elsewhere in this file, including just outside the context seen above.\n> Okay.\n\nok, I'll clean that up as we go.\n\n>> + free((void *)submodule->update);\n>\n> Minor: Every other 'free((void *) foo)' in this file has a space after\n> \"(void *)\", one of which can be seen in the context just above.\n\ndone\n"},{"id":"272543","messageId":"CAPig+cRh9J0izFvLzRjjU4FEBKJsiJaYFv=9WdxFVfJ3xs0JiQ@mail.gmail.com","threadId":"40655","inReplyTo":"CAGZ79kZ1usWVutWwyFQKeujyyTPVRtSQM6dvkU9gWUDSTNpB6w@mail.gmail.com","subject":"Re: [PATCHv2 2/8] submodule config: keep update strategy around","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-10-30T18:16:19Z","receivedAt":"2015-10-30T18:16:19Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Oct 30, 2015 at 1:38 PM, Stefan Beller wrote:\n> On Thu, Oct 29, 2015 at 6:14 PM, Eric Sunshine wrote:\n>>> + else if (!me->overwrite && submodule->update != NULL)\n>>\n>> Although \"foo != NULL\" is unusual in this code-base, it is used\n>> elsewhere in this file, including just outside the context seen above.\n>> Okay.\n>\n> ok, I'll clean that up as we go.\n\nOh, I wasn't suggesting that you clean this up (though you may if you\nwant). I was merely commenting (for the sake of others reviewing this\npatch) that, while not the norm for the project, this instance is\nconsistent with surrounding code.\n\n>>> + free((void *)submodule->update);\n>>\n>> Minor: Every other 'free((void *) foo)' in this file has a space after\n>> \"(void *)\", one of which can be seen in the context just above.\n>\n> done\n"},{"id":"272547","messageId":"CAGZ79kYCmqv6vqRERWmihs5Ym-ug_xiqebMjQMDzjAmHgwKPGw@mail.gmail.com","threadId":"40655","inReplyTo":"CAPig+cRh9J0izFvLzRjjU4FEBKJsiJaYFv=9WdxFVfJ3xs0JiQ@mail.gmail.com","subject":"Re: [PATCHv2 2/8] submodule config: keep update strategy around","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-30T18:25:25Z","receivedAt":"2015-10-30T18:25:25Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Oct 30, 2015 at 11:16 AM, Eric Sunshine wrote:\n> On Fri, Oct 30, 2015 at 1:38 PM, Stefan Beller wrote:\n>> On Thu, Oct 29, 2015 at 6:14 PM, Eric Sunshine wrote:\n>>>> + else if (!me->overwrite && submodule->update != NULL)\n>>>\n>>> Although \"foo != NULL\" is unusual in this code-base, it is used\n>>> elsewhere in this file, including just outside the context seen above.\n>>> Okay.\n>>\n>> ok, I'll clean that up as we go.\n>\n> Oh, I wasn't suggesting that you clean this up (though you may if you\n> want). I was merely commenting (for the sake of others reviewing this\n> patch) that, while not the norm for the project, this instance is\n> consistent with surrounding code.\n\nI only did a separate patch on top cleaning up 4 occurrences in that file.\nWe use != NULL quite often throughout the code base, specially in\nconditions with side effects like:\n\n while ((char *c = string++) != NULL) {\n ...\n\nwhere I think that makes even sense. But there are a minor number of\ncases where we have no side effects\n\n $ grep -rI \"!= NULL\" |grep -v \"((\" |grep -v \"))\" |wc -l\n 135\n\n\n\n>\n>>>> + free((void *)submodule->update);\n>>>\n>>> Minor: Every other 'free((void *) foo)' in this file has a space after\n>>> \"(void *)\", one of which can be seen in the context just above.\n>>\n>> done\n"},{"id":"272554","messageId":"CAGZ79kZekEa5CwaisXNvpXLxfEp9zdDQKYULEhwiikwj1AyiFA@mail.gmail.com","threadId":"40655","inReplyTo":"CAPig+cSpnGE5Acgvd+b0arcFx8oStuRRKR4fcSwZG2fbEZ6wSQ@mail.gmail.com","subject":"Re: [PATCHv2 3/8] submodule config: remove name_and_item_from_var","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-30T18:37:18Z","receivedAt":"2015-10-30T18:37:18Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 29, 2015 at 6:23 PM, Eric Sunshine wrote:\n> On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller wrote:\n>> submodule config: remove name_and_item_from_var\n>>\n>> By inlining `name_and_item_from_var` it is easy to add later options\n>> which are not required to have a submodule name.\n>\n> I guess you're trying to say that name_and_item_from_var() didn't\n> provide a proper abstraction, thus wasn't as useful as expected.\n> Perhaps that commit message could make this shortcoming clearer.\n>\n\nok\n\n>\n> if (parse_config_key(var, \"submodule\", &subsection,\n> &subsection_len, &key) < 0 || !subsection_len)\n> return 0;\n\ndone\n\n>> submodule = lookup_or_create_by_name(me->cache,\n>> me->gitmodules_sha1,\n>> - name.buf);\n>> + name);\n>> + free(name);\n>\n> Since this is all private to submodule-config.c, I wonder if it would\n> be cleaner to change lookup_or_create_by_name() to accept a\n> name_length argument?\n>\n\nThat looks amazingly clean. :)\n"},{"id":"272567","messageId":"CAGZ79kbPkc_+g1QHxAoN2yYKG-Tft=yR=uJ-NCddRjrc5Wy20A@mail.gmail.com","threadId":"40655","inReplyTo":"CAPig+cRHy5iT940scnKyMNDx8zgXt50ZsFqF0tALVRpueKdo-A@mail.gmail.com","subject":"Re: [PATCHv2 4/8] submodule-config: parse_config","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-30T19:29:11Z","receivedAt":"2015-10-30T19:29:11Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 29, 2015 at 6:53 PM, Eric Sunshine wrote:\n> On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller wrote:\n>> submodule-config: parse_config\n>\n> Um, what?\n\nsubmodule-config: Introduce parse_generic_submodule_config\n\n>\n>> This rewrites parse_config to distinguish between configs specific to\n>> one submodule and configs which apply generically to all submodules.\n>> We do not have generic submodule configs yet, but the next patch will\n>> introduce \"submodule.jobs\".\n>>\n>> Signed-off-by: Stefan Beller \n>>\n>> # Conflicts:\n>> # submodule-config.c\n>\n> Interesting.\n\nfixed\n\n>\n> Minor: Are these 'key', 'value', 'var' arguments analogous to the\n> like-named arguments of parse_generic_submodule_config()? If so, why\n> is the order of arguments different?\n\nReordered. I thought how they made most sense individually, but consistency\nacross functions is better.\n\n>\n>> +{\n>> + int ret = 0;\n>> + struct submodule *submodule = lookup_or_create_by_name(me->cache,\n>> + me->gitmodules_sha1,\n>> + name);\n>>\n>> if (!strcmp(key, \"path\")) {\n>> if (!value)\n>> @@ -318,6 +314,30 @@ static int parse_config(const char *var, const char *value, void *data)\n>> return ret;\n>> }\n>>\n>> +static int parse_config(const char *var, const char *value, void *data)\n>> +{\n>> + struct parse_config_parameter *me = data;\n>> +\n>> + int subsection_len;\n>> + const char *subsection, *key;\n>> + char *name;\n>> +\n>> + if (parse_config_key(var, \"submodule\", &subsection,\n>> + &subsection_len, &key) < 0)\n>> + return 0;\n>> +\n>> + if (!subsection_len)\n>> + return parse_generic_submodule_config(var, key, value);\n>> + else {\n>> + int ret;\n>> + /* subsection is not null terminated */\n>> + name = xmemdupz(subsection, subsection_len);\n>> + ret = parse_specific_submodule_config(me, name, key, value, var);\n>> + free(name);\n>> + return ret;\n>> + }\n>> +}\n>\n> Minor: You could drop the 'else' and outdent its body, thus losing one\n> indentation level.\n\nBy passing on the subsection, subsection_len, we only have one statement there\n\n if (!subsection_len)\n return parse_generic_submodule_config(key, var, value, me);\n else\n return parse_specific_submodule_config(subsection,\n subsection_len, key,\n var, value, me);\n\nwill do without dedenting I guess.\n"},{"id":"272653","messageId":"CAPig+cRLQeiHcP=fseT0r-Ata6FE-EjAzFpKdtex-_NH4WMBdQ@mail.gmail.com","threadId":"40655","inReplyTo":"1446074504-6014-9-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 8/8] clone: allow an explicit argument for parallel submodule clones","fromName":"Eric Sunshine","fromEmail":"ericsunshine@gmail.com","sentAt":"2015-11-01T08:58:38Z","receivedAt":"2015-11-01T08:58:38Z","isPatch":false,"sender":{"key":"ericsunshine@gmail.com","avatar":null},"body":"On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller wrote:\n> Just pass it along to \"git submodule update\", which may pick reasonable\n> defaults if you don't specify an explicit number.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> @@ -724,8 +723,20 @@ static int checkout(void)\n> err |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n> sha1_to_hex(sha1), \"1\", NULL);\n>\n> - if (!err && option_recursive)\n> - err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);\n> + if (!err && option_recursive) {\n> + struct argv_array args = ARGV_ARRAY_INIT;\n> + argv_array_pushl(&args, \"submodule\", \"update\", \"--init\", \"--recursive\", NULL);\n> +\n> + if (max_jobs != -1) {\n> + struct strbuf sb = STRBUF_INIT;\n> + strbuf_addf(&sb, \"--jobs=%d\", max_jobs);\n> + argv_array_push(&args, sb.buf);\n> + strbuf_release(&sb);\n\nThe above four lines can be collapsed to:\n\n argv_array_pushf(&args, \"--jobs=%d\", max_jobs);\n\n> + }\n> +\n> + err = run_command_v_opt(args.argv, RUN_GIT_CMD);\n> + argv_array_clear(&args);\n> + }\n>\n> return err;\n> }\n"},{"id":"272844","messageId":"CAGZ79kbWbN_8XSMyYnkxstqV-+fHEixceeGaR4NYGqrvw0ZaUQ@mail.gmail.com","threadId":"40655","inReplyTo":"5632B0E1.8040309@ramsayjones.plus.com","subject":"Re: [PATCHv2 0/8] Expose the submodule parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-03T19:41:52Z","receivedAt":"2015-11-03T19:41:52Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 29, 2015 at 4:50 PM, Ramsay Jones\n wrote:\n>\n>\n> On 29/10/15 15:51, Stefan Beller wrote:\n>> On Thu, Oct 29, 2015 at 6:19 AM, Ramsay Jones\n>> wrote:\n>>\n>>> Hmm, is there a way to _not_ fetch in parallel (override the\n>>> config) from the command line for a given command?\n>>>\n>>> ATB,\n>>> Ramsay Jones\n>>\n>> git config submodule.jobs 42\n>> git --jobs 1 # should run just one task, despite having 42 configured\n>\n> Heh, yes ... I didn't pose the question quite right ...\n>>\n>> It does use the parallel processing machinery though, but with a maximum of\n>> one subcommand being spawned. Is that what you're asking?\n>\n> ... but, despite that, you correctly inferred what I was really\n> asking about! :)\n>\n> I was just wondering what overhead the parallel processing machinery\n> adds to the original 'non-parallel' code path (for the j=1 case).\n> I suspect the answer is 'not much', but that's just a guess.\n> Have you measured it?\n\nTotally unscientific:\n * Make a copy of my current gerrit repository and time the fetch.\n * That repo contains 5 submodules, one needs fetching\n\ntime git fetch --recurse-submodules=yes --jobs=1 # this series\nreal 0m7.150s\nuser 0m3.459s\nsys 0m1.126s\n\ntime git fetch --recurse-submodules=yes # origin/master\nreal 0m7.667s\nuser 0m3.439s\nsys 0m1.190s\n\nNow let's test a few more times repeatedly to avoid cold caches or\nnetwork hiccups, (also there is nothing to fetch, so it's more like doing\n6 ls-remotes in a row, one for gerrit and 5 submodules)\n\nthis series, best out of 5:\nreal 0m3.971s\nuser 0m2.447s\nsys 0m0.452s\n\nthis series, worst out of 5:\nreal 0m4.229s\nuser 0m2.506s\nsys 0m0.413s\n\norigin/master, best out of 5:\nreal 0m3.968s\nuser 0m2.516s\nsys 0m0.380s\n\norigin/master, worst out of 5:\nreal 0m4.217s\nuser 0m2.472s\nsys 0m0.408s\n\nThe ratio of real time taken longer is < 1 % in\nboth the best and worst case.\n\nIf you really care about 1 % of performance, you'd want to fetch in\nparallel anyway?\n\n\n> What happens if there is only a single\n> submodule to fetch?\n\nOk let's see. I created https://github.com/stefanbeller/test-sub-1\nto play around with it. However\ntime git fetch --recurse-submodules=yes\nor\ntime git fetch --recurse-submodules=yes --jobs 100\nseems to be lost in the noise.\n\nSo I am not sure what the question is w.r.t. having just one\nsubmodule.\n\n\n>\n> ATB,\n> Ramsay Jones\n>\n>\n"}]}