[PATCHv3 00/11] Expose the submodule parallelism to the user

[PATCHv3 00/11] Expose the submodule parallelism to the user

[Stefan Beller]

Where does it apply?
---
This series applies on top of d075d2604c0f92045caa8d5bb6ab86cf4921a4ae (Merge
branch 'rs/daemon-plug-child-leak' into sb/submodule-parallel-update) and replaces
the previous patches in sb/submodule-parallel-update

What does it do?
---
This series should finish the on going efforts of parallelizing
submodule network traffic. The patches contain tests for clone,
fetch and submodule update to use the actual parallelism both via
command line as well as a configured option. I decided to go with
"submodule.jobs" for all three for now.

What's new in v3?
---

 * 3 new patches (make it compile in Windows, better warnings in posix environment
   for setting fds to non blocking, drop check against NULL)
 * adressed reviews by Eric for readability. :) 
 * addressed Junios comments for the new clone helper function

Stefan Beller (11):
  run_processes_parallel: delimit intermixed task output
  run-command: report failure for degraded output just once
  run-command: omit setting file descriptors to non blocking in Windows
  submodule-config: keep update strategy around
  submodule-config: drop check against NULL
  submodule-config: remove name_and_item_from_var
  submodule-config: introduce parse_generic_submodule_config
  fetching submodules: respect `submodule.jobs` config option
  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        |   7 ++
 Documentation/git-clone.txt     |   6 +-
 Documentation/git-submodule.txt |   7 +-
 builtin/clone.c                 |  19 +++-
 builtin/fetch.c                 |   2 +-
 builtin/submodule--helper.c     | 239 ++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh                |  54 ++++-----
 run-command.c                   |  26 ++++-
 submodule-config.c              | 109 +++++++++++-------
 submodule-config.h              |   3 +
 submodule.c                     |   5 +
 t/t5526-fetch-submodules.sh     |  14 +++
 t/t7400-submodule-basic.sh      |   4 +-
 t/t7406-submodule-update.sh     |  27 +++++
 14 files changed, 433 insertions(+), 89 deletions(-)

-- 
2.6.1.247.ge8f2a41.dirty

[PATCHv3 01/11] run_processes_parallel: delimit intermixed task output

[Stefan Beller]

This commit serves 2 purposes. First this may help the user who
tries to diagnose intermixed process calls. Second this may be used
in a later patch for testing. As the output of a command should not
change visibly except for going faster, grepping for the trace output
seems like a viable testing strategy.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 run-command.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/run-command.c b/run-command.c
index 0a3c24e..7c00c21 100644
--- a/run-command.c
+++ b/run-command.c
@@ -959,6 +959,9 @@ static struct parallel_processes *pp_init(int n,
 		n = online_cpus();
 
 	pp->max_processes = n;
+
+	trace_printf("run_processes_parallel: preparing to run up to %d tasks", n);
+
 	pp->data = data;
 	if (!get_next_task)
 		die("BUG: you need to specify a get_next_task function");
@@ -988,6 +991,7 @@ static void pp_cleanup(struct parallel_processes *pp)
 {
 	int i;
 
+	trace_printf("run_processes_parallel: done");
 	for (i = 0; i < pp->max_processes; i++) {
 		strbuf_release(&pp->children[i].err);
 		child_process_clear(&pp->children[i].process);
-- 
2.6.1.247.ge8f2a41.dirty

[PATCHv3 02/11] run-command: report failure for degraded output just once

[Stefan Beller]

The warning message is cluttering the output itself,
so just report it once.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 run-command.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/run-command.c b/run-command.c
index 7c00c21..3ae563f 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1012,13 +1012,21 @@ static void pp_cleanup(struct parallel_processes *pp)
 
 static void set_nonblocking(int fd)
 {
+	static int reported_degrade = 0;
 	int flags = fcntl(fd, F_GETFL);
-	if (flags < 0)
-		warning("Could not get file status flags, "
-			"output will be degraded");
-	else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
-		warning("Could not set file status flags, "
-			"output will be degraded");
+	if (flags < 0) {
+		if (!reported_degrade) {
+			warning("Could not get file status flags, "
+				"output will be degraded");
+			reported_degrade = 1;
+		}
+	} else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK)) {
+		if (!reported_degrade) {
+			warning("Could not set file status flags, "
+				"output will be degraded");
+			reported_degrade = 1;
+		}
+	}
 }
 
 /* returns
-- 
2.6.1.247.ge8f2a41.dirty

[PATCHv3 03/11] run-command: omit setting file descriptors to non blocking in Windows

[Stefan Beller]

In Windows there is no fcntl apparently and as it only affects output
slightly we can just run with degraded output in Windows.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 run-command.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/run-command.c b/run-command.c
index 3ae563f..8db7df8 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1012,6 +1012,7 @@ static void pp_cleanup(struct parallel_processes *pp)
 
 static void set_nonblocking(int fd)
 {
+#ifndef GIT_WINDOWS_NATIVE
 	static int reported_degrade = 0;
 	int flags = fcntl(fd, F_GETFL);
 	if (flags < 0) {
@@ -1027,6 +1028,7 @@ static void set_nonblocking(int fd)
 			reported_degrade = 1;
 		}
 	}
+#endif
 }
 
 /* returns
-- 
2.6.1.247.ge8f2a41.dirty

[PATCHv3 04/11] submodule-config: keep update strategy around

[Stefan Beller]

We need the submodule update strategies in a later patch.

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

diff --git a/submodule-config.c b/submodule-config.c
index afe0ea8..4239b0e 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -194,6 +194,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
 
 	submodule->path = NULL;
 	submodule->url = NULL;
+	submodule->update = NULL;
 	submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
 	submodule->ignore = NULL;
 
@@ -311,6 +312,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 != NULL)
+			warn_multiple_config(me->commit_sha1, submodule->name,
+					     "update");
+		else {
+			free((void *) submodule->update);
+			submodule->update = xstrdup(value);
+		}
 	}
 
 	strbuf_release(&name);
diff --git a/submodule-config.h b/submodule-config.h
index 9061e4e..f9e2a29 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -14,6 +14,7 @@ struct submodule {
 	const char *url;
 	int fetch_recurse;
 	const char *ignore;
+	const char *update;
 	/* the sha1 blob id of the responsible .gitmodules file */
 	unsigned char gitmodules_sha1[20];
 };
-- 
2.6.1.247.ge8f2a41.dirty

