{"thread":{"id":"41378","subject":"[PATCHv9 2/6] submodule-config: drop check against NULL","startedAt":"2016-02-09T20:54:28Z","lastAt":"2016-02-11T20:33:40Z","messageCount":30,"participants":["Stefan Beller","Junio C Hamano","Jonathan Nieder","Jacob Keller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"277824","messageId":"1455051274-15256-1-git-send-email-sbeller@google.com","threadId":"41378","inReplyTo":null,"subject":"[PATCHv9 0/6] Expose submodule parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-09T20:54:28Z","receivedAt":"2016-02-09T20:54:28Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This replaces sb/submodule-parallel-update.\n(which is based on sb/submodule-parallel-fetch)\n\nThanks to Junio and Jonathan for review!\n\n* s/config_parallel_submodules/parallel_submodules/ as it is not in config any\n more. Also ease up the default setting. \n\n* use an enum for submodule update strategy\n\n* parsing of submodule.fetchJobs in submodule.c instead of submodule-config.\n -> no need for 2 refactoring patches\n \n* This seems to clash with 00/20] refs backend.\n> Applied this on top of a merge between the current 'master' and\n> 'sb/submodule-parallel-update' topic to untangle the dependency;\n> otherwise there is no way for this topic to make progress X-<.\n\nAnything I can do to help with easing the clash?\n\nThanks,\nStefan\n\ninterdiff to v8:\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 7e01b85..6c74474 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -257,7 +257,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\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+\treturn submodule_config(var, value, cb);\n }\n \n struct submodule_update_clone {\n@@ -316,7 +316,6 @@ static int update_clone_get_next_task(struct child_process *cp,\n \t\tstruct strbuf displaypath_sb = STRBUF_INIT;\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tconst char *displaypath = NULL;\n-\t\tconst char *update_module = NULL;\n \t\tchar *url = NULL;\n \t\tint needs_cloning = 0;\n \n@@ -340,13 +339,8 @@ static int update_clone_get_next_task(struct child_process *cp,\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\tif ((pp->update && !strcmp(pp->update, \"none\")) ||\n+\t\t (!pp->update && sub->update == SM_UPDATE_NONE)) {\n \t\t\tstrbuf_addf(err, \"Skipping submodule '%s'\\n\",\n \t\t\t\t displaypath);\n \t\t\tgoto cleanup_and_continue;\n@@ -479,9 +473,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \tgit_config(git_submodule_config, NULL);\n \n \tif (max_jobs < 0)\n-\t\tmax_jobs = config_parallel_submodules();\n-\tif (max_jobs < 0)\n-\t\tmax_jobs = 1;\n+\t\tmax_jobs = parallel_submodules();\n \n \trun_processes_parallel(max_jobs,\n \t\t\t update_clone_get_next_task,\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 339b59d..d6c8d9c 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -32,7 +32,6 @@ enum lookup_type {\n \n static struct submodule_cache cache;\n static int is_cache_init;\n-static unsigned long 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@@ -162,10 +161,27 @@ 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 \tstruct submodule *submodule;\n+\tstruct strbuf name_buf = STRBUF_INIT;\n \n \tsubmodule = cache_lookup_name(cache, gitmodules_sha1, name);\n \tif (submodule)\n@@ -173,10 +189,13 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,\n \n \tsubmodule = xmalloc(sizeof(*submodule));\n \n-\tsubmodule->name = xstrdup(name);\n+\tstrbuf_addstr(&name_buf, name);\n+\tsubmodule->name = strbuf_detach(&name_buf, NULL);\n+\n \tsubmodule->path = NULL;\n \tsubmodule->url = NULL;\n-\tsubmodule->update = NULL;\n+\tsubmodule->update = SM_UPDATE_UNSPECIFIED;\n+\tsubmodule->update_command = NULL;\n \tsubmodule->fetch_recurse = RECURSE_SUBMODULES_NONE;\n \tsubmodule->ignore = NULL;\n \n@@ -229,34 +248,22 @@ struct parse_config_parameter {\n \tint overwrite;\n };\n \n-static int parse_generic_submodule_config(const char *key,\n-\t\t\t\t\t const char *var,\n-\t\t\t\t\t const char *value,\n-\t\t\t\t\t struct parse_config_parameter *me)\n+static int parse_config(const char *var, const char *value, void *data)\n {\n-\tif (!strcmp(key, \"fetchjobs\")) {\n-\t\tif (!git_parse_ulong(value, ¶llel_jobs)) {\n-\t\t\twarning(_(\"Error parsing submodule.fetchJobs; Defaulting to 1.\"));\n-\t\t\tparallel_jobs = 1;\n-\t\t}\n-\t}\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 \n-\treturn 0;\n-}\n+\t/* this also ensures that we only parse submodule entries */\n+\tif (!name_and_item_from_var(var, &name, &item))\n+\t\treturn 0;\n \n-static int parse_specific_submodule_config(const char *subsection,\n-\t\t\t\t\t const char *key,\n-\t\t\t\t\t const char *var,\n-\t\t\t\t\t const char *value,\n-\t\t\t\t\t struct parse_config_parameter *me)\n-{\n-\tint ret = 0;\n-\tstruct submodule *submodule;\n \tsubmodule = lookup_or_create_by_name(me->cache,\n \t\t\t\t\t me->gitmodules_sha1,\n-\t\t\t\t\t subsection);\n+\t\t\t\t\t name.buf);\n \n-\tif (!strcmp(key, \"path\")) {\n+\tif (!strcmp(item.buf, \"path\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->path)\n@@ -269,7 +276,7 @@ static int parse_specific_submodule_config(const char *subsection,\n \t\t\tsubmodule->path = xstrdup(value);\n \t\t\tcache_put_path(me->cache, submodule);\n \t\t}\n-\t} else if (!strcmp(key, \"fetchrecursesubmodules\")) {\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@@ -280,7 +287,7 @@ static int parse_specific_submodule_config(const char *subsection,\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(key, \"ignore\")) {\n+\t} else if (!strcmp(item.buf, \"ignore\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->ignore)\n@@ -296,7 +303,7 @@ static int parse_specific_submodule_config(const char *subsection,\n \t\t\tfree((void *) submodule->ignore);\n \t\t\tsubmodule->ignore = xstrdup(value);\n \t\t}\n-\t} else if (!strcmp(key, \"url\")) {\n+\t} else if (!strcmp(item.buf, \"url\")) {\n \t\tif (!value) {\n \t\t\tret = config_error_nonbool(var);\n \t\t} else if (!me->overwrite && submodule->url) {\n@@ -306,39 +313,31 @@ static int parse_specific_submodule_config(const char *subsection,\n \t\t\tfree((void *) submodule->url);\n \t\t\tsubmodule->url = xstrdup(value);\n \t\t}\n-\t} else if (!strcmp(key, \"update\")) {\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)\n+\t\telse if (!me->overwrite &&\n+\t\t submodule->update != SM_UPDATE_UNSPECIFIED)\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\tsubmodule->update_command = NULL;\n+\t\t\tif (!strcmp(value, \"none\"))\n+\t\t\t\tsubmodule->update = SM_UPDATE_NONE;\n+\t\t\telse if (!strcmp(value, \"checkout\"))\n+\t\t\t\tsubmodule->update = SM_UPDATE_CHECKOUT;\n+\t\t\telse if (!strcmp(value, \"rebase\"))\n+\t\t\t\tsubmodule->update = SM_UPDATE_REBASE;\n+\t\t\telse if (!strcmp(value, \"merge\"))\n+\t\t\t\tsubmodule->update = SM_UPDATE_MERGE;\n+\t\t\telse if (!skip_prefix(value, \"!\", &submodule->update_command))\n+\t\t\t\tdie(_(\"invalid value for %s\"), var);\n \t\t}\n \t}\n \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-\tint subsection_len, ret;\n-\tconst char *subsection, *key;\n-\tchar *sub;\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)\n-\t\treturn parse_generic_submodule_config(key, var, value, me);\n+\tstrbuf_release(&name);\n+\tstrbuf_release(&item);\n \n-\tsub = xmemdupz(subsection, subsection_len);\n-\tret = parse_specific_submodule_config(sub, key,\n-\t\t\t\t\t var, value, me);\n-\tfree(sub);\n \treturn ret;\n }\n \n@@ -487,8 +486,3 @@ void submodule_free(void)\n \tcache_free(&cache);\n \tis_cache_init = 0;\n }\n-\n-unsigned long config_parallel_submodules(void)\n-{\n-\treturn parallel_jobs;\n-}\ndiff --git a/submodule-config.h b/submodule-config.h\nindex bab1e8d..e3bd56e 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -4,6 +4,15 @@\n #include \"hashmap.h\"\n #include \"strbuf.h\"\n \n+enum submodule_update_type {\n+\tSM_UPDATE_CHECKOUT,\n+\tSM_UPDATE_REBASE,\n+\tSM_UPDATE_MERGE,\n+\tSM_UPDATE_NONE,\n+\tSM_UPDATE_COMMAND,\n+\tSM_UPDATE_UNSPECIFIED\n+};\n+\n /*\n * Submodule entry containing the information about a certain submodule\n * in a certain revision.\n@@ -14,7 +23,8 @@ struct submodule {\n \tconst char *url;\n \tint fetch_recurse;\n \tconst char *ignore;\n-\tconst char *update;\n+\tenum submodule_update_type update;\n+\tconst char *update_command;\n \t/* the sha1 blob id of the responsible .gitmodules file */\n \tunsigned char gitmodules_sha1[20];\n };\n@@ -27,6 +37,4 @@ const struct submodule *submodule_from_path(const unsigned char *commit_sha1,\n \t\tconst char *path);\n void submodule_free(void);\n \n-unsigned long config_parallel_submodules(void);\n-\n #endif /* SUBMODULE_CONFIG_H */\ndiff --git a/submodule.c b/submodule.c\nindex eb7d54b..fd763f5 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -15,6 +15,7 @@\n #include \"thread-utils.h\"\n \n static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n+static int parallel_jobs = 1;\n static struct string_list changed_submodule_paths;\n static int initialized_fetch_ref_tips;\n static struct sha1_array ref_tips_before_fetch;\n@@ -169,7 +170,13 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n \n int submodule_config(const char *var, const char *value, void *cb)\n {\n-\tif (starts_with(var, \"submodule.\"))\n+\tif (!strcmp(var, \"submodule.fetchjobs\")) {\n+\t\tunsigned long val;\n+\t\tif (!git_parse_ulong(value, &val) || 0 > val || val >= INT_MAX)\n+\t\t\tdie(_(\"Error parsing submodule.fetchJobs %s\"), value);\n+\t\tparallel_jobs = val;\n+\t\treturn 0;\n+\t} else if (starts_with(var, \"submodule.\"))\n \t\treturn parse_submodule_config_option(var, value);\n \telse if (!strcmp(var, \"fetch.recursesubmodules\")) {\n \t\tconfig_fetch_recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);\n@@ -752,9 +759,7 @@ int fetch_populated_submodules(const struct argv_array *options,\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+\t\tmax_parallel_jobs = parallel_jobs;\n \n \tcalculate_changed_submodule_paths();\n \trun_processes_parallel(max_parallel_jobs,\n@@ -1102,3 +1107,8 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)\n \tstrbuf_release(&rel_path);\n \tfree((void *)real_work_tree);\n }\n+\n+int parallel_submodules(void)\n+{\n+\treturn parallel_jobs;\n+}\ndiff --git a/submodule.h b/submodule.h\nindex cbc0003..95babc5 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam\n \t\tstruct string_list *needs_pushing);\n int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);\n void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n+int parallel_submodules(void);\n \n #endif\n\n\nStefan Beller (6):\n submodule-config: keep update strategy around\n submodule-config: drop check against NULL\n fetching submodules: respect `submodule.fetchJobs` 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 | 6 +\n Documentation/git-clone.txt | 6 +-\n Documentation/git-submodule.txt | 7 +-\n builtin/clone.c | 19 +++-\n builtin/fetch.c | 2 +-\n builtin/submodule--helper.c | 238 ++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 54 ++++-----\n submodule-config.c | 28 ++++-\n submodule-config.h | 11 ++\n submodule.c | 17 ++-\n submodule.h | 1 +\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, 385 insertions(+), 49 deletions(-)\n\n-- \n2.7.0.rc0.35.gb0f7869.dirty\n"},{"id":"277828","messageId":"1455051274-15256-2-git-send-email-sbeller@google.com","threadId":"41378","inReplyTo":"1455051274-15256-1-git-send-email-sbeller@google.com","subject":"[PATCHv9 1/6] submodule-config: keep update strategy around","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-09T20:54:29Z","receivedAt":"2016-02-09T20:54:29Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Currently submodule..update is only handled by git-submodule.sh.\nC code will start to need to make use of that value as more of the\nfunctionality of git-submodule.sh moves into library code in C.\n\nAdd the update field to 'struct submodule' and populate it so it can\nbe read as sm->update or from sm->update_command.\n\nSigned-off-by: Stefan Beller \n---\n submodule-config.c | 22 ++++++++++++++++++++++\n submodule-config.h | 11 +++++++++++\n 2 files changed, 33 insertions(+)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex afe0ea8..a1af5de 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -194,6 +194,8 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,\n \n \tsubmodule->path = NULL;\n \tsubmodule->url = NULL;\n+\tsubmodule->update = SM_UPDATE_UNSPECIFIED;\n+\tsubmodule->update_command = NULL;\n \tsubmodule->fetch_recurse = RECURSE_SUBMODULES_NONE;\n \tsubmodule->ignore = NULL;\n \n@@ -311,6 +313,26 @@ 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 &&\n+\t\t submodule->update != SM_UPDATE_UNSPECIFIED)\n+\t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n+\t\t\t\t\t \"update\");\n+\t\telse {\n+\t\t\tsubmodule->update_command = NULL;\n+\t\t\tif (!strcmp(value, \"none\"))\n+\t\t\t\tsubmodule->update = SM_UPDATE_NONE;\n+\t\t\telse if (!strcmp(value, \"checkout\"))\n+\t\t\t\tsubmodule->update = SM_UPDATE_CHECKOUT;\n+\t\t\telse if (!strcmp(value, \"rebase\"))\n+\t\t\t\tsubmodule->update = SM_UPDATE_REBASE;\n+\t\t\telse if (!strcmp(value, \"merge\"))\n+\t\t\t\tsubmodule->update = SM_UPDATE_MERGE;\n+\t\t\telse if (!skip_prefix(value, \"!\", &submodule->update_command))\n+\t\t\t\tdie(_(\"invalid value for %s\"), var);\n+\t\t}\n \t}\n \n \tstrbuf_release(&name);\ndiff --git a/submodule-config.h b/submodule-config.h\nindex 9061e4e..e3bd56e 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -4,6 +4,15 @@\n #include \"hashmap.h\"\n #include \"strbuf.h\"\n \n+enum submodule_update_type {\n+\tSM_UPDATE_CHECKOUT,\n+\tSM_UPDATE_REBASE,\n+\tSM_UPDATE_MERGE,\n+\tSM_UPDATE_NONE,\n+\tSM_UPDATE_COMMAND,\n+\tSM_UPDATE_UNSPECIFIED\n+};\n+\n /*\n * Submodule entry containing the information about a certain submodule\n * in a certain revision.\n@@ -14,6 +23,8 @@ struct submodule {\n \tconst char *url;\n \tint fetch_recurse;\n \tconst char *ignore;\n+\tenum submodule_update_type update;\n+\tconst char *update_command;\n \t/* the sha1 blob id of the responsible .gitmodules file */\n \tunsigned char gitmodules_sha1[20];\n };\n-- \n2.7.0.rc0.35.gb0f7869.dirty\n"},{"id":"277822","messageId":"1455051274-15256-3-git-send-email-sbeller@google.com","threadId":"41378","inReplyTo":"1455051274-15256-1-git-send-email-sbeller@google.com","subject":"[PATCHv9 2/6] submodule-config: drop check against NULL","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-09T20:54:30Z","receivedAt":"2016-02-09T20:54:30Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Adhere to the common coding style of Git and not check explicitly\nfor NULL throughout the file. There are still other occurrences in the\ncode base but that is usually inside of conditions with side effects.\n\nSigned-off-by: Stefan Beller \n---\n submodule-config.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex a1af5de..d6c8d9c 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -266,7 +266,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \tif (!strcmp(item.buf, \"path\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n-\t\telse if (!me->overwrite && submodule->path != NULL)\n+\t\telse if (!me->overwrite && submodule->path)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"path\");\n \t\telse {\n@@ -290,7 +290,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t} else if (!strcmp(item.buf, \"ignore\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n-\t\telse if (!me->overwrite && submodule->ignore != NULL)\n+\t\telse if (!me->overwrite && submodule->ignore)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"ignore\");\n \t\telse if (strcmp(value, \"untracked\") &&\n@@ -306,7 +306,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t} else if (!strcmp(item.buf, \"url\")) {\n \t\tif (!value) {\n \t\t\tret = config_error_nonbool(var);\n-\t\t} else if (!me->overwrite && submodule->url != NULL) {\n+\t\t} else if (!me->overwrite && submodule->url) {\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"url\");\n \t\t} else {\n-- \n2.7.0.rc0.35.gb0f7869.dirty\n"},{"id":"277823","messageId":"1455051274-15256-4-git-send-email-sbeller@google.com","threadId":"41378","inReplyTo":"1455051274-15256-1-git-send-email-sbeller@google.com","subject":"[PATCHv9 3/6] fetching submodules: respect `submodule.fetchJobs` config option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-09T20:54:31Z","receivedAt":"2016-02-09T20:54:31Z","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 | 6 ++++++\n builtin/fetch.c | 2 +-\n submodule.c | 17 ++++++++++++++++-\n submodule.h | 1 +\n t/t5526-fetch-submodules.sh | 14 ++++++++++++++\n 5 files changed, 38 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 2d06b11..3b02732 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2646,6 +2646,12 @@ submodule..ignore::\n \t\"--ignore-submodules\" option. The 'git submodule' commands are not\n \taffected by this setting.\n \n+submodule.fetchJobs::\n+\tSpecifies how many submodules are fetched/cloned at the same time.\n+\tA positive integer allows up to that number of submodules fetched\n+\tin parallel. A value of 0 will give some reasonable default.\n+\tIf unset, it defaults to 1.\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 586840d..5aa1c2d 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.c b/submodule.c\nindex b83939c..fd763f5 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -15,6 +15,7 @@\n #include \"thread-utils.h\"\n \n static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n+static int parallel_jobs = 1;\n static struct string_list changed_submodule_paths;\n static int initialized_fetch_ref_tips;\n static struct sha1_array ref_tips_before_fetch;\n@@ -169,7 +170,13 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n \n int submodule_config(const char *var, const char *value, void *cb)\n {\n-\tif (starts_with(var, \"submodule.\"))\n+\tif (!strcmp(var, \"submodule.fetchjobs\")) {\n+\t\tunsigned long val;\n+\t\tif (!git_parse_ulong(value, &val) || 0 > val || val >= INT_MAX)\n+\t\t\tdie(_(\"Error parsing submodule.fetchJobs %s\"), value);\n+\t\tparallel_jobs = val;\n+\t\treturn 0;\n+\t} else if (starts_with(var, \"submodule.\"))\n \t\treturn parse_submodule_config_option(var, value);\n \telse if (!strcmp(var, \"fetch.recursesubmodules\")) {\n \t\tconfig_fetch_recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);\n@@ -751,6 +758,9 @@ 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 = parallel_jobs;\n+\n \tcalculate_changed_submodule_paths();\n \trun_processes_parallel(max_parallel_jobs,\n \t\t\t get_next_submodule,\n@@ -1097,3 +1107,8 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)\n \tstrbuf_release(&rel_path);\n \tfree((void *)real_work_tree);\n }\n+\n+int parallel_submodules(void)\n+{\n+\treturn parallel_jobs;\n+}\ndiff --git a/submodule.h b/submodule.h\nindex cbc0003..95babc5 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam\n \t\tstruct string_list *needs_pushing);\n int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);\n void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n+int parallel_submodules(void);\n \n #endif\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 1241146..954d0e4 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -471,4 +471,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 tasks\" trace.out &&\n+\t\tgit config submodule.fetchJobs 8 &&\n+\t\tGIT_TRACE=$(pwd)/trace.out git fetch &&\n+\t\tgrep \"8 tasks\" trace.out &&\n+\t\tGIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&\n+\t\tgrep \"9 tasks\" trace.out\n+\t)\n+'\n+\n test_done\n-- \n2.7.0.rc0.35.gb0f7869.dirty\n"},{"id":"277827","messageId":"1455051274-15256-5-git-send-email-sbeller@google.com","threadId":"41378","inReplyTo":"1455051274-15256-1-git-send-email-sbeller@google.com","subject":"[PATCHv9 4/6] git submodule update: have a dedicated helper for cloning","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-09T20:54:32Z","receivedAt":"2016-02-09T20:54:32Z","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 | 230 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 45 +++------\n t/t7400-submodule-basic.sh | 4 +-\n 3 files changed, 243 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff..53356dc 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -255,6 +255,235 @@ 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 submodule_config(var, value, cb);\n+}\n+\n+struct submodule_update_clone {\n+\t/* states */\n+\tint count;\n+\tint print_unmatched;\n+\t/* configuration */\n+\tint quiet;\n+\tconst char *reference;\n+\tconst char *depth;\n+\tconst char *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_pushl(&cp->args, \"--prefix\", prefix, NULL);\n+\n+\targv_array_pushl(&cp->args, \"--path\", path, NULL);\n+\targv_array_pushl(&cp->args, \"--name\", name, NULL);\n+\targv_array_pushl(&cp->args, \"--url\", url, NULL);\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(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+\tfor (; pp->count < pp->list.nr; pp->count++) {\n+\t\tconst struct submodule *sub = NULL;\n+\t\tconst struct cache_entry *ce = pp->list.entries[pp->count];\n+\t\tstruct strbuf displaypath_sb = STRBUF_INIT;\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tconst char *displaypath = NULL;\n+\t\tchar *url = NULL;\n+\t\tint needs_cloning = 0;\n+\n+\t\tif (ce_stage(ce)) {\n+\t\t\tif (pp->recursive_prefix)\n+\t\t\t\tstrbuf_addf(err,\n+\t\t\t\t\t\"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,\n+\t\t\t\t\t\"Skipping unmerged submodule %s\\n\",\n+\t\t\t\t\tce->name);\n+\t\t\tgoto cleanup_and_continue;\n+\t\t}\n+\n+\t\tsub = submodule_from_path(null_sha1, ce->name);\n+\n+\t\tif (pp->recursive_prefix)\n+\t\t\tdisplaypath = relative_path(pp->recursive_prefix,\n+\t\t\t\t\t\t ce->name, &displaypath_sb);\n+\t\telse\n+\t\t\tdisplaypath = ce->name;\n+\n+\t\tif ((pp->update && !strcmp(pp->update, \"none\")) ||\n+\t\t (!pp->update && sub->update == SM_UPDATE_NONE)) {\n+\t\t\tstrbuf_addf(err, \"Skipping submodule '%s'\\n\",\n+\t\t\t\t displaypath);\n+\t\t\tgoto cleanup_and_continue;\n+\t\t}\n+\n+\t\t/*\n+\t\t * Looking up the url in .git/config.\n+\t\t * We must not fall back to .gitmodules as we only want\n+\t\t * to process configured submodules.\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,\n+\t\t\t\t\t _(\"Submodule path '%s' not initialized\\n\"\n+\t\t\t\t\t \"Maybe you want to use 'update --init'?\"),\n+\t\t\t\t\t displaypath);\n+\t\t\tgoto cleanup_and_continue;\n+\t\t}\n+\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n+\t\tneeds_cloning = !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\tneeds_cloning, ce->name);\n+\t\tstring_list_append(&pp->projectlines, sb.buf);\n+\n+\t\tif (needs_cloning) {\n+\t\t\tfill_clone_command(cp, pp->quiet, pp->prefix,\n+\t\t\t\t\t ce->name, sub->name, strdup(url),\n+\t\t\t\t\t pp->reference, pp->depth);\n+\t\t\tpp->count++;\n+\t\t}\n+\n+cleanup_and_continue:\n+\t\tfree(url);\n+\t\tstrbuf_reset(&displaypath_sb);\n+\t\tstrbuf_reset(&sb);\n+\n+\t\tif (needs_cloning)\n+\t\t\treturn 1;\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+\n+\treturn 0;\n+}\n+\n struct cmd_struct {\n \tconst char *cmd;\n \tint (*fn)(int, const char **, const char *);\n@@ -264,6 +493,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.7.0.rc0.35.gb0f7869.dirty\n"},{"id":"277825","messageId":"1455051274-15256-6-git-send-email-sbeller@google.com","threadId":"41378","inReplyTo":"1455051274-15256-1-git-send-email-sbeller@google.com","subject":"[PATCHv9 5/6] submodule update: expose parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-09T20:54:33Z","receivedAt":"2016-02-09T20:54:33Z","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.fetchJobs\" 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 | 16 ++++++++++++----\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 1572f05..13adebf 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@@ -377,6 +377,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.fetchJobs` 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 53356dc..6c74474 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -427,6 +427,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@@ -447,6 +448,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@@ -468,10 +471,15 @@ 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 = parallel_submodules();\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..7fd5142 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 tasks\" trace.out &&\n+\t git config submodule.fetchJobs 8 &&\n+\t GIT_TRACE=$(pwd)/trace.out git submodule update &&\n+\t grep \"8 tasks\" trace.out &&\n+\t GIT_TRACE=$(pwd)/trace.out git submodule update --jobs 9 &&\n+\t grep \"9 tasks\" trace.out\n+\t)\n+'\n test_done\n-- \n2.7.0.rc0.35.gb0f7869.dirty\n"},{"id":"277826","messageId":"1455051274-15256-7-git-send-email-sbeller@google.com","threadId":"41378","inReplyTo":"1455051274-15256-1-git-send-email-sbeller@google.com","subject":"[PATCHv9 6/6] clone: allow an explicit argument for parallel submodule clones","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-09T20:54:34Z","receivedAt":"2016-02-09T20:54:34Z","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 | 19 +++++++++++++------\n t/t7406-submodule-update.sh | 15 +++++++++++++++\n 3 files changed, 33 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex 6bf000d..6db7b6d 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@@ -221,6 +221,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.fetchJobs` option.\n \n ::\n \tThe (possibly remote) repository to clone from. See the\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a0b3cd9..b004fb4 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,16 @@ 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\targv_array_pushf(&args, \"--jobs=%d\", max_jobs);\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 7fd5142..090891e 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 tasks\" 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 tasks\" trace.out &&\n+\trm -rf super4 &&\n+\tgit config --global submodule.fetchJobs 8 &&\n+\tGIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules . super4 &&\n+\tgrep \"8 tasks\" trace.out &&\n+\trm -rf super4 &&\n+\tGIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 9 . super4 &&\n+\tgrep \"9 tasks\" trace.out &&\n+\trm -rf super4\n+'\n+\n test_done\n-- \n2.7.0.rc0.35.gb0f7869.dirty\n"},{"id":"277830","messageId":"xmqqh9hh382i.fsf@gitster.mtv.corp.google.com","threadId":"41378","inReplyTo":"1455051274-15256-2-git-send-email-sbeller@google.com","subject":"Re: [PATCHv9 1/6] submodule-config: keep update strategy around","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-09T21:08:53Z","receivedAt":"2016-02-09T21:08:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Currently submodule..update is only handled by git-submodule.sh.\n> C code will start to need to make use of that value as more of the\n> functionality of git-submodule.sh moves into library code in C.\n>\n> Add the update field to 'struct submodule' and populate it so it can\n> be read as sm->update or from sm->update_command.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> submodule-config.c | 22 ++++++++++++++++++++++\n> submodule-config.h | 11 +++++++++++\n> 2 files changed, 33 insertions(+)\n>\n> diff --git a/submodule-config.c b/submodule-config.c\n> index afe0ea8..a1af5de 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -194,6 +194,8 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,\n> \n> \tsubmodule->path = NULL;\n> \tsubmodule->url = NULL;\n> +\tsubmodule->update = SM_UPDATE_UNSPECIFIED;\n> +\tsubmodule->update_command = NULL;\n> \tsubmodule->fetch_recurse = RECURSE_SUBMODULES_NONE;\n> \tsubmodule->ignore = NULL;\n> \n> @@ -311,6 +313,26 @@ 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 &&\n> +\t\t submodule->update != SM_UPDATE_UNSPECIFIED)\n\nFunny indentation here (locally fixable).\n\nThanks to this change, we do not need \"'!= NULL' removal\" for this\nlocation in 2/6.\n\n> +\t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n> +\t\t\t\t\t \"update\");\n> +\t\telse {\n> +\t\t\tsubmodule->update_command = NULL;\n> +\t\t\tif (!strcmp(value, \"none\"))\n> +\t\t\t\tsubmodule->update = SM_UPDATE_NONE;\n> +\t\t\telse if (!strcmp(value, \"checkout\"))\n> +\t\t\t\tsubmodule->update = SM_UPDATE_CHECKOUT;\n> +\t\t\telse if (!strcmp(value, \"rebase\"))\n> +\t\t\t\tsubmodule->update = SM_UPDATE_REBASE;\n> +\t\t\telse if (!strcmp(value, \"merge\"))\n> +\t\t\t\tsubmodule->update = SM_UPDATE_MERGE;\n> +\t\t\telse if (!skip_prefix(value, \"!\", &submodule->update_command))\n> +\t\t\t\tdie(_(\"invalid value for %s\"), var);\n> +\t\t}\n> \t}\n> \n> \tstrbuf_release(&name);\n> diff --git a/submodule-config.h b/submodule-config.h\n> index 9061e4e..e3bd56e 100644\n> --- a/submodule-config.h\n> +++ b/submodule-config.h\n> @@ -4,6 +4,15 @@\n> #include \"hashmap.h\"\n> #include \"strbuf.h\"\n> \n> +enum submodule_update_type {\n> +\tSM_UPDATE_CHECKOUT,\n> +\tSM_UPDATE_REBASE,\n> +\tSM_UPDATE_MERGE,\n> +\tSM_UPDATE_NONE,\n> +\tSM_UPDATE_COMMAND,\n> +\tSM_UPDATE_UNSPECIFIED\n> +};\n> +\n> /*\n> * Submodule entry containing the information about a certain submodule\n> * in a certain revision.\n> @@ -14,6 +23,8 @@ struct submodule {\n> \tconst char *url;\n> \tint fetch_recurse;\n> \tconst char *ignore;\n> +\tenum submodule_update_type update;\n> +\tconst char *update_command;\n> \t/* the sha1 blob id of the responsible .gitmodules file */\n> \tunsigned char gitmodules_sha1[20];\n> };\n"},{"id":"277831","messageId":"xmqqd1s537wy.fsf@gitster.mtv.corp.google.com","threadId":"41378","inReplyTo":"1455051274-15256-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv9 3/6] fetching submodules: respect `submodule.fetchJobs` config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-09T21:12:13Z","receivedAt":"2016-02-09T21:12:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\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\nThis got a lot simpler to read, I think, even though this comes with\na better error checking ;-)\n\nThanks.\n\n> Documentation/config.txt | 6 ++++++\n> builtin/fetch.c | 2 +-\n> submodule.c | 17 ++++++++++++++++-\n> submodule.h | 1 +\n> t/t5526-fetch-submodules.sh | 14 ++++++++++++++\n> 5 files changed, 38 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 2d06b11..3b02732 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2646,6 +2646,12 @@ submodule..ignore::\n> \t\"--ignore-submodules\" option. The 'git submodule' commands are not\n> \taffected by this setting.\n> \n> +submodule.fetchJobs::\n> +\tSpecifies how many submodules are fetched/cloned at the same time.\n> +\tA positive integer allows up to that number of submodules fetched\n> +\tin parallel. A value of 0 will give some reasonable default.\n> +\tIf unset, it defaults to 1.\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\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 586840d..5aa1c2d 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;\n> diff --git a/submodule.c b/submodule.c\n> index b83939c..fd763f5 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -15,6 +15,7 @@\n> #include \"thread-utils.h\"\n> \n> static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n> +static int parallel_jobs = 1;\n> static struct string_list changed_submodule_paths;\n> static int initialized_fetch_ref_tips;\n> static struct sha1_array ref_tips_before_fetch;\n> @@ -169,7 +170,13 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n> \n> int submodule_config(const char *var, const char *value, void *cb)\n> {\n> -\tif (starts_with(var, \"submodule.\"))\n> +\tif (!strcmp(var, \"submodule.fetchjobs\")) {\n> +\t\tunsigned long val;\n> +\t\tif (!git_parse_ulong(value, &val) || 0 > val || val >= INT_MAX)\n> +\t\t\tdie(_(\"Error parsing submodule.fetchJobs %s\"), value);\n> +\t\tparallel_jobs = val;\n> +\t\treturn 0;\n> +\t} else if (starts_with(var, \"submodule.\"))\n> \t\treturn parse_submodule_config_option(var, value);\n> \telse if (!strcmp(var, \"fetch.recursesubmodules\")) {\n> \t\tconfig_fetch_recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);\n> @@ -751,6 +758,9 @@ 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 = parallel_jobs;\n> +\n> \tcalculate_changed_submodule_paths();\n> \trun_processes_parallel(max_parallel_jobs,\n> \t\t\t get_next_submodule,\n> @@ -1097,3 +1107,8 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)\n> \tstrbuf_release(&rel_path);\n> \tfree((void *)real_work_tree);\n> }\n> +\n> +int parallel_submodules(void)\n> +{\n> +\treturn parallel_jobs;\n> +}\n> diff --git a/submodule.h b/submodule.h\n> index cbc0003..95babc5 100644\n> --- a/submodule.h\n> +++ b/submodule.h\n> @@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam\n> \t\tstruct string_list *needs_pushing);\n> int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);\n> void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n> +int parallel_submodules(void);\n> \n> #endif\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index 1241146..954d0e4 100755\n> --- a/t/t5526-fetch-submodules.sh\n> +++ b/t/t5526-fetch-submodules.sh\n> @@ -471,4 +471,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 tasks\" trace.out &&\n> +\t\tgit config submodule.fetchJobs 8 &&\n> +\t\tGIT_TRACE=$(pwd)/trace.out git fetch &&\n> +\t\tgrep \"8 tasks\" trace.out &&\n> +\t\tGIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&\n> +\t\tgrep \"9 tasks\" trace.out\n> +\t)\n> +'\n> +\n> test_done\n"},{"id":"277832","messageId":"xmqq8u2t37bt.fsf@gitster.mtv.corp.google.com","threadId":"41378","inReplyTo":"1455051274-15256-5-git-send-email-sbeller@google.com","subject":"Re: [PATCHv9 4/6] git submodule update: have a dedicated helper for cloning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-09T21:24:54Z","receivedAt":"2016-02-09T21:24:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> +\tfor (; pp->count < pp->list.nr; pp->count++) {\n> +\t\tconst struct submodule *sub = NULL;\n> +\t\tconst struct cache_entry *ce = pp->list.entries[pp->count];\n> +\t\tstruct strbuf displaypath_sb = STRBUF_INIT;\n> +\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\tconst char *displaypath = NULL;\n> +\t\tchar *url = NULL;\n> +\t\tint needs_cloning = 0;\n> +\n> +\t\tif (ce_stage(ce)) {\n> +\t\t\tif (pp->recursive_prefix)\n> +\t\t\t\tstrbuf_addf(err,\n> +\t\t\t\t\t\"Skipping unmerged submodule %s/%s\\n\",\n> +\t\t\t\t\tpp->recursive_prefix, ce->name);\n\nThe funny indentation of the string is a workaround for overly deep\nnesting, but is the overly deep nesting telling us that perhaps one\niteration of this loop can be an invocation of a helper function, I\nwonder?\n\n> +\t\t\telse\n> +\t\t\t\tstrbuf_addf(err,\n> +\t\t\t\t\t\"Skipping unmerged submodule %s\\n\",\n> +\t\t\t\t\tce->name);\n> +\t\t\tgoto cleanup_and_continue;\n> +\t\t}\n\n> +\n> +\t\tsub = submodule_from_path(null_sha1, ce->name);\n> +\n> +\t\tif (pp->recursive_prefix)\n> +\t\t\tdisplaypath = relative_path(pp->recursive_prefix,\n> +\t\t\t\t\t\t ce->name, &displaypath_sb);\n> +\t\telse\n> +\t\t\tdisplaypath = ce->name;\n> +\n> +\t\tif ((pp->update && !strcmp(pp->update, \"none\")) ||\n> +\t\t (!pp->update && sub->update == SM_UPDATE_NONE)) {\n\nThis looks a bit strange. I wonder pp->update should also become\nenum for the same reason why sub->update has become enum. That way,\nwe need to be worried about parsing these tokens in one place where\na textual string \"none\" is translated to SM_UPDATE_NONE. If we\nstarted allowing \"None\" in the sub->update parse_config() in\nsubmodule-config.c, we would want that new parsing rule propagated\nto pp->update, right?\n\n> +\t\t\tstrbuf_addf(err, \"Skipping submodule '%s'\\n\",\n> +\t\t\t\t displaypath);\n> +\t\t\tgoto cleanup_and_continue;\n> +\t\t}\n"},{"id":"277833","messageId":"xmqq4mdh36o8.fsf@gitster.mtv.corp.google.com","threadId":"41378","inReplyTo":"1455051274-15256-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv9 0/6] Expose submodule parallelism to the user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-09T21:39:03Z","receivedAt":"2016-02-09T21:39:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This replaces sb/submodule-parallel-update.\n> (which is based on sb/submodule-parallel-fetch)\n>\n> Thanks to Junio and Jonathan for review!\n>\n> * s/config_parallel_submodules/parallel_submodules/ as it is not in config any\n> more. Also ease up the default setting. \n>\n> * use an enum for submodule update strategy\n\nYour earlier sb/submodule-init will need to be rerolled on top, as\nit depends on ->update being a string, I think.\n\n> * This seems to clash with 00/20] refs backend.\n>> Applied this on top of a merge between the current 'master' and\n>> 'sb/submodule-parallel-update' topic to untangle the dependency;\n>> otherwise there is no way for this topic to make progress X-<.\n>\n> Anything I can do to help with easing the clash?\n\nPerhaps try to rebase the series on top of such a merge (with this\nupdated series) yourself and propose it as a basis for the next\nreroll for David? In short, working together with topic(s) that\ntouch the same area?\n\nThanks.\n"},{"id":"277834","messageId":"CAGZ79kYt9bw9MreiBhA_ZQNjS+1Xi71aNGwkjcfC1hwxkOoyYA@mail.gmail.com","threadId":"41378","inReplyTo":"xmqq4mdh36o8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv9 0/6] Expose submodule parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-09T21:46:59Z","receivedAt":"2016-02-09T21:46:59Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Feb 9, 2016 at 1:39 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> This replaces sb/submodule-parallel-update.\n>> (which is based on sb/submodule-parallel-fetch)\n>>\n>> Thanks to Junio and Jonathan for review!\n>>\n>> * s/config_parallel_submodules/parallel_submodules/ as it is not in config any\n>> more. Also ease up the default setting.\n>>\n>> * use an enum for submodule update strategy\n>\n> Your earlier sb/submodule-init will need to be rerolled on top, as\n> it depends on ->update being a string, I think.\n\nSure, I just want to prioritize this series as it is the bottleneck for all\nother following things (init as well as the groups thing), I think.\n\n>\n>> * This seems to clash with 00/20] refs backend.\n>>> Applied this on top of a merge between the current 'master' and\n>>> 'sb/submodule-parallel-update' topic to untangle the dependency;\n>>> otherwise there is no way for this topic to make progress X-<.\n>>\n>> Anything I can do to help with easing the clash?\n>\n> Perhaps try to rebase the series on top of such a merge (with this\n> updated series) yourself and propose it as a basis for the next\n> reroll for David? In short, working together with topic(s) that\n> touch the same area?\n\nOk, I'll see if I can find a better commit to base this series on.\n\n>\n> Thanks.\n"},{"id":"277835","messageId":"20160209214937.GG28749@google.com","threadId":"41378","inReplyTo":"1455051274-15256-2-git-send-email-sbeller@google.com","subject":"Re: [PATCHv9 1/6] submodule-config: keep update strategy around","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-02-09T21:49:37Z","receivedAt":"2016-02-09T21:49:37Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> Signed-off-by: Stefan Beller \n> ---\n> submodule-config.c | 22 ++++++++++++++++++++++\n> submodule-config.h | 11 +++++++++++\n> 2 files changed, 33 insertions(+)\n\nYay, this is much clearer. Thanks for indulging.\n\n[...]\n> +++ b/submodule-config.c\n[...]\n> @@ -311,6 +313,26 @@ static int parse_config(const char *var, const char *value, void *data)\n> +\t\t\telse if (!skip_prefix(value, \"!\", &submodule->update_command))\n> +\t\t\t\tdie(_(\"invalid value for %s\"), var);\n\nShould this say\n\t\t\telse if (skip_prefix(value, \"!\", &submodule->update_command))\n\t\t\t\tsubmodule->update = SM_UPDATE_COMMAND;\n\t\t\telse\n\t\t\t\tdie(...);\n\n?\n\n[...]\n> --- a/submodule-config.h\n> +++ b/submodule-config.h\n> @@ -4,6 +4,15 @@\n> #include \"hashmap.h\"\n> #include \"strbuf.h\"\n> \n> +enum submodule_update_type {\n> +\tSM_UPDATE_CHECKOUT,\n> +\tSM_UPDATE_REBASE,\n> +\tSM_UPDATE_MERGE,\n> +\tSM_UPDATE_NONE,\n> +\tSM_UPDATE_COMMAND,\n> +\tSM_UPDATE_UNSPECIFIED\n> +};\n\nIf _UNSPECIFIED is equal to 0, that would futureproof this better\nagainst zero-initialized submodule structs without ->update explicitly\nset.\n\nAfter those two changes and the whitespace change Junio suggested,\nReviewed-by: Jonathan Nieder \n"},{"id":"277836","messageId":"20160209215025.GH28749@google.com","threadId":"41378","inReplyTo":"1455051274-15256-3-git-send-email-sbeller@google.com","subject":"Re: [PATCHv9 2/6] submodule-config: drop check against NULL","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-02-09T21:50:25Z","receivedAt":"2016-02-09T21:50:25Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Signed-off-by: Stefan Beller \n> ---\n> submodule-config.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"277837","messageId":"xmqqziv91qzi.fsf@gitster.mtv.corp.google.com","threadId":"41378","inReplyTo":"CAGZ79kYt9bw9MreiBhA_ZQNjS+1Xi71aNGwkjcfC1hwxkOoyYA@mail.gmail.com","subject":"Re: [PATCHv9 0/6] Expose submodule parallelism to the user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-09T22:03:13Z","receivedAt":"2016-02-09T22:03:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>>> * This seems to clash with 00/20] refs backend.\n>>>> Applied this on top of a merge between the current 'master' and\n>>>> 'sb/submodule-parallel-update' topic to untangle the dependency;\n>>>> otherwise there is no way for this topic to make progress X-<.\n>>>\n>>> Anything I can do to help with easing the clash?\n>>\n>> Perhaps try to rebase the series on top of such a merge (with this\n>> updated series) yourself and propose it as a basis for the next\n>> reroll for David? In short, working together with topic(s) that\n>> touch the same area?\n>\n> Ok, I'll see if I can find a better commit to base this series on.\n\nThat is not what I meant. I meant rebasing the refs-backend series\non top of a merge between this one and 'master', just like the way I\nqueued the refs-backend series on top of a merge between the\nprevious round of this series and 'master'.\n\nThese two topics want to update the same piece of code, so another\npossibility is to rebase this series on top of a merge between\nrefs-backend and 'master', but the current iteration of refs-backend\nalready depends on the previous round of this topic. Rebasing this\non top of refs-backend would involve first adjusting parts of\nrefs-backend that touched the same code as the previous round of\nsubmodule-parallel-update touched so that refs-backend would work\ndirectly on top of 'master', and then including the necessary change\nto the refs-backend code while rebuilding submodule-parallel-update\non top of the result. So I do not think you would go in that\ndirection.\n"},{"id":"277838","messageId":"CAGZ79kYm79C7tLECeJCrS3JoCudUT-8geovoMpT8qoHpYBjUAg@mail.gmail.com","threadId":"41378","inReplyTo":"xmqqh9hh382i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv9 1/6] submodule-config: keep update strategy around","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-09T22:19:42Z","receivedAt":"2016-02-09T22:19:42Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Feb 9, 2016 at 1:08 PM, Junio C Hamano wrote:\n>> + } else if (!strcmp(item.buf, \"update\")) {\n>> + if (!value)\n>> + ret = config_error_nonbool(var);\n>> + else if (!me->overwrite &&\n>> + submodule->update != SM_UPDATE_UNSPECIFIED)\n>\n> Funny indentation here (locally fixable).\n\nI looked through the code base and reread our CodingGuidelines\nto find out what is considered correct. (I assumed we had a gnu-ish\ncoding style w.r.t. breaking overly long lines in conditions, which is\nhaving the next line be indented with 4 spaces.)\n\nSo I assume by funny you mean \"the next line doesn't start below the\nopening parenthesis\"?\n\nThat would seem to be consistent as it fits both the 4 space indentation\nwhich is always found below \"if (\", but we have more than 4 spaces in\nother places such as overly long return statements, i.e. refs.c, l 610\n(@origin/master)\n return starts_with(refname, \"refs/heads/\") ||\n starts_with(refname, \"refs/remotes/\") ||\n starts_with(refname, \"refs/notes/\") ||\n !strcmp(refname, \"HEAD\");\n\nwhich also doesn't seem to align perfectly to me.\n\nLooking for places, which have the pattern \"else if (...\" with line\nbreak, I found several different styles.\n\nbuiltin/mv.c (@origin/master) has two occurrences of\n\n else if (...\n <2 additional tabs> condition continues here\n\nand another of\n\n else if (...\n <1 tab + 1 SP> condition nearly aligned to statement below\n\nLooking at remote.c, I can find 1 tab, plus 3 spaces.\n...\nGiving up to come up with an idea for space based rule other than\n\"visually align it below the original statement\".\n\nPuzzled,\nStefan\n"},{"id":"277841","messageId":"xmqqk2md1pro.fsf@gitster.mtv.corp.google.com","threadId":"41378","inReplyTo":"CAGZ79kYm79C7tLECeJCrS3JoCudUT-8geovoMpT8qoHpYBjUAg@mail.gmail.com","subject":"Re: [PATCHv9 1/6] submodule-config: keep update strategy around","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-09T22:29:31Z","receivedAt":"2016-02-09T22:29:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Tue, Feb 9, 2016 at 1:08 PM, Junio C Hamano wrote:\n>>> + } else if (!strcmp(item.buf, \"update\")) {\n>>> + if (!value)\n>>> + ret = config_error_nonbool(var);\n>>> + else if (!me->overwrite &&\n>>> + submodule->update != SM_UPDATE_UNSPECIFIED)\n>>\n>> Funny indentation here (locally fixable).\n>\n> I looked through the code base and reread our CodingGuidelines\n> to find out what is considered correct. (I assumed we had a gnu-ish\n> coding style w.r.t. breaking overly long lines in conditions, which is\n> having the next line be indented with 4 spaces.)\n>\n> So I assume by funny you mean \"the next line doesn't start below the\n> opening parenthesis\"?\n\nI think we typically do one of the two:\n\n if (A &&\n B && C && ...)\n\nor\n\n if (A &&\n B && C && ...)\n\nThat is, the second line may align just inside the open paren on the\nfirst line, or even deeper, but never shallower.\n"},{"id":"277842","messageId":"xmqqfux11pm8.fsf@gitster.mtv.corp.google.com","threadId":"41378","inReplyTo":"1455051274-15256-2-git-send-email-sbeller@google.com","subject":"Re: [PATCHv9 1/6] submodule-config: keep update strategy around","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-09T22:32:47Z","receivedAt":"2016-02-09T22:32:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> +\t\telse {\n> +\t\t\tsubmodule->update_command = NULL;\n> +\t\t\tif (!strcmp(value, \"none\"))\n> +\t\t\t\tsubmodule->update = SM_UPDATE_NONE;\n> +\t\t\telse if (!strcmp(value, \"checkout\"))\n> +\t\t\t\tsubmodule->update = SM_UPDATE_CHECKOUT;\n> +\t\t\telse if (!strcmp(value, \"rebase\"))\n> +\t\t\t\tsubmodule->update = SM_UPDATE_REBASE;\n> +\t\t\telse if (!strcmp(value, \"merge\"))\n> +\t\t\t\tsubmodule->update = SM_UPDATE_MERGE;\n> +\t\t\telse if (!skip_prefix(value, \"!\", &submodule->update_command))\n> +\t\t\t\tdie(_(\"invalid value for %s\"), var);\n> +\t\t}\n\nI think this \"string to enum\" parser can become a separate helper\nfunction in this patch, so that later patch can use it to parse the\n\"--update\" option in the update_clone() function.\n\nThat would solve the slight inconsistency we have seen\n\n+\t\tif ((pp->update && !strcmp(pp->update, \"none\")) ||\n+\t\t (!pp->update && sub->update == SM_UPDATE_NONE)) {\n\nin that patch.\n"},{"id":"277843","messageId":"20160209223400.GI28749@google.com","threadId":"41378","inReplyTo":"1455051274-15256-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv9 3/6] fetching submodules: respect `submodule.fetchJobs` config option","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-02-09T22:34:00Z","receivedAt":"2016-02-09T22:34:00Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> +++ b/submodule.c\n[...]\n> @@ -169,7 +170,13 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n> \n> int submodule_config(const char *var, const char *value, void *cb)\n> {\n> -\tif (starts_with(var, \"submodule.\"))\n> +\tif (!strcmp(var, \"submodule.fetchjobs\")) {\n> +\t\tunsigned long val;\n> +\t\tif (!git_parse_ulong(value, &val) || 0 > val || val >= INT_MAX)\n> +\t\t\tdie(_(\"Error parsing submodule.fetchJobs %s\"), value);\n\n'val < 0' would be more idiomatic than '0 > val'. More importantly,\nval is an unsigned long. How could it be negative?\n\nIs it intended that val == INT_MAX is not permitted? I would have\nexpected something like the following to work:\n\n\t\tunsigned long val = git_config_ulong(var, value);\n\t\tif (val > INT_MAX) {\n\t\t\terrno = ERANGE;\n\t\t\tdie_bad_number(var, value);\n\t\t}\n\n(using die_bad_number from config.c). Or config.c could gain a\ngit_config_nonnegative_int helper:\n\n\tstatic int git_parse_nonnegative_int(const char *value, int *ret)\n\t{\n\t\tuintmax_t tmp;\n\t\tif (!git_parse_unsigned(value, &tmp, maximum_signed_value_of_type(int)))\n\t\t\treturn 0;\n\t\t*ret = tmp;\n\t\treturn 1;\n\t}\n\n\tint git_config_nonnegative_int(const char *name, const char *value)\n\t{\n\t\tint ret;\n\t\tif (!git_parse_nonnegative_int(value, &ret))\n\t\t\tdie_bad_number(name, value);\n\t\treturn ret;\n\t}\n\nallowing\n\n\t\tparallel_jobs = git_config_nonnegative_int(var, val);\n\t\treturn 0;\n\n[...]\n> @@ -751,6 +758,9 @@ 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 = parallel_jobs;\n\nMakes sense.\n\n[...]\n> @@ -1097,3 +1107,8 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)\n> \tstrbuf_release(&rel_path);\n> \tfree((void *)real_work_tree);\n> }\n> +\n> +int parallel_submodules(void)\n> +{\n> +\treturn parallel_jobs;\n> +}\n\nIs this helper used?\n\n[...]\n> +++ b/submodule.h\n> @@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam\n> \t\tstruct string_list *needs_pushing);\n> int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);\n> void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n> +int parallel_submodules(void);\n\noptional trick: one way to avoid merge conflicts is to add each function\nat some logical place in the file instead of at the end. Another nice\nside-effect is that it makes it easier to read through the header since\nfunctions appear in some appropriate order.\n\nE.g. this method could go near the config functions, I suppose.\n\n> --- a/t/t5526-fetch-submodules.sh\n> +++ b/t/t5526-fetch-submodules.sh\n> @@ -471,4 +471,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\nMakes sense. Same trick about inserting in some appropriate place in\nthe middle of the file applies here, too. In tests it also ends up\nbeing useful for finding when tests overlap or when there's a gap in\ncoverage.\n\nThe documentation says that '0' does something appropriate (DWIM or\nsomething? I didn't understand it). Perhaps a test for that behavior\nwould be useful, too.\n\nThanks,\nJonathan\n"},{"id":"277849","messageId":"CAGZ79kazD22FLv19CvmG+-dzGEP-uwr=fC_1VXB=sdzPBU_uww@mail.gmail.com","threadId":"41378","inReplyTo":"20160209223400.GI28749@google.com","subject":"Re: [PATCHv9 3/6] fetching submodules: respect `submodule.fetchJobs` config option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-10T00:11:14Z","receivedAt":"2016-02-10T00:11:14Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Feb 9, 2016 at 2:34 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> Stefan Beller wrote:\n>\n>> +++ b/submodule.c\n> [...]\n>> @@ -169,7 +170,13 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n>>\n>> int submodule_config(const char *var, const char *value, void *cb)\n>> {\n>> - if (starts_with(var, \"submodule.\"))\n>> + if (!strcmp(var, \"submodule.fetchjobs\")) {\n>> + unsigned long val;\n>> + if (!git_parse_ulong(value, &val) || 0 > val || val >= INT_MAX)\n>> + die(_(\"Error parsing submodule.fetchJobs %s\"), value);\n>\n> 'val < 0' would be more idiomatic than '0 > val'. More importantly,\n> val is an unsigned long. How could it be negative?\n>\n> Is it intended that val == INT_MAX is not permitted? I would have\n> expected something like the following to work:\n>\n> unsigned long val = git_config_ulong(var, value);\n> if (val > INT_MAX) {\n> errno = ERANGE;\n> die_bad_number(var, value);\n> }\n>\n> (using die_bad_number from config.c). Or config.c could gain a\n> git_config_nonnegative_int helper:\n>\n> static int git_parse_nonnegative_int(const char *value, int *ret)\n> {\n> uintmax_t tmp;\n> if (!git_parse_unsigned(value, &tmp, maximum_signed_value_of_type(int)))\n> return 0;\n> *ret = tmp;\n> return 1;\n> }\n>\n> int git_config_nonnegative_int(const char *name, const char *value)\n> {\n> int ret;\n> if (!git_parse_nonnegative_int(value, &ret))\n> die_bad_number(name, value);\n> return ret;\n> }\n>\n> allowing\n>\n> parallel_jobs = git_config_nonnegative_int(var, val);\n> return 0;\n\nAnd I thought we wanted to prevent inventing yet another helper?\nAlthough this looks very appealing, so let's do that instead.\n\n>\n> [...]\n>> @@ -751,6 +758,9 @@ 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 = parallel_jobs;\n>\n> Makes sense.\n>\n> [...]\n>> @@ -1097,3 +1107,8 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)\n>> strbuf_release(&rel_path);\n>> free((void *)real_work_tree);\n>> }\n>> +\n>> +int parallel_submodules(void)\n>> +{\n>> + return parallel_jobs;\n>> +}\n>\n> Is this helper used?\n\nNot yet, but I rather want to introduce it here as it is easier to review here\nthan in a later patch?\n\n>\n> [...]\n>> +++ b/submodule.h\n>> @@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam\n>> struct string_list *needs_pushing);\n>> int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);\n>> void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n>> +int parallel_submodules(void);\n>\n> optional trick: one way to avoid merge conflicts is to add each function\n> at some logical place in the file instead of at the end. Another nice\n> side-effect is that it makes it easier to read through the header since\n> functions appear in some appropriate order.\n>\n> E.g. this method could go near the config functions, I suppose.\n\nI was not sure how relevant this is to configuration as it seems a bit\nspecial w.r.t. submodules to me (it's not configured in .gitmodules, but a\nstandard configuration in .git/config after all).\n\n>\n>> --- a/t/t5526-fetch-submodules.sh\n>> +++ b/t/t5526-fetch-submodules.sh\n>> @@ -471,4 +471,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>\n> Makes sense. Same trick about inserting in some appropriate place in\n> the middle of the file applies here, too. In tests it also ends up\n> being useful for finding when tests overlap or when there's a gap in\n> coverage.\n>\n> The documentation says that '0' does something appropriate (DWIM or\n> something? I didn't understand it). Perhaps a test for that behavior\n> would be useful, too.\n\nThe 0 is currently using 'online_cpus()' by default as a 'something\nappropriate',\nwhich is not really appropriate though. The documentation is worded\nsuch that people should not rely on certain behaviors of '0'.\n\nMaybe instead we could just hardcode 0 to 2 for now and test for that\ninstead. My reasoning for '2':\n* provides the least amount of parallelism\n* when fetching/cloning you have phases when the server works or when\n the client works. In absence of any good metric, I estimate the work load\n to be split 50:50. If that is the case the actual optimal number is 2 with\n these two jobs being shifted such that each client and server are\n loaded all the time.\n\n\n>\n> Thanks,\n> Jonathan\n\nThanks,\nStefan\n"},{"id":"277850","messageId":"20160210003741.GJ28749@google.com","threadId":"41378","inReplyTo":"1455051274-15256-5-git-send-email-sbeller@google.com","subject":"Re: [PATCHv9 4/6] git submodule update: have a dedicated helper for cloning","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-02-10T00:37:41Z","receivedAt":"2016-02-10T00:37:41Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> This introduces a new helper function in git submodule--helper\n> which takes care of cloning all submodules, which we want to\n> parallelize eventually.\n\nNeat.\n\n[...]\n> As we can only access the stderr channel from within the parallel\n> processing engine, we need to reroute the error message for\n> specified but initialized submodules to stderr. As it is an error\n> message, this should have gone to stderr in the first place, so it\n> is a bug fix along the way.\n\nIn principle this could have happened in a separate preparatory patch\n(so that the move to C would have no functional effect) but I don't\nthink that benefit alone is worth the churn of going back and doing\nthat.\n\n[...]\n> +++ b/builtin/submodule--helper.c\n> @@ -255,6 +255,235 @@ 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 submodule_config(var, value, cb);\n> +}\n\nCan callers use submodule_config directly?\n\n\n> +struct submodule_update_clone {\n> +\t/* states */\n> +\tint count;\n> +\tint print_unmatched;\n\nI'd add a blank line here.\n\n> +\t/* configuration */\n> +\tint quiet;\n> +\tconst char *reference;\n> +\tconst char *depth;\n> +\tconst char *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\nWhat is this struct used for? Maybe this would be clearer if it\nappeared immediately before the function that used it.\n\nThis is the shared cb object passed to run_processes_parallel, right?\nSome name like parallel_clone_opts or parallel_clone_cb might work.\n\nWhat do the fields represent? E.g., what is count a count of, what\ndoes it mean when print_unmatched is true, etc?\n\nWould it make sense to put the options in a separate struct from the\nstate fields (so e.g. it could be const)? The options are easier to\nunderstand because they correspond to command-line options, while the\nstate fields are something different and internal.\n\n[...]\n> +static int update_clone_get_next_task(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> +\tfor (; pp->count < pp->list.nr; pp->count++) {\n\nCould some of this loop body be factored out into separate functions?\n(e.g. whether to skip a submodule, getting the display path, ...).\n\n[...]\n> +\t\t/*\n> +\t\t * Looking up the url in .git/config.\n> +\t\t * We must not fall back to .gitmodules as we only want\n> +\t\t * to process configured submodules.\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\nI can see how the submodule API would be overkill for this. But it's\nstill tempting to use it. 'struct submodule' could gain a field\nrepresenting whether the submodule is initialized (i.e., whether it\nappears in .git/config).\n\nI wonder whether the strbuf_reset + addf idiom would be a good thing\nto factor out into a mkpath()-like function --- i.e., something like\n\n\t\tgit_config_get_string(fmt(&sb, \"submodule.%s.url\", sub->name), &url);\n\nThat's a little less risky than mkpath was because it explicitly\nmentions &sb but maybe it's too magical.\n\n[...]\n> +static int update_clone_start_failure(struct child_process *cp,\n\nWill review the rest when I get home.\n\nThanks,\nJonathan\n"},{"id":"277851","messageId":"xmqqy4atz7u0.fsf@gitster.mtv.corp.google.com","threadId":"41378","inReplyTo":"CAGZ79kazD22FLv19CvmG+-dzGEP-uwr=fC_1VXB=sdzPBU_uww@mail.gmail.com","subject":"Re: [PATCHv9 3/6] fetching submodules: respect `submodule.fetchJobs` config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-10T01:12:55Z","receivedAt":"2016-02-10T01:12:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> int git_config_nonnegative_int(const char *name, const char *value)\n>> {\n>> int ret;\n>> if (!git_parse_nonnegative_int(value, &ret))\n>> die_bad_number(name, value);\n>> return ret;\n>> }\n>>\n>> allowing\n>>\n>> parallel_jobs = git_config_nonnegative_int(var, val);\n>> return 0;\n>\n> And I thought we wanted to prevent inventing yet another helper?\n\nI actually do not think we mind git_parse_int(), git_parse_long(),\nand git_parse_uint() to complement git_parse_ulong(). I am not\nenthused by the \"nonnegative-int\" thing, though.\n\nDo we have enough cases where we want to use signed type and reserve\nnegative value for our own internal use (e.g. \"unspecified yet\")?\nIf not, a very generic git_config_int() with a caller specific range\ncheck wouldn't look _so_ bad.\n\n\tparallel_jobs = git_config_int(var, val);\n if (parallel_jobs < 0)\n\t \tsome corrective action;\n\treturn 0;\n"},{"id":"277852","messageId":"xmqqtwlhz5fm.fsf@gitster.mtv.corp.google.com","threadId":"41378","inReplyTo":"xmqqy4atz7u0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv9 3/6] fetching submodules: respect `submodule.fetchJobs` config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-10T02:04:45Z","receivedAt":"2016-02-10T02:04:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I actually do not think we mind git_parse_int(), git_parse_long(),\n> and git_parse_uint() to complement git_parse_ulong(). I am not\n> enthused by the \"nonnegative-int\" thing, though.\n>\n> Do we have enough cases where we want to use signed type and reserve\n> negative value for our own internal use (e.g. \"unspecified yet\")?\n> If not, a very generic git_config_int() with a caller specific range\n> check wouldn't look _so_ bad.\n>\n> \tparallel_jobs = git_config_int(var, val);\n> if (parallel_jobs < 0)\n> \t \tsome corrective action;\n> \treturn 0;\n\nAnd of course, if we _do_ have many callsites where the caller wants\na sub-range of the range allowed by the underlying type, we can have\n\n \t git_config__bounded(var, val, lb, ub)\n\nwhere T are the usual integral types and lb and ub are (inclusive)\nlower and upper bounds. With that, Jonathan's non-negative-int\nwould fall out as a natural consequence:\n\n\tparallel_jobs = git_config_int_bounded(var, val, 0, INT_MAX);\n\nI vaguely recall that Michael Haggerty had a series to add and use\nhelpers to parse values of integral types and something like that\ninterface was suggested in the discussion; and it may even have used\na word better than \"bounded\".\n"},{"id":"277853","messageId":"CA+P7+xo=GK79DRvV6ZYds3wx6bayoWHC49UmkWL20=xo=jpS2g@mail.gmail.com","threadId":"41378","inReplyTo":"1455051274-15256-5-git-send-email-sbeller@google.com","subject":"Re: [PATCHv9 4/6] git submodule update: have a dedicated helper for cloning","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-02-10T02:26:47Z","receivedAt":"2016-02-10T02:26:47Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Feb 9, 2016 at 12:54 PM, Stefan Beller wrote:\n> This introduces a new helper function in git submodule--helper\n> which takes care of cloning all submodules, which we want to\n> parallelize eventually.\n>\n> Some tests (such as empty URL, update_mode=none) are required in the\n> helper to make the decision for cloning. These checks have been\n> moved into the C function as well (no need to repeat them in the\n> shell script).\n>\n> As we can only access the stderr channel from within the parallel\n> processing engine, we need to reroute the error message for\n> specified but initialized submodules to stderr. As it is an error\n> message, this should have gone to stderr in the first place, so it\n> is a bug fix along the way.\n>\n\nI was recently working on a way to make submodule pass some parameters\nfrom the parent project into the submodule project (specifically\nsettings regarding authentication such as using an authentication\nhelper), and I think this would make it easier to do.\n\nThanks,\nJake\n"},{"id":"277881","messageId":"CAGZ79kb70tXVsb0DCpwdU9QJB2PQh_4zxZ6Ab7CLQkxCHG6Xuw@mail.gmail.com","threadId":"41378","inReplyTo":"CA+P7+xo=GK79DRvV6ZYds3wx6bayoWHC49UmkWL20=xo=jpS2g@mail.gmail.com","subject":"Re: [PATCHv9 4/6] git submodule update: have a dedicated helper for cloning","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-10T17:49:36Z","receivedAt":"2016-02-10T17:49:36Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Feb 9, 2016 at 6:26 PM, Jacob Keller wrote:\n> On Tue, Feb 9, 2016 at 12:54 PM, Stefan Beller wrote:\n>> This introduces a new helper function in git submodule--helper\n>> which takes care of cloning all submodules, which we want to\n>> parallelize eventually.\n>>\n>> Some tests (such as empty URL, update_mode=none) are required in the\n>> helper to make the decision for cloning. These checks have been\n>> moved into the C function as well (no need to repeat them in the\n>> shell script).\n>>\n>> As we can only access the stderr channel from within the parallel\n>> processing engine, we need to reroute the error message for\n>> specified but initialized submodules to stderr. As it is an error\n>> message, this should have gone to stderr in the first place, so it\n>> is a bug fix along the way.\n>>\n>\n> I was recently working on a way to make submodule pass some parameters\n> from the parent project into the submodule project (specifically\n> settings regarding authentication such as using an authentication\n> helper), and I think this would make it easier to do.\n\nDo you mean the separate bug fix patch as proposed by Jonathan or\nthis patch in general helps your idea of passing settings?\n\nThanks,\nStefan\n\n>\n> Thanks,\n> Jake\n"},{"id":"277907","messageId":"CA+P7+xq5ckQ02OZ5O2vR8BT37EiThrNUdY4-WcRVJYr1sSDGbw@mail.gmail.com","threadId":"41378","inReplyTo":"CAGZ79kb70tXVsb0DCpwdU9QJB2PQh_4zxZ6Ab7CLQkxCHG6Xuw@mail.gmail.com","subject":"Re: [PATCHv9 4/6] git submodule update: have a dedicated helper for cloning","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-02-11T07:46:37Z","receivedAt":"2016-02-11T07:46:37Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Feb 10, 2016 at 9:49 AM, Stefan Beller wrote:\n> Do you mean the separate bug fix patch as proposed by Jonathan or\n> this patch in general helps your idea of passing settings?\n>\n> Thanks,\n> Stefan\n\nThis patch in general, I think.\n\nI didn't mean to reply to the whole thread.\n\nThanks,\nJake\n"},{"id":"277953","messageId":"xmqqpow3vwzb.fsf@gitster.mtv.corp.google.com","threadId":"41378","inReplyTo":"xmqqfux11pm8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv9 1/6] submodule-config: keep update strategy around","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-11T20:00:08Z","receivedAt":"2016-02-11T20:00:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Stefan Beller writes:\n>\n>> +\t\telse {\n>> +\t\t\tsubmodule->update_command = NULL;\n>> +\t\t\tif (!strcmp(value, \"none\"))\n>> +\t\t\t\tsubmodule->update = SM_UPDATE_NONE;\n>> +\t\t\telse if (!strcmp(value, \"checkout\"))\n>> +\t\t\t\tsubmodule->update = SM_UPDATE_CHECKOUT;\n>> +\t\t\telse if (!strcmp(value, \"rebase\"))\n>> +\t\t\t\tsubmodule->update = SM_UPDATE_REBASE;\n>> +\t\t\telse if (!strcmp(value, \"merge\"))\n>> +\t\t\t\tsubmodule->update = SM_UPDATE_MERGE;\n>> +\t\t\telse if (!skip_prefix(value, \"!\", &submodule->update_command))\n>> +\t\t\t\tdie(_(\"invalid value for %s\"), var);\n>> +\t\t}\n>\n> I think this \"string to enum\" parser can become a separate helper\n> function in this patch, so that later patch can use it to parse the\n> \"--update\" option in the update_clone() function.\n>\n> That would solve the slight inconsistency we have seen\n>\n> +\t\tif ((pp->update && !strcmp(pp->update, \"none\")) ||\n> +\t\t (!pp->update && sub->update == SM_UPDATE_NONE)) {\n>\n> in that patch.\n\nIn addition, you would eventually want the reverse translation,\ni.e. SM_UPDATE_ENUM to string, when you rebase the second patch of\nyour sb/submodule-init series (I just tried it myself and realized\nthat a patch to refactor the above part would be the best place to\nadd such a helper).\n"},{"id":"277955","messageId":"xmqqlh6rvvwb.fsf@gitster.mtv.corp.google.com","threadId":"41378","inReplyTo":"xmqqziv91qzi.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv9 0/6] Expose submodule parallelism to the user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-11T20:23:32Z","receivedAt":"2016-02-11T20:23:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Stefan Beller writes:\n>\n>>>> * This seems to clash with 00/20] refs backend.\n>>>>> Applied this on top of a merge between the current 'master' and\n>>>>> 'sb/submodule-parallel-update' topic to untangle the dependency;\n>>>>> otherwise there is no way for this topic to make progress X-<.\n>>>>\n>>>> Anything I can do to help with easing the clash?\n>>>\n>>> Perhaps try to rebase the series on top of such a merge (with this\n>>> updated series) yourself and propose it as a basis for the next\n>>> reroll for David? In short, working together with topic(s) that\n>>> touch the same area?\n>>\n>> Ok, I'll see if I can find a better commit to base this series on.\n>\n> That is not what I meant. I meant rebasing the refs-backend series\n> on top of a merge between this one and 'master', just like the way I\n> queued the refs-backend series on top of a merge between the\n> previous round of this series and 'master'.\n>\n> These two topics want to update the same piece of code, so another\n> possibility is to rebase this series on top of a merge between\n> refs-backend and 'master', but the current iteration of refs-backend\n> already depends on the previous round of this topic. Rebasing this\n> on top of refs-backend would involve first adjusting parts of\n> refs-backend that touched the same code as the previous round of\n> submodule-parallel-update touched so that refs-backend would work\n> directly on top of 'master', and then including the necessary change\n> to the refs-backend code while rebuilding submodule-parallel-update\n> on top of the result. So I do not think you would go in that\n> direction.\n\nHaving said that, at least for this round, I do not think there is\nnothing to do at this point on your end; I just created a merge\nbetween master and your updated sb/submodule-parallel-update and\nthen rebased the LMDB series on top of it. It at least applies\ncleanly and I expect it would test OK as well (the test is still\nrunning).\n\nOn your plate is to adjust the submodule-init topic so that it knows\nthat the .update field no longer is a string (but is now an enum).\n\nI did try doing that myself to see the extent of necessary changes\nbut did not finish it myself, because I suspect that\nsb/submodule-parallel-update may need further updates.\n\nThanks.\n"},{"id":"277956","messageId":"xmqqh9hfvvoc.fsf@gitster.mtv.corp.google.com","threadId":"41378","inReplyTo":"xmqqlh6rvvwb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv9 0/6] Expose submodule parallelism to the user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-11T20:28:19Z","receivedAt":"2016-02-11T20:28:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Having said that, at least for this round, I do not think there is\n> nothing to do at this point on your end;...\n\nI obviously meant \"I think there is nothing\" here. Sorry.\n"},{"id":"277957","messageId":"CAGZ79kZE51PwFMtZL2BaUtiD87QbGJ67gBnxE2rK7LWzJbnoFA@mail.gmail.com","threadId":"41378","inReplyTo":"xmqqlh6rvvwb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv9 0/6] Expose submodule parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-11T20:33:40Z","receivedAt":"2016-02-11T20:33:40Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 11, 2016 at 12:23 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Stefan Beller writes:\n>>\n>>>>> * This seems to clash with 00/20] refs backend.\n>>>>>> Applied this on top of a merge between the current 'master' and\n>>>>>> 'sb/submodule-parallel-update' topic to untangle the dependency;\n>>>>>> otherwise there is no way for this topic to make progress X-<.\n>>>>>\n>>>>> Anything I can do to help with easing the clash?\n>>>>\n>>>> Perhaps try to rebase the series on top of such a merge (with this\n>>>> updated series) yourself and propose it as a basis for the next\n>>>> reroll for David? In short, working together with topic(s) that\n>>>> touch the same area?\n>>>\n>>> Ok, I'll see if I can find a better commit to base this series on.\n>>\n>> That is not what I meant. I meant rebasing the refs-backend series\n>> on top of a merge between this one and 'master', just like the way I\n>> queued the refs-backend series on top of a merge between the\n>> previous round of this series and 'master'.\n>>\n>> These two topics want to update the same piece of code, so another\n>> possibility is to rebase this series on top of a merge between\n>> refs-backend and 'master', but the current iteration of refs-backend\n>> already depends on the previous round of this topic. Rebasing this\n>> on top of refs-backend would involve first adjusting parts of\n>> refs-backend that touched the same code as the previous round of\n>> submodule-parallel-update touched so that refs-backend would work\n>> directly on top of 'master', and then including the necessary change\n>> to the refs-backend code while rebuilding submodule-parallel-update\n>> on top of the result. So I do not think you would go in that\n>> direction.\n>\n> Having said that, at least for this round, I do not think there is\n> nothing to do at this point on your end; I just created a merge\n> between master and your updated sb/submodule-parallel-update and\n> then rebased the LMDB series on top of it. It at least applies\n> cleanly and I expect it would test OK as well (the test is still\n> running).\n\nI was about to send another round of this series with all the discussion\naddressed and then take a look how to resolve any conflicts if any.\n\nThis sounds promising.\n\n>\n> On your plate is to adjust the submodule-init topic so that it knows\n> that the .update field no longer is a string (but is now an enum).\n\nAfter the reroll of this series.\n\n>\n> I did try doing that myself to see the extent of necessary changes\n> but did not finish it myself, because I suspect that\n> sb/submodule-parallel-update may need further updates.\n\nI would hope to address that all within the next round, as the review\ndiscussion seemed to have died down and I'll be fixing all the issues\npointed at.\n\nThanks,\nStefan\n\n>\n> Thanks.\n"}]}