[PATCHv14 0/7] Expose submodule parallelism to the user

[PATCHv14 0/7] Expose submodule parallelism to the user

[Stefan Beller]

Thanks Junio, Jeff and Eric for reviewing once again!

* fixes as proposed by Junio for readability in parse_submodule_update_strategy
* Do not leak the `url`, as found by Jeff.
  (I dug into the code of argv_array_pushl and you're obviously correct)

Thanks,
Stefan

Interdiff to v13:

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 65bdc14..c435c53 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -356,13 +356,11 @@ static int update_clone_inspect_next_task(struct child_process *cp,
 
                argv_array_pushl(&cp->args, "--path", sub->path, NULL);
                argv_array_pushl(&cp->args, "--name", sub->name, NULL);
-               argv_array_pushl(&cp->args, "--url", strdup(url), NULL);
+               argv_array_pushl(&cp->args, "--url", url, NULL);
                if (pp->reference)
                        argv_array_push(&cp->args, pp->reference);
                if (pp->depth)
                        argv_array_push(&cp->args, pp->depth);
-
-
        }
 
 cleanup:
diff --git a/submodule.c b/submodule.c
index b54d92d..051f722 100644
--- a/submodule.c
+++ b/submodule.c
@@ -219,7 +219,7 @@ void gitmodules_config(void)
 int parse_submodule_update_strategy(const char *value,
                struct submodule_update_strategy *dst)
 {
-       free((void*)dst->command);
+       free((void *)dst->command);
        dst->command = NULL;
        if (!strcmp(value, "none"))
                dst->type = SM_UPDATE_NONE;
@@ -229,9 +229,9 @@ int parse_submodule_update_strategy(const char *value,
                dst->type = SM_UPDATE_REBASE;
        else if (!strcmp(value, "merge"))
                dst->type = SM_UPDATE_MERGE;
-       else if (value[0] == '!') {
+       else if (skip_prefix(value, "!", &value)) {
                dst->type = SM_UPDATE_COMMAND;
-               dst->command = xstrdup(value + 1);
+               dst->command = xstrdup(value);
        } else
                return -1;
        return 0;

Stefan Beller (7):
  submodule-config: keep update strategy around
  submodule-config: drop check against NULL
  fetching submodules: respect `submodule.fetchJobs` config option
  submodule update: direct error message to stderr
  git submodule update: have a dedicated helper for cloning
  submodule update: expose parallelism to the user
  clone: allow an explicit argument for parallel submodule clones

 Documentation/config.txt        |   6 +
 Documentation/git-clone.txt     |   6 +-
 Documentation/git-submodule.txt |   7 +-
 builtin/clone.c                 |  19 +++-
 builtin/fetch.c                 |   2 +-
 builtin/submodule--helper.c     | 237 ++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh                |  54 ++++-----
 submodule-config.c              |  19 +++-
 submodule-config.h              |   2 +
 submodule.c                     |  37 ++++++-
 submodule.h                     |  18 +++
 t/t5526-fetch-submodules.sh     |  14 +++
 t/t7400-submodule-basic.sh      |   4 +-
 t/t7406-submodule-update.sh     |  27 +++++
 14 files changed, 403 insertions(+), 49 deletions(-)

-- 
2.7.0.rc0.34.ga06e0b3.dirty

[PATCHv14 1/7] submodule-config: keep update strategy around

[Stefan Beller]

Currently submodule.<name>.update is only handled by git-submodule.sh.
C code will start to need to make use of that value as more of the
functionality of git-submodule.sh moves into library code in C.

Add the update field to 'struct submodule' and populate it so it can
be read as sm->update or from sm->update_command.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 submodule-config.c | 13 +++++++++++++
 submodule-config.h |  2 ++
 submodule.c        | 21 +++++++++++++++++++++
 submodule.h        | 16 ++++++++++++++++
 4 files changed, 52 insertions(+)

diff --git a/submodule-config.c b/submodule-config.c
index afe0ea8..a5cd2ee 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -59,6 +59,7 @@ static void free_one_config(struct submodule_entry *entry)
 {
 	free((void *) entry->config->path);
 	free((void *) entry->config->name);
+	free((void *) entry->config->update_strategy.command);
 	free(entry->config);
 }
 
@@ -194,6 +195,8 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
 
 	submodule->path = NULL;
 	submodule->url = NULL;
+	submodule->update_strategy.type = SM_UPDATE_UNSPECIFIED;
+	submodule->update_strategy.command = NULL;
 	submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
 	submodule->ignore = NULL;
 
@@ -311,6 +314,16 @@ static int parse_config(const char *var, const char *value, void *data)
 			free((void *) submodule->url);
 			submodule->url = xstrdup(value);
 		}
+	} else if (!strcmp(item.buf, "update")) {
+		if (!value)
+			ret = config_error_nonbool(var);
+		else if (!me->overwrite &&
+			 submodule->update_strategy.type != SM_UPDATE_UNSPECIFIED)
+			warn_multiple_config(me->commit_sha1, submodule->name,
+					     "update");
+		else if (parse_submodule_update_strategy(value,
+			 &submodule->update_strategy) < 0)
+				die(_("invalid value for %s"), var);
 	}
 
 	strbuf_release(&name);
diff --git a/submodule-config.h b/submodule-config.h
index 9061e4e..092ebfc 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -2,6 +2,7 @@
 #define SUBMODULE_CONFIG_CACHE_H
 
 #include "hashmap.h"
+#include "submodule.h"
 #include "strbuf.h"
 
 /*
@@ -14,6 +15,7 @@ struct submodule {
 	const char *url;
 	int fetch_recurse;
 	const char *ignore;
+	struct submodule_update_strategy update_strategy;
 	/* the sha1 blob id of the responsible .gitmodules file */
 	unsigned char gitmodules_sha1[20];
 };
diff --git a/submodule.c b/submodule.c
index b83939c..b38dd51 100644
--- a/submodule.c
+++ b/submodule.c
@@ -210,6 +210,27 @@ void gitmodules_config(void)
 	}
 }
 
+int parse_submodule_update_strategy(const char *value,
+		struct submodule_update_strategy *dst)
+{
+	free((void *)dst->command);
+	dst->command = NULL;
+	if (!strcmp(value, "none"))
+		dst->type = SM_UPDATE_NONE;
+	else if (!strcmp(value, "checkout"))
+		dst->type = SM_UPDATE_CHECKOUT;
+	else if (!strcmp(value, "rebase"))
+		dst->type = SM_UPDATE_REBASE;
+	else if (!strcmp(value, "merge"))
+		dst->type = SM_UPDATE_MERGE;
+	else if (skip_prefix(value, "!", &value)) {
+		dst->type = SM_UPDATE_COMMAND;
+		dst->command = xstrdup(value);
+	} else
+		return -1;
+	return 0;
+}
+
 void handle_ignore_submodules_arg(struct diff_options *diffopt,
 				  const char *arg)
 {
diff --git a/submodule.h b/submodule.h
index cbc0003..3464500 100644
--- a/submodule.h
+++ b/submodule.h
@@ -13,6 +13,20 @@ enum {
 	RECURSE_SUBMODULES_ON = 2
 };
 
+enum submodule_update_type {
+	SM_UPDATE_UNSPECIFIED = 0,
+	SM_UPDATE_CHECKOUT,
+	SM_UPDATE_REBASE,
+	SM_UPDATE_MERGE,
+	SM_UPDATE_NONE,
+	SM_UPDATE_COMMAND
+};
+
+struct submodule_update_strategy {
+	enum submodule_update_type type;
+	const char *command;
+};
+
 int is_staging_gitmodules_ok(void);
 int update_path_in_gitmodules(const char *oldpath, const char *newpath);
 int remove_path_from_gitmodules(const char *path);
@@ -21,6 +35,8 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
 		const char *path);
 int submodule_config(const char *var, const char *value, void *cb);
 void gitmodules_config(void);