[PATCHv3 05/11] 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>
---
 submodule-config.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 4239b0e..6d01941 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -265,7 +265,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 {
@@ -289,7 +289,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") &&
@@ -305,7 +305,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 {
@@ -315,7 +315,7 @@ static int parse_config(const char *var, const char *value, void *data)
 	} else if (!strcmp(item.buf, "update")) {
 		if (!value)
 			ret = config_error_nonbool(var);
-		else if (!me->overwrite && submodule->update != NULL)
+		else if (!me->overwrite && submodule->update)
 			warn_multiple_config(me->commit_sha1, submodule->name,
 					     "update");
 		else {
-- 
2.6.1.247.ge8f2a41.dirty

[PATCHv3 06/11] submodule-config: remove name_and_item_from_var

[Stefan Beller]

`name_and_item_from_var` does not provide the proper abstraction
we need here in a later patch.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule-config.c | 48 ++++++++++++++++--------------------------------
 1 file changed, 16 insertions(+), 32 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 6d01941..b826841 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -161,31 +161,17 @@ static struct submodule *cache_lookup_name(struct submodule_cache *cache,
 	return NULL;
 }
 
-static int name_and_item_from_var(const char *var, struct strbuf *name,
-				  struct strbuf *item)
-{
-	const char *subsection, *key;
-	int subsection_len, parse;
-	parse = parse_config_key(var, "submodule", &subsection,
-			&subsection_len, &key);
-	if (parse < 0 || !subsection)
-		return 0;
-
-	strbuf_add(name, subsection, subsection_len);
-	strbuf_addstr(item, key);
-
-	return 1;
-}
-
 static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
-		const unsigned char *gitmodules_sha1, const char *name)
+						  const unsigned char *gitmodules_sha1,
+						  const char *name_ptr, int name_len)
 {
 	struct submodule *submodule;
 	struct strbuf name_buf = STRBUF_INIT;
+	char *name = xmemdupz(name_ptr, name_len);
 
 	submodule = cache_lookup_name(cache, gitmodules_sha1, name);
 	if (submodule)
-		return submodule;
+		goto out;
 
 	submodule = xmalloc(sizeof(*submodule));
 
@@ -201,7 +187,8 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
 	hashcpy(submodule->gitmodules_sha1, gitmodules_sha1);
 
 	cache_add(cache, submodule);
-
+out:
+	free(name);
 	return submodule;
 }
 
@@ -251,18 +238,18 @@ static int parse_config(const char *var, const char *value, void *data)
 {
 	struct parse_config_parameter *me = data;
 	struct submodule *submodule;
-	struct strbuf name = STRBUF_INIT, item = STRBUF_INIT;
-	int ret = 0;
+	int subsection_len, ret = 0;
+	const char *subsection, *key;
 
-	/* this also ensures that we only parse submodule entries */
-	if (!name_and_item_from_var(var, &name, &item))
+	if (parse_config_key(var, "submodule", &subsection,
+			     &subsection_len, &key) < 0 || !subsection_len)
 		return 0;
 
 	submodule = lookup_or_create_by_name(me->cache,
 					     me->gitmodules_sha1,
-					     name.buf);
+					     subsection, subsection_len);
 
-	if (!strcmp(item.buf, "path")) {
+	if (!strcmp(key, "path")) {
 		if (!value)
 			ret = config_error_nonbool(var);
 		else if (!me->overwrite && submodule->path)
@@ -275,7 +262,7 @@ static int parse_config(const char *var, const char *value, void *data)
 			submodule->path = xstrdup(value);
 			cache_put_path(me->cache, submodule);
 		}
-	} else if (!strcmp(item.buf, "fetchrecursesubmodules")) {
+	} else if (!strcmp(key, "fetchrecursesubmodules")) {
 		/* when parsing worktree configurations we can die early */
 		int die_on_error = is_null_sha1(me->gitmodules_sha1);
 		if (!me->overwrite &&
@@ -286,7 +273,7 @@ static int parse_config(const char *var, const char *value, void *data)
 			submodule->fetch_recurse = parse_fetch_recurse(
 								var, value,
 								die_on_error);
-	} else if (!strcmp(item.buf, "ignore")) {
+	} else if (!strcmp(key, "ignore")) {
 		if (!value)
 			ret = config_error_nonbool(var);
 		else if (!me->overwrite && submodule->ignore)
@@ -302,7 +289,7 @@ static int parse_config(const char *var, const char *value, void *data)
 			free((void *) submodule->ignore);
 			submodule->ignore = xstrdup(value);
 		}
-	} else if (!strcmp(item.buf, "url")) {
+	} else if (!strcmp(key, "url")) {
 		if (!value) {
 			ret = config_error_nonbool(var);
 		} else if (!me->overwrite && submodule->url) {
@@ -312,7 +299,7 @@ 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")) {
+	} else if (!strcmp(key, "update")) {
 		if (!value)
 			ret = config_error_nonbool(var);
 		else if (!me->overwrite && submodule->update)
@@ -324,9 +311,6 @@ static int parse_config(const char *var, const char *value, void *data)
 		}
 	}
 
-	strbuf_release(&name);
-	strbuf_release(&item);
-
 	return ret;
 }
 
-- 
2.6.1.247.ge8f2a41.dirty

[PATCHv3 07/11] submodule-config: introduce parse_generic_submodule_config

[Stefan Beller]

This rewrites parse_config to distinguish between configs specific to
one submodule and configs which apply generically to all submodules.
We do not have generic submodule configs yet, but the next patch will
introduce "submodule.jobs".

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule-config.c | 41 ++++++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index b826841..29e21b2 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -234,17 +234,22 @@ struct parse_config_parameter {
 	int overwrite;
 };
 
-static int parse_config(const char *var, const char *value, void *data)
+static int parse_generic_submodule_config(const char *key,
+					  const char *var,
+					  const char *value,
+					  struct parse_config_parameter *me)
 {
-	struct parse_config_parameter *me = data;
-	struct submodule *submodule;
-	int subsection_len, ret = 0;
-	const char *subsection, *key;
-
-	if (parse_config_key(var, "submodule", &subsection,
-			     &subsection_len, &key) < 0 || !subsection_len)
-		return 0;
+	return 0;
+}
 
+static int parse_specific_submodule_config(const char *subsection, int subsection_len,
+					   const char *key,
+					   const char *var,
+					   const char *value,
+					   struct parse_config_parameter *me)
+{
+	int ret = 0;
+	struct submodule *submodule;
 	submodule = lookup_or_create_by_name(me->cache,
 					     me->gitmodules_sha1,
 					     subsection, subsection_len);
@@ -314,6 +319,24 @@ static int parse_config(const char *var, const char *value, void *data)
 	return ret;
 }
 
+static int parse_config(const char *var, const char *value, void *data)
+{
+	struct parse_config_parameter *me = data;
+	int subsection_len;
+	const char *subsection, *key;
+
+	if (parse_config_key(var, "submodule", &subsection,
+			     &subsection_len, &key) < 0)
+		return 0;
+
+	if (!subsection_len)
+		return parse_generic_submodule_config(key, var, value, me);
+	else
+		return parse_specific_submodule_config(subsection,
+						       subsection_len, key,
+						       var, value, me);
+}
+
 static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
 				      unsigned char *gitmodules_sha1)
 {
-- 
2.6.1.247.ge8f2a41.dirty

[PATCHv3 08/11] fetching submodules: respect `submodule.jobs` 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>
---
 Documentation/config.txt    |  7 +++++++
 builtin/fetch.c             |  2 +-
 submodule-config.c          | 15 +++++++++++++++
 submodule-config.h          |  2 ++
 submodule.c                 |  5 +++++
 t/t5526-fetch-submodules.sh | 14 ++++++++++++++
 6 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 391a0c3..70e1b88 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2643,6 +2643,13 @@ submodule.<name>.ignore::
 	"--ignore-submodules" option. The 'git submodule' commands are not
 	affected by this setting.
 
+submodule.jobs::
+	This is used to determine how many submodules can be operated on in
+	parallel. Specifying a positive integer allows up to that number
+	of submodules being fetched in parallel. This is used in fetch
+	and clone operations only. A value of 0 will give some reasonable
+	configuration. 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 9cc1c9d..60e6797 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-config.c b/submodule-config.c
index 29e21b2..475551a 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -32,6 +32,7 @@ enum lookup_type {
 
 static struct submodule_cache cache;
 static int is_cache_init;
+static int parallel_jobs = -1;
 
 static int config_path_cmp(const struct submodule_entry *a,
 			   const struct submodule_entry *b,
@@ -239,6 +240,15 @@ static int parse_generic_submodule_config(const char *key,
 					  const char *value,
 					  struct parse_config_parameter *me)
 {
+	if (!strcmp(key, "jobs")) {
+		parallel_jobs = strtol(value, NULL, 10);
+		if (parallel_jobs < 0) {
+			warning("submodule.jobs not allowed to be negative.");
+			parallel_jobs = 1;
+			return 1;
+		}
+	}
+
 	return 0;
 }
 
@@ -482,3 +492,8 @@ void submodule_free(void)
 	cache_free(&cache);
 	is_cache_init = 0;
 }
+
+int config_parallel_submodules(void)
+{
+	return parallel_jobs;
+}
diff --git a/submodule-config.h b/submodule-config.h
index f9e2a29..d9bbf9a 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -27,4 +27,6 @@ const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
 		const char *path);
 void submodule_free(void);
 
+int config_parallel_submodules(void);
+
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/submodule.c b/submodule.c
index 0257ea3..188ba02 100644
--- a/submodule.c
+++ b/submodule.c
@@ -752,6 +752,11 @@ 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 = config_parallel_submodules();
+	if (max_parallel_jobs < 0)
+		max_parallel_jobs = 1;
+
 	calculate_changed_submodule_paths();
 	run_processes_parallel(max_parallel_jobs,
 			       get_next_submodule,
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 1b4ce69..5c3579c 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -470,4 +470,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 children" trace.out &&
+		git config submodule.jobs 8 &&
+		GIT_TRACE=$(pwd)/trace.out git fetch &&
+		grep "8 children" trace.out &&
+		GIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&
+		grep "9 children" trace.out
+	)
+'
+
 test_done
-- 
2.6.1.247.ge8f2a41.dirty

[PATCHv3 09/11] 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).

