{"thread":{"id":"41439","subject":"[PATCHv12 2/7] submodule-config: drop check against NULL","startedAt":"2016-02-18T22:19:17Z","lastAt":"2016-02-19T03:14:42Z","messageCount":14,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"278580","messageId":"1455833964-3629-1-git-send-email-sbeller@google.com","threadId":"41439","inReplyTo":null,"subject":"[PATCHv12 0/7] Expose submodule parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-18T22:19:17Z","receivedAt":"2016-02-18T22:19:17Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Thanks Junio for a review of v11!\n\nI addressed the memory issue with the interdiff (in patch 1/7) as follows:\nInterdiff to v11:\n\ndiff --git a/submodule.c b/submodule.c\nindex 263cb2a..45d0967 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -219,6 +219,9 @@ void gitmodules_config(void)\n int parse_submodule_update_strategy(const char *value,\n struct submodule_update_strategy *dst)\n {\n+ const char *com;\n+\n+ free((void*)dst->command);\n dst->command = NULL;\n if (!strcmp(value, \"none\"))\n dst->type = SM_UPDATE_NONE;\n@@ -228,9 +231,10 @@ int parse_submodule_update_strategy(const char *value,\n dst->type = SM_UPDATE_REBASE;\n else if (!strcmp(value, \"merge\"))\n dst->type = SM_UPDATE_MERGE;\n- else if (skip_prefix(value, \"!\", &dst->command))\n+ else if (skip_prefix(value, \"!\", &com)) {\n dst->type = SM_UPDATE_COMMAND;\n- else\n+ dst->command = xstrdup(com);\n+ } else\n return -1;\n return 0;\n }\n\nStefan Beller (7):\n submodule-config: keep update strategy around\n submodule-config: drop check against NULL\n fetching submodules: respect `submodule.fetchJobs` config option\n submodule update: direct error message to stderr\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 | 239 ++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 54 ++++-----\n submodule-config.c | 18 ++-\n submodule-config.h | 2 +\n submodule.c | 39 ++++++-\n submodule.h | 18 +++\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, 406 insertions(+), 49 deletions(-)\n\n-- \n2.7.0.rc0.34.g65aed89\n"},{"id":"278579","messageId":"1455833964-3629-2-git-send-email-sbeller@google.com","threadId":"41439","inReplyTo":"1455833964-3629-1-git-send-email-sbeller@google.com","subject":"[PATCHv12 1/7] submodule-config: keep update strategy around","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-18T22:19:18Z","receivedAt":"2016-02-18T22:19:18Z","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 \nSigned-off-by: Junio C Hamano \n---\n submodule-config.c | 12 ++++++++++++\n submodule-config.h | 2 ++\n submodule.c | 23 +++++++++++++++++++++++\n submodule.h | 16 ++++++++++++++++\n 4 files changed, 53 insertions(+)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex afe0ea8..f8d1be9 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_strategy.type = SM_UPDATE_UNSPECIFIED;\n+\tsubmodule->update_strategy.command = NULL;\n \tsubmodule->fetch_recurse = RECURSE_SUBMODULES_NONE;\n \tsubmodule->ignore = NULL;\n \n@@ -311,6 +313,16 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tfree((void *) submodule->url);\n \t\t\tsubmodule->url = xstrdup(value);\n \t\t}\n+\t} else if (!strcmp(item.buf, \"update\")) {\n+\t\tif (!value)\n+\t\t\tret = config_error_nonbool(var);\n+\t\telse if (!me->overwrite &&\n+\t\t\t submodule->update_strategy.type != SM_UPDATE_UNSPECIFIED)\n+\t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n+\t\t\t\t\t \"update\");\n+\t\telse if (parse_submodule_update_strategy(value,\n+\t\t\t &submodule->update_strategy) < 0)\n+\t\t\t\tdie(_(\"invalid value for %s\"), var);\n \t}\n \n \tstrbuf_release(&name);\ndiff --git a/submodule-config.h b/submodule-config.h\nindex 9061e4e..092ebfc 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -2,6 +2,7 @@\n #define SUBMODULE_CONFIG_CACHE_H\n \n #include \"hashmap.h\"\n+#include \"submodule.h\"\n #include \"strbuf.h\"\n \n /*\n@@ -14,6 +15,7 @@ struct submodule {\n \tconst char *url;\n \tint fetch_recurse;\n \tconst char *ignore;\n+\tstruct submodule_update_strategy update_strategy;\n \t/* the sha1 blob id of the responsible .gitmodules file */\n \tunsigned char gitmodules_sha1[20];\n };\ndiff --git a/submodule.c b/submodule.c\nindex b83939c..1de465f 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -210,6 +210,29 @@ void gitmodules_config(void)\n \t}\n }\n \n+int parse_submodule_update_strategy(const char *value,\n+\t\tstruct submodule_update_strategy *dst)\n+{\n+\tconst char *com;\n+\n+\tfree((void*)dst->command);\n+\tdst->command = NULL;\n+\tif (!strcmp(value, \"none\"))\n+\t\tdst->type = SM_UPDATE_NONE;\n+\telse if (!strcmp(value, \"checkout\"))\n+\t\tdst->type = SM_UPDATE_CHECKOUT;\n+\telse if (!strcmp(value, \"rebase\"))\n+\t\tdst->type = SM_UPDATE_REBASE;\n+\telse if (!strcmp(value, \"merge\"))\n+\t\tdst->type = SM_UPDATE_MERGE;\n+\telse if (skip_prefix(value, \"!\", &com)) {\n+\t\tdst->type = SM_UPDATE_COMMAND;\n+\t\tdst->command = xstrdup(com);\n+\t} else\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n void handle_ignore_submodules_arg(struct diff_options *diffopt,\n \t\t\t\t const char *arg)\n {\ndiff --git a/submodule.h b/submodule.h\nindex cbc0003..3464500 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -13,6 +13,20 @@ enum {\n \tRECURSE_SUBMODULES_ON = 2\n };\n \n+enum submodule_update_type {\n+\tSM_UPDATE_UNSPECIFIED = 0,\n+\tSM_UPDATE_CHECKOUT,\n+\tSM_UPDATE_REBASE,\n+\tSM_UPDATE_MERGE,\n+\tSM_UPDATE_NONE,\n+\tSM_UPDATE_COMMAND\n+};\n+\n+struct submodule_update_strategy {\n+\tenum submodule_update_type type;\n+\tconst char *command;\n+};\n+\n int is_staging_gitmodules_ok(void);\n int update_path_in_gitmodules(const char *oldpath, const char *newpath);\n int remove_path_from_gitmodules(const char *path);\n@@ -21,6 +35,8 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n \t\tconst char *path);\n int submodule_config(const char *var, const char *value, void *cb);\n void gitmodules_config(void);\n+int parse_submodule_update_strategy(const char *value,\n+\t\tstruct submodule_update_strategy *dst);\n void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);\n void show_submodule_summary(FILE *f, const char *path,\n \t\tconst char *line_prefix,\n-- \n2.7.0.rc0.34.g65aed89\n"},{"id":"278578","messageId":"1455833964-3629-3-git-send-email-sbeller@google.com","threadId":"41439","inReplyTo":"1455833964-3629-1-git-send-email-sbeller@google.com","subject":"[PATCHv12 2/7] submodule-config: drop check against NULL","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-18T22:19:19Z","receivedAt":"2016-02-18T22:19:19Z","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 \nSigned-off-by: Junio C Hamano \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 f8d1be9..02bcaa7 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.34.g65aed89\n"},{"id":"278583","messageId":"1455833964-3629-4-git-send-email-sbeller@google.com","threadId":"41439","inReplyTo":"1455833964-3629-1-git-send-email-sbeller@google.com","subject":"[PATCHv12 3/7] fetching submodules: respect `submodule.fetchJobs` config option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-18T22:19:20Z","receivedAt":"2016-02-18T22:19:20Z","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 \nSigned-off-by: Junio C Hamano \n---\n Documentation/config.txt | 6 ++++++\n builtin/fetch.c | 2 +-\n submodule.c | 16 +++++++++++++++-\n submodule.h | 2 ++\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 1de465f..45d0967 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,12 @@ 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\tparallel_jobs = git_config_int(var, value);\n+\t\tif (parallel_jobs < 0)\n+\t\t\tdie(_(\"negative values not allowed for submodule.fetchJobs\"));\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@@ -774,6 +780,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@@ -1120,3 +1129,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 3464500..3166608 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -26,6 +26,7 @@ struct submodule_update_strategy {\n \tenum submodule_update_type type;\n \tconst char *command;\n };\n+#define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}\n \n int is_staging_gitmodules_ok(void);\n int update_path_in_gitmodules(const char *oldpath, const char *newpath);\n@@ -57,5 +58,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.34.g65aed89\n"},{"id":"278584","messageId":"1455833964-3629-5-git-send-email-sbeller@google.com","threadId":"41439","inReplyTo":"1455833964-3629-1-git-send-email-sbeller@google.com","subject":"[PATCHv12 4/7] submodule update: direct error message to stderr","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-18T22:19:21Z","receivedAt":"2016-02-18T22:19:21Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Reroute the error message for specified but initialized submodules\nto stderr instead of stdout.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n git-submodule.sh | 4 ++--\n t/t7400-submodule-basic.sh | 4 ++--\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 9bc5c5f..9ee86d4 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -693,7 +693,7 @@ cmd_update()\n \n \t\tif test \"$update_module\" = \"none\"\n \t\tthen\n-\t\t\techo \"Skipping submodule '$displaypath'\"\n+\t\t\techo >&2 \"Skipping submodule '$displaypath'\"\n \t\t\tcontinue\n \t\tfi\n \n@@ -702,7 +702,7 @@ cmd_update()\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+\t\t\tsay >&2 \"$(eval_gettext \"Submodule path '\\$displaypath' not initialized\n Maybe you want to use 'update --init'?\")\"\n \t\t\tcontinue\n \t\tfi\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.34.g65aed89\n"},{"id":"278581","messageId":"1455833964-3629-6-git-send-email-sbeller@google.com","threadId":"41439","inReplyTo":"1455833964-3629-1-git-send-email-sbeller@google.com","subject":"[PATCHv12 5/7] git submodule update: have a dedicated helper for cloning","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-18T22:19:22Z","receivedAt":"2016-02-18T22:19:22Z","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\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n builtin/submodule--helper.c | 231 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 45 +++------\n 2 files changed, 242 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff..7629a41 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -255,6 +255,236 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\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 *recursive_prefix;\n+\tconst char *prefix;\n+\tstruct module_list list;\n+\tstruct string_list projectlines;\n+\tstruct submodule_update_strategy update;\n+\tstruct pathspec pathspec;\n+};\n+#define SUBMODULE_UPDATE_CLONE_INIT {0, 0, 0, NULL, NULL, NULL, NULL, MODULE_LIST_INIT, STRING_LIST_INIT_DUP, SUBMODULE_UPDATE_STRATEGY_INIT}\n+\n+static int update_clone_inspect_next_task(struct child_process *cp,\n+\t\t\t\t\t struct strbuf *err,\n+\t\t\t\t\t struct submodule_update_clone *pp,\n+\t\t\t\t\t void **pp_task_cb,\n+\t\t\t\t\t const struct cache_entry *ce)\n+{\n+\tconst struct submodule *sub = NULL;\n+\tstruct strbuf displaypath_sb = STRBUF_INIT;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tconst char *displaypath = NULL;\n+\tchar *url = NULL;\n+\tint needs_cloning = 0;\n+\n+\tif (ce_stage(ce)) {\n+\t\tif (pp->recursive_prefix)\n+\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s/%s\\n\",\n+\t\t\t\t pp->recursive_prefix, ce->name);\n+\t\telse\n+\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s\\n\",\n+\t\t\t\t ce->name);\n+\t\tgoto cleanup;\n+\t}\n+\n+\tsub = submodule_from_path(null_sha1, ce->name);\n+\n+\tif (pp->recursive_prefix)\n+\t\tdisplaypath = relative_path(pp->recursive_prefix,\n+\t\t\t\t\t ce->name, &displaypath_sb);\n+\telse\n+\t\tdisplaypath = ce->name;\n+\n+\tif (pp->update.type == SM_UPDATE_NONE ||\n+\t (pp->update.type == SM_UPDATE_UNSPECIFIED &&\n+\t sub->update_strategy.type == SM_UPDATE_NONE)) {\n+\t\tstrbuf_addf(err, \"Skipping submodule '%s'\\n\",\n+\t\t\t displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\t/*\n+\t * Looking up the url in .git/config.\n+\t * We must not fall back to .gitmodules as we only want\n+\t * to process configured submodules.\n+\t */\n+\tstrbuf_reset(&sb);\n+\tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n+\tgit_config_get_string(sb.buf, &url);\n+\tif (!url) {\n+\t\t/*\n+\t\t * Only mention uninitialized submodules when its\n+\t\t * path have been specified\n+\t\t */\n+\t\tif (pp->pathspec.nr)\n+\t\t\tstrbuf_addf(err, _(\"Submodule path '%s' not initialized\\n\"\n+\t\t\t\t \"Maybe you want to use 'update --init'?\"),\n+\t\t\t\t displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\tstrbuf_reset(&sb);\n+\tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n+\tneeds_cloning = !file_exists(sb.buf);\n+\n+\tstrbuf_reset(&sb);\n+\tstrbuf_addf(&sb, \"%06o %s %d %d\\t%s\\n\", ce->ce_mode,\n+\t\t\tsha1_to_hex(ce->sha1), ce_stage(ce),\n+\t\t\tneeds_cloning, ce->name);\n+\tstring_list_append(&pp->projectlines, sb.buf);\n+\n+\tif (needs_cloning) {\n+\t\tcp->git_cmd = 1;\n+\t\tcp->no_stdin = 1;\n+\t\tcp->stdout_to_stderr = 1;\n+\t\tcp->err = -1;\n+\t\targv_array_push(&cp->args, \"submodule--helper\");\n+\t\targv_array_push(&cp->args, \"clone\");\n+\t\tif (pp->quiet)\n+\t\t\targv_array_push(&cp->args, \"--quiet\");\n+\n+\t\tif (pp->prefix)\n+\t\t\targv_array_pushl(&cp->args, \"--prefix\", pp->prefix, NULL);\n+\n+\t\targv_array_pushl(&cp->args, \"--path\", sub->path, NULL);\n+\t\targv_array_pushl(&cp->args, \"--name\", sub->name, NULL);\n+\t\targv_array_pushl(&cp->args, \"--url\", strdup(url), NULL);\n+\t\tif (pp->reference)\n+\t\t\targv_array_push(&cp->args, pp->reference);\n+\t\tif (pp->depth)\n+\t\t\targv_array_push(&cp->args, pp->depth);\n+\n+\n+\t}\n+\n+cleanup:\n+\tfree(url);\n+\tstrbuf_reset(&displaypath_sb);\n+\tstrbuf_reset(&sb);\n+\n+\treturn needs_cloning;\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 cache_entry *ce = pp->list.entries[pp->count];\n+\t\tif (update_clone_inspect_next_task(cp, err, pp,\n+\t\t\t\t\t\t pp_task_cb, ce)) {\n+\t\t\tpp->count++;\n+\t\t\treturn 1;\n+\t\t}\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+\tconst char *update = NULL;\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\", &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 (update)\n+\t\tif (parse_submodule_update_strategy(update, &pp.update) < 0)\n+\t\t\tdie(_(\"bad value for update parameter\"));\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(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 +494,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 9ee86d4..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 >&2 \"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 >&2 \"$(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)\n-- \n2.7.0.rc0.34.g65aed89\n"},{"id":"278582","messageId":"1455833964-3629-7-git-send-email-sbeller@google.com","threadId":"41439","inReplyTo":"1455833964-3629-1-git-send-email-sbeller@google.com","subject":"[PATCHv12 6/7] submodule update: expose parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-18T22:19:23Z","receivedAt":"2016-02-18T22:19:23Z","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 \nSigned-off-by: Junio C Hamano \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 7629a41..65bdc14 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -424,6 +424,7 @@ static int update_clone_task_finished(int result,\n static int update_clone(int argc, const char **argv, const char *prefix)\n {\n \tconst char *update = NULL;\n+\tint max_jobs = -1;\n \tstruct string_list_item *item;\n \tstruct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;\n \n@@ -444,6 +445,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@@ -469,10 +472,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(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.34.g65aed89\n"},{"id":"278585","messageId":"1455833964-3629-8-git-send-email-sbeller@google.com","threadId":"41439","inReplyTo":"1455833964-3629-1-git-send-email-sbeller@google.com","subject":"[PATCHv12 7/7] clone: allow an explicit argument for parallel submodule clones","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-18T22:19:24Z","receivedAt":"2016-02-18T22:19:24Z","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 \nSigned-off-by: Junio C Hamano \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.34.g65aed89\n"},{"id":"278589","messageId":"xmqqr3g9eii9.fsf@gitster.mtv.corp.google.com","threadId":"41439","inReplyTo":"1455833964-3629-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv12 0/7] Expose submodule parallelism to the user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-18T22:55:10Z","receivedAt":"2016-02-18T22:55:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Thanks Junio for a review of v11!\n>\n> I addressed the memory issue with the interdiff (in patch 1/7) as follows:\n> Interdiff to v11:\n>\n> diff --git a/submodule.c b/submodule.c\n> index 263cb2a..45d0967 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -219,6 +219,9 @@ void gitmodules_config(void)\n> int parse_submodule_update_strategy(const char *value,\n> struct submodule_update_strategy *dst)\n> {\n> + const char *com;\n> +\n> + free((void*)dst->command);\n> dst->command = NULL;\n> if (!strcmp(value, \"none\"))\n> dst->type = SM_UPDATE_NONE;\n> @@ -228,9 +231,10 @@ int parse_submodule_update_strategy(const char *value,\n> dst->type = SM_UPDATE_REBASE;\n> else if (!strcmp(value, \"merge\"))\n> dst->type = SM_UPDATE_MERGE;\n> - else if (skip_prefix(value, \"!\", &dst->command))\n> + else if (skip_prefix(value, \"!\", &com)) {\n> dst->type = SM_UPDATE_COMMAND;\n> - else\n> + dst->command = xstrdup(com);\n> + } else\n> return -1;\n> return 0;\n> }\n\nUnless you count \"I want to write differently from what was\nsuggested\" is a desirable thing to do, I do not see a point in\nfavouring the above that uses an extra variable and skip_prefix()\nover what I gave you as \"how about\" patch. But whatever.\n\n - Is dst->command always initialized to a NULL (otherwise the\n unconditional upfront free() makes it unsafe)?\n\n - Is there a global free_something() or something_clear() function\n that are used to release the resource held by a structure that\n has submodule_update_strategy structure embedded in it? If so\n dst->command needs to be freed there as well.\n\nThanks.\n\n> Stefan Beller (7):\n> submodule-config: keep update strategy around\n> submodule-config: drop check against NULL\n> fetching submodules: respect `submodule.fetchJobs` config option\n> submodule update: direct error message to stderr\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 | 239 ++++++++++++++++++++++++++++++++++++++++\n> git-submodule.sh | 54 ++++-----\n> submodule-config.c | 18 ++-\n> submodule-config.h | 2 +\n> submodule.c | 39 ++++++-\n> submodule.h | 18 +++\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, 406 insertions(+), 49 deletions(-)\n"},{"id":"278591","messageId":"CAGZ79kZZRENjVhfrhSfPmSNNQ_qrwpzWk=pocQ4GY6kBos1r6Q@mail.gmail.com","threadId":"41439","inReplyTo":"xmqqr3g9eii9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv12 0/7] Expose submodule parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-18T23:12:21Z","receivedAt":"2016-02-18T23:12:21Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 18, 2016 at 2:55 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> Thanks Junio for a review of v11!\n>>\n>> I addressed the memory issue with the interdiff (in patch 1/7) as follows:\n>> Interdiff to v11:\n>>\n>> diff --git a/submodule.c b/submodule.c\n>> index 263cb2a..45d0967 100644\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -219,6 +219,9 @@ void gitmodules_config(void)\n>> int parse_submodule_update_strategy(const char *value,\n>> struct submodule_update_strategy *dst)\n>> {\n>> + const char *com;\n>> +\n>> + free((void*)dst->command);\n>> dst->command = NULL;\n>> if (!strcmp(value, \"none\"))\n>> dst->type = SM_UPDATE_NONE;\n>> @@ -228,9 +231,10 @@ int parse_submodule_update_strategy(const char *value,\n>> dst->type = SM_UPDATE_REBASE;\n>> else if (!strcmp(value, \"merge\"))\n>> dst->type = SM_UPDATE_MERGE;\n>> - else if (skip_prefix(value, \"!\", &dst->command))\n>> + else if (skip_prefix(value, \"!\", &com)) {\n>> dst->type = SM_UPDATE_COMMAND;\n>> - else\n>> + dst->command = xstrdup(com);\n>> + } else\n>> return -1;\n>> return 0;\n>> }\n>\n> Unless you count \"I want to write differently from what was\n> suggested\" is a desirable thing to do, I do not see a point in\n> favouring the above that uses an extra variable and skip_prefix()\n> over what I gave you as \"how about\" patch. But whatever.\n\nThe skip_prefix was there before, so it stuck there.\nAlso it seems a bit more high level to me hence easier to read,\n(though I am biased). I'll use your suggestion.\n\n>\n> - Is dst->command always initialized to a NULL (otherwise the\n> unconditional upfront free() makes it unsafe)?\n\nYes, although just currently. It seems hard to maintain going forward as\nthe struct submodule_update_strategy is part of the struct submodule\n(as defined in submodule.h) as well as the struct submodule_update_clone\n(as defined in submodule--helper.c) and both places take care of initializing\nit to null.\n\n>\n> - Is there a global free_something() or something_clear() function\n> that are used to release the resource held by a structure that\n> has submodule_update_strategy structure embedded in it? If so\n> dst->command needs to be freed there as well.\n\nSure, I'll just reroll the series now.\n\n>\n> Thanks.\n>\n>> Stefan Beller (7):\n>> submodule-config: keep update strategy around\n>> submodule-config: drop check against NULL\n>> fetching submodules: respect `submodule.fetchJobs` config option\n>> submodule update: direct error message to stderr\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 | 239 ++++++++++++++++++++++++++++++++++++++++\n>> git-submodule.sh | 54 ++++-----\n>> submodule-config.c | 18 ++-\n>> submodule-config.h | 2 +\n>> submodule.c | 39 ++++++-\n>> submodule.h | 18 +++\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, 406 insertions(+), 49 deletions(-)\n"},{"id":"278592","messageId":"CAGZ79kZwgBJQ2bkvKSuT+ULiSPL9x5WTcwVJG0FATNv-D_-AhQ@mail.gmail.com","threadId":"41439","inReplyTo":"CAGZ79kZZRENjVhfrhSfPmSNNQ_qrwpzWk=pocQ4GY6kBos1r6Q@mail.gmail.com","subject":"Re: [PATCHv12 0/7] Expose submodule parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-18T23:14:06Z","receivedAt":"2016-02-18T23:14:06Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 18, 2016 at 3:12 PM, Stefan Beller wrote:\n>> Unless you count \"I want to write differently from what was\n>> suggested\" is a desirable thing to do, I do not see a point in\n>> favouring the above that uses an extra variable and skip_prefix()\n>> over what I gave you as \"how about\" patch. But whatever.\n>\n> The skip_prefix was there before, so it stuck there.\n> Also it seems a bit more high level to me hence easier to read,\n> (though I am biased). I'll use your suggestion.\n\nand it doesn't crash when passing in value == NULL.\n(We don't do that currently, just a side observation)\n"},{"id":"278594","messageId":"xmqqio1lehbp.fsf@gitster.mtv.corp.google.com","threadId":"41439","inReplyTo":"CAGZ79kZwgBJQ2bkvKSuT+ULiSPL9x5WTcwVJG0FATNv-D_-AhQ@mail.gmail.com","subject":"Re: [PATCHv12 0/7] Expose submodule parallelism to the user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-18T23:20:42Z","receivedAt":"2016-02-18T23:20:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Thu, Feb 18, 2016 at 3:12 PM, Stefan Beller wrote:\n>>> Unless you count \"I want to write differently from what was\n>>> suggested\" is a desirable thing to do, I do not see a point in\n>>> favouring the above that uses an extra variable and skip_prefix()\n>>> over what I gave you as \"how about\" patch. But whatever.\n>>\n>> The skip_prefix was there before, so it stuck there.\n\nSorry, but I thought this \"parsing update strategy\" was all new\ncode.\n\n>> Also it seems a bit more high level to me hence easier to read,\n>> (though I am biased). I'll use your suggestion.\n>\n> and it doesn't crash when passing in value == NULL.\n> (We don't do that currently, just a side observation)\n\nHmph. If you pass str==NULL with prefix=\"!\" to what we have below,\nI would think the first iteration would try to read from *str and do\na bizarre thing.\n\nstatic inline int skip_prefix(const char *str, const char *prefix,\n\t\t\t const char **out)\n{\n\tdo {\n\t\tif (!*prefix) {\n\t\t\t*out = str;\n\t\t\treturn 1;\n\t\t}\n\t} while (*str++ == *prefix++);\n\treturn 0;\n}\n\nPuzzled.\n"},{"id":"278595","messageId":"CAGZ79kY7aYmDcz+JYwpzizfzRV3bUeN8TU-4jRjKqToJJTBHoA@mail.gmail.com","threadId":"41439","inReplyTo":"xmqqio1lehbp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv12 0/7] Expose submodule parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-18T23:27:13Z","receivedAt":"2016-02-18T23:27:13Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 18, 2016 at 3:20 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> On Thu, Feb 18, 2016 at 3:12 PM, Stefan Beller wrote:\n>>>> Unless you count \"I want to write differently from what was\n>>>> suggested\" is a desirable thing to do, I do not see a point in\n>>>> favouring the above that uses an extra variable and skip_prefix()\n>>>> over what I gave you as \"how about\" patch. But whatever.\n>>>\n>>> The skip_prefix was there before, so it stuck there.\n>\n> Sorry, but I thought this \"parsing update strategy\" was all new\n> code.\n\nI meant previous patches or in my mind. That's why I was hesitant to\nthrow out the skip_prefix.\n\n>\n>>> Also it seems a bit more high level to me hence easier to read,\n>>> (though I am biased). I'll use your suggestion.\n>>\n>> and it doesn't crash when passing in value == NULL.\n>> (We don't do that currently, just a side observation)\n>\n> Hmph. If you pass str==NULL with prefix=\"!\" to what we have below,\n> I would think the first iteration would try to read from *str and do\n> a bizarre thing.\n>\n> static inline int skip_prefix(const char *str, const char *prefix,\n> const char **out)\n> {\n> do {\n> if (!*prefix) {\n> *out = str;\n> return 1;\n> }\n> } while (*str++ == *prefix++);\n> return 0;\n> }\n>\n> Puzzled.\n\nAnd there I was asserting properties about methods\nwithout looking them up.\n\nok.\n"},{"id":"278608","messageId":"xmqqbn7de6hp.fsf@gitster.mtv.corp.google.com","threadId":"41439","inReplyTo":"CAGZ79kY7aYmDcz+JYwpzizfzRV3bUeN8TU-4jRjKqToJJTBHoA@mail.gmail.com","subject":"Re: [PATCHv12 0/7] Expose submodule parallelism to the user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-19T03:14:42Z","receivedAt":"2016-02-19T03:14:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Thu, Feb 18, 2016 at 3:20 PM, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>\n>>> On Thu, Feb 18, 2016 at 3:12 PM, Stefan Beller wrote:\n>>>>> Unless you count \"I want to write differently from what was\n>>>>> suggested\" is a desirable thing to do, I do not see a point in\n>>>>> favouring the above that uses an extra variable and skip_prefix()\n>>>>> over what I gave you as \"how about\" patch. But whatever.\n>>>>\n>>>> The skip_prefix was there before, so it stuck there.\n>>\n>> Sorry, but I thought this \"parsing update strategy\" was all new\n>> code.\n>\n> I meant previous patches or in my mind. That's why I was hesitant to\n> throw out the skip_prefix.\n\nI actually think the attached on top of your final version would be\nthe best. It would not make too big a difference in this codepath\nthat skips just one byte, the pattern naturally would apply to\nprefix of any length, and this would serve as the BCP, ready to be\ncopied-and-pasted by others when writing new code.\n\nAnd of course it does not waste an otherwise unnecessary temporary\nvariable ;-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 911fa3b..8e08159 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -223,9 +223,9 @@ int parse_submodule_update_strategy(const char *value,\n \t\tdst->type = SM_UPDATE_REBASE;\n \telse if (!strcmp(value, \"merge\"))\n \t\tdst->type = SM_UPDATE_MERGE;\n-\telse if (value[0] == '!') {\n+\telse if (skip_prefix(value, \"!\", &value)) {\n \t\tdst->type = SM_UPDATE_COMMAND;\n-\t\tdst->command = xstrdup(value + 1);\n+\t\tdst->command = xstrdup(value);\n \t} else\n \t\treturn -1;\n \treturn 0;\n"}]}