+int parse_submodule_update_strategy(const char *value,
+		struct submodule_update_strategy *dst);
 void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
 void show_submodule_summary(FILE *f, const char *path,
 		const char *line_prefix,
-- 
2.7.0.rc0.34.ga06e0b3.dirty

[PATCHv14 2/7] submodule-config: drop check against NULL

[Stefan Beller]

Adhere to the common coding style of Git and not check explicitly
for NULL throughout the file. There are still other occurrences in the
code base but that is usually inside of conditions with side effects.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 submodule-config.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index a5cd2ee..9fa2269 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -267,7 +267,7 @@ static int parse_config(const char *var, const char *value, void *data)
 	if (!strcmp(item.buf, "path")) {
 		if (!value)
 			ret = config_error_nonbool(var);
-		else if (!me->overwrite && submodule->path != NULL)
+		else if (!me->overwrite && submodule->path)
 			warn_multiple_config(me->commit_sha1, submodule->name,
 					"path");
 		else {
@@ -291,7 +291,7 @@ static int parse_config(const char *var, const char *value, void *data)
 	} else if (!strcmp(item.buf, "ignore")) {
 		if (!value)
 			ret = config_error_nonbool(var);
-		else if (!me->overwrite && submodule->ignore != NULL)
+		else if (!me->overwrite && submodule->ignore)
 			warn_multiple_config(me->commit_sha1, submodule->name,
 					"ignore");
 		else if (strcmp(value, "untracked") &&
@@ -307,7 +307,7 @@ static int parse_config(const char *var, const char *value, void *data)
 	} else if (!strcmp(item.buf, "url")) {
 		if (!value) {
 			ret = config_error_nonbool(var);
-		} else if (!me->overwrite && submodule->url != NULL) {
+		} else if (!me->overwrite && submodule->url) {
 			warn_multiple_config(me->commit_sha1, submodule->name,
 					"url");
 		} else {
-- 
2.7.0.rc0.34.ga06e0b3.dirty

[PATCHv14 3/7] fetching submodules: respect `submodule.fetchJobs` config option

[Stefan Beller]

This allows to configure fetching and updating in parallel
without having the command line option.

This moved the responsibility to determine how many parallel processes
to start from builtin/fetch to submodule.c as we need a way to communicate
"The user did not specify the number of parallel processes in the command
line options" in the builtin fetch. The submodule code takes care of
the precedence (CLI > config > default).

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt    |  6 ++++++
 builtin/fetch.c             |  2 +-
 submodule.c                 | 16 +++++++++++++++-
 submodule.h                 |  2 ++
 t/t5526-fetch-submodules.sh | 14 ++++++++++++++
 5 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2d06b11..3b02732 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2646,6 +2646,12 @@ submodule.<name>.ignore::
 	"--ignore-submodules" option. The 'git submodule' commands are not
 	affected by this setting.
 
+submodule.fetchJobs::
+	Specifies how many submodules are fetched/cloned at the same time.
+	A positive integer allows up to that number of submodules fetched
+	in parallel. A value of 0 will give some reasonable default.
+	If unset, it defaults to 1.
+
 tag.sort::
 	This variable controls the sort ordering of tags when displayed by
 	linkgit:git-tag[1]. Without the "--sort=<value>" option provided, the
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 586840d..5aa1c2d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -37,7 +37,7 @@ static int prune = -1; /* unspecified */
 static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity;
 static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int tags = TAGS_DEFAULT, unshallow, update_shallow;
-static int max_children = 1;
+static int max_children = -1;
 static const char *depth;
 static const char *upload_pack;
 static struct strbuf default_rla = STRBUF_INIT;
diff --git a/submodule.c b/submodule.c
index b38dd51..051f722 100644
--- a/submodule.c
+++ b/submodule.c
@@ -15,6 +15,7 @@
 #include "thread-utils.h"
 
 static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
+static int parallel_jobs = 1;
 static struct string_list changed_submodule_paths;
 static int initialized_fetch_ref_tips;
 static struct sha1_array ref_tips_before_fetch;
@@ -169,7 +170,12 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
 
 int submodule_config(const char *var, const char *value, void *cb)
 {
-	if (starts_with(var, "submodule."))
+	if (!strcmp(var, "submodule.fetchjobs")) {
+		parallel_jobs = git_config_int(var, value);
+		if (parallel_jobs < 0)
+			die(_("negative values not allowed for submodule.fetchJobs"));
+		return 0;
+	} else if (starts_with(var, "submodule."))
 		return parse_submodule_config_option(var, value);
 	else if (!strcmp(var, "fetch.recursesubmodules")) {
 		config_fetch_recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);
@@ -772,6 +778,9 @@ int fetch_populated_submodules(const struct argv_array *options,
 	argv_array_push(&spf.args, "--recurse-submodules-default");
 	/* default value, "--submodule-prefix" and its value are added later */
 
+	if (max_parallel_jobs < 0)
+		max_parallel_jobs = parallel_jobs;
+
 	calculate_changed_submodule_paths();
 	run_processes_parallel(max_parallel_jobs,
 			       get_next_submodule,
@@ -1118,3 +1127,8 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
 	strbuf_release(&rel_path);
 	free((void *)real_work_tree);
 }
+
+int parallel_submodules(void)
+{
+	return parallel_jobs;
+}
diff --git a/submodule.h b/submodule.h
index 3464500..3166608 100644
--- a/submodule.h
+++ b/submodule.h
@@ -26,6 +26,7 @@ struct submodule_update_strategy {
 	enum submodule_update_type type;
 	const char *command;
 };
+#define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
 
 int is_staging_gitmodules_ok(void);
 int update_path_in_gitmodules(const char *oldpath, const char *newpath);
@@ -57,5 +58,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam
 		struct string_list *needs_pushing);
 int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
 void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
+int parallel_submodules(void);
 
 #endif
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 1241146..954d0e4 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -471,4 +471,18 @@ test_expect_success "don't fetch submodule when newly recorded commits are alrea
 	test_i18ncmp expect.err actual.err
 '
 
+test_expect_success 'fetching submodules respects parallel settings' '
+	git config fetch.recurseSubmodules true &&
+	(
+		cd downstream &&
+		GIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 &&
+		grep "7 tasks" trace.out &&
+		git config submodule.fetchJobs 8 &&
+		GIT_TRACE=$(pwd)/trace.out git fetch &&
+		grep "8 tasks" trace.out &&
+		GIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&
+		grep "9 tasks" trace.out
+	)
+'
+
 test_done
-- 
2.7.0.rc0.34.ga06e0b3.dirty

[PATCHv14 4/7] submodule update: direct error message to stderr

[Stefan Beller]

Reroute the error message for specified but initialized submodules
to stderr instead of stdout.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-submodule.sh           | 4 ++--
 t/t7400-submodule-basic.sh | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 9bc5c5f..9ee86d4 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -693,7 +693,7 @@ cmd_update()
 
 		if test "$update_module" = "none"
 		then
-			echo "Skipping submodule '$displaypath'"
+			echo >&2 "Skipping submodule '$displaypath'"
 			continue
 		fi
 
@@ -702,7 +702,7 @@ cmd_update()
 			# Only mention uninitialized submodules when its
 			# path have been specified
 			test "$#" != "0" &&
-			say "$(eval_gettext "Submodule path '\$displaypath' not initialized
+			say >&2 "$(eval_gettext "Submodule path '\$displaypath' not initialized
 Maybe you want to use 'update --init'?")"
 			continue
 		fi
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 540771c..5991e3c 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -462,7 +462,7 @@ test_expect_success 'update --init' '
 	git config --remove-section submodule.example &&
 	test_must_fail git config submodule.example.url &&
 
-	git submodule update init > update.out &&
+	git submodule update init 2> update.out &&
 	cat update.out &&
 	test_i18ngrep "not initialized" update.out &&
 	test_must_fail git rev-parse --resolve-git-dir init/.git &&
@@ -480,7 +480,7 @@ test_expect_success 'update --init from subdirectory' '
 	mkdir -p sub &&
 	(
 		cd sub &&
-		git submodule update ../init >update.out &&
+		git submodule update ../init 2>update.out &&
 		cat update.out &&
 		test_i18ngrep "not initialized" update.out &&
 		test_must_fail git rev-parse --resolve-git-dir ../init/.git &&
-- 
2.7.0.rc0.34.ga06e0b3.dirty

[PATCHv14 5/7] git submodule update: have a dedicated helper for cloning

[Stefan Beller]

This introduces a new helper function in git submodule--helper
which takes care of cloning all submodules, which we want to
parallelize eventually.

Some tests (such as empty URL, update_mode=none) are required in the
helper to make the decision for cloning. These checks have been
moved into the C function as well (no need to repeat them in the
shell script).

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/submodule--helper.c | 229 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  45 +++------
 2 files changed, 240 insertions(+), 34 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f4c3eff..c356aaf 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -255,6 +255,234 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct submodule_update_clone {
+	/* states */
+	int count;
+	int print_unmatched;
+	/* configuration */
+	int quiet;
+	const char *reference;
+	const char *depth;
+	const char *recursive_prefix;
+	const char *prefix;
+	struct module_list list;
+	struct string_list projectlines;
+	struct submodule_update_strategy update;
+	struct pathspec pathspec;
+};
+#define SUBMODULE_UPDATE_CLONE_INIT {0, 0, 0, NULL, NULL, NULL, NULL, MODULE_LIST_INIT, STRING_LIST_INIT_DUP, SUBMODULE_UPDATE_STRATEGY_INIT}
+
+static int update_clone_inspect_next_task(struct child_process *cp,
+					  struct strbuf *err,
+					  struct submodule_update_clone *pp,
+					  void **pp_task_cb,
+					  const struct cache_entry *ce)
+{
+	const struct submodule *sub = NULL;
+	struct strbuf displaypath_sb = STRBUF_INIT;
+	struct strbuf sb = STRBUF_INIT;
+	const char *displaypath = NULL;
+	char *url = NULL;
+	int needs_cloning = 0;
+
+	if (ce_stage(ce)) {
+		if (pp->recursive_prefix)
+			strbuf_addf(err, "Skipping unmerged submodule %s/%s\n",
+				    pp->recursive_prefix, ce->name);
+		else
+			strbuf_addf(err, "Skipping unmerged submodule %s\n",
+				    ce->name);
+		goto cleanup;
+	}
+
+	sub = submodule_from_path(null_sha1, ce->name);
+
+	if (pp->recursive_prefix)
+		displaypath = relative_path(pp->recursive_prefix,
+					    ce->name, &displaypath_sb);
+	else
+		displaypath = ce->name;
+
+	if (pp->update.type == SM_UPDATE_NONE ||
+	    (pp->update.type == SM_UPDATE_UNSPECIFIED &&
+	     sub->update_strategy.type == SM_UPDATE_NONE)) {
+		strbuf_addf(err, "Skipping submodule '%s'\n",
+			    displaypath);
+		goto cleanup;
+	}
+
+	/*
+	 * Looking up the url in .git/config.
+	 * We must not fall back to .gitmodules as we only want
+	 * to process configured submodules.
+	 */
+	strbuf_reset(&sb);
+	strbuf_addf(&sb, "submodule.%s.url", sub->name);
+	git_config_get_string(sb.buf, &url);
+	if (!url) {
+		/*
+		 * Only mention uninitialized submodules when its
+		 * path have been specified
+		 */
+		if (pp->pathspec.nr)
+			strbuf_addf(err, _("Submodule path '%s' not initialized\n"
+				    "Maybe you want to use 'update --init'?"),
+				    displaypath);
+		goto cleanup;
+	}
+
+	strbuf_reset(&sb);
+	strbuf_addf(&sb, "%s/.git", ce->name);
+	needs_cloning = !file_exists(sb.buf);
+
+	strbuf_reset(&sb);
+	strbuf_addf(&sb, "%06o %s %d %d\t%s\n", ce->ce_mode,
+			sha1_to_hex(ce->sha1), ce_stage(ce),
+			needs_cloning, ce->name);
+	string_list_append(&pp->projectlines, sb.buf);
+
+	if (needs_cloning) {
+		cp->git_cmd = 1;
+		cp->no_stdin = 1;
+		cp->stdout_to_stderr = 1;
+		cp->err = -1;
+		argv_array_push(&cp->args, "submodule--helper");
+		argv_array_push(&cp->args, "clone");
+		if (pp->quiet)
+			argv_array_push(&cp->args, "--quiet");
+
+		if (pp->prefix)
+			argv_array_pushl(&cp->args, "--prefix", pp->prefix, NULL);
+
+		argv_array_pushl(&cp->args, "--path", sub->path, NULL);
+		argv_array_pushl(&cp->args, "--name", sub->name, NULL);
+		argv_array_pushl(&cp->args, "--url", url, NULL);
+		if (pp->reference)
+			argv_array_push(&cp->args, pp->reference);
+		if (pp->depth)
+			argv_array_push(&cp->args, pp->depth);
+	}
+
+cleanup:
+	free(url);
+	strbuf_reset(&displaypath_sb);
+	strbuf_reset(&sb);
+
+	return needs_cloning;
+}
+
+static int update_clone_get_next_task(struct child_process *cp,
+				      struct strbuf *err,
+				      void *pp_cb,
+				      void **pp_task_cb)
+{
+	struct submodule_update_clone *pp = pp_cb;
+
+	for (; pp->count < pp->list.nr; pp->count++) {
+		const struct cache_entry *ce = pp->list.entries[pp->count];
+		if (update_clone_inspect_next_task(cp, err, pp,
+						   pp_task_cb, ce)) {
+			pp->count++;
+			return 1;
+		}
+	}
+	return 0;
+}
+
+static int update_clone_start_failure(struct child_process *cp,
+				      struct strbuf *err,
+				      void *pp_cb,
+				      void *pp_task_cb)
+{
+	struct submodule_update_clone *pp = pp_cb;
+
+	strbuf_addf(err, "error when starting a child process");
+	pp->print_unmatched = 1;
+
+	return 1;
+}
+
+static int update_clone_task_finished(int result,
+				      struct child_process *cp,
+				      struct strbuf *err,
+				      void *pp_cb,
+				      void *pp_task_cb)
+{
+	struct submodule_update_clone *pp = pp_cb;
+
+	if (!result) {
+		return 0;
+	} else {
+		strbuf_addf(err, "error in one child process");
+		pp->print_unmatched = 1;
+		return 1;
+	}
+}
+
+static int update_clone(int argc, const char **argv, const char *prefix)
+{
+	const char *update = NULL;
+	struct string_list_item *item;
+	struct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;
+
+	struct option module_list_options[] = {
+		OPT_STRING(0, "prefix", &prefix,
+			   N_("path"),
+			   N_("path into the working tree")),
+		OPT_STRING(0, "recursive_prefix", &pp.recursive_prefix,
+			   N_("path"),
+			   N_("path into the working tree, across nested "
+			      "submodule boundaries")),
+		OPT_STRING(0, "update", &update,
+			   N_("string"),
+			   N_("update command for submodules")),
+		OPT_STRING(0, "reference", &pp.reference, "<repository>",
+			   N_("Use the local reference repository "
+			      "instead of a full clone")),
+		OPT_STRING(0, "depth", &pp.depth, "<depth>",
+			   N_("Create a shallow clone truncated to the "
+			      "specified number of revisions")),
+		OPT__QUIET(&pp.quiet, N_("do't print cloning progress")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper list [--prefix=<path>] [<path>...]"),
+		NULL
+	};
+	pp.prefix = prefix;
+
+	argc = parse_options(argc, argv, prefix, module_list_options,
+			     git_submodule_helper_usage, 0);
+
+	if (update)
+		if (parse_submodule_update_strategy(update, &pp.update) < 0)
+			die(_("bad value for update parameter"));
+
+	if (module_list_compute(argc, argv, prefix, &pp.pathspec, &pp.list) < 0) {
+		printf("#unmatched\n");
+		return 1;
+	}
+
+	gitmodules_config();
+	/* Overlay the parsed .gitmodules file with .git/config */
+	git_config(submodule_config, NULL);
+	run_processes_parallel(1, update_clone_get_next_task,
+				  update_clone_start_failure,
+				  update_clone_task_finished,
+				  &pp);
+
+	if (pp.print_unmatched) {
+		printf("#unmatched\n");
+		return 1;
+	}
+
+	for_each_string_list_item(item, &pp.projectlines)
+		utf8_fprintf(stdout, "%s", item->string);
+
+	return 0;
+}
+
 struct cmd_struct {
 	const char *cmd;
 	int (*fn)(int, const char **, const char *);
@@ -264,6 +492,7 @@ static struct cmd_struct commands[] = {
 	{"list", module_list},
 	{"name", module_name},
 	{"clone", module_clone},
+	{"update-clone", update_clone}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 9ee86d4..9f554fb 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -664,17 +664,18 @@ cmd_update()
 		cmd_init "--" "$@" || return
 	fi
 
-	cloned_modules=
-	git submodule--helper list --prefix "$wt_prefix" "$@" | {
+	git submodule--helper update-clone ${GIT_QUIET:+--quiet} \
+		${wt_prefix:+--prefix "$wt_prefix"} \
+		${prefix:+--recursive_prefix "$prefix"} \
+		${update:+--update "$update"} \
+		${reference:+--reference "$reference"} \
+		${depth:+--depth "$depth"} \
+		"$@" | {
 	err=
-	while read mode sha1 stage sm_path
+	while read mode sha1 stage just_cloned sm_path
 	do
 		die_if_unmatched "$mode"
-		if test "$stage" = U
-		then
-			echo >&2 "Skipping unmerged submodule $prefix$sm_path"
-			continue
-		fi
+
 		name=$(git submodule--helper name "$sm_path") || exit
 		url=$(git config submodule."$name".url)
 		branch=$(get_submodule_config "$name" branch master)
@@ -691,27 +692,10 @@ cmd_update()
 
 		displaypath=$(relative_path "$prefix$sm_path")
 
-		if test "$update_module" = "none"
-		then
-			echo >&2 "Skipping submodule '$displaypath'"
-			continue
-		fi
-
-		if test -z "$url"
-		then
-			# Only mention uninitialized submodules when its
-			# path have been specified
-			test "$#" != "0" &&
-			say >&2 "$(eval_gettext "Submodule path '\$displaypath' not initialized
-Maybe you want to use 'update --init'?")"
-			continue
-		fi
-
-		if ! test -d "$sm_path"/.git && ! test -f "$sm_path"/.git
+		if test $just_cloned -eq 1
 		then
-			git submodule--helper clone ${GIT_QUIET:+--quiet} --prefix "$prefix" --path "$sm_path" --name "$name" --url "$url" "$reference" "$depth" || exit
-			cloned_modules="$cloned_modules;$name"
 			subsha1=
+			update_module=checkout
 		else
 			subsha1=$(clear_local_git_env; cd "$sm_path" &&
 				git rev-parse --verify HEAD) ||
@@ -751,13 +735,6 @@ Maybe you want to use 'update --init'?")"
 				die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
 			fi
 
-			# Is this something we just cloned?
-			case ";$cloned_modules;" in
-			*";$name;"*)
-				# then there is no local change to integrate
-				update_module=checkout ;;
-			esac
-
 			must_die_on_failure=
 			case "$update_module" in
 			checkout)
-- 
2.7.0.rc0.34.ga06e0b3.dirty

[PATCHv14 6/7] submodule update: expose parallelism to the user

[Stefan Beller]

Expose possible parallelism either via the "--jobs" CLI parameter or
the "submodule.fetchJobs" setting.

By having the variable initialized to -1, we make sure 0 can be passed
into the parallel processing machine, which will then pick as many parallel
workers as there are CPUs.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-submodule.txt |  7 ++++++-
 builtin/submodule--helper.c     | 16 ++++++++++++----
 git-submodule.sh                |  9 +++++++++
 t/t7406-submodule-update.sh     | 12 ++++++++++++
 4 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 1572f05..13adebf 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -16,7 +16,7 @@ SYNOPSIS
 'git submodule' [--quiet] deinit [-f|--force] [--] <path>...
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
 	      [-f|--force] [--rebase|--merge] [--reference <repository>]
-	      [--depth <depth>] [--recursive] [--] [<path>...]
+	      [--depth <depth>] [--recursive] [--jobs <n>] [--] [<path>...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]
 	      [commit] [--] [<path>...]
 'git submodule' [--quiet] foreach [--recursive] <command>
@@ -377,6 +377,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` options carefully.
 	clone with a history truncated to the specified number of revisions.
 	See linkgit:git-clone[1]
 
+-j <n>::
+--jobs <n>::
+	This option is only valid for the update command.
+	Clone new submodules in parallel with as many jobs.
+	Defaults to the `submodule.fetchJobs` option.
 
 <path>...::
 	Paths to submodule(s). When specified this will restrict the command
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c356aaf..c435c53 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -422,6 +422,7 @@ static int update_clone_task_finished(int result,
 static int update_clone(int argc, const char **argv, const char *prefix)
 {
 	const char *update = NULL;
+	int max_jobs = -1;
 	struct string_list_item *item;
 	struct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;
 
@@ -442,6 +443,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "depth", &pp.depth, "<depth>",
 			   N_("Create a shallow clone truncated to the "
 			      "specified number of revisions")),
+		OPT_INTEGER('j', "jobs", &max_jobs,
+			    N_("parallel jobs")),
 		OPT__QUIET(&pp.quiet, N_("do't print cloning progress")),
 		OPT_END()
 	};
@@ -467,10 +470,15 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	gitmodules_config();
 	/* Overlay the parsed .gitmodules file with .git/config */
 	git_config(submodule_config, NULL);
-	run_processes_parallel(1, update_clone_get_next_task,
-				  update_clone_start_failure,
-				  update_clone_task_finished,
-				  &pp);
+
+	if (max_jobs < 0)
+		max_jobs = parallel_submodules();
+
+	run_processes_parallel(max_jobs,
+			       update_clone_get_next_task,
+			       update_clone_start_failure,
+			       update_clone_task_finished,
+			       &pp);
 
 	if (pp.print_unmatched) {
 		printf("#unmatched\n");
diff --git a/git-submodule.sh b/git-submodule.sh
index 9f554fb..10c5af9 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -645,6 +645,14 @@ cmd_update()
 		--depth=*)
 			depth=$1
 			;;
+		-j|--jobs)
+			case "$2" in '') usage ;; esac
+			jobs="--jobs=$2"
+			shift
+			;;
+		--jobs=*)
+			jobs=$1
+			;;
 		--)
 			shift
 			break
@@ -670,6 +678,7 @@ cmd_update()
 		${update:+--update "$update"} \
 		${reference:+--reference "$reference"} \
 		${depth:+--depth "$depth"} \
+		${jobs:+$jobs} \
 		"$@" | {
 	err=
 	while read mode sha1 stage just_cloned sm_path
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index dda3929..7fd5142 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops module name before recur
 	 test_i18ngrep "Submodule path .deeper/submodule/subsubmodule.: checked out" actual
 	)
 '
+
+test_expect_success 'submodule update can be run in parallel' '
+	(cd super2 &&
+	 GIT_TRACE=$(pwd)/trace.out git submodule update --jobs 7 &&
+	 grep "7 tasks" trace.out &&
+	 git config submodule.fetchJobs 8 &&
+	 GIT_TRACE=$(pwd)/trace.out git submodule update &&
+	 grep "8 tasks" trace.out &&
+	 GIT_TRACE=$(pwd)/trace.out git submodule update --jobs 9 &&
+	 grep "9 tasks" trace.out
+	)
+'
 test_done
-- 
2.7.0.rc0.34.ga06e0b3.dirty

[PATCHv14 7/7] clone: allow an explicit argument for parallel submodule clones

[Stefan Beller]

Just pass it along to "git submodule update", which may pick reasonable
defaults if you don't specify an explicit number.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-clone.txt |  6 +++++-
 builtin/clone.c             | 19 +++++++++++++------
 t/t7406-submodule-update.sh | 15 +++++++++++++++
 3 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 6bf000d..6db7b6d 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -14,7 +14,7 @@ SYNOPSIS
 	  [-o <name>] [-b <name>] [-u <upload-pack>] [--reference <repository>]
 	  [--dissociate] [--separate-git-dir <git dir>]
 	  [--depth <depth>] [--[no-]single-branch]
-	  [--recursive | --recurse-submodules] [--] <repository>
+	  [--recursive | --recurse-submodules] [--jobs <n>] [--] <repository>
 	  [<directory>]
 
 DESCRIPTION
@@ -221,6 +221,10 @@ objects from the source repository into a pack in the cloned repository.
 	The result is Git repository can be separated from working
 	tree.
 
+-j <n>::
+--jobs <n>::
+	The number of submodules fetched at the same time.
+	Defaults to the `submodule.fetchJobs` option.
 
 <repository>::
 	The (possibly remote) repository to clone from.  See the
diff --git a/builtin/clone.c b/builtin/clone.c
index a0b3cd9..b004fb4 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,6 +50,7 @@ static int option_progress = -1;
 static struct string_list option_config;
 static struct string_list option_reference;
 static int option_dissociate;
+static int max_jobs = -1;
 
 static struct option builtin_clone_options[] = {
 	OPT__VERBOSITY(&option_verbosity),
@@ -72,6 +73,8 @@ static struct option builtin_clone_options[] = {
 		    N_("initialize submodules in the clone")),
 	OPT_BOOL(0, "recurse-submodules", &option_recursive,
 		    N_("initialize submodules in the clone")),
+	OPT_INTEGER('j', "jobs", &max_jobs,
+		    N_("number of submodules cloned in parallel")),
 	OPT_STRING(0, "template", &option_template, N_("template-directory"),
 		   N_("directory from which templates will be used")),
 	OPT_STRING_LIST(0, "reference", &option_reference, N_("repo"),
@@ -95,10 +98,6 @@ static struct option builtin_clone_options[] = {
 	OPT_END()
 };
 
-static const char *argv_submodule[] = {
-	"submodule", "update", "--init", "--recursive", NULL
-};
-
 static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)
 {
 	static char *suffix[] = { "/.git", "", ".git/.git", ".git" };
@@ -724,8 +723,16 @@ static int checkout(void)
 	err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
 			   sha1_to_hex(sha1), "1", NULL);
 
-	if (!err && option_recursive)
-		err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
+	if (!err && option_recursive) {
+		struct argv_array args = ARGV_ARRAY_INIT;
+		argv_array_pushl(&args, "submodule", "update", "--init", "--recursive", NULL);
+
+		if (max_jobs != -1)
+			argv_array_pushf(&args, "--jobs=%d", max_jobs);
+
+		err = run_command_v_opt(args.argv, RUN_GIT_CMD);
+		argv_array_clear(&args);
+	}
 
 	return err;
 }
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 7fd5142..090891e 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -786,4 +786,19 @@ test_expect_success 'submodule update can be run in parallel' '
 	 grep "9 tasks" trace.out
 	)
 '
+
+test_expect_success 'git clone passes the parallel jobs config on to submodules' '
+	test_when_finished "rm -rf super4" &&
+	GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 . super4 &&
+	grep "7 tasks" trace.out &&
+	rm -rf super4 &&
+	git config --global submodule.fetchJobs 8 &&
+	GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules . super4 &&
+	grep "8 tasks" trace.out &&
+	rm -rf super4 &&
+	GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 9 . super4 &&
+	grep "9 tasks" trace.out &&
+	rm -rf super4
+'
+
 test_done
-- 
2.7.0.rc0.34.ga06e0b3.dirty

Re: [PATCHv14 0/7] Expose submodule parallelism to the user

[Junio C Hamano]

Looks good.  I didn't notice these unnecessary blank lines while
reading the previous rounds, and it is good to see them go.

Let's wait for a few days and merge them to 'next'.  David's ref
backend topic textually depends on this, and we'd better give it a
solid foundation to build on soon.

Re: [PATCHv14 5/7] git submodule update: have a dedicated helper for cloning

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> This introduces a new helper function in git submodule--helper
> which takes care of cloning all submodules, which we want to
> parallelize eventually.

Patches 1-4 are still
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

I still have trouble reading this patch (patch 5).  Some musing below
to figure out what could change to make it more readable (perhaps in a
patch on top).

[...]
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -255,6 +255,234 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +struct submodule_update_clone {
> +	/* states */
> +	int count;
> +	int print_unmatched;
> +	/* configuration */
> +	int quiet;
> +	const char *reference;
> +	const char *depth;
> +	const char *recursive_prefix;
> +	const char *prefix;
> +	struct module_list list;
> +	struct string_list projectlines;
> +	struct submodule_update_strategy update;
> +	struct pathspec pathspec;
> +};
> +#define SUBMODULE_UPDATE_CLONE_INIT {0, 0, 0, NULL, NULL, NULL, NULL, MODULE_LIST_INIT, STRING_LIST_INIT_DUP, SUBMODULE_UPDATE_STRATEGY_INIT}

I think these struct fields need some comments.  It's not clear what
most of them represent.

[...]
> +static int update_clone_inspect_next_task(struct child_process *cp,

What is being inspected here?  What does the return value represent?

Would a name like 'prepare_to_clone_next_submodule' make sense?  A
comment could say that 'ce' points to the candidate submodule to clone,
that the return value represents whether we want to clone it, and
that the first parameter is an output parameter representing a command
to run to carry out the clone.

[...]
> +	if (ce_stage(ce)) {
> +		if (pp->recursive_prefix)
> +			strbuf_addf(err, "Skipping unmerged submodule %s/%s\n",
> +				    pp->recursive_prefix, ce->name);
> +		else
> +			strbuf_addf(err, "Skipping unmerged submodule %s\n",
> +				    ce->name);

Nit: this would be easier to scan with braces.

[...]
> +	sub = submodule_from_path(null_sha1, ce->name);

It's common to call submodule_from_path with null_sha1 as a parameter
but I have trouble continuing to remember what that means.  Maybe
there should be a separate function that handles that?  As a
side-effect, the name and docstring of that function could explain
what it means, which I still am not sure about. :)


> +	if (pp->recursive_prefix)
> +		displaypath = relative_path(pp->recursive_prefix,
> +					    ce->name, &displaypath_sb);

Nit: could use braces.

> +	else
> +		displaypath = ce->name;
> +
> +	if (pp->update.type == SM_UPDATE_NONE ||
> +	    (pp->update.type == SM_UPDATE_UNSPECIFIED &&
> +	     sub->update_strategy.type == SM_UPDATE_NONE)) {

Nit: this might be more readable with the operators starting each
line:

	if (pp->update.type == SM_UPDATE_NONE
	    || (pp->update.type == SM_UPDATE_UNSPECIFIED
	        && sub->update_strategy.type == SM_UPDATE_NONE)) {

What does pp stand for?

Is the --update parameter ever set to 'none'?  What does it mean
when someone sets it to none?

> +		strbuf_addf(err, "Skipping submodule '%s'\n",
> +			    displaypath);

Does the caller expect a newline at the end of err?

In the refs code that uses an err strbuf, the convention is to
not end the message with a newline.  That way, a function like
die() can insert a newline and messages are guaranteed to be
newline-terminated even if someone is sloppy and does the wrong thing
when generating an error.

[...]
> +	if (needs_cloning) {

Could de-indent:

	if (!needs_cloning)
		goto cleanup;

> +		cp->git_cmd = 1;
> +		cp->no_stdin = 1;
> +		cp->stdout_to_stderr = 1;
> +		cp->err = -1;
> +		argv_array_push(&cp->args, "submodule--helper");
> +		argv_array_push(&cp->args, "clone");
> +		if (pp->quiet)
> +			argv_array_push(&cp->args, "--quiet");
> +
> +		if (pp->prefix)
> +			argv_array_pushl(&cp->args, "--prefix", pp->prefix, NULL);
> +

Odd whitespace.  I think it would be fine to remove the blank lines.

> +		argv_array_pushl(&cp->args, "--path", sub->path, NULL);
> +		argv_array_pushl(&cp->args, "--name", sub->name, NULL);
> +		argv_array_pushl(&cp->args, "--url", url, NULL);
> +		if (pp->reference)
> +			argv_array_push(&cp->args, pp->reference);
> +		if (pp->depth)
> +			argv_array_push(&cp->args, pp->depth);

What does 'cp' stand for mean?  Would a name like 'child' work?

[...]
> +static int update_clone_get_next_task(struct child_process *cp,
> +				      struct strbuf *err,
> +				      void *pp_cb,
> +				      void **pp_task_cb)
> +{
> +	struct submodule_update_clone *pp = pp_cb;
> +
> +	for (; pp->count < pp->list.nr; pp->count++) {
> +		const struct cache_entry *ce = pp->list.entries[pp->count];
> +		if (update_clone_inspect_next_task(cp, err, pp,
> +						   pp_task_cb, ce)) {
> +			pp->count++;
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}

Ah, that clarifies things a bit.

The 'count' variable is more of a cursor than a count.  Would a name +
comment like

	/* index into 'list' representing the next submodule to consider cloning */
	int current;

work?

> +
> +static int update_clone_start_failure(struct child_process *cp,
> +				      struct strbuf *err,
> +				      void *pp_cb,
> +				      void *pp_task_cb)
> +{
> +	struct submodule_update_clone *pp = pp_cb;
> +
> +	strbuf_addf(err, "error when starting a child process");
> +	pp->print_unmatched = 1;

What does print_unmatched mean?

[...]
> +static int update_clone_task_finished(int result,
> +				      struct child_process *cp,
> +				      struct strbuf *err,
> +				      void *pp_cb,
> +				      void *pp_task_cb)
> +{
> +	struct submodule_update_clone *pp = pp_cb;
> +
> +	if (!result) {
> +		return 0;
> +	} else {

No need for an 'else' here --- the 'if' already returned.

> +		strbuf_addf(err, "error in one child process");
> +		pp->print_unmatched = 1;

What does print_unmatched mean?

[...]
> +static int update_clone(int argc, const char **argv, const char *prefix)
> +{
> +	const char *update = NULL;
> +	struct string_list_item *item;
> +	struct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;
> +
> +	struct option module_list_options[] = {

The name looks stale.

> +		OPT_STRING(0, "prefix", &prefix,
> +			   N_("path"),
> +			   N_("path into the working tree")),
> +		OPT_STRING(0, "recursive_prefix", &pp.recursive_prefix,
> +			   N_("path"),
> +			   N_("path into the working tree, across nested "
> +			      "submodule boundaries")),

What do these options represent?  I'm used to the 'prefix' parameter to
a command coming from git machinery that remembers what the path to the
original cwd was relative to the worktree or repository root.  Here
there's an option to set it --- is that intentional?  Would setting the
environment variable GIT_PREFIX (that git already knows how to respect)
work in its place?

What is recursive_prefix relative to?

Nit: it would be more idiomatic to use a dash in place of an underscore
in the second one's name.  But this is an internal interface so it
doesn't matter much.

> +		OPT_STRING(0, "update", &update,
> +			   N_("string"),
> +			   N_("update command for submodules")),

This one is confusing to me because while the script supports --rebase /
--merge / --checkout this option seems to be more general.

If the help string said '(rebase, merge, or checkout)' then I wouldn't
mind.

> +		OPT_STRING(0, "reference", &pp.reference, "<repository>",
> +			   N_("Use the local reference repository "
> +			      "instead of a full clone")),

Is this allowed to be relative?  If so, what is it relative to?

[...]
> +		OPT__QUIET(&pp.quiet, N_("do't print cloning progress")),

Typo.

[...]
> +	const char *const git_submodule_helper_usage[] = {
> +		N_("git submodule--helper list [--prefix=<path>] [<path>...]"),

Is this the 'list' subcommand?

[...]
> +	gitmodules_config();
> +	/* Overlay the parsed .gitmodules file with .git/config */
> +	git_config(submodule_config, NULL);
> +	run_processes_parallel(1, update_clone_get_next_task,
> +				  update_clone_start_failure,
> +				  update_clone_task_finished,
> +				  &pp);

Neat. \o/

> +	if (pp.print_unmatched) {
> +		printf("#unmatched\n");

I'm still confused about this.  I think a comment where
'print_unmatched' is declared would probably clear it up.

[...]
> +	for_each_string_list_item(item, &pp.projectlines)
> +		utf8_fprintf(stdout, "%s", item->string);

(just curious) why are these saved up and printed all at once instead
of being printed as it goes?

[...]
> --- a/git-submodule.sh
> +++ b/git-submodule.sh

Very nice.

Thanks,
Jonathan

Re: [PATCHv14 5/7] git submodule update: have a dedicated helper for cloning

[Stefan Beller]

On Fri, Feb 19, 2016 at 3:07 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> I still have trouble reading this patch (patch 5).  Some musing below
> to figure out what could change to make it more readable (perhaps in a
> patch on top).

After this review (I added replies from bottom to top, the same way I
write my code ;)
I think we need another reroll at least.

>> +#define SUBMODULE_UPDATE_CLONE_INIT {0, 0, 0, NULL, NULL, NULL, NULL, MODULE_LIST_INIT, STRING_LIST_INIT_DUP, SUBMODULE_UPDATE_STRATEGY_INIT}
>
> I think these struct fields need some comments.  It's not clear what
> most of them represent.

ok.

>
> [...]
>> +static int update_clone_inspect_next_task(struct child_process *cp,
>
> What is being inspected here?  What does the return value represent?
>
> Would a name like 'prepare_to_clone_next_submodule' make sense?  A
> comment could say that 'ce' points to the candidate submodule to clone,
> that the return value represents whether we want to clone it, and
> that the first parameter is an output parameter representing a command
> to run to carry out the clone.
>

ok.

>
> It's common to call submodule_from_path with null_sha1 as a parameter
> but I have trouble continuing to remember what that means.  Maybe
> there should be a separate function that handles that?  As a
> side-effect, the name and docstring of that function could explain
> what it means, which I still am not sure about. :)

Good idea!

>         if (pp->update.type == SM_UPDATE_NONE
>             || (pp->update.type == SM_UPDATE_UNSPECIFIED
>                 && sub->update_strategy.type == SM_UPDATE_NONE)) {
>
> What does pp stand for?

parallel processes similar to cp standing for child process.

>
> Is the --update parameter ever set to 'none'?  What does it mean
> when someone sets it to none?

In the initialization. This is to preserve precedence.
Yeah there is a test for --update=none.

>
>> +             strbuf_addf(err, "Skipping submodule '%s'\n",
>> +                         displaypath);
>
> Does the caller expect a newline at the end of err?
>
> In the refs code that uses an err strbuf, the convention is to
> not end the message with a newline.  That way, a function like
> die() can insert a newline and messages are guaranteed to be
> newline-terminated even if someone is sloppy and does the wrong thing
> when generating an error.

Oh! So we need to add new lines ourselves here.

>> +             if (pp->reference)
>> +                     argv_array_push(&cp->args, pp->reference);
>> +             if (pp->depth)
>> +                     argv_array_push(&cp->args, pp->depth);
>
> What does 'cp' stand for mean?  Would a name like 'child' work?

child process ? (any code which had a struct child_process referred to
it as *cp)


>
> The 'count' variable is more of a cursor than a count.  Would a name +
> comment like
>
>         /* index into 'list' representing the next submodule to consider cloning */
>         int current;
>
> work?
>

I think that's better.

>
> [...]
>> +static int update_clone(int argc, const char **argv, const char *prefix)
>> +{
>> +     const char *update = NULL;
>> +     struct string_list_item *item;
>> +     struct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;
>> +
>> +     struct option module_list_options[] = {
>
> The name looks stale.
>
>> +             OPT_STRING(0, "prefix", &prefix,
>> +                        N_("path"),
>> +                        N_("path into the working tree")),
>> +             OPT_STRING(0, "recursive_prefix", &pp.recursive_prefix,
>> +                        N_("path"),
>> +                        N_("path into the working tree, across nested "
>> +                           "submodule boundaries")),
>
> What do these options represent?  I'm used to the 'prefix' parameter to
> a command coming from git machinery that remembers what the path to the
> original cwd was relative to the worktree or repository root.  Here
> there's an option to set it --- is that intentional?  Would setting the
> environment variable GIT_PREFIX (that git already knows how to respect)
> work in its place?
>
> What is recursive_prefix relative to?
>
> Nit: it would be more idiomatic to use a dash in place of an underscore
> in the second one's name.  But this is an internal interface so it
> doesn't matter much.
>
>> +             OPT_STRING(0, "update", &update,
>> +                        N_("string"),
>> +                        N_("update command for submodules")),
>
> This one is confusing to me because while the script supports --rebase /
> --merge / --checkout this option seems to be more general.
>
> If the help string said '(rebase, merge, or checkout)' then I wouldn't
> mind.
>
>> +             OPT_STRING(0, "reference", &pp.reference, "<repository>",
>> +                        N_("Use the local reference repository "
>> +                           "instead of a full clone")),
>
> Is this allowed to be relative?  If so, what is it relative to?
>
> [...]
>> +             OPT__QUIET(&pp.quiet, N_("do't print cloning progress")),
>
> Typo.
>
> [...]
>> +     const char *const git_submodule_helper_usage[] = {
>> +             N_("git submodule--helper list [--prefix=<path>] [<path>...]"),
>
> Is this the 'list' subcommand?

no. :(

>
> [...]
>> +     gitmodules_config();
>> +     /* Overlay the parsed .gitmodules file with .git/config */
>> +     git_config(submodule_config, NULL);
>> +     run_processes_parallel(1, update_clone_get_next_task,
>> +                               update_clone_start_failure,
>> +                               update_clone_task_finished,
>> +                               &pp);
>
> Neat. \o/
>
>> +     if (pp.print_unmatched) {
>> +             printf("#unmatched\n");
>
> I'm still confused about this.  I think a comment where
> 'print_unmatched' is declared would probably clear it up.

Probably this is a too literal translation from shell to C.
By printing #unmatched the shell on the other end of the pipe
of this helper program knows to just stop and error out.

So this is telling the downstream program to stop.

Maybe we can name the variable 'quickstop' ?
'abort_and_exit' ?

>
> [...]
>> +     for_each_string_list_item(item, &pp.projectlines)
>> +             utf8_fprintf(stdout, "%s", item->string);
>
> (just curious) why are these saved up and printed all at once instead
> of being printed as it goes?

To have a clean abort path, i.e. we need to be done with all the parallel stuff
before we start on doing local on-disk stuff. So we need to be sure we
do not print "#unmatched\n" but rather all the repositories.

In a next step, we can do the checkout in parallel if there is nothing to assume
to lead to trouble. i.e. update strategy is checkout and the submodule is
in a clean state at the tip of a branch.

All problems which need to be solved by the user should be presented in
a sequential way, i.e. present one problem and then full stop.
That seems to be more encouraging as if we'd throw a "Here are a dozen
broken submodule repositories which we expect the user fixes up".

>
> [...]
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>
> Very nice.
>
> Thanks,
> Jonathan

Re: [PATCHv14 5/7] git submodule update: have a dedicated helper for cloning

[Jonathan Nieder]

Stefan Beller wrote:
> On Fri, Feb 19, 2016 at 3:07 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

[...]
>>> +             strbuf_addf(err, "Skipping submodule '%s'\n",
>>> +                         displaypath);
>>
>> Does the caller expect a newline at the end of err?
>>
>> In the refs code that uses an err strbuf, the convention is to
>> not end the message with a newline.  That way, a function like
>> die() can insert a newline and messages are guaranteed to be
>> newline-terminated even if someone is sloppy and does the wrong thing
>> when generating an error.
>
> Oh! So we need to add new lines ourselves here.

Now I'm confused.  The code in this patch is inconsistent.  I was
recommending consistently leaving out the \n.

It looks like pp_start_one takes the content of err without adding
a \n.  That's a bug in pp_start_one and pp_collect_finished IMHO.

For example, default_task_finished and default_start_failure don't
put a newline don't put a newline at the end of the message.  I
don't think that's a bug --- they're doing the right thing, but
pp_start_one and pp_collect_finished is just not handling it
correctly.

>>> +             if (pp->reference)
>>> +                     argv_array_push(&cp->args, pp->reference);
>>> +             if (pp->depth)
>>> +                     argv_array_push(&cp->args, pp->depth);
>>
>> What does 'cp' stand for mean?  Would a name like 'child' work?
>
> child process ? (any code which had a struct child_process referred to
> it as *cp)

Output from 'git grep --F -e "child_process *"' finds variables with
various names, corresponding to what kind of child it is.

[...]
>> Is this the 'list' subcommand?
>
> no. :(

No problem --- that's what review is for.

[...]
>>> +     if (pp.print_unmatched) {
>>> +             printf("#unmatched\n");
>>
>> I'm still confused about this.  I think a comment where
>> 'print_unmatched' is declared would probably clear it up.
>
> Probably this is a too literal translation from shell to C.
> By printing #unmatched the shell on the other end of the pipe
> of this helper program knows to just stop and error out.
>
> So this is telling the downstream program to stop.
>
> Maybe we can name the variable 'quickstop' ?
> 'abort_and_exit' ?

Interesting.

Would it work for the helper to exit at that point with nonzero status?

Lacking "set -o pipefail", it's a little messy to handle in the shell,
but it's possible:

	{
		git submodule--helper \
			--foo \
			--bar \
			--baz ||
		... handle error, e.g. by printing something
		that the other end of the pipe wants to see ...
	} |
	...

[...]
>> (just curious) why are these saved up and printed all at once instead
>> of being printed as it goes?
>
> To have a clean abort path, i.e. we need to be done with all the parallel stuff
> before we start on doing local on-disk stuff.

Interesting.

That's subtle.  Probably worth a comment so people know not to break
it.  (E.g.

	/*
	 * We saved the output and splat it all at once now.
	 * That means:
	 * - the listener does not have to interleave their (checkout)
	 *   work with our fetching.  The writes involved in a
	 *   checkout involve more straightforward sequential I/O.
	 * - the listener can avoid doing any work if fetching failed.
	 */

).

Thinking out loud: the other side could write their output to a
temporary file (e.g.  under .git) and save us the trouble of buffering
it.  But the list of submodules and statuses is not likely to be huge
--- buffering doesn't hurt much.

[...]
> In a next step, we can do the checkout in parallel if there is nothing to assume
> to lead to trouble. i.e. update strategy is checkout and the submodule is
> in a clean state at the tip of a branch.

Somebody tried parallelizing file output during checkout and found it
sped up the checkout on some systems by a small amount.  But then
later operations to read back the files were much slower.  It seems
that the output pattern affected the layout of the files on disk in a
bad way.

I'm not too afraid.  Just saying that the benefit of parallel checkout
would be something to measure.

A bigger worry is that checkout might be I/O bound and not benefit
much from parallelism.

> All problems which need to be solved by the user should be presented in
> a sequential way, i.e. present one problem and then full stop.
> That seems to be more encouraging as if we'd throw a "Here are a dozen
> broken submodule repositories which we expect the user fixes up".

It depends on the problem --- sometimes people want to see a list of
errors and fix them all (hence tools like "make -k").  I agree that
stop-on-first-error is a good default and that it's not worth worrying
about introducing --keep-going until someone asks for it.

Thanks for your thoughtfulness,
Jonathan

Re: [PATCHv14 0/7] Expose submodule parallelism to the user

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Looks good.  I didn't notice these unnecessary blank lines while
> reading the previous rounds, and it is good to see them go.
>
> Let's wait for a few days and merge them to 'next'.  David's ref
> backend topic textually depends on this, and we'd better give it a
> solid foundation to build on soon.

So... it seems that you and Jonathan had a few rounds of back and
forth on 5/7 that didn't sound like it saw a satisfactory end.  Will
we see a reroll to address the issues raised?

Thanks.

Re: [PATCHv14 0/7] Expose submodule parallelism to the user

[Stefan Beller]

On Tue, Feb 23, 2016 at 3:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Looks good.  I didn't notice these unnecessary blank lines while
>> reading the previous rounds, and it is good to see them go.
>>
>> Let's wait for a few days and merge them to 'next'.  David's ref
>> backend topic textually depends on this, and we'd better give it a
>> solid foundation to build on soon.
>
> So... it seems that you and Jonathan had a few rounds of back and
> forth on 5/7 that didn't sound like it saw a satisfactory end.  Will
> we see a reroll to address the issues raised?

Yes, I was about to send one out, but then I wanted to fix the last
of Jonathans small nits, which resulted in some heavy refactoring.

I'll reroll, but I suspect it won't make it out today in time for integration.

Thanks,
Stefan

>
> Thanks.
>

Re: [PATCHv14 0/7] Expose submodule parallelism to the user

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Feb 23, 2016 at 3:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Looks good.  I didn't notice these unnecessary blank lines while
>>> reading the previous rounds, and it is good to see them go.
>>>
>>> Let's wait for a few days and merge them to 'next'.  David's ref
>>> backend topic textually depends on this, and we'd better give it a
>>> solid foundation to build on soon.
>>
>> So... it seems that you and Jonathan had a few rounds of back and
>> forth on 5/7 that didn't sound like it saw a satisfactory end.  Will
>> we see a reroll to address the issues raised?
>
> Yes, I was about to send one out, but then I wanted to fix the last
> of Jonathans small nits, which resulted in some heavy refactoring.
>
> I'll reroll, but I suspect it won't make it out today in time for integration.

Ah, that's OK, I've already pushed today's out.  Take time to read
it over one more time, perhaps?

Thanks.