As we can only access the stderr channel from within the parallel
processing engine, we need to reroute the error message for
specified but initialized submodules to stderr. As it is an error
message, this should have gone to stderr in the first place, so it
is a bug fix along the way.

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 +++------
 t/t7400-submodule-basic.sh  |   4 +-
 3 files changed, 242 insertions(+), 36 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f4c3eff..95b45a2 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;
 }
 
+static int git_submodule_config(const char *var, const char *value, void *cb)
+{
+	return parse_submodule_config_option(var, value);
+}
+
+struct submodule_update_clone {
+	/* states */
+	int count;
+	int print_unmatched;
+	/* configuration */
+	int quiet;
+	const char *reference;
+	const char *depth;
+	const char *update;
+	const char *recursive_prefix;
+	const char *prefix;
+	struct module_list list;
+	struct string_list projectlines;
+	struct pathspec pathspec;
+};
+#define SUBMODULE_UPDATE_CLONE_INIT {0, 0, 0, NULL, NULL, NULL, NULL, NULL, MODULE_LIST_INIT, STRING_LIST_INIT_DUP}
+
+static void fill_clone_command(struct child_process *cp, int quiet,
+			       const char *prefix, const char *path,
+			       const char *name, const char *url,
+			       const char *reference, const char *depth)
+{
+	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 (quiet)
+		argv_array_push(&cp->args, "--quiet");
+
+	if (prefix)
+		argv_array_pushl(&cp->args, "--prefix", prefix, NULL);
+
+	argv_array_pushl(&cp->args, "--path", path, NULL);
+	argv_array_pushl(&cp->args, "--name", name, NULL);
+	argv_array_pushl(&cp->args, "--url", url, NULL);
+	if (reference)
+		argv_array_push(&cp->args, reference);
+	if (depth)
+		argv_array_push(&cp->args, depth);
+}
+
+static int update_clone_get_next_task(void **pp_task_cb,
+				      struct child_process *cp,
+				      struct strbuf *err,
+				      void *pp_cb)
+{
+	struct submodule_update_clone *pp = pp_cb;
+
+	for (; pp->count < pp->list.nr; pp->count++) {
+		const struct submodule *sub = NULL;
+		const char *displaypath = NULL;
+		const struct cache_entry *ce = pp->list.entries[pp->count];
+		struct strbuf sb = STRBUF_INIT;
+		const char *update_module = 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);
+			continue;
+		}
+
+		sub = submodule_from_path(null_sha1, ce->name);
+		if (!sub) {
+			strbuf_addf(err, "BUG: internal error managing submodules. "
+				    "The cache could not locate '%s'", ce->name);
+			pp->print_unmatched = 1;
+			continue;
+		}
+
+		if (pp->recursive_prefix)
+			displaypath = relative_path(pp->recursive_prefix, ce->name, &sb);
+		else
+			displaypath = ce->name;
+
+		if (pp->update)
+			update_module = pp->update;
+		if (!update_module)
+			update_module = sub->update;
+		if (!update_module)
+			update_module = "checkout";
+		if (!strcmp(update_module, "none")) {
+			strbuf_addf(err, "Skipping submodule '%s'\n", displaypath);
+			continue;
+		}
+
+		/*
+		 * 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);
+			continue;
+		}
+
+		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) {
+			fill_clone_command(cp, pp->quiet, pp->prefix, ce->name,
+					   sub->name, url, pp->reference, pp->depth);
+			pp->count++;
+			free(url);
+			return 1;
+		} else
+			free(url);
+	}
+	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)
+{
+	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", &pp.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 (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(git_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 9bc5c5f..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 "Skipping submodule '$displaypath'"
-			continue
-		fi
-
-		if test -z "$url"
-		then
-			# Only mention uninitialized submodules when its
-			# path have been specified
-			test "$#" != "0" &&
-			say "$(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)
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.6.1.247.ge8f2a41.dirty

[PATCHv3 10/11] submodule update: expose parallelism to the user

[Stefan Beller]

Expose possible parallelism either via the "--jobs" CLI parameter or
the "submodule.jobs" 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>
---
 Documentation/git-submodule.txt |  7 ++++++-
 builtin/submodule--helper.c     | 18 ++++++++++++++----
 git-submodule.sh                |  9 +++++++++
 t/t7406-submodule-update.sh     | 12 ++++++++++++
 4 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index f17687e..c70fafd 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>
@@ -374,6 +374,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.jobs` 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 95b45a2..662d329 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -426,6 +426,7 @@ static int update_clone_task_finished(int result,
 
 static int update_clone(int argc, const char **argv, const char *prefix)
 {
+	int max_jobs = -1;
 	struct string_list_item *item;
 	struct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;
 
@@ -446,6 +447,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,17 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	gitmodules_config();
 	/* Overlay the parsed .gitmodules file with .git/config */
 	git_config(git_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 = config_parallel_submodules();
+	if (max_jobs < 0)
+		max_jobs = 1;
+
+	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..05ea66f 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 children" trace.out &&
+	 git config submodule.jobs 8 &&
+	 GIT_TRACE=$(pwd)/trace.out git submodule update &&
+	 grep "8 children" trace.out &&
+	 GIT_TRACE=$(pwd)/trace.out git submodule update --jobs 9 &&
+	 grep "9 children" trace.out
+	)
+'
 test_done
-- 
2.6.1.247.ge8f2a41.dirty

[PATCHv3 11/11] 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>
---
 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 f1f2a3f..01bd6b7 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
@@ -216,6 +216,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.jobs` option.
 
 <repository>::
 	The (possibly remote) repository to clone from.  See the
diff --git a/builtin/clone.c b/builtin/clone.c
index 9eaecd9..ce578d2 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 05ea66f..ade0524 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 children" 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 children" trace.out &&
+	rm -rf super4 &&
+	git config --global submodule.jobs 8 &&
+	GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules . super4 &&
+	grep "8 children" trace.out &&
+	rm -rf super4 &&
+	GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 9 . super4 &&
+	grep "9 children" trace.out &&
+	rm -rf super4
+'
+
 test_done
-- 
2.6.1.247.ge8f2a41.dirty

Re: [PATCHv3 00/11] Expose the submodule parallelism to the user

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Where does it apply?
> ---
> This series applies on top of d075d2604c0f92045caa8d5bb6ab86cf4921a4ae (Merge
> branch 'rs/daemon-plug-child-leak' into sb/submodule-parallel-update) and replaces
> the previous patches in sb/submodule-parallel-update
>
> What does it do?
> ---
> This series should finish the on going efforts of parallelizing
> submodule network traffic. The patches contain tests for clone,
> fetch and submodule update to use the actual parallelism both via
> command line as well as a configured option. I decided to go with
> "submodule.jobs" for all three for now.

The order of patches and where the series builds makes me suspect
that I have been expecting too much from the "parallel-fetch" topic.

I've been hoping that it would be useful for the project as a whole
to polish the other topic and make it available to wider audience
sooner by itself (both from "end users get improved Git early"
aspect and from "the core machinery to be reused in follow-up
improvements are made closer to perfection sooner" perspective).  So
I've been expecting that "Let's fix it on Windows" change directly
on top of sb/submodule-parallel-fetch to make that topic usable
before everything else.  Other patches in this series may require
the child_process_cleanup() change, so they may be applied on top of
the merge between sb/submodule-parallel-fetch (updated for Windows)
and rs/daemon-plug-child-leak topic.

That does not seem to be what's happening here (note: I am not
complaining; I am just trying to make sure expectation matches
reality).  Am I reading you correctly?

I think sb/submodule-parallel-fetch + sb/submodule-parallel-update
as a single topic would need more time to mature to be in a tagged
release than we have in the remainder of this cycle.  It is likely
that the former topic has a chance to get rebased after 2.7 happens.
And that would allow us to (1) use the child_process_cleanup() from
get-go instead of _deinit and to (2) get the machinery right both
for UNIX and Windows from get-go.  Which would make the result
easier to understand.  As this is one of the more important areas,
it matters to keep the resulting code and the rationale behind it
understandable by reading "log --reverse -p".

Thanks.

Re: [PATCHv3 00/11] Expose the submodule parallelism to the user

[Stefan Beller]

On Wed, Nov 4, 2015 at 9:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Where does it apply?
>> ---
>> This series applies on top of d075d2604c0f92045caa8d5bb6ab86cf4921a4ae (Merge
>> branch 'rs/daemon-plug-child-leak' into sb/submodule-parallel-update) and replaces
>> the previous patches in sb/submodule-parallel-update
>>
>> What does it do?
>> ---
>> This series should finish the on going efforts of parallelizing
>> submodule network traffic. The patches contain tests for clone,
>> fetch and submodule update to use the actual parallelism both via
>> command line as well as a configured option. I decided to go with
>> "submodule.jobs" for all three for now.
>
> The order of patches and where the series builds makes me suspect
> that I have been expecting too much from the "parallel-fetch" topic.
>
> I've been hoping that it would be useful for the project as a whole
> to polish the other topic and make it available to wider audience
> sooner by itself (both from "end users get improved Git early"
> aspect and from "the core machinery to be reused in follow-up
> improvements are made closer to perfection sooner" perspective).  So
> I've been expecting that "Let's fix it on Windows" change directly
> on top of sb/submodule-parallel-fetch to make that topic usable
> before everything else.

I can resend the patches on top of sb/submodule-parallel-fetch
(though looking at sb/submodule-parallel-fetch..d075d2604c0f920
[Merge branch 'rs/daemon-plug-child-leak' into sb/submodule-parallel-update]
I don't expect conflicts, so it would be a verbatim resend)


> Other patches in this series may require
> the child_process_cleanup() change, so they may be applied on top of
> the merge between sb/submodule-parallel-fetch (updated for Windows)
> and rs/daemon-plug-child-leak topic.

I assumed the rs/daemon-plug-child-leak topic is no feature, but cleanup.
Which is why I would have expected a sb/submodule-parallel-fetch-for-windows
pointing at maybe the third patch of the series on top of
rs/daemon-plug-child-leak

>
> That does not seem to be what's happening here (note: I am not
> complaining; I am just trying to make sure expectation matches
> reality).  Am I reading you correctly?

I really wanted to send out just one series, my bad.
The ordering made sense to me (first the run-command related fixes
and then the new features in later patches)

>
> I think sb/submodule-parallel-fetch + sb/submodule-parallel-update
> as a single topic would need more time to mature to be in a tagged
> release than we have in the remainder of this cycle.

I agree on that.

>  It is likely
> that the former topic has a chance to get rebased after 2.7 happens.
> And that would allow us to (1) use the child_process_cleanup() from
> get-go instead of _deinit and to (2) get the machinery right both
> for UNIX and Windows from get-go.  Which would make the result
> easier to understand.  As this is one of the more important areas,
> it matters to keep the resulting code and the rationale behind it
> understandable by reading "log --reverse -p".

So you are saying that reading the Windows cleanup patch
before the s/deinit/clear/ Patch by Rene makes it way easier to understand?
Which is why you would prefer another history. (Merging an updated
sb/submodule-parallel-fetch again to  rs/daemon-plug-child-leak or even
sb/submodule-parallel-update)

Thanks,
Stefan

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The warning message is cluttering the output itself,
> so just report it once.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  run-command.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 7c00c21..3ae563f 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1012,13 +1012,21 @@ static void pp_cleanup(struct parallel_processes *pp)
>  
>  static void set_nonblocking(int fd)
>  {
> +	static int reported_degrade = 0;
>  	int flags = fcntl(fd, F_GETFL);
> -	if (flags < 0)
> -		warning("Could not get file status flags, "
> -			"output will be degraded");
> -	else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
> -		warning("Could not set file status flags, "
> -			"output will be degraded");
> +	if (flags < 0) {
> +		if (!reported_degrade) {
> +			warning("Could not get file status flags, "
> +				"output will be degraded");
> +			reported_degrade = 1;
> +		}
> +	} else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK)) {
> +		if (!reported_degrade) {
> +			warning("Could not set file status flags, "
> +				"output will be degraded");
> +			reported_degrade = 1;
> +		}
> +	}
>  }

Imagine that we are running two things A and B at the same time.  We
ask poll(2) and it says both A and B have some data ready to be
read, and we try to read from A.  strbuf_read_once() would try to
read up to 8K, relying on the fact that you earlier set the IO to be
nonblock.  It will get stuck reading from A without allowing output
from B to drain.  B's write may get stuck because we are not reading
from it, and would cause B to stop making progress.

What if the other sides of the connection from A and B are talking
with each other, and B's non-progress caused the processing for A on
the other side of the connection to block, causing it not to produce
more output to allow us to make progress reading from A (so that
eventually we can give B a chance to drain its output)?  Imagine A
and B are pushes to the same remote, B may be pushing a change to a
submodule while A may be pushing a matching change to its
superproject, and the server may be trying to make sure that the
submodule update completes and updates the ref before making the
superproject's tree that binds that updated submodule's commit
availble, for example?  Can we make any progress from that point?

I am not convinced that the failure to set nonblock IO is merely
"output will be degraded".  It feels more like a fatal error if we
are driving more than one task at the same time.

Re: [PATCHv3 00/11] Expose the submodule parallelism to the user

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> So you are saying that reading the Windows cleanup patch
> before the s/deinit/clear/ Patch by Rene makes it way easier to understand?

No.

The run-parallel API added in parallel-fetch that needs to be fixed
up (because the topic is in 'next', my bad merging prematurely) with
a separate "oops that was not friendly to Windows" is the primary
concern I have for those who later want to learn how it was designed
by going through "log --reverse -p".

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

[Stefan Beller]

On Wed, Nov 4, 2015 at 10:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The warning message is cluttering the output itself,
>> so just report it once.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  run-command.c | 20 ++++++++++++++------
>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/run-command.c b/run-command.c
>> index 7c00c21..3ae563f 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -1012,13 +1012,21 @@ static void pp_cleanup(struct parallel_processes *pp)
>>
>>  static void set_nonblocking(int fd)
>>  {
>> +     static int reported_degrade = 0;
>>       int flags = fcntl(fd, F_GETFL);
>> -     if (flags < 0)
>> -             warning("Could not get file status flags, "
>> -                     "output will be degraded");
>> -     else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
>> -             warning("Could not set file status flags, "
>> -                     "output will be degraded");
>> +     if (flags < 0) {
>> +             if (!reported_degrade) {
>> +                     warning("Could not get file status flags, "
>> +                             "output will be degraded");
>> +                     reported_degrade = 1;
>> +             }
>> +     } else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK)) {
>> +             if (!reported_degrade) {
>> +                     warning("Could not set file status flags, "
>> +                             "output will be degraded");
>> +                     reported_degrade = 1;
>> +             }
>> +     }
>>  }
>
> Imagine that we are running two things A and B at the same time.  We
> ask poll(2) and it says both A and B have some data ready to be
> read, and we try to read from A.  strbuf_read_once() would try to
> read up to 8K, relying on the fact that you earlier set the IO to be
> nonblock.  It will get stuck reading from A without allowing output
> from B to drain.  B's write may get stuck because we are not reading
> from it, and would cause B to stop making progress.
>
> What if the other sides of the connection from A and B are talking
> with each other,

I am not sure if we want to allow this ever. How would that work with
jobs==1? How do we guarantee to have A and B running at the same time?
In a later version of the parallel processing we may have some other ramping
up mechanisms, such as: "First run only one process until it outputted at least
250 bytes", which would also produce such a lock. So instead a time based ramp
up may be better. But my general concern is how much guarantees are we selling
here? Maybe the documentation needs to explicitly state that we cannot talk to
each or at least should assume the blocking of stdout/err.

> and B's non-progress caused the processing for A on
> the other side of the connection to block, causing it not to produce
> more output to allow us to make progress reading from A (so that
> eventually we can give B a chance to drain its output)?  Imagine A
> and B are pushes to the same remote, B may be pushing a change to a
> submodule while A may be pushing a matching change to its
> superproject, and the server may be trying to make sure that the
> submodule update completes and updates the ref before making the
> superproject's tree that binds that updated submodule's commit
> availble, for example?  Can we make any progress from that point?
>
> I am not convinced that the failure to set nonblock IO is merely
> "output will be degraded".  It feels more like a fatal error if we
> are driving more than one task at the same time.
>

Another approach would be to test if we can set to non blocking and if
that is not possible, do not buffer it, but redirect the subcommand
directly to stderr of the calling process.

    if (set_nonblocking(pp->children[i].process.err) < 0) {
        pp->children[i].process.err = 2;
        degraded_parallelism = 1;
    }

and once we observe the degraded_parallelism flag, we can only
schedule a maximum of one job at a time, having direct output?

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

[Johannes Sixt]

Am 04.11.2015 um 21:14 schrieb Stefan Beller:
> On Wed, Nov 4, 2015 at 10:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Imagine that we are running two things A and B at the same time.  We
>> ask poll(2) and it says both A and B have some data ready to be
>> read, and we try to read from A.  strbuf_read_once() would try to
>> read up to 8K, relying on the fact that you earlier set the IO to be
>> nonblock.  It will get stuck reading from A without allowing output
>> from B to drain.  B's write may get stuck because we are not reading
>> from it, and would cause B to stop making progress.
>>
>> What if the other sides of the connection from A and B are talking
>> with each other,
>
> I am not sure if we want to allow this ever. How would that work with
> jobs==1? How do we guarantee to have A and B running at the same time?

I think that a scenario where A and B are communicating is rather 
far-fetched. We are talking about parallelizing independent tasks. I 
would not worry.

-- Hannes

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Another approach would be to test if we can set to non blocking and if
> that is not possible, do not buffer it, but redirect the subcommand
> directly to stderr of the calling process.
>
>     if (set_nonblocking(pp->children[i].process.err) < 0) {
>         pp->children[i].process.err = 2;
>         degraded_parallelism = 1;
>     }
>
> and once we observe the degraded_parallelism flag, we can only
> schedule a maximum of one job at a time, having direct output?

I would even say that on a platform that is _capable_ of setting fd
non-blocking, we should signal a grave error and die if an attempt
to do so fails, period.

On the other hand, on a platform that is known to be incapable
(e.g. lacks SETFL or NONBLOCK), we have two options.

1. If we can arrange to omit the intermediary buffer processing
   without butchering the flow of the main logic with many
   #ifdef..#endif, then that would make a lot of sense to do so, and
   running the processes in parallel with mixed output might be OK.
   It may not be very nice, but should be an acceptable compromise.

2. If we need to sprinkle conditional compilation all over the place
   to do so, then I do not think it is worth it.  Instead, we should
   keep a single code structure, and forbid setting numtasks to more
   than one, which would also remove the need for nonblock IO.

Either way, bringing "parallelism with sequential output" to
platforms without nonblock IO can be left for a later day, when we
find either (1) a good approach that does not require nonblock IO to
do this, or (2) a good approach to do a nonblock IO on these
platforms (we know about Windows, but there may be others; I dunno).

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> I think that a scenario where A and B are communicating is rather
> far-fetched. We are talking about parallelizing independent tasks. I
> would not worry.

I wouldn't worry too much if this were merely a hack that is only
applicable to submodules, but I do not think it is a healthy
attitude to dismiss potential problem as far-fetched without
thinking things through, when you are designing what goes into
the run-command API.

I'd grant you that a complete deadlock is unlikely to be a problem
on its own.  Somewhere somebody will eventually time out and unblock
the deadlock anyway.

But the symptom does not have to be as severe as a total deadlock to
be problematic.  If we block B (and other tasks) by not reading from
them quickly because we are blocked on reading from A, which may
take forever (in timescale of B and other tasks) to feed us enough
to satisfy strbuf_read_once(), we are wasting resource by spawning B
(and other tasks) early when we are not prepared to service them
well, on both our end and on the other side of the connection.

By the way, A and B do not have to be directly communicating to
deadlock.  The underlying system, either the remote end or the local
end or even a relaying system in between (think: network) can
throttle to cause the same symptom without A and B knowing (which
was the example I gave).

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

[Stefan Beller]

On Wed, Nov 4, 2015 at 12:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Another approach would be to test if we can set to non blocking and if
>> that is not possible, do not buffer it, but redirect the subcommand
>> directly to stderr of the calling process.
>>
>>     if (set_nonblocking(pp->children[i].process.err) < 0) {
>>         pp->children[i].process.err = 2;
>>         degraded_parallelism = 1;
>>     }
>>
>> and once we observe the degraded_parallelism flag, we can only
>> schedule a maximum of one job at a time, having direct output?
>
> I would even say that on a platform that is _capable_ of setting fd
> non-blocking, we should signal a grave error and die if an attempt
> to do so fails, period.

So more like:

    if (platform_capable_non_blocking_IO())
        set_nonblocking_or_die(&pp->children[i].process.err);
    else
        pp->children[i].process.err = 2; /* ugly intermixed output is possible*/

>
> On the other hand, on a platform that is known to be incapable
> (e.g. lacks SETFL or NONBLOCK), we have two options.
>
> 1. If we can arrange to omit the intermediary buffer processing
>    without butchering the flow of the main logic with many
>    #ifdef..#endif, then that would make a lot of sense to do so, and
>    running the processes in parallel with mixed output might be OK.
>    It may not be very nice, but should be an acceptable compromise.

>From what I hear this kind of output is very annoying. (One of the
main complaints of repo users beside missing atomic fetch transactions)

>
> 2. If we need to sprinkle conditional compilation all over the place
>    to do so, then I do not think it is worth it.  Instead, we should
>    keep a single code structure, and forbid setting numtasks to more
>    than one, which would also remove the need for nonblock IO.

So additional to the code above, we can add the
platform_capable_non_blocking_IO() condition to either the ramp up process,
or have a

    if (!platform_capable_non_blocking_IO())
        pp.max_processes = 1;

in the init phase. Then we have only 2 places that deal with the
problem, no #ifdefs
elsewhere.

>
> Either way, bringing "parallelism with sequential output" to
> platforms without nonblock IO can be left for a later day, when we
> find either (1) a good approach that does not require nonblock IO to
> do this, or (2) a good approach to do a nonblock IO on these
> platforms (we know about Windows, but there may be others; I dunno).
>

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> So more like:
>
>     if (platform_capable_non_blocking_IO())
>         set_nonblocking_or_die(&pp->children[i].process.err);
>     else
>         pp->children[i].process.err = 2; /* ugly intermixed output is possible*/

When I mentioned #if..#endif, I didn't mean it as a dogmatic
"conditional compilation is wrong" sense.  It was more about "the
high-level flow of logic should not have to know too much about
platform peculiarities".  As platform_capable_non_blocking_IO()
function would be a constant function after you compile it for a
single platform, if you add 10 instances of such if/else in a patch
that adds 250 lines, unless the change is to add a set of lowest
level helpers to be called from the higher-level flow of logic so
that the callers do not have to know about the platform details,
that's just as bad as adding 10 instances of #if..#endif.

>> On the other hand, on a platform that is known to be incapable
>> (e.g. lacks SETFL or NONBLOCK), we have two options.
>>
>> 1. If we can arrange to omit the intermediary buffer processing
>>    without butchering the flow of the main logic with many
>>    #ifdef..#endif, then that would make a lot of sense to do so, and
>>    running the processes in parallel with mixed output might be OK.
>>    It may not be very nice, but should be an acceptable compromise.
>
> From what I hear this kind of output is very annoying. (One of the
> main complaints of repo users beside missing atomic fetch transactions)

When (1) "parallelism with sequential output" is the desired
outcome, (2) on some platforms we haven't found a way to achieve
both, and (3) a non-sequential output is unacceptable, then
parallelism has to give :-(.

I was getting an impression from your "not buffer" suggestion that
"sequential output" would be the one that can be sacrificed, but
that is OK.  Until we find a way to achieve both at the same time,
achieving only either one or the other is better than achieving
nothing.

>> Either way, bringing "parallelism with sequential output" to
>> platforms without nonblock IO can be left for a later day, when we
>> find either (1) a good approach that does not require nonblock IO to
>> do this, or (2) a good approach to do a nonblock IO on these
>> platforms (we know about Windows, but there may be others; I dunno).

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

[Stefan Beller]

On Wed, Nov 4, 2015 at 1:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> So more like:
>>
>>     if (platform_capable_non_blocking_IO())
>>         set_nonblocking_or_die(&pp->children[i].process.err);
>>     else
>>         pp->children[i].process.err = 2; /* ugly intermixed output is possible*/
>
> When I mentioned #if..#endif, I didn't mean it as a dogmatic
> "conditional compilation is wrong" sense.  It was more about "the
> high-level flow of logic should not have to know too much about
> platform peculiarities".  As platform_capable_non_blocking_IO()
> function would be a constant function after you compile it for a
> single platform, if you add 10 instances of such if/else in a patch
> that adds 250 lines, unless the change is to add a set of lowest
> level helpers to be called from the higher-level flow of logic so
> that the callers do not have to know about the platform details,
> that's just as bad as adding 10 instances of #if..#endif.

Right. Yeah I was just outlining the program flow as I understood it.

Specially:
 * It's fine if the platform doesn't support non blocking IO
 * but if the platform claims to support it and fails to, this is a hard error.
    How is this worse than the platform claiming to not support it at all?

I mean another solution there would be to try to set the non blocking IO
and if that fails we put that process-to-be-started into a special queue,
which will start the process once the stderr channel is not occupied any more
(i.e. the process which is the current output owner has ended or there is
no such process)

That way we would fall back to no parallelism in Windows and could
even gracefully fallback in other systems, which suddenly have problems
setting non blocking IO. (Though I suspect that isn't required here).

>
>>> On the other hand, on a platform that is known to be incapable
>>> (e.g. lacks SETFL or NONBLOCK), we have two options.
>>>
>>> 1. If we can arrange to omit the intermediary buffer processing
>>>    without butchering the flow of the main logic with many
>>>    #ifdef..#endif, then that would make a lot of sense to do so, and
>>>    running the processes in parallel with mixed output might be OK.
>>>    It may not be very nice, but should be an acceptable compromise.
>>
>> From what I hear this kind of output is very annoying. (One of the
>> main complaints of repo users beside missing atomic fetch transactions)
>
> When (1) "parallelism with sequential output" is the desired
> outcome, (2) on some platforms we haven't found a way to achieve
> both, and (3) a non-sequential output is unacceptable, then
> parallelism has to give :-(.

Ok, then I will go that route.

>
> I was getting an impression from your "not buffer" suggestion that
> "sequential output" would be the one that can be sacrificed, but
> that is OK.  Until we find a way to achieve both at the same time,
> achieving only either one or the other is better than achieving
> nothing.

I was not sure what is better to sacrifice. Scarifying parallelism sounds
safer to me (no apparent change compared to before).

>
>>> Either way, bringing "parallelism with sequential output" to
>>> platforms without nonblock IO can be left for a later day, when we
>>> find either (1) a good approach that does not require nonblock IO to
>>> do this, or (2) a good approach to do a nonblock IO on these
>>> platforms (we know about Windows, but there may be others; I dunno).

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

[Jeff King]

On Wed, Nov 04, 2015 at 01:01:53PM -0800, Junio C Hamano wrote:

> But the symptom does not have to be as severe as a total deadlock to
> be problematic.  If we block B (and other tasks) by not reading from
> them quickly because we are blocked on reading from A, which may
> take forever (in timescale of B and other tasks) to feed us enough
> to satisfy strbuf_read_once(), we are wasting resource by spawning B
> (and other tasks) early when we are not prepared to service them
> well, on both our end and on the other side of the connection.

I'm not sure I understand this line of reasoning. It is entirely
possible that I have not been paying close enough attention and am
missing something subtle, so please feel free to hit me with the clue
stick.

But why would we ever block reading from A? If poll() reported to us
that "A" is ready to read, and we call strbuf_read_once(), we will make
a _single_ read call (which was, after all, the point of adding
strbuf_read_once in the first place).

So even if descriptor "A" isn't non-blocking, why would we block? Only
if the OS told us we are ready to read via poll(), but we are somehow
not (which, AFAIK, would be a bug in the OS).

So I'm not sure I see why we need to be non-blocking at all here, if we
are correctly hitting poll() and doing a single read on anybody who
claims to be ready (rather than trying to soak up all of their available
data), then we should never block, and we should never starve one
process (even without blocking, we could be in a busy loop slurping from
A and starve B, but by hitting the descriptors in round-robin for each
poll(), we make sure they all progress).

What am I missing?

-Peff

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So I'm not sure I see why we need to be non-blocking at all here, if we
> are correctly hitting poll() and doing a single read on anybody who
> claims to be ready (rather than trying to soak up all of their available
> data), then we should never block, and we should never starve one
> process (even without blocking, we could be in a busy loop slurping from
> A and starve B, but by hitting the descriptors in round-robin for each
> poll(), we make sure they all progress).
>
> What am I missing?

I've always assumed that the original reason why we wanted to set
the fd to nonblock was because poll(2) only tells us there is
something to read (even a single byte), and the xread_nonblock()
call strbuf_read_once() makes with the default size of 8KB is
allowed to consume all available bytes and then get stuck waiting
for the remainder of 8KB before returning.

If the read(2) in xread_nonblock() always returns as soon as we
receive as much as there is data available without waiting for any
more, ignoring the size of the buffer (rather, taking the size of
the buffer only as the upper bound), then there is no need for
nonblock anywhere.

So perhaps the original reasoning of doing nonblock was faulty, you
are saying?

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

[Jeff King]

On Wed, Nov 04, 2015 at 06:05:17PM -0800, Junio C Hamano wrote:

> I've always assumed that the original reason why we wanted to set
> the fd to nonblock was because poll(2) only tells us there is
> something to read (even a single byte), and the xread_nonblock()
> call strbuf_read_once() makes with the default size of 8KB is
> allowed to consume all available bytes and then get stuck waiting
> for the remainder of 8KB before returning.
> 
> If the read(2) in xread_nonblock() always returns as soon as we
> receive as much as there is data available without waiting for any
> more, ignoring the size of the buffer (rather, taking the size of
> the buffer only as the upper bound), then there is no need for
> nonblock anywhere.

This latter paragraph was my impression of how pipe reading generally
worked, for blocking or non-blocking. That is, if there is data, both
cases return what we have (up to the length specified by the user), and
it is only when there is _no_ data that we might choose to block.

It's easy to verify experimentally. E.g.:

  perl -e 'while(1) { syswrite(STDOUT, "a", 1); sleep(1); }' |
  strace perl -e 'while(1) { sysread(STDIN, my $buf, 1024) }'

should show a series of 1-byte reads. But of course that only shows that
it works on my system[1], not everywhere.

POSIX implies it is the case in the definition of read[2] in two ways:

  1. The O_NONBLOCK behavior for pipes is mentioned only when dealing
     with empty pipes.

  2. Later, it says:

       The value returned may be less than nbyte if the number of bytes
       left in the file is less than nbyte, if the read() request was
       interrupted by a signal, or if the file is a pipe or FIFO or
       special file and has fewer than nbyte bytes immediately available
       for reading.

     That is not explicit, but the "immediately" there seems to imply
     it.

> So perhaps the original reasoning of doing nonblock was faulty, you
> are saying?

Exactly. And therefore a convenient way to deal with the portability
issue is to get rid of it. :)

-Peff

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> POSIX implies it is the case in the definition of read[2] in two ways:
>
>   1. The O_NONBLOCK behavior for pipes is mentioned only when dealing
>      with empty pipes.
>
>   2. Later, it says:
>
>        The value returned may be less than nbyte if the number of bytes
>        left in the file is less than nbyte, if the read() request was
>        interrupted by a signal, or if the file is a pipe or FIFO or
>        special file and has fewer than nbyte bytes immediately available
>        for reading.
>
>      That is not explicit, but the "immediately" there seems to imply
>      it.

We were reading the same book, but I was more worried about that
"may" there; it merely tells the caller of read(2) not to be alarmed
when the call returned without filling the entire buffer, without
mandating the implementation of read(2) never to block.

Having said that,...

>> So perhaps the original reasoning of doing nonblock was faulty, you
>> are saying?
>
> Exactly. And therefore a convenient way to deal with the portability
> issue is to get rid of it. :)

... I do like the simplification you alluded to in the other
message.  Not having to worry about the nonblock (at least until it
is found problematic in the real world) is a very good first step,
especially because the approach allows us to collectively make
progress by letting all of us in various platforms build and
experiment with "something that works".

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

[Stefan Beller]

On Wed, Nov 4, 2015 at 11:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> POSIX implies it is the case in the definition of read[2] in two ways:
>>
>>   1. The O_NONBLOCK behavior for pipes is mentioned only when dealing
>>      with empty pipes.
>>
>>   2. Later, it says:
>>
>>        The value returned may be less than nbyte if the number of bytes
>>        left in the file is less than nbyte, if the read() request was
>>        interrupted by a signal, or if the file is a pipe or FIFO or
>>        special file and has fewer than nbyte bytes immediately available
>>        for reading.
>>
>>      That is not explicit, but the "immediately" there seems to imply
>>      it.
>
> We were reading the same book, but I was more worried about that
> "may" there; it merely tells the caller of read(2) not to be alarmed
> when the call returned without filling the entire buffer, without
> mandating the implementation of read(2) never to block.
>
> Having said that,...
>
>>> So perhaps the original reasoning of doing nonblock was faulty, you
>>> are saying?

I agree that the original reasoning was faulty. It happened in the first place,
because of how I approached the problem. (strbuf_read should return immediately
after reading and to communicate that we had non blocking read and checked for
EAGAIN).

Having read the man pages again, I agree with you that the non blocking is
bogus to begin with.

>>
>> Exactly. And therefore a convenient way to deal with the portability
>> issue is to get rid of it. :)
>
> ... I do like the simplification you alluded to in the other
> message.  Not having to worry about the nonblock (at least until it
> is found problematic in the real world) is a very good first step,
> especially because the approach allows us to collectively make
> progress by letting all of us in various platforms build and
> experiment with "something that works".

I'll send a patch to just remove set_nonblocking which should fix the compile
problems on Windows and make it work regardless on all platforms.

After that I continue with the update series.

Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option

[Jens Lehmann]

Am 04.11.2015 um 01:37 schrieb 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>
> ---
>   Documentation/config.txt    |  7 +++++++
>   builtin/fetch.c             |  2 +-
>   submodule-config.c          | 15 +++++++++++++++
>   submodule-config.h          |  2 ++
>   submodule.c                 |  5 +++++
>   t/t5526-fetch-submodules.sh | 14 ++++++++++++++
>   6 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 391a0c3..70e1b88 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2643,6 +2643,13 @@ submodule.<name>.ignore::
>   	"--ignore-submodules" option. The 'git submodule' commands are not
>   	affected by this setting.
>
> +submodule.jobs::
> +	This is used to determine how many submodules can be operated on in
> +	parallel. Specifying a positive integer allows up to that number
> +	of submodules being fetched in parallel. This is used in fetch
> +	and clone operations only. A value of 0 will give some reasonable
> +	configuration. It defaults to 1.
> +

Just curious (and sorry if this has already been discussed and I missed
it, but the volume of your output is too much for my current git time
budget ;-): While this config is for fetching only, do I recall correctly
that you have plans to do submodule work tree updates in parallel too?
If so, would it make sense to have different settings for fetching and
updating?

>   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 9cc1c9d..60e6797 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-config.c b/submodule-config.c
> index 29e21b2..475551a 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -32,6 +32,7 @@ enum lookup_type {
>
>   static struct submodule_cache cache;
>   static int is_cache_init;
> +static int parallel_jobs = -1;
>
>   static int config_path_cmp(const struct submodule_entry *a,
>   			   const struct submodule_entry *b,
> @@ -239,6 +240,15 @@ static int parse_generic_submodule_config(const char *key,
>   					  const char *value,
>   					  struct parse_config_parameter *me)
>   {
> +	if (!strcmp(key, "jobs")) {
> +		parallel_jobs = strtol(value, NULL, 10);
> +		if (parallel_jobs < 0) {
> +			warning("submodule.jobs not allowed to be negative.");
> +			parallel_jobs = 1;
> +			return 1;
> +		}
> +	}
> +
>   	return 0;
>   }
>
> @@ -482,3 +492,8 @@ void submodule_free(void)
>   	cache_free(&cache);
>   	is_cache_init = 0;
>   }
> +
> +int config_parallel_submodules(void)
> +{
> +	return parallel_jobs;
> +}
> diff --git a/submodule-config.h b/submodule-config.h
> index f9e2a29..d9bbf9a 100644
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -27,4 +27,6 @@ const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
>   		const char *path);
>   void submodule_free(void);
>
> +int config_parallel_submodules(void);
> +
>   #endif /* SUBMODULE_CONFIG_H */
> diff --git a/submodule.c b/submodule.c
> index 0257ea3..188ba02 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -752,6 +752,11 @@ 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 = config_parallel_submodules();
> +	if (max_parallel_jobs < 0)
> +		max_parallel_jobs = 1;
> +
>   	calculate_changed_submodule_paths();
>   	run_processes_parallel(max_parallel_jobs,
>   			       get_next_submodule,
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index 1b4ce69..5c3579c 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -470,4 +470,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 children" trace.out &&
> +		git config submodule.jobs 8 &&
> +		GIT_TRACE=$(pwd)/trace.out git fetch &&
> +		grep "8 children" trace.out &&
> +		GIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&
> +		grep "9 children" trace.out
> +	)
> +'
> +
>   test_done
>

Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option

[Stefan Beller]

On Tue, Nov 10, 2015 at 2:21 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>> +submodule.jobs::
>> +       This is used to determine how many submodules can be operated on in
>> +       parallel. Specifying a positive integer allows up to that number
>> +       of submodules being fetched in parallel. This is used in fetch
>> +       and clone operations only. A value of 0 will give some reasonable
>> +       configuration. It defaults to 1.
>> +
>
>
> Just curious (and sorry if this has already been discussed and I missed
> it, but the volume of your output is too much for my current git time
> budget ;-): While this config is for fetching only, do I recall correctly
> that you have plans to do submodule work tree updates in parallel too?
> If so, would it make sense to have different settings for fetching and
> updating?

TL;DR: checkout is serial, network-related stuff only will be using
submodule.jobs

In the next series (origin/sb/submodule-parallel-update) this is reused for
fetches, clones, so only the network stuff. The checkout (as all local
operations)
is still done serially, as then you don't run into problems in
parallel at the same time.
(checkouts may be parallelized but I haven't done that yet, and postpone that
until it has settled a bit more)

Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option

[Jens Lehmann]

Am 10.11.2015 um 23:29 schrieb Stefan Beller:
> On Tue, Nov 10, 2015 at 2:21 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>> +submodule.jobs::
>>> +       This is used to determine how many submodules can be operated on in
>>> +       parallel. Specifying a positive integer allows up to that number
>>> +       of submodules being fetched in parallel. This is used in fetch
>>> +       and clone operations only. A value of 0 will give some reasonable
>>> +       configuration. It defaults to 1.
>>> +
>>
>>
>> Just curious (and sorry if this has already been discussed and I missed
>> it, but the volume of your output is too much for my current git time
>> budget ;-): While this config is for fetching only, do I recall correctly
>> that you have plans to do submodule work tree updates in parallel too?
>> If so, would it make sense to have different settings for fetching and
>> updating?
>
> TL;DR: checkout is serial, network-related stuff only will be using
> submodule.jobs

My point being: isn't "jobs" a bit too generic for a config option that
is only relevant for network-related stuff? Maybe "submodule.fetchJobs"
or similar would be better, as you are already thinking about adding
other parallelisms with different constraints later?

> In the next series (origin/sb/submodule-parallel-update) this is reused for
> fetches, clones, so only the network stuff. The checkout (as all local
> operations)
> is still done serially, as then you don't run into problems in
> parallel at the same time.
> (checkouts may be parallelized but I haven't done that yet, and postpone that
> until it has settled a bit more)

Makes sense.

Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option

[Stefan Beller]

On Wed, Nov 11, 2015 at 11:55 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>
>> TL;DR: checkout is serial, network-related stuff only will be using
>> submodule.jobs
>
>
> My point being: isn't "jobs" a bit too generic for a config option that
> is only relevant for network-related stuff? Maybe "submodule.fetchJobs"
> or similar would be better, as you are already thinking about adding
> other parallelisms with different constraints later?

Actually I don't think that far ahead.

(I assume network to be the bottleneck for clone/fetch operations)
All I want is a saturated network all the time, and as the native git protocol
doesn't provide that (tcp startup takes time until full band witdth is reached,
local operations both on client and server) I added the parallel stuff
to 'smear' different submodule network traffics along the timeline,
such that we have a better approximation of an always fully saturated link
for the whole operation. So in the long term future, we maybe want to
reuse an http/ssh session for a different submodule, possibly interleaving
the different submodules on the wire to make it even faster. Though that
may not be helping much.

So we're back at bike shedding about the name. submodule.fetchJobs
sounds like it only applies to fetching, do you think it's sufficient for clone
as well?

Once upon a time, Junio used  'submodule.fetchParallel' or  'submodule.paralle'
in a discussion[1] for the distinction of the local and networked things.
[1] Discussing "[PATCH] Add fetch.recurseSubmoduleParallelism config option"

How about submodules.parallelNetwork for the networking part and
submodules.parallelLocal for the local part? (I don't implement parallelLocal in
the next few weeks I'd estimate).

Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option

[Jens Lehmann]

Am 12.11.2015 um 00:34 schrieb Stefan Beller:
> On Wed, Nov 11, 2015 at 11:55 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>>
>>> TL;DR: checkout is serial, network-related stuff only will be using
>>> submodule.jobs
>>
>>
>> My point being: isn't "jobs" a bit too generic for a config option that
>> is only relevant for network-related stuff? Maybe "submodule.fetchJobs"
>> or similar would be better, as you are already thinking about adding
>> other parallelisms with different constraints later?
>
> Actually I don't think that far ahead.

Maybe I've been bitten once too often by too generic names that became
a problem later on ... ;-)

> (I assume network to be the bottleneck for clone/fetch operations)
> All I want is a saturated network all the time, and as the native git protocol
> doesn't provide that (tcp startup takes time until full band witdth is reached,
> local operations both on client and server) I added the parallel stuff
> to 'smear' different submodule network traffics along the timeline,
> such that we have a better approximation of an always fully saturated link
> for the whole operation. So in the long term future, we maybe want to
> reuse an http/ssh session for a different submodule, possibly interleaving
> the different submodules on the wire to make it even faster. Though that
> may not be helping much.
>
> So we're back at bike shedding about the name. submodule.fetchJobs
> sounds like it only applies to fetching, do you think it's sufficient for clone
> as well?

Hmm, to me fetching is a part of cloning, so I don't have a problem with
that. And documenting it accordingly should make it clear to everyone.

> Once upon a time, Junio used  'submodule.fetchParallel' or  'submodule.paralle'
> in a discussion[1] for the distinction of the local and networked things.
> [1] Discussing "[PATCH] Add fetch.recurseSubmoduleParallelism config option"
>
> How about submodules.parallelNetwork for the networking part and
> submodules.parallelLocal for the local part? (I don't implement parallelLocal in
> the next few weeks I'd estimate).

If 'submodules.parallelNetwork' will be used for submodule push too as
soon as that learns parallel operation, I'm ok with that. But if we don't
have good reason to believe the number of jobs for fetch can simply be
reused for push, me thinks we should have one config option containing the
term "fetch" now and another that contains "push" when we need it later,
just to be on the safe side. Otherwise it might be hard to explain to
users why 'submodules.parallelNetwork' is only used for fetch and clone
and why they have to set 'submodules.parallelPush' for pushing ...

So either 'submodule.fetchParallel' or 'submodule.fetchJobs' is fine for
me, and 'submodules.parallelNetwork' is ok too as long as we have reason
to believe this value can be used for push later too.

Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option

[Stefan Beller]

On Fri, Nov 13, 2015 at 12:47 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> Am 12.11.2015 um 00:34 schrieb Stefan Beller:
>>
>> On Wed, Nov 11, 2015 at 11:55 AM, Jens Lehmann <Jens.Lehmann@web.de>
>> wrote:
>>>>
>>>>
>>>> TL;DR: checkout is serial, network-related stuff only will be using
>>>> submodule.jobs
>>>
>>>
>>>
>>> My point being: isn't "jobs" a bit too generic for a config option that
>>> is only relevant for network-related stuff? Maybe "submodule.fetchJobs"
>>> or similar would be better, as you are already thinking about adding
>>> other parallelisms with different constraints later?
>>
>>
>> Actually I don't think that far ahead.
>
>
> Maybe I've been bitten once too often by too generic names that became
> a problem later on ... ;-)
>
>> (I assume network to be the bottleneck for clone/fetch operations)
>> All I want is a saturated network all the time, and as the native git
>> protocol
>> doesn't provide that (tcp startup takes time until full band witdth is
>> reached,
>> local operations both on client and server) I added the parallel stuff
>> to 'smear' different submodule network traffics along the timeline,
>> such that we have a better approximation of an always fully saturated link
>> for the whole operation. So in the long term future, we maybe want to
>> reuse an http/ssh session for a different submodule, possibly interleaving
>> the different submodules on the wire to make it even faster. Though that
>> may not be helping much.
>>
>> So we're back at bike shedding about the name. submodule.fetchJobs
>> sounds like it only applies to fetching, do you think it's sufficient for
>> clone
>> as well?
>
>
> Hmm, to me fetching is a part of cloning, so I don't have a problem with
> that. And documenting it accordingly should make it clear to everyone.
>
>> Once upon a time, Junio used  'submodule.fetchParallel' or
>> 'submodule.paralle'
>> in a discussion[1] for the distinction of the local and networked things.
>> [1] Discussing "[PATCH] Add fetch.recurseSubmoduleParallelism config
>> option"
>>
>> How about submodules.parallelNetwork for the networking part and
>> submodules.parallelLocal for the local part? (I don't implement
>> parallelLocal in
>> the next few weeks I'd estimate).
>
>
> If 'submodules.parallelNetwork' will be used for submodule push too as
> soon as that learns parallel operation, I'm ok with that. But if we don't
> have good reason to believe the number of jobs for fetch can simply be
> reused for push, me thinks we should have one config option containing the
> term "fetch" now and another that contains "push" when we need it later,
> just to be on the safe side. Otherwise it might be hard to explain to
> users why 'submodules.parallelNetwork' is only used for fetch and clone
> and why they have to set 'submodules.parallelPush' for pushing ...
>
> So either 'submodule.fetchParallel' or 'submodule.fetchJobs' is fine for
> me, and 'submodules.parallelNetwork' is ok too as long as we have reason
> to believe this value can be used for push later too.

Ok, got it. So fetchJobs is fine with me.
Mind the difference in the first part, submodule[s] in singular/plural.
I thought submodule as a prefix for any individual submodule, but any
settings applying to all of the submodules, you'd take the plural submodules.*
settings.