{"thread":{"id":"41314","subject":"[PATCHv7 0/8] Expose submodule parallelism to the user","startedAt":"2016-02-02T17:51:29Z","lastAt":"2016-02-04T21:57:06Z","messageCount":22,"participants":["Stefan Beller","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"277254","messageId":"1454435497-26429-1-git-send-email-sbeller@google.com","threadId":"41314","inReplyTo":null,"subject":"[PATCHv7 0/8] Expose submodule parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-02T17:51:29Z","receivedAt":"2016-02-02T17:51:29Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is a resend of sb/submodule-parallel-update as is (directly taken from\nJunios tree), and I put it on the list to ease the life of reviewers.\n\nIt applies on top of origin/sb/submodule-parallel-fetch\n(which is part of master already)\n\nI have looked over the patches and they still look fine to me.\nThey had some early reviews end of last year, however not enough review\nyet to make them proceed.\n\nAny review is welcome!\nThanks,\nStefan\n\nStefan Beller (8):\n  submodule-config: keep update strategy around\n  submodule-config: drop check against NULL\n  submodule-config: remove name_and_item_from_var\n  submodule-config: introduce parse_generic_submodule_config\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        |   7 ++\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              | 109 +++++++++++-------\n submodule-config.h              |   3 +\n submodule.c                     |   5 +\n t/t5526-fetch-submodules.sh     |  14 +++\n t/t7400-submodule-basic.sh      |   4 +-\n t/t7406-submodule-update.sh     |  27 +++++\n 13 files changed, 413 insertions(+), 83 deletions(-)\n\n-- \n2.7.0.rc0.42.ge5f5e2d\n"},{"id":"277260","messageId":"1454435497-26429-2-git-send-email-sbeller@google.com","threadId":"41314","inReplyTo":"1454435497-26429-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/8] submodule-config: keep update strategy around","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-02T17:51:30Z","receivedAt":"2016-02-02T17:51:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We need the submodule update strategies in a later patch.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n submodule-config.c | 11 +++++++++++\n submodule-config.h |  1 +\n 2 files changed, 12 insertions(+)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex afe0ea8..4239b0e 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -194,6 +194,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,\n \n \tsubmodule->path = NULL;\n \tsubmodule->url = NULL;\n+\tsubmodule->update = NULL;\n \tsubmodule->fetch_recurse = RECURSE_SUBMODULES_NONE;\n \tsubmodule->ignore = NULL;\n \n@@ -311,6 +312,16 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tfree((void *) submodule->url);\n \t\t\tsubmodule->url = xstrdup(value);\n \t\t}\n+\t} else if (!strcmp(item.buf, \"update\")) {\n+\t\tif (!value)\n+\t\t\tret = config_error_nonbool(var);\n+\t\telse if (!me->overwrite && submodule->update != NULL)\n+\t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n+\t\t\t\t\t     \"update\");\n+\t\telse {\n+\t\t\tfree((void *) submodule->update);\n+\t\t\tsubmodule->update = xstrdup(value);\n+\t\t}\n \t}\n \n \tstrbuf_release(&name);\ndiff --git a/submodule-config.h b/submodule-config.h\nindex 9061e4e..f9e2a29 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -14,6 +14,7 @@ struct submodule {\n \tconst char *url;\n \tint fetch_recurse;\n \tconst char *ignore;\n+\tconst char *update;\n \t/* the sha1 blob id of the responsible .gitmodules file */\n \tunsigned char gitmodules_sha1[20];\n };\n-- \n2.7.0.rc0.42.ge5f5e2d\n"},{"id":"277259","messageId":"1454435497-26429-3-git-send-email-sbeller@google.com","threadId":"41314","inReplyTo":"1454435497-26429-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/8] submodule-config: drop check against NULL","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-02T17:51:31Z","receivedAt":"2016-02-02T17:51:31Z","isPatch":true,"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 <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n submodule-config.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 4239b0e..6d01941 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -265,7 +265,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@@ -289,7 +289,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@@ -305,7 +305,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@@ -315,7 +315,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t} else if (!strcmp(item.buf, \"update\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n-\t\telse if (!me->overwrite && submodule->update != NULL)\n+\t\telse if (!me->overwrite && submodule->update)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t     \"update\");\n \t\telse {\n-- \n2.7.0.rc0.42.ge5f5e2d\n"},{"id":"277256","messageId":"1454435497-26429-4-git-send-email-sbeller@google.com","threadId":"41314","inReplyTo":"1454435497-26429-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/8] submodule-config: remove name_and_item_from_var","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-02T17:51:32Z","receivedAt":"2016-02-02T17:51:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`name_and_item_from_var` does not provide the proper abstraction\nwe need here in a later patch.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n submodule-config.c | 48 ++++++++++++++++--------------------------------\n 1 file changed, 16 insertions(+), 32 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 6d01941..b826841 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -161,31 +161,17 @@ 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+\t\t\t\t\t\t  const unsigned char *gitmodules_sha1,\n+\t\t\t\t\t\t  const char *name_ptr, int name_len)\n {\n \tstruct submodule *submodule;\n \tstruct strbuf name_buf = STRBUF_INIT;\n+\tchar *name = xmemdupz(name_ptr, name_len);\n \n \tsubmodule = cache_lookup_name(cache, gitmodules_sha1, name);\n \tif (submodule)\n-\t\treturn submodule;\n+\t\tgoto out;\n \n \tsubmodule = xmalloc(sizeof(*submodule));\n \n@@ -201,7 +187,8 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,\n \thashcpy(submodule->gitmodules_sha1, gitmodules_sha1);\n \n \tcache_add(cache, submodule);\n-\n+out:\n+\tfree(name);\n \treturn submodule;\n }\n \n@@ -251,18 +238,18 @@ static int parse_config(const char *var, const char *value, void *data)\n {\n \tstruct parse_config_parameter *me = data;\n \tstruct submodule *submodule;\n-\tstruct strbuf name = STRBUF_INIT, item = STRBUF_INIT;\n-\tint ret = 0;\n+\tint subsection_len, ret = 0;\n+\tconst char *subsection, *key;\n \n-\t/* this also ensures that we only parse submodule entries */\n-\tif (!name_and_item_from_var(var, &name, &item))\n+\tif (parse_config_key(var, \"submodule\", &subsection,\n+\t\t\t     &subsection_len, &key) < 0 || !subsection_len)\n \t\treturn 0;\n \n \tsubmodule = lookup_or_create_by_name(me->cache,\n \t\t\t\t\t     me->gitmodules_sha1,\n-\t\t\t\t\t     name.buf);\n+\t\t\t\t\t     subsection, subsection_len);\n \n-\tif (!strcmp(item.buf, \"path\")) {\n+\tif (!strcmp(key, \"path\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->path)\n@@ -275,7 +262,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tsubmodule->path = xstrdup(value);\n \t\t\tcache_put_path(me->cache, submodule);\n \t\t}\n-\t} else if (!strcmp(item.buf, \"fetchrecursesubmodules\")) {\n+\t} else if (!strcmp(key, \"fetchrecursesubmodules\")) {\n \t\t/* when parsing worktree configurations we can die early */\n \t\tint die_on_error = is_null_sha1(me->gitmodules_sha1);\n \t\tif (!me->overwrite &&\n@@ -286,7 +273,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tsubmodule->fetch_recurse = parse_fetch_recurse(\n \t\t\t\t\t\t\t\tvar, value,\n \t\t\t\t\t\t\t\tdie_on_error);\n-\t} else if (!strcmp(item.buf, \"ignore\")) {\n+\t} else if (!strcmp(key, \"ignore\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->ignore)\n@@ -302,7 +289,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tfree((void *) submodule->ignore);\n \t\t\tsubmodule->ignore = xstrdup(value);\n \t\t}\n-\t} else if (!strcmp(item.buf, \"url\")) {\n+\t} else if (!strcmp(key, \"url\")) {\n \t\tif (!value) {\n \t\t\tret = config_error_nonbool(var);\n \t\t} else if (!me->overwrite && submodule->url) {\n@@ -312,7 +299,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tfree((void *) submodule->url);\n \t\t\tsubmodule->url = xstrdup(value);\n \t\t}\n-\t} else if (!strcmp(item.buf, \"update\")) {\n+\t} else if (!strcmp(key, \"update\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->update)\n@@ -324,9 +311,6 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t}\n \t}\n \n-\tstrbuf_release(&name);\n-\tstrbuf_release(&item);\n-\n \treturn ret;\n }\n \n-- \n2.7.0.rc0.42.ge5f5e2d\n"},{"id":"277255","messageId":"1454435497-26429-5-git-send-email-sbeller@google.com","threadId":"41314","inReplyTo":"1454435497-26429-1-git-send-email-sbeller@google.com","subject":"[PATCH 4/8] submodule-config: introduce parse_generic_submodule_config","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-02T17:51:33Z","receivedAt":"2016-02-02T17:51:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This rewrites parse_config to distinguish between configs specific to\none submodule and configs which apply generically to all submodules.\nWe do not have generic submodule configs yet, but the next patch will\nintroduce \"submodule.fetchJobs\".\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n submodule-config.c | 41 ++++++++++++++++++++++++++++++++---------\n 1 file changed, 32 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex b826841..29e21b2 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -234,17 +234,22 @@ struct parse_config_parameter {\n \tint overwrite;\n };\n \n-static int parse_config(const char *var, const char *value, void *data)\n+static int parse_generic_submodule_config(const char *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-\tstruct parse_config_parameter *me = data;\n-\tstruct submodule *submodule;\n-\tint subsection_len, ret = 0;\n-\tconst char *subsection, *key;\n-\n-\tif (parse_config_key(var, \"submodule\", &subsection,\n-\t\t\t     &subsection_len, &key) < 0 || !subsection_len)\n-\t\treturn 0;\n+\treturn 0;\n+}\n \n+static int parse_specific_submodule_config(const char *subsection, int subsection_len,\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, subsection_len);\n@@ -314,6 +319,24 @@ static int parse_config(const char *var, const char *value, void *data)\n \treturn ret;\n }\n \n+static int parse_config(const char *var, const char *value, void *data)\n+{\n+\tstruct parse_config_parameter *me = data;\n+\tint subsection_len;\n+\tconst char *subsection, *key;\n+\n+\tif (parse_config_key(var, \"submodule\", &subsection,\n+\t\t\t     &subsection_len, &key) < 0)\n+\t\treturn 0;\n+\n+\tif (!subsection_len)\n+\t\treturn parse_generic_submodule_config(key, var, value, me);\n+\telse\n+\t\treturn parse_specific_submodule_config(subsection,\n+\t\t\t\t\t\t       subsection_len, key,\n+\t\t\t\t\t\t       var, value, me);\n+}\n+\n static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,\n \t\t\t\t      unsigned char *gitmodules_sha1)\n {\n-- \n2.7.0.rc0.42.ge5f5e2d\n"},{"id":"277258","messageId":"1454435497-26429-6-git-send-email-sbeller@google.com","threadId":"41314","inReplyTo":"1454435497-26429-1-git-send-email-sbeller@google.com","subject":"[PATCH 5/8] fetching submodules: respect `submodule.fetchJobs` config option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-02T17:51:34Z","receivedAt":"2016-02-02T17:51:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This allows to configure fetching and updating in parallel\nwithout having the command line option.\n\nThis moved the responsibility to determine how many parallel processes\nto start from builtin/fetch to submodule.c as we need a way to communicate\n\"The user did not specify the number of parallel processes in the command\nline options\" in the builtin fetch. The submodule code takes care of\nthe precedence (CLI > config > default)\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/config.txt    |  7 +++++++\n builtin/fetch.c             |  2 +-\n submodule-config.c          | 15 +++++++++++++++\n submodule-config.h          |  2 ++\n submodule.c                 |  5 +++++\n t/t5526-fetch-submodules.sh | 14 ++++++++++++++\n 6 files changed, 44 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 2d06b11..eda3276 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2646,6 +2646,13 @@ submodule.<name>.ignore::\n \t\"--ignore-submodules\" option. The 'git submodule' commands are not\n \taffected by this setting.\n \n+submodule.fetchJobs::\n+\tThis is used to determine how many submodules will be\n+\tfetched/cloned at the same time. Specifying a positive integer\n+\tallows up to that number of submodules being fetched in parallel.\n+\tThis is used in fetch and clone operations only. A value of 0 will\n+\tgive some reasonable configuration. 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=<value>\" 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-config.c b/submodule-config.c\nindex 29e21b2..a32259e 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -32,6 +32,7 @@ enum lookup_type {\n \n static struct submodule_cache cache;\n static int is_cache_init;\n+static int parallel_jobs = -1;\n \n static int config_path_cmp(const struct submodule_entry *a,\n \t\t\t   const struct submodule_entry *b,\n@@ -239,6 +240,15 @@ static int parse_generic_submodule_config(const char *key,\n \t\t\t\t\t  const char *value,\n \t\t\t\t\t  struct parse_config_parameter *me)\n {\n+\tif (!strcmp(key, \"fetchjobs\")) {\n+\t\tparallel_jobs = strtol(value, NULL, 10);\n+\t\tif (parallel_jobs < 0) {\n+\t\t\twarning(\"submodule.fetchJobs not allowed to be negative.\");\n+\t\t\tparallel_jobs = 1;\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n+\n \treturn 0;\n }\n \n@@ -482,3 +492,8 @@ void submodule_free(void)\n \tcache_free(&cache);\n \tis_cache_init = 0;\n }\n+\n+int config_parallel_submodules(void)\n+{\n+\treturn parallel_jobs;\n+}\ndiff --git a/submodule-config.h b/submodule-config.h\nindex f9e2a29..d9bbf9a 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -27,4 +27,6 @@ const struct submodule *submodule_from_path(const unsigned char *commit_sha1,\n \t\tconst char *path);\n void submodule_free(void);\n \n+int config_parallel_submodules(void);\n+\n #endif /* SUBMODULE_CONFIG_H */\ndiff --git a/submodule.c b/submodule.c\nindex b83939c..eb7d54b 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -751,6 +751,11 @@ int fetch_populated_submodules(const struct argv_array *options,\n \targv_array_push(&spf.args, \"--recurse-submodules-default\");\n \t/* default value, \"--submodule-prefix\" and its value are added later */\n \n+\tif (max_parallel_jobs < 0)\n+\t\tmax_parallel_jobs = config_parallel_submodules();\n+\tif (max_parallel_jobs < 0)\n+\t\tmax_parallel_jobs = 1;\n+\n \tcalculate_changed_submodule_paths();\n \trun_processes_parallel(max_parallel_jobs,\n \t\t\t       get_next_submodule,\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 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.42.ge5f5e2d\n"},{"id":"277262","messageId":"1454435497-26429-7-git-send-email-sbeller@google.com","threadId":"41314","inReplyTo":"1454435497-26429-1-git-send-email-sbeller@google.com","subject":"[PATCH 6/8] git submodule update: have a dedicated helper for cloning","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-02T17:51:35Z","receivedAt":"2016-02-02T17:51:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This introduces a new helper function in git submodule--helper\nwhich takes care of cloning all submodules, which we want to\nparallelize eventually.\n\nSome tests (such as empty URL, update_mode=none) are required in the\nhelper to make the decision for cloning. These checks have been\nmoved into the C function as well (no need to repeat them in the\nshell script).\n\nAs we can only access the stderr channel from within the parallel\nprocessing engine, we need to reroute the error message for\nspecified but initialized submodules to stderr. As it is an error\nmessage, this should have gone to stderr in the first place, so it\nis a bug fix along the way.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/submodule--helper.c | 229 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            |  45 +++------\n t/t7400-submodule-basic.sh  |   4 +-\n 3 files changed, 242 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff..9441f20 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -255,6 +255,234 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int git_submodule_config(const char *var, const char *value, void *cb)\n+{\n+\treturn parse_submodule_config_option(var, value);\n+}\n+\n+struct submodule_update_clone {\n+\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 char *displaypath = NULL;\n+\t\tconst struct cache_entry *ce = pp->list.entries[pp->count];\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tconst char *update_module = NULL;\n+\t\tchar *url = NULL;\n+\t\tint needs_cloning = 0;\n+\n+\t\tif (ce_stage(ce)) {\n+\t\t\tif (pp->recursive_prefix)\n+\t\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s/%s\\n\",\n+\t\t\t\t\tpp->recursive_prefix, ce->name);\n+\t\t\telse\n+\t\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s\\n\",\n+\t\t\t\t\tce->name);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tsub = submodule_from_path(null_sha1, ce->name);\n+\t\tif (!sub) {\n+\t\t\tstrbuf_addf(err, \"BUG: internal error managing submodules. \"\n+\t\t\t\t    \"The cache could not locate '%s'\", ce->name);\n+\t\t\tpp->print_unmatched = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (pp->recursive_prefix)\n+\t\t\tdisplaypath = relative_path(pp->recursive_prefix, ce->name, &sb);\n+\t\telse\n+\t\t\tdisplaypath = ce->name;\n+\n+\t\tif (pp->update)\n+\t\t\tupdate_module = pp->update;\n+\t\tif (!update_module)\n+\t\t\tupdate_module = sub->update;\n+\t\tif (!update_module)\n+\t\t\tupdate_module = \"checkout\";\n+\t\tif (!strcmp(update_module, \"none\")) {\n+\t\t\tstrbuf_addf(err, \"Skipping submodule '%s'\\n\", displaypath);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/*\n+\t\t * Looking up the url in .git/config.\n+\t\t * We must not fall back to .gitmodules as we only want to process\n+\t\t * 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, _(\"Submodule path '%s' not initialized\\n\"\n+\t\t\t\t\t\"Maybe you want to use 'update --init'?\"), displaypath);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n+\t\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, ce->name,\n+\t\t\t\t\t   sub->name, url, pp->reference, pp->depth);\n+\t\t\tpp->count++;\n+\t\t\tfree(url);\n+\t\t\treturn 1;\n+\t\t} else\n+\t\t\tfree(url);\n+\t}\n+\treturn 0;\n+}\n+\n+static int update_clone_start_failure(struct child_process *cp,\n+\t\t\t\t      struct strbuf *err,\n+\t\t\t\t      void *pp_cb,\n+\t\t\t\t      void *pp_task_cb)\n+{\n+\tstruct submodule_update_clone *pp = pp_cb;\n+\n+\tstrbuf_addf(err, \"error when starting a child process\");\n+\tpp->print_unmatched = 1;\n+\n+\treturn 1;\n+}\n+\n+static int update_clone_task_finished(int result,\n+\t\t\t\t      struct child_process *cp,\n+\t\t\t\t      struct strbuf *err,\n+\t\t\t\t      void *pp_cb,\n+\t\t\t\t      void *pp_task_cb)\n+{\n+\tstruct submodule_update_clone *pp = pp_cb;\n+\n+\tif (!result) {\n+\t\treturn 0;\n+\t} else {\n+\t\tstrbuf_addf(err, \"error in one child process\");\n+\t\tpp->print_unmatched = 1;\n+\t\treturn 1;\n+\t}\n+}\n+\n+static int update_clone(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct string_list_item *item;\n+\tstruct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;\n+\n+\tstruct option module_list_options[] = {\n+\t\tOPT_STRING(0, \"prefix\", &prefix,\n+\t\t\t   N_(\"path\"),\n+\t\t\t   N_(\"path into the working tree\")),\n+\t\tOPT_STRING(0, \"recursive_prefix\", &pp.recursive_prefix,\n+\t\t\t   N_(\"path\"),\n+\t\t\t   N_(\"path into the working tree, across nested \"\n+\t\t\t      \"submodule boundaries\")),\n+\t\tOPT_STRING(0, \"update\", &pp.update,\n+\t\t\t   N_(\"string\"),\n+\t\t\t   N_(\"update command for submodules\")),\n+\t\tOPT_STRING(0, \"reference\", &pp.reference, \"<repository>\",\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, \"<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=<path>] [<path>...]\"),\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 +492,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.42.ge5f5e2d\n"},{"id":"277261","messageId":"1454435497-26429-8-git-send-email-sbeller@google.com","threadId":"41314","inReplyTo":"1454435497-26429-1-git-send-email-sbeller@google.com","subject":"[PATCH 7/8] submodule update: expose parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-02T17:51:36Z","receivedAt":"2016-02-02T17:51:36Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Expose possible parallelism either via the \"--jobs\" CLI parameter or\nthe \"submodule.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 <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/git-submodule.txt |  7 ++++++-\n builtin/submodule--helper.c     | 18 ++++++++++++++----\n git-submodule.sh                |  9 +++++++++\n t/t7406-submodule-update.sh     | 12 ++++++++++++\n 4 files changed, 41 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex 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] [--] <path>...\n 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]\n \t      [-f|--force] [--rebase|--merge] [--reference <repository>]\n-\t      [--depth <depth>] [--recursive] [--] [<path>...]\n+\t      [--depth <depth>] [--recursive] [--jobs <n>] [--] [<path>...]\n 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]\n \t      [commit] [--] [<path>...]\n 'git submodule' [--quiet] foreach [--recursive] <command>\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>::\n+--jobs <n>::\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 <path>...::\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 9441f20..8002187 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -426,6 +426,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@@ -446,6 +447,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"depth\", &pp.depth, \"<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@@ -467,10 +470,17 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \tgitmodules_config();\n \t/* Overlay the parsed .gitmodules file with .git/config */\n \tgit_config(git_submodule_config, NULL);\n-\trun_processes_parallel(1, update_clone_get_next_task,\n-\t\t\t\t  update_clone_start_failure,\n-\t\t\t\t  update_clone_task_finished,\n-\t\t\t\t  &pp);\n+\n+\tif (max_jobs < 0)\n+\t\tmax_jobs = config_parallel_submodules();\n+\tif (max_jobs < 0)\n+\t\tmax_jobs = 1;\n+\n+\trun_processes_parallel(max_jobs,\n+\t\t\t       update_clone_get_next_task,\n+\t\t\t       update_clone_start_failure,\n+\t\t\t       update_clone_task_finished,\n+\t\t\t       &pp);\n \n \tif (pp.print_unmatched) {\n \t\tprintf(\"#unmatched\\n\");\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 9f554fb..10c5af9 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -645,6 +645,14 @@ cmd_update()\n \t\t--depth=*)\n \t\t\tdepth=$1\n \t\t\t;;\n+\t\t-j|--jobs)\n+\t\t\tcase \"$2\" in '') usage ;; esac\n+\t\t\tjobs=\"--jobs=$2\"\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--jobs=*)\n+\t\t\tjobs=$1\n+\t\t\t;;\n \t\t--)\n \t\t\tshift\n \t\t\tbreak\n@@ -670,6 +678,7 @@ cmd_update()\n \t\t${update:+--update \"$update\"} \\\n \t\t${reference:+--reference \"$reference\"} \\\n \t\t${depth:+--depth \"$depth\"} \\\n+\t\t${jobs:+$jobs} \\\n \t\t\"$@\" | {\n \terr=\n \twhile read mode sha1 stage just_cloned sm_path\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex dda3929..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.42.ge5f5e2d\n"},{"id":"277257","messageId":"1454435497-26429-9-git-send-email-sbeller@google.com","threadId":"41314","inReplyTo":"1454435497-26429-1-git-send-email-sbeller@google.com","subject":"[PATCH 8/8] clone: allow an explicit argument for parallel submodule clones","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-02T17:51:37Z","receivedAt":"2016-02-02T17:51:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Just pass it along to \"git submodule update\", which may pick reasonable\ndefaults if you don't specify an explicit number.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\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 <name>] [-b <name>] [-u <upload-pack>] [--reference <repository>]\n \t  [--dissociate] [--separate-git-dir <git dir>]\n \t  [--depth <depth>] [--[no-]single-branch]\n-\t  [--recursive | --recurse-submodules] [--] <repository>\n+\t  [--recursive | --recurse-submodules] [--jobs <n>] [--] <repository>\n \t  [<directory>]\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>::\n+--jobs <n>::\n+\tThe number of submodules fetched at the same time.\n+\tDefaults to the `submodule.fetchJobs` option.\n \n <repository>::\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.42.ge5f5e2d\n"},{"id":"277346","messageId":"xmqqy4b1l7cd.fsf@gitster.mtv.corp.google.com","threadId":"41314","inReplyTo":"1454435497-26429-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/8] submodule-config: keep update strategy around","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-03T23:09:06Z","receivedAt":"2016-02-03T23:09:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> We need the submodule update strategies in a later patch.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  submodule-config.c | 11 +++++++++++\n>  submodule-config.h |  1 +\n>  2 files changed, 12 insertions(+)\n>\n> diff --git a/submodule-config.c b/submodule-config.c\n> index afe0ea8..4239b0e 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -194,6 +194,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,\n>  \n>  \tsubmodule->path = NULL;\n>  \tsubmodule->url = NULL;\n> +\tsubmodule->update = NULL;\n>  \tsubmodule->fetch_recurse = RECURSE_SUBMODULES_NONE;\n>  \tsubmodule->ignore = NULL;\n>  \n> @@ -311,6 +312,16 @@ static int parse_config(const char *var, const char *value, void *data)\n>  \t\t\tfree((void *) submodule->url);\n>  \t\t\tsubmodule->url = xstrdup(value);\n>  \t\t}\n> +\t} else if (!strcmp(item.buf, \"update\")) {\n> +\t\tif (!value)\n> +\t\t\tret = config_error_nonbool(var);\n> +\t\telse if (!me->overwrite && submodule->update != NULL)\n> +\t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n> +\t\t\t\t\t     \"update\");\n> +\t\telse {\n> +\t\t\tfree((void *) submodule->update);\n> +\t\t\tsubmodule->update = xstrdup(value);\n\nThis is a tangent, but whenever I see us needing to cast the\nconstness away to call free(), it makes me wonder how much value we\nare getting by marking the field as a pointer to \"const\" in the\nfirst place.  With this patch alone, we cannot really see it,\nbecause we cannot see how it is used.\n\n> +\t\t}\n>  \t}\n>  \n>  \tstrbuf_release(&name);\n> diff --git a/submodule-config.h b/submodule-config.h\n> index 9061e4e..f9e2a29 100644\n> --- a/submodule-config.h\n> +++ b/submodule-config.h\n> @@ -14,6 +14,7 @@ struct submodule {\n>  \tconst char *url;\n>  \tint fetch_recurse;\n>  \tconst char *ignore;\n> +\tconst char *update;\n>  \t/* the sha1 blob id of the responsible .gitmodules file */\n>  \tunsigned char gitmodules_sha1[20];\n>  };\n"},{"id":"277344","messageId":"xmqqr3gtl7c9.fsf@gitster.mtv.corp.google.com","threadId":"41314","inReplyTo":"1454435497-26429-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/8] submodule-config: drop check against NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-03T23:09:10Z","receivedAt":"2016-02-03T23:09:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Adhere to the common coding style of Git and not check explicitly\n> for NULL throughout the file. There are still other occurrences in the\n> code base but that is usually inside of conditions with side effects.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n\n>  submodule-config.c | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/submodule-config.c b/submodule-config.c\n> index 4239b0e..6d01941 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -265,7 +265,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\nGood.\n"},{"id":"277345","messageId":"xmqqk2mll7c1.fsf@gitster.mtv.corp.google.com","threadId":"41314","inReplyTo":"1454435497-26429-4-git-send-email-sbeller@google.com","subject":"Re: [PATCH 3/8] submodule-config: remove name_and_item_from_var","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-03T23:09:18Z","receivedAt":"2016-02-03T23:09:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> `name_and_item_from_var` does not provide the proper abstraction\n> we need here in a later patch.\n\n... so the idea is to first open-code the calling site in this\npatch, and bring a different abstraction that is better suited to\nthis machinery in a later step?  Makes sense, especially it only\nhave a single callsite--let's continue reading...\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  submodule-config.c | 48 ++++++++++++++++--------------------------------\n>  1 file changed, 16 insertions(+), 32 deletions(-)\n>\n> diff --git a/submodule-config.c b/submodule-config.c\n> index 6d01941..b826841 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -161,31 +161,17 @@ 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> +\t\t\t\t\t\t  const unsigned char *gitmodules_sha1,\n> +\t\t\t\t\t\t  const char *name_ptr, int name_len)\n>  {\n>  \tstruct submodule *submodule;\n>  \tstruct strbuf name_buf = STRBUF_INIT;\n> +\tchar *name = xmemdupz(name_ptr, name_len);\n\nThis used to be fed the whole name, but now the caller uses\nparse_config_key() and we get a <ptr,len> pair, hence a pair of new\nvariable allocation and release in this function.  OK.\n\n>  \tsubmodule = cache_lookup_name(cache, gitmodules_sha1, name);\n>  \tif (submodule)\n> -\t\treturn submodule;\n> +\t\tgoto out;\n>  \n>  \tsubmodule = xmalloc(sizeof(*submodule));\n>  \n> @@ -201,7 +187,8 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,\n>  \thashcpy(submodule->gitmodules_sha1, gitmodules_sha1);\n>  \n>  \tcache_add(cache, submodule);\n> -\n> +out:\n> +\tfree(name);\n>  \treturn submodule;\n>  }\n>  \n> @@ -251,18 +238,18 @@ static int parse_config(const char *var, const char *value, void *data)\n>  {\n>  \tstruct parse_config_parameter *me = data;\n>  \tstruct submodule *submodule;\n> -\tstruct strbuf name = STRBUF_INIT, item = STRBUF_INIT;\n> -\tint ret = 0;\n> +\tint subsection_len, ret = 0;\n> +\tconst char *subsection, *key;\n>  \n> -\t/* this also ensures that we only parse submodule entries */\n> -\tif (!name_and_item_from_var(var, &name, &item))\n> +\tif (parse_config_key(var, \"submodule\", &subsection,\n> +\t\t\t     &subsection_len, &key) < 0 || !subsection_len)\n>  \t\treturn 0;\n\nThe strbuf name became subsection ptr,len pair [*1*].\n>  \tsubmodule = lookup_or_create_by_name(me->cache,\n>  \t\t\t\t\t     me->gitmodules_sha1,\n> -\t\t\t\t\t     name.buf);\n> +\t\t\t\t\t     subsection, subsection_len);\n\n... and that is used here.\n\n>  \n> -\tif (!strcmp(item.buf, \"path\")) {\n> +\tif (!strcmp(key, \"path\")) {\n\n... and the \"item\" that used to hold the third-level variable name\nis now a variable \"key\".  Looks correct.\n\n\n[Footnote]\n\n*1* I notice that the error detection has been slightly changed.\n    The original code used to detect a two-level variable name by\n    checking if the subsection pointer is NULL.  Now you check the\n    length.\n\n    I didn't check the implementation of parse_config_key(), but it\n    feels to me that the original would be \"more\" correct.  Even\n    though we may not accept an empty second level name (or do we\n    already?), it would be a good idea to get the code prepared to\n    differentiate between the lack of subsection and having a\n    subsection that happens to be an empty string, i.e.\n\n\t[section \"\"]\n        \tvariable = value\n"},{"id":"277347","messageId":"xmqqd1sdl7bx.fsf@gitster.mtv.corp.google.com","threadId":"41314","inReplyTo":"1454435497-26429-5-git-send-email-sbeller@google.com","subject":"Re: [PATCH 4/8] submodule-config: introduce parse_generic_submodule_config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-03T23:09:22Z","receivedAt":"2016-02-03T23:09:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> This rewrites parse_config to distinguish between configs specific to\n> one submodule and configs which apply generically to all submodules.\n> We do not have generic submodule configs yet, but the next patch will\n> introduce \"submodule.fetchJobs\".\n\nOK.\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;\n> +\tconst char *subsection, *key;\n> +\n> +\tif (parse_config_key(var, \"submodule\", &subsection,\n> +\t\t\t     &subsection_len, &key) < 0)\n> +\t\treturn 0;\n> +\n> +\tif (!subsection_len)\n> +\t\treturn parse_generic_submodule_config(key, var, value, me);\n\nThe same comment as in the footnote for the review on [3/8] applies\nhere.\n\n> +\telse\n> +\t\treturn parse_specific_submodule_config(subsection,\n> +\t\t\t\t\t\t       subsection_len, key,\n> +\t\t\t\t\t\t       var, value, me);\n> +}\n> +\n>  static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,\n>  \t\t\t\t      unsigned char *gitmodules_sha1)\n>  {\n"},{"id":"277348","messageId":"xmqq60y5l7bq.fsf@gitster.mtv.corp.google.com","threadId":"41314","inReplyTo":"1454435497-26429-6-git-send-email-sbeller@google.com","subject":"Re: [PATCH 5/8] fetching submodules: respect `submodule.fetchJobs` config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-03T23:09:29Z","receivedAt":"2016-02-03T23:09:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> 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 <sbeller@google.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  Documentation/config.txt    |  7 +++++++\n>  builtin/fetch.c             |  2 +-\n>  submodule-config.c          | 15 +++++++++++++++\n>  submodule-config.h          |  2 ++\n>  submodule.c                 |  5 +++++\n>  t/t5526-fetch-submodules.sh | 14 ++++++++++++++\n>  6 files changed, 44 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 2d06b11..eda3276 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2646,6 +2646,13 @@ submodule.<name>.ignore::\n>  \t\"--ignore-submodules\" option. The 'git submodule' commands are not\n>  \taffected by this setting.\n>  \n> +submodule.fetchJobs::\n> +\tThis is used to determine how many submodules will be\n> +\tfetched/cloned at the same time. Specifying a positive integer\n> +\tallows up to that number of submodules being fetched in parallel.\n\ns/being //?  Either would be fine, but shorter might be better?  I\ndunno.\n\ns/is used to determine/specifies/?  That may shorten the whole\nthing.\n\n\tSpecifies how many submodules are fetched/cloned at the same\n\ttime.  A positive integer allows up to that number of ...\n\n> +\tThis is used in fetch and clone operations only. A value of 0 will\n> +\tgive some reasonable configuration. It defaults to 1.\n\ns/used/respected/, perhaps?\ns/reasonable configuration/reasonable default/, perhaps?\n\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-config.c b/submodule-config.c\n> index 29e21b2..a32259e 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -32,6 +32,7 @@ enum lookup_type {\n>  \n>  static struct submodule_cache cache;\n>  static int is_cache_init;\n> +static int parallel_jobs = -1;\n>  \n>  static int config_path_cmp(const struct submodule_entry *a,\n>  \t\t\t   const struct submodule_entry *b,\n> @@ -239,6 +240,15 @@ static int parse_generic_submodule_config(const char *key,\n>  \t\t\t\t\t  const char *value,\n>  \t\t\t\t\t  struct parse_config_parameter *me)\n>  {\n> +\tif (!strcmp(key, \"fetchjobs\")) {\n> +\t\tparallel_jobs = strtol(value, NULL, 10);\n\nWe do not notice trailing 'x' in \"[submodule] fetchjobs = 10x\";\nintended?  I am wondering this is a good excuse to invent\ngit_parse_long() and git_config_long() that do not exist yet.\nIn general, I'd personally prefer to reduce the number of calls to\nstrto[u]l that does not check trailing garbage, not increase it.\n\nIf we reject negatives anyway, we might be able to just use\ngit_parse_ulong() without adding new \"signed\" variants.\n"},{"id":"277349","messageId":"xmqqr3gtjs23.fsf@gitster.mtv.corp.google.com","threadId":"41314","inReplyTo":"1454435497-26429-7-git-send-email-sbeller@google.com","subject":"Re: [PATCH 6/8] git submodule update: have a dedicated helper for cloning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-03T23:24:36Z","receivedAt":"2016-02-03T23:24:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> +\t\tif (ce_stage(ce)) {\n> +\t\t\tif (pp->recursive_prefix)\n> +\t\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s/%s\\n\",\n> +\t\t\t\t\tpp->recursive_prefix, ce->name);\n> +\t\t\telse\n> +\t\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s\\n\",\n> +\t\t\t\t\tce->name);\n> +\t\t\tcontinue;\n\nThis raised my eyebrow somewhat, until I realized that it is OK\nbecause module_list prepared by the caller has only one of the\nunmerged entries for the same path to avoid duplicates.\n\n> +\t\t}\n> +\n> +\t\tsub = submodule_from_path(null_sha1, ce->name);\n> +\t\tif (!sub) {\n> +\t\t\tstrbuf_addf(err, \"BUG: internal error managing submodules. \"\n> +\t\t\t\t    \"The cache could not locate '%s'\", ce->name);\n> +\t\t\tpp->print_unmatched = 1;\n> +\t\t\tcontinue;\n\nInteresting.\n\nI am wondering if this check should go to module_list_compute().\n\n> +\t\t}\n> +\n> +\t\tif (pp->recursive_prefix)\n> +\t\t\tdisplaypath = relative_path(pp->recursive_prefix, ce->name, &sb);\n> +\t\telse\n> +\t\t\tdisplaypath = ce->name;\n> +\n> +\t\tif (pp->update)\n> +\t\t\tupdate_module = pp->update;\n> +\t\tif (!update_module)\n> +\t\t\tupdate_module = sub->update;\n> +\t\tif (!update_module)\n> +\t\t\tupdate_module = \"checkout\";\n> +\t\tif (!strcmp(update_module, \"none\")) {\n> +\t\t\tstrbuf_addf(err, \"Skipping submodule '%s'\\n\", displaypath);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Looking up the url in .git/config.\n> +\t\t * We must not fall back to .gitmodules as we only want to process\n> +\t\t * configured submodules.\n> +\t\t */\n> +\t\tstrbuf_reset(&sb);\n\nDoesn't this invalidate displaypath, when pp->recursive_prefix is in\neffect?  It still seems to be used to create an error message just a\nfew lines below...\n\n> +\t\tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n> +\t\tgit_config_get_string(sb.buf, &url);\n> +\t\tif (!url) {\n> +\t\t\t/*\n> +\t\t\t * Only mention uninitialized submodules when its\n> +\t\t\t * path have been specified\n> +\t\t\t */\n> +\t\t\tif (pp->pathspec.nr)\n> +\t\t\t\tstrbuf_addf(err, _(\"Submodule path '%s' not initialized\\n\"\n> +\t\t\t\t\t\"Maybe you want to use 'update --init'?\"), displaypath);\n> +\t\t\tcontinue;\n> +\t\t}\n"},{"id":"277350","messageId":"CAGZ79kZwGPNHeZFcu7-60U8cTZhC8t6TZ5=4ZzBbP+4MrLqRZQ@mail.gmail.com","threadId":"41314","inReplyTo":"xmqqd1sdl7bx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/8] submodule-config: introduce parse_generic_submodule_config","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-03T23:26:51Z","receivedAt":"2016-02-03T23:26:51Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Feb 3, 2016 at 3:09 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> This rewrites parse_config to distinguish between configs specific to\n>> one submodule and configs which apply generically to all submodules.\n>> We do not have generic submodule configs yet, but the next patch will\n>> introduce \"submodule.fetchJobs\".\n>\n> OK.\n>\n>> +static int parse_config(const char *var, const char *value, void *data)\n>> +{\n>> +     struct parse_config_parameter *me = data;\n>> +     int subsection_len;\n>> +     const char *subsection, *key;\n>> +\n>> +     if (parse_config_key(var, \"submodule\", &subsection,\n>> +                          &subsection_len, &key) < 0)\n>> +             return 0;\n>> +\n>> +     if (!subsection_len)\n>> +             return parse_generic_submodule_config(key, var, value, me);\n>\n> The same comment as in the footnote for the review on [3/8] applies\n> here.\n\nOk, so we want to have\n\n    [section \"\"]\n\nand\n\n    [section]\n\nbe different, such that we could have a submodule with an \"\" name,\nbut still be able to differentiate to the common section which should be\napplied to all submodules?\n\nIt sounds like a corner case to me, but if we want to guarantee such a behavior,\nlet me see if test cases for that need some special love in this series.\n\nThanks,\nStefan\n"},{"id":"277351","messageId":"CAGZ79kbeDBf=AfA2RUhkfjwJqJ7pr30xW3UTXqiha_EPpisvnw@mail.gmail.com","threadId":"41314","inReplyTo":"xmqqr3gtjs23.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 6/8] git submodule update: have a dedicated helper for cloning","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-03T23:41:36Z","receivedAt":"2016-02-03T23:41:36Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Feb 3, 2016 at 3:24 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> +             if (ce_stage(ce)) {\n>> +                     if (pp->recursive_prefix)\n>> +                             strbuf_addf(err, \"Skipping unmerged submodule %s/%s\\n\",\n>> +                                     pp->recursive_prefix, ce->name);\n\nAs a side question: Do we care about proper visual directory\nseparators in Windows?\n\n>> +                     else\n>> +                             strbuf_addf(err, \"Skipping unmerged submodule %s\\n\",\n>> +                                     ce->name);\n>> +                     continue;\n>\n> This raised my eyebrow somewhat, until I realized that it is OK\n> because module_list prepared by the caller has only one of the\n> unmerged entries for the same path to avoid duplicates.\n>\n>> +             }\n>> +\n>> +             sub = submodule_from_path(null_sha1, ce->name);\n>> +             if (!sub) {\n>> +                     strbuf_addf(err, \"BUG: internal error managing submodules. \"\n>> +                                 \"The cache could not locate '%s'\", ce->name);\n>> +                     pp->print_unmatched = 1;\n>> +                     continue;\n>\n> Interesting.\n>\n> I am wondering if this check should go to module_list_compute().\n\nActually this has evolved from a debugging leftover camouflaged as a\nuseful thing.\nI added that and then realized I did not add\n\n        gitmodules_config();\n        git_config(git_submodule_config, NULL);\n\nbefore, but that code remained there as it seemed useful to me at the time.\n\nI never run into this BUG after having proper initialization, so maybe it's not\nworth carrying this code around. (We have many other places where\nsubmodule_from_{path, name} is used unchecked, so why would this place\nbe special?)\n\n>\n>> +             }\n>> +\n>> +             if (pp->recursive_prefix)\n>> +                     displaypath = relative_path(pp->recursive_prefix, ce->name, &sb);\n>> +             else\n>> +                     displaypath = ce->name;\n>> +\n>> +             if (pp->update)\n>> +                     update_module = pp->update;\n>> +             if (!update_module)\n>> +                     update_module = sub->update;\n>> +             if (!update_module)\n>> +                     update_module = \"checkout\";\n>> +             if (!strcmp(update_module, \"none\")) {\n>> +                     strbuf_addf(err, \"Skipping submodule '%s'\\n\", displaypath);\n>> +                     continue;\n>> +             }\n>> +\n>> +             /*\n>> +              * Looking up the url in .git/config.\n>> +              * We must not fall back to .gitmodules as we only want to process\n>> +              * configured submodules.\n>> +              */\n>> +             strbuf_reset(&sb);\n>\n> Doesn't this invalidate displaypath, when pp->recursive_prefix is in\n> effect?  It still seems to be used to create an error message just a\n> few lines below...\n\nYes that is programmer error. Also the final cleanup of the strbuf is missing.\n\n>\n>> +             strbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n>> +             git_config_get_string(sb.buf, &url);\n>> +             if (!url) {\n>> +                     /*\n>> +                      * Only mention uninitialized submodules when its\n>> +                      * path have been specified\n>> +                      */\n>> +                     if (pp->pathspec.nr)\n>> +                             strbuf_addf(err, _(\"Submodule path '%s' not initialized\\n\"\n>> +                                     \"Maybe you want to use 'update --init'?\"), displaypath);\n>> +                     continue;\n>> +             }\n"},{"id":"277354","messageId":"xmqqio25jo1n.fsf@gitster.mtv.corp.google.com","threadId":"41314","inReplyTo":"CAGZ79kZwGPNHeZFcu7-60U8cTZhC8t6TZ5=4ZzBbP+4MrLqRZQ@mail.gmail.com","subject":"Re: [PATCH 4/8] submodule-config: introduce parse_generic_submodule_config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-04T00:51:16Z","receivedAt":"2016-02-04T00:51:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> The same comment as in the footnote for the review on [3/8] applies\n>> here.\n>\n> Ok, so we want to have\n>\n>     [section \"\"]\n>\n> and\n>\n>     [section]\n>\n> be different, such that we could have a submodule with an \"\" name,\n> but still be able to differentiate to the common section which should be\n> applied to all submodules?\n\nIt is not about submodule per-se, but what is the right way to use\nthe parse_config_key() function.  If the caller is interested in\nseeing if the name is two-level or three-level, NULL-ness of the\nsubsection pointer is the right thing to check, not the length of\nthe subsection (set to 0 or positive number if exists--and otherwise\nit is set to 0).\n\nPeople copy & paste existing uses of API functions, and even if for\nthe specific use of your code (i.e. submodule) it is illegal to have\nan empty second level, we shouldn't leaving a bad practice in the\ncode to be imitated.\n"},{"id":"277355","messageId":"xmqqegctjnvm.fsf@gitster.mtv.corp.google.com","threadId":"41314","inReplyTo":"CAGZ79kbeDBf=AfA2RUhkfjwJqJ7pr30xW3UTXqiha_EPpisvnw@mail.gmail.com","subject":"Re: [PATCH 6/8] git submodule update: have a dedicated helper for cloning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-04T00:54:53Z","receivedAt":"2016-02-04T00:54:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Wed, Feb 3, 2016 at 3:24 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> +             if (ce_stage(ce)) {\n>>> +                     if (pp->recursive_prefix)\n>>> +                             strbuf_addf(err, \"Skipping unmerged submodule %s/%s\\n\",\n>>> +                                     pp->recursive_prefix, ce->name);\n>\n> As a side question: Do we care about proper visual directory\n> separators in Windows?\n\nYou know I do not do Windows ;-)  I'll leave the question for others\nto answer (I am trying not to be one of the the only small number of\npeople who review code around here).\n\n> I never run into this BUG after having proper initialization, so maybe it's not\n> worth carrying this code around. (We have many other places where\n> submodule_from_{path, name} is used unchecked, so why would this place\n> be special?)\n\nThat is why I wondered if moudule_list() is a better place to do so.\nThat is where the list of everybody works on come from.\n\n>>> +             /*\n>>> +              * Looking up the url in .git/config.\n>>> +              * We must not fall back to .gitmodules as we only want to process\n>>> +              * configured submodules.\n>>> +              */\n>>> +             strbuf_reset(&sb);\n>>\n>> Doesn't this invalidate displaypath, when pp->recursive_prefix is in\n>> effect?  It still seems to be used to create an error message just a\n>> few lines below...\n>\n> Yes that is programmer error. Also the final cleanup of the strbuf is missing.\n\nYeah, the calling convention to relative_path() might be performant,\nbut I agree that it is not the easiest API function to use correctly.\n"},{"id":"277361","messageId":"20160204031527.GB21105@sigill.intra.peff.net","threadId":"41314","inReplyTo":"xmqqy4b1l7cd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/8] submodule-config: keep update strategy around","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-04T03:15:27Z","receivedAt":"2016-02-04T03:15:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 03, 2016 at 03:09:06PM -0800, Junio C Hamano wrote:\n\n> > +\t} else if (!strcmp(item.buf, \"update\")) {\n> > +\t\tif (!value)\n> > +\t\t\tret = config_error_nonbool(var);\n> > +\t\telse if (!me->overwrite && submodule->update != NULL)\n> > +\t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n> > +\t\t\t\t\t     \"update\");\n> > +\t\telse {\n> > +\t\t\tfree((void *) submodule->update);\n> > +\t\t\tsubmodule->update = xstrdup(value);\n> \n> This is a tangent, but whenever I see us needing to cast the\n> constness away to call free(), it makes me wonder how much value we\n> are getting by marking the field as a pointer to \"const\" in the\n> first place.  With this patch alone, we cannot really see it,\n> because we cannot see how it is used.\n\nI suspect we may have just inherited this from other config code. We\ntypically use a const pointer to declare config strings, because\ngit_config_string() takes a pointer to a const pointer for its\ndestination. And we do that because some strings need BSS defaults, and\nwe cannot assign a string literal to a non-const pointer without a cast\n(and if we did, it is inviting somebody to accidentally free() the\nstring literal).\n\nSo _somebody_ generally has to cast to cover all situations. But here we\nare not using git_config_string() at all, so I think we could get away\nwith a non-const pointer.\n\nAs a tangent to your tangent, another problem with git_config_string()\nand const pointers is that we never free the \"old\" value for a string,\nand we leak it.  It's usually OK in practice because people do not have\nunbounded repetition of their config keys. We don't free because it\nwould be an error to do so for a default-assigned string literal. But\nfor any string variable which defaults to NULL, it would be fine.\n\nIOW, git_config_string (and probably git_config_pathname) could be\nimplemented like this:\n\n  int git_config_string(char **dest, const char *var, const char *value)\n  {\n\tif (!value)\n\t\treturn config_error_nonbool(var);\n\tfree(*dest);\n\t*dest = xstrdup(value);\n\treturn 0;\n  }\n\nand then:\n\n  char *git_foo;\n  ...\n      return git_config_string(&git_foo, var, value);\n\nwould do the right thing. But I'm not sure how much that would ripple\nthrough the code-base. Any string who wants:\n\n  const char *git_foo = \"default\";\n\nwould have to use another function (and keep the leak), or convert its\nuses to treat NULL the same as \"default\". I have a suspicion that _most_\nsites would prefer it the non-const way, and it would be a net win. But\nI think you'd have to replace the function, then convert all of the\nvariables used (which the compiler will helpfully point out to you\nbecause of the type mismatch), and then see how many are broken (which\nthe compiler will also tell you).\n\n-Peff\n"},{"id":"277457","messageId":"CAGZ79kb-G=GHdtcH323LbHQ4V_azif04eHVuf8Z3gCeb-e=iKQ@mail.gmail.com","threadId":"41314","inReplyTo":"xmqqegctjnvm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 6/8] git submodule update: have a dedicated helper for cloning","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-04T20:22:28Z","receivedAt":"2016-02-04T20:22:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Feb 3, 2016 at 4:54 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> On Wed, Feb 3, 2016 at 3:24 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Stefan Beller <sbeller@google.com> writes:\n>>>\n>>>> +             if (ce_stage(ce)) {\n>>>> +                     if (pp->recursive_prefix)\n>>>> +                             strbuf_addf(err, \"Skipping unmerged submodule %s/%s\\n\",\n>>>> +                                     pp->recursive_prefix, ce->name);\n>>\n>> As a side question: Do we care about proper visual directory\n>> separators in Windows?\n>\n> You know I do not do Windows ;-)  I'll leave the question for others\n> to answer (I am trying not to be one of the the only small number of\n> people who review code around here).\n>\n>> I never run into this BUG after having proper initialization, so maybe it's not\n>> worth carrying this code around. (We have many other places where\n>> submodule_from_{path, name} is used unchecked, so why would this place\n>> be special?)\n>\n> That is why I wondered if moudule_list() is a better place to do so.\n> That is where the list of everybody works on come from.\n\nI do not think that is a better place as not every consumer of module_list\n(and module_list_compute as the nested function) will need to use the\nsubmodule caching API. So these consumers are not interested in possible\nbugs in the submodule cache API nor do they want the performance hit\nwhich comes from checking unrelated stuff in there.\n\nAs said, I only saw this bug when the cache was not initialized properly,\nand then such a bug is to be expected. I'd rather remove it in a reroll.\n"},{"id":"277465","messageId":"xmqq4mdogmvh.fsf@gitster.mtv.corp.google.com","threadId":"41314","inReplyTo":"CAGZ79kb-G=GHdtcH323LbHQ4V_azif04eHVuf8Z3gCeb-e=iKQ@mail.gmail.com","subject":"Re: [PATCH 6/8] git submodule update: have a dedicated helper for cloning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-04T21:57:06Z","receivedAt":"2016-02-04T21:57:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> I do not think that is a better place as not every consumer of module_list\n> (and module_list_compute as the nested function) will need to use the\n> submodule caching API.\n\nAh, if that is the case, then OK.\n\nI was imagining that module-list may someday would return a list of\n\"struct\" instances each of which describes a submodule and has .name\nand .path fields, or something, instead of just being a \"here is the\npath to the submodule, go look it up yourself\" interface.  But we\nare not there (at least not yet--and I am not saying we agreed that\nwe would eventually want to go there)\n"}]}