threads / patch / 40201

patch, 9 partsthread-utils: add a threaded task queue

Subject: [PATCH 4/9] thread-utils: add a threaded task queue

## tl;dr

33 messages between Aug 28, 2015 and Aug 31, 2015. Diffs are folded; open one to read it.

replies: 32people: 5as markdown or json

Stefan Beller· Aug 28, 2015, 01:14 UTC · lore

[PATCH 0/9] Progress with git submodule

This series replaces origin/sb/submodule-helper and is based on 5a1ba6b48a62bf55f9c8305d9850c3a8d22365c5, (Merge 'hv/submodule-config' to 'sb/submodule-helper', which includes jk/git-path and hv/submodule-config)

What changed?
* The help text of the submodule--helper was adapted to our standard,
  and removed long line, outdated docs, thanks Dscho!
  
* The run-command API was extended to be able to sync the output from
  various commands running at the same time.
  (In the previous series this was a one-shot only solution cooked for
   git submodule foreach_parallel)
   
* `git fetch recurse-submodules` will fetch the submodules in parallel!

Any feedback welcome, specially on patch 4..7

Thanks, Stefan

Stefan Beller (9):
  submodule: implement `module_list` as a builtin helper
  submodule: implement `module_name` as a builtin helper
  submodule: implement `module_clone` as a builtin helper
  thread-utils: add a threaded task queue
  run-command: add synced output
  submodule: helper to run foreach in parallel
  fetch: fetch submodules in parallel
  index-pack: Use the new worker pool
  pack-objects: Use new worker pool
 .gitignore                      |   1 +
 Documentation/fetch-options.txt |   7 +
 Makefile                        |   1 +
 builtin.h                       |   1 +
 builtin/fetch.c                 |   6 +-
 builtin/index-pack.c            |  23 +--
 builtin/pack-objects.c          | 175 ++++++-----------
 builtin/pull.c                  |   6 +
 builtin/submodule--helper.c     | 417 ++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh                | 177 +++--------------
 git.c                           |   1 +
 run-command.c                   |  99 ++++++++--
 run-command.h                   |  21 ++
 submodule.c                     | 100 ++++++++--
 submodule.h                     |   2 +-
 t/t7407-submodule-foreach.sh    |  11 ++
 thread-utils.c                  | 192 ++++++++++++++++++
 thread-utils.h                  |  35 ++++
 18 files changed, 960 insertions(+), 315 deletions(-)
 create mode 100644 builtin/submodule--helper.c
-- 
2.5.0.264.g5e52b0d
Stefan Beller· Aug 28, 2015, 01:14 UTC · re: Stefan Beller · lore

[PATCH 1/9] submodule: implement `module_list` as a builtin helper

Most of the submodule operations work on a set of submodules. Calculating and using this set is usually done via:

       module_list "$@" | {
           while read mode sha1 stage sm_path
           do
                # the actual operation
           done
       }

Currently the function `module_list` is implemented in the git-submodule.sh as a shell script wrapping a perl script. The rewrite is in C, such that it is faster and can later be easily adapted when other functions are rewritten in C.

git-submodule.sh similar to the builtin commands will navigate to the top most directory of the repository and keeping the subdirectories as a variable. As the helper is called from within the git-submodule.sh script, we are already navigated to the root level, but the path arguments are stil relative to the subdirectory we were in when calling git-submodule.sh. That's why there is a `--prefix` option pointing to an alternative path where to anchor relative path arguments.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 .gitignore                  |   1 +
 Makefile                    |   1 +
 builtin.h                   |   1 +
 builtin/submodule--helper.c | 114 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  54 +++------------------
 git.c                       |   1 +
 6 files changed, 124 insertions(+), 48 deletions(-)
 create mode 100644 builtin/submodule--helper.c
Show changes to 6 files +124 −48

.gitignore, Makefile, builtin.h, builtin/submodule--helper.c, git-submodule.sh, git.c

diff --git a/.gitignore b/.gitignore
index 4fd81ba..1c2f832 100644
--- a/.gitignore
+++ b/.gitignore
@@ -155,6 +155,7 @@
 /git-status
 /git-stripspace
 /git-submodule
+/git-submodule--helper
 /git-svn
 /git-symbolic-ref
 /git-tag
diff --git a/Makefile b/Makefile
index 24b636d..d434e73 100644
--- a/Makefile
+++ b/Makefile
@@ -901,6 +901,7 @@ BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-ref.o
 BUILTIN_OBJS += builtin/stripspace.o
+BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
 BUILTIN_OBJS += builtin/tag.o
 BUILTIN_OBJS += builtin/unpack-file.o
diff --git a/builtin.h b/builtin.h
index 839483d..924e6c4 100644
--- a/builtin.h
+++ b/builtin.h
@@ -119,6 +119,7 @@ extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
+extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
 extern int cmd_tag(int argc, const char **argv, const char *prefix);
 extern int cmd_tar_tree(int argc, const char **argv, const char *prefix);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
new file mode 100644
index 0000000..beaab7d
--- /dev/null
+++ b/builtin/submodule--helper.c
@@ -0,0 +1,114 @@
+#include "builtin.h"
+#include "cache.h"
+#include "parse-options.h"
+#include "quote.h"
+#include "pathspec.h"
+#include "dir.h"
+#include "utf8.h"
+
+static const struct cache_entry **ce_entries;
+static int ce_alloc, ce_used;
+static const char *alternative_path;
+
+static int module_list_compute(int argc, const char **argv,
+				const char *prefix,
+				struct pathspec *pathspec)
+{
+	int i;
+	char *max_prefix, *ps_matched = NULL;
+	int max_prefix_len;
+	parse_pathspec(pathspec, 0,
+		       PATHSPEC_PREFER_FULL |
+		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
+		       prefix, argv);
+
+	/* Find common prefix for all pathspec's */
+	max_prefix = common_prefix(pathspec);
+	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
+
+	if (pathspec->nr)
+		ps_matched = xcalloc(pathspec->nr, 1);
+
+	if (read_cache() < 0)
+		die("index file corrupt");
+
+	for (i = 0; i < active_nr; i++) {
+		const struct cache_entry *ce = active_cache[i];
+
+		if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
+				    max_prefix_len, ps_matched,
+				    S_ISGITLINK(ce->ce_mode) | S_ISDIR(ce->ce_mode)))
+			continue;
+
+		if (S_ISGITLINK(ce->ce_mode)) {
+			ALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);
+			ce_entries[ce_used++] = ce;
+		}
+
+		while (i + 1 < active_nr && !strcmp(ce->name, active_cache[i + 1]->name))
+			/*
+			 * Skip entries with the same name in different stages
+			 * to make sure an entry is returned only once.
+			 */
+			i++;
+	}
+	free(max_prefix);
+
+	if (ps_matched && report_path_error(ps_matched, pathspec, prefix))
+		return -1;
+
+	return 0;
+}
+
+static int module_list(int argc, const char **argv, const char *prefix)
+{
+	int i;
+	static struct pathspec pathspec;
+
+	struct option module_list_options[] = {
+		OPT_STRING(0, "prefix", &alternative_path,
+			   N_("path"),
+			   N_("alternative anchor for relative paths")),
+		OPT_END()
+	};
+
+	static const char * const git_submodule_helper_usage[] = {
+		N_("git submodule--helper module_list [--prefix=<path>] [<path>...]"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_list_options,
+			     git_submodule_helper_usage, 0);
+
+	if (module_list_compute(argc, argv, alternative_path
+					    ? alternative_path
+					    : prefix, &pathspec) < 0) {
+		printf("#unmatched\n");
+		return 1;
+	}
+
+	for (i = 0; i < ce_used; i++) {
+		const struct cache_entry *ce = ce_entries[i];
+
+		if (ce_stage(ce)) {
+			printf("%06o %s U\t", ce->ce_mode, sha1_to_hex(null_sha1));
+		} else {
+			printf("%06o %s %d\t", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce));
+		}
+
+		utf8_fprintf(stdout, "%s\n", ce->name);
+	}
+	return 0;
+}
+
+int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
+{
+	if (argc < 2)
+		goto usage;
+
+	if (!strcmp(argv[1], "module_list"))
+		return module_list(argc - 1, argv + 1, prefix);
+
+usage:
+	usage("git submodule--helper module_list\n");
+}
diff --git a/git-submodule.sh b/git-submodule.sh
index 36797c3..af9ecef 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -145,48 +145,6 @@ relative_path ()
 	echo "$result$target"
 }
 
-#
-# Get submodule info for registered submodules
-# $@ = path to limit submodule list
-#
-module_list()
-{
-	eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"
-	(
-		git ls-files -z --error-unmatch --stage -- "$@" ||
-		echo "unmatched pathspec exists"
-	) |
-	@@PERL@@ -e '
-	my %unmerged = ();
-	my ($null_sha1) = ("0" x 40);
-	my @out = ();
-	my $unmatched = 0;
-	$/ = "\0";
-	while (<STDIN>) {
-		if (/^unmatched pathspec/) {
-			$unmatched = 1;
-			next;
-		}
-		chomp;
-		my ($mode, $sha1, $stage, $path) =
-			/^([0-7]+) ([0-9a-f]{40}) ([0-3])\t(.*)$/;
-		next unless $mode eq "160000";
-		if ($stage ne "0") {
-			if (!$unmerged{$path}++) {
-				push @out, "$mode $null_sha1 U\t$path\n";
-			}
-			next;
-		}
-		push @out, "$_\n";
-	}
-	if ($unmatched) {
-		print "#unmatched\n";
-	} else {
-		print for (@out);
-	}
-	'
-}
-
 die_if_unmatched ()
 {
 	if test "$1" = "#unmatched"
@@ -532,7 +490,7 @@ cmd_foreach()
 	# command in the subshell (and a recursive call to this function)
 	exec 3<&0
 
-	module_list |
+	git submodule--helper module_list --prefix "$wt_prefix"|
 	while read mode sha1 stage sm_path
 	do
 		die_if_unmatched "$mode"
@@ -592,7 +550,7 @@ cmd_init()
 		shift
 	done
 
-	module_list "$@" |
+	git submodule--helper module_list --prefix "$wt_prefix" "$@" |
 	while read mode sha1 stage sm_path
 	do
 		die_if_unmatched "$mode"
@@ -674,7 +632,7 @@ cmd_deinit()
 		die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
 	fi
 
-	module_list "$@" |
+	git submodule--helper module_list --prefix "$wt_prefix" "$@" |
 	while read mode sha1 stage sm_path
 	do
 		die_if_unmatched "$mode"
@@ -790,7 +748,7 @@ cmd_update()
 	fi
 
 	cloned_modules=
-	module_list "$@" | {
+	git submodule--helper module_list --prefix "$wt_prefix" "$@" | {
 	err=
 	while read mode sha1 stage sm_path
 	do
@@ -1222,7 +1180,7 @@ cmd_status()
 		shift
 	done
 
-	module_list "$@" |
+	git submodule--helper module_list --prefix "$wt_prefix" "$@" |
 	while read mode sha1 stage sm_path
 	do
 		die_if_unmatched "$mode"
@@ -1299,7 +1257,7 @@ cmd_sync()
 		esac
 	done
 	cd_to_toplevel
-	module_list "$@" |
+	git submodule--helper module_list --prefix "$wt_prefix" "$@" |
 	while read mode sha1 stage sm_path
 	do
 		die_if_unmatched "$mode"
diff --git a/git.c b/git.c
index 55c327c..deecba0 100644
--- a/git.c
+++ b/git.c
@@ -469,6 +469,7 @@ static struct cmd_struct commands[] = {
 	{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
 	{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
 	{ "stripspace", cmd_stripspace },
+	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP },
 	{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
 	{ "tag", cmd_tag, RUN_SETUP },
 	{ "unpack-file", cmd_unpack_file, RUN_SETUP },
-- 
2.5.0.264.g5e52b0d
Stefan Beller· Aug 28, 2015, 01:14 UTC · re: Stefan Beller · lore

[PATCH 2/9] submodule: implement `module_name` as a builtin helper

This implements the helper `module_name` in C instead of shell, yielding a nice performance boost.

Before this patch, I measured a time (best out of three):
  $ time ./t7400-submodule-basic.sh  >/dev/null
    real	0m11.066s
    user	0m3.348s
    sys	0m8.534s
With this patch applied I measured (also best out of three)
  $ time ./t7400-submodule-basic.sh  >/dev/null
    real	0m10.063s
    user	0m3.044s
    sys	0m7.487s
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/submodule--helper.c | 28 +++++++++++++++++++++++++++-
 git-submodule.sh            | 32 +++++++-------------------------
 2 files changed, 34 insertions(+), 26 deletions(-)
Show changes to 2 files +34 −26

builtin/submodule--helper.c, git-submodule.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index beaab7d..c8f7e0c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -5,6 +5,9 @@
 #include "pathspec.h"
 #include "dir.h"
 #include "utf8.h"
+#include "submodule.h"
+#include "submodule-config.h"
+#include "string-list.h"
 
 static const struct cache_entry **ce_entries;
 static int ce_alloc, ce_used;
@@ -101,6 +104,26 @@ static int module_list(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static int module_name(int argc, const char **argv, const char *prefix)
+{
+	const char *name;
+	const struct submodule *sub;
+
+	if (argc != 1)
+		usage("git submodule--helper module_name <path>\n");
+
+	gitmodules_config();
+	sub = submodule_from_path(null_sha1, argv[0]);
+
+	if (!sub)
+		die("No submodule mapping found in .gitmodules for path '%s'", argv[0]);
+
+	name = sub->name;
+	printf("%s\n", name);
+
+	return 0;
+}
+
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
 {
 	if (argc < 2)
@@ -109,6 +132,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
 	if (!strcmp(argv[1], "module_list"))
 		return module_list(argc - 1, argv + 1, prefix);
 
+	if (!strcmp(argv[1], "module_name"))
+		return module_name(argc - 2, argv + 2, prefix);
+
 usage:
-	usage("git submodule--helper module_list\n");
+	usage("git submodule--helper [module_list | module_name]\n");
 }
diff --git a/git-submodule.sh b/git-submodule.sh
index af9ecef..e6ff38d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -178,24 +178,6 @@ get_submodule_config () {
 	printf '%s' "${value:-$default}"
 }
 
-
-#
-# Map submodule path to submodule name
-#
-# $1 = path
-#
-module_name()
-{
-	# Do we have "submodule.<something>.path = $1" defined in .gitmodules file?
-	sm_path="$1"
-	re=$(printf '%s\n' "$1" | sed -e 's/[].[^$\\*]/\\&/g')
-	name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
-		sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' )
-	test -z "$name" &&
-	die "$(eval_gettext "No submodule mapping found in .gitmodules for path '\$sm_path'")"
-	printf '%s\n' "$name"
-}
-
 #
 # Clone a submodule
 #
@@ -498,7 +480,7 @@ cmd_foreach()
 		then
 			displaypath=$(relative_path "$sm_path")
 			say "$(eval_gettext "Entering '\$prefix\$displaypath'")"
-			name=$(module_name "$sm_path")
+			name=$(git submodule--helper module_name "$sm_path")
 			(
 				prefix="$prefix$sm_path/"
 				clear_local_git_env
@@ -554,7 +536,7 @@ cmd_init()
 	while read mode sha1 stage sm_path
 	do
 		die_if_unmatched "$mode"
-		name=$(module_name "$sm_path") || exit
+		name=$(git submodule--helper module_name "$sm_path") || exit
 
 		displaypath=$(relative_path "$sm_path")
 
@@ -636,7 +618,7 @@ cmd_deinit()
 	while read mode sha1 stage sm_path
 	do
 		die_if_unmatched "$mode"
-		name=$(module_name "$sm_path") || exit
+		name=$(git submodule--helper module_name "$sm_path") || exit
 
 		displaypath=$(relative_path "$sm_path")
 
@@ -758,7 +740,7 @@ cmd_update()
 			echo >&2 "Skipping unmerged submodule $prefix$sm_path"
 			continue
 		fi
-		name=$(module_name "$sm_path") || exit
+		name=$(git submodule--helper module_name "$sm_path") || exit
 		url=$(git config submodule."$name".url)
 		branch=$(get_submodule_config "$name" branch master)
 		if ! test -z "$update"
@@ -1022,7 +1004,7 @@ cmd_summary() {
 			# Respect the ignore setting for --for-status.
 			if test -n "$for_status"
 			then
-				name=$(module_name "$sm_path")
+				name=$(git submodule--helper module_name "$sm_path")
 				ignore_config=$(get_submodule_config "$name" ignore none)
 				test $status != A && test $ignore_config = all && continue
 			fi
@@ -1184,7 +1166,7 @@ cmd_status()
 	while read mode sha1 stage sm_path
 	do
 		die_if_unmatched "$mode"
-		name=$(module_name "$sm_path") || exit
+		name=$(git submodule--helper module_name "$sm_path") || exit
 		url=$(git config submodule."$name".url)
 		displaypath=$(relative_path "$prefix$sm_path")
 		if test "$stage" = U
@@ -1261,7 +1243,7 @@ cmd_sync()
 	while read mode sha1 stage sm_path
 	do
 		die_if_unmatched "$mode"
-		name=$(module_name "$sm_path")
+		name=$(git submodule--helper module_name "$sm_path")
 		url=$(git config -f .gitmodules --get submodule."$name".url)
 
 		# Possibly a url relative to parent
-- 
2.5.0.264.g5e52b0d
Stefan Beller· Aug 28, 2015, 01:14 UTC · re: Stefan Beller · lore

[PATCH 3/9] submodule: implement `module_clone` as a builtin helper

`module_clone` is part of the update command, which I want to convert to C next.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/submodule--helper.c | 148 +++++++++++++++++++++++++++++++++++++++++++-
 git-submodule.sh            |  80 +-----------------------
 2 files changed, 150 insertions(+), 78 deletions(-)
Show changes to 2 files +150 −78

builtin/submodule--helper.c, git-submodule.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c8f7e0c..d29499c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -8,6 +8,7 @@
 #include "submodule.h"
 #include "submodule-config.h"
 #include "string-list.h"
+#include "run-command.h"
 
 static const struct cache_entry **ce_entries;
 static int ce_alloc, ce_used;
@@ -124,6 +125,147 @@ static int module_name(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static int clone_submodule(const char *path, const char *gitdir, const char *url,
+			   const char *depth, const char *reference, int quiet)
+{
+	struct child_process cp;
+	child_process_init(&cp);
+
+	argv_array_push(&cp.args, "clone");
+	argv_array_push(&cp.args, "--no-checkout");
+	if (quiet)
+		argv_array_push(&cp.args, "--quiet");
+	if (depth && strcmp(depth, "")) {
+		argv_array_push(&cp.args, "--depth");
+		argv_array_push(&cp.args, depth);
+	}
+	if (reference && strcmp(reference, "")) {
+		argv_array_push(&cp.args, "--reference");
+		argv_array_push(&cp.args, reference);
+	}
+	if (gitdir) {
+		argv_array_push(&cp.args, "--separate-git-dir");
+		argv_array_push(&cp.args, gitdir);
+	}
+	argv_array_push(&cp.args, url);
+	argv_array_push(&cp.args, path);
+
+	cp.git_cmd = 1;
+	cp.env = local_repo_env;
+
+	cp.no_stdin = 1;
+	cp.no_stdout = 1;
+	cp.no_stderr = 1;
+
+	return run_command(&cp);
+}
+
+static int module_clone(int argc, const char **argv, const char *prefix)
+{
+	const char *path = NULL, *name = NULL, *url = NULL;
+	const char *reference = NULL, *depth = NULL;
+	int quiet = 0;
+	FILE *submodule_dot_git;
+	const char *sm_gitdir, *p;
+	struct strbuf rel_path = STRBUF_INIT;
+	struct strbuf sb = STRBUF_INIT;
+
+	struct option module_update_options[] = {
+		OPT_STRING(0, "prefix", &alternative_path,
+			   N_("path"),
+			   N_("alternative anchor for relative paths")),
+		OPT_STRING(0, "path", &path,
+			   N_("path"),
+			   N_("where the new submodule will be cloned to")),
+		OPT_STRING(0, "name", &name,
+			   N_("string"),
+			   N_("name of the new submodule")),
+		OPT_STRING(0, "url", &url,
+			   N_("string"),
+			   N_("url where to clone the submodule from")),
+		OPT_STRING(0, "reference", &reference,
+			   N_("string"),
+			   N_("reference repository")),
+		OPT_STRING(0, "depth", &depth,
+			   N_("string"),
+			   N_("depth for shallow clones")),
+		OPT_END()
+	};
+
+	static const char * const git_submodule_helper_usage[] = {
+		N_("git submodule--helper update [--prefix=<path>] [--quiet] [--remote] [-N|--no-fetch]"
+		   "[-f|--force] [--rebase|--merge] [--reference <repository>]"
+		   "[--depth <depth>] [--recursive] [--] [<path>...]"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_update_options,
+			     git_submodule_helper_usage, 0);
+
+	if (getenv("GIT_QUIET"))
+		quiet = 1;
+
+	strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
+	sm_gitdir = strbuf_detach(&sb, NULL);
+
+	if (!file_exists(sm_gitdir)) {
+		safe_create_leading_directories_const(sm_gitdir);
+		if (clone_submodule(path, sm_gitdir, url, depth, reference, quiet))
+			die(N_("Clone of '%s' into submodule path '%s' failed"),
+			    url, path);
+	} else {
+		safe_create_leading_directories_const(path);
+		unlink(sm_gitdir);
+	}
+
+	/* Write a .git file in the submodule to redirect to the superproject. */
+	if (alternative_path && !strcmp(alternative_path, "")) {
+		p = relative_path(path, alternative_path, &sb);
+		strbuf_reset(&sb);
+	} else
+		p = path;
+
+	if (safe_create_leading_directories_const(p) < 0)
+		die("Could not create directory '%s'", p);
+
+	strbuf_addf(&sb, "%s/.git", p);
+
+	if (safe_create_leading_directories_const(sb.buf) < 0)
+		die(_("could not create leading directories of '%s'"), sb.buf);
+	submodule_dot_git = fopen(sb.buf, "w");
+	if (!submodule_dot_git)
+		die ("Cannot open file '%s': %s", sb.buf, strerror(errno));
+
+	fprintf(submodule_dot_git, "gitdir: %s\n",
+		relative_path(sm_gitdir, path, &rel_path));
+	if (fclose(submodule_dot_git))
+		die("Could not close file %s", sb.buf);
+	strbuf_reset(&sb);
+
+	/* Redirect the worktree of the submodule in the superprojects config */
+	if (!is_absolute_path(sm_gitdir)) {
+		char *s = (char*)sm_gitdir;
+		if (strbuf_getcwd(&sb))
+			die_errno("unable to get current working directory");
+		strbuf_addf(&sb, "/%s", sm_gitdir);
+		sm_gitdir = strbuf_detach(&sb, NULL);
+		free(s);
+	}
+
+	if (strbuf_getcwd(&sb))
+		die_errno("unable to get current working directory");
+	strbuf_addf(&sb, "/%s", path);
+
+	p = git_pathdup_submodule(path, "config");
+	if (!p)
+		die("Could not get submodule directory for '%s'", path);
+	git_config_set_in_file(p, "core.worktree",
+			       relative_path(sb.buf, sm_gitdir, &rel_path));
+	strbuf_release(&sb);
+	free((char*)sm_gitdir);
+	return 0;
+}
+
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
 {
 	if (argc < 2)
@@ -135,6 +277,10 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
 	if (!strcmp(argv[1], "module_name"))
 		return module_name(argc - 2, argv + 2, prefix);
 
+	if (!strcmp(argv[1], "module_clone"))
+		return module_clone(argc - 1, argv + 1, prefix);
+
 usage:
-	usage("git submodule--helper [module_list | module_name]\n");
+	usage("git submodule--helper [module_list | module_name | "
+	      "module_clone]\n");
 }
diff --git a/git-submodule.sh b/git-submodule.sh
index e6ff38d..fb5155e 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -178,80 +178,6 @@ get_submodule_config () {
 	printf '%s' "${value:-$default}"
 }
 
-#
-# Clone a submodule
-#
-# $1 = submodule path
-# $2 = submodule name
-# $3 = URL to clone
-# $4 = reference repository to reuse (empty for independent)
-# $5 = depth argument for shallow clones (empty for deep)
-#
-# Prior to calling, cmd_update checks that a possibly existing
-# path is not a git repository.
-# Likewise, cmd_add checks that path does not exist at all,
-# since it is the location of a new submodule.
-#
-module_clone()
-{
-	sm_path=$1
-	name=$2
-	url=$3
-	reference="$4"
-	depth="$5"
-	quiet=
-	if test -n "$GIT_QUIET"
-	then
-		quiet=-q
-	fi
-
-	gitdir=
-	gitdir_base=
-	base_name=$(dirname "$name")
-
-	gitdir=$(git rev-parse --git-dir)
-	gitdir_base="$gitdir/modules/$base_name"
-	gitdir="$gitdir/modules/$name"
-
-	if test -d "$gitdir"
-	then
-		mkdir -p "$sm_path"
-		rm -f "$gitdir/index"
-	else
-		mkdir -p "$gitdir_base"
-		(
-			clear_local_git_env
-			git clone $quiet ${depth:+"$depth"} -n ${reference:+"$reference"} \
-				--separate-git-dir "$gitdir" "$url" "$sm_path"
-		) ||
-		die "$(eval_gettext "Clone of '\$url' into submodule path '\$sm_path' failed")"
-	fi
-
-	# We already are at the root of the work tree but cd_to_toplevel will
-	# resolve any symlinks that might be present in $PWD
-	a=$(cd_to_toplevel && cd "$gitdir" && pwd)/
-	b=$(cd_to_toplevel && cd "$sm_path" && pwd)/
-	# Remove all common leading directories after a sanity check
-	if test "${a#$b}" != "$a" || test "${b#$a}" != "$b"; then
-		die "$(eval_gettext "Gitdir '\$a' is part of the submodule path '\$b' or vice versa")"
-	fi
-	while test "${a%%/*}" = "${b%%/*}"
-	do
-		a=${a#*/}
-		b=${b#*/}
-	done
-	# Now chop off the trailing '/'s that were added in the beginning
-	a=${a%/}
-	b=${b%/}
-
-	# Turn each leading "*/" component into "../"
-	rel=$(printf '%s\n' "$b" | sed -e 's|[^/][^/]*|..|g')
-	printf '%s\n' "gitdir: $rel/$a" >"$sm_path/.git"
-
-	rel=$(printf '%s\n' "$a" | sed -e 's|[^/][^/]*|..|g')
-	(clear_local_git_env; cd "$sm_path" && GIT_WORK_TREE=. git config core.worktree "$rel/$b")
-}
-
 isnumber()
 {
 	n=$(($1 + 0)) 2>/dev/null && test "$n" = "$1"
@@ -301,7 +227,7 @@ cmd_add()
 			shift
 			;;
 		--depth=*)
-			depth=$1
+			depth="$1"
 			;;
 		--)
 			shift
@@ -412,7 +338,7 @@ Use -f if you really want to add it." >&2
 				echo "$(eval_gettext "Reactivating local git directory for submodule '\$sm_name'.")"
 			fi
 		fi
-		module_clone "$sm_path" "$sm_name" "$realrepo" "$reference" "$depth" || exit
+		git submodule--helper module_clone --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" "$reference" "$depth" || exit
 		(
 			clear_local_git_env
 			cd "$sm_path" &&
@@ -774,7 +700,7 @@ Maybe you want to use 'update --init'?")"
 
 		if ! test -d "$sm_path"/.git && ! test -f "$sm_path"/.git
 		then
-			module_clone "$sm_path" "$name" "$url" "$reference" "$depth" || exit
+			git submodule--helper module_clone --prefix "$prefix" --path "$sm_path" --name "$name" --url "$url" "$reference" "$depth" || exit
 			cloned_modules="$cloned_modules;$name"
 			subsha1=
 		else
-- 
2.5.0.264.g5e52b0d
Junio C Hamano· Aug 31, 2015, 18:53 UTC · re: Stefan Beller · lore

Re: [PATCH 3/9] submodule: implement `module_clone` as a builtin helper

Stefan Beller <sbeller@google.com> writes:
> +		char *s = (char*)sm_gitdir;
	char *s = (char *)sm_gitdir;
Many other places have similar, also in 7/9.
Stefan Beller· Aug 28, 2015, 01:14 UTC · re: Stefan Beller · lore

This adds functionality to do work in a parallel threaded fashion while the boiler plate code for setting up threads and tearing them down as well as queuing up tasks is hidden behind the new API.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 run-command.c  |  39 +++++++-----
 run-command.h  |   3 +
 thread-utils.c | 192 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 thread-utils.h |  35 +++++++++++
 4 files changed, 253 insertions(+), 16 deletions(-)
Show changes to 4 files +253 −16

run-command.c, run-command.h, thread-utils.c, thread-utils.h

diff --git a/run-command.c b/run-command.c
index 28e1d55..3d37f8c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -610,10 +610,12 @@ static NORETURN void die_async(const char *err, va_list params)
 
 	if (!pthread_equal(main_thread, pthread_self())) {
 		struct async *async = pthread_getspecific(async_key);
-		if (async->proc_in >= 0)
-			close(async->proc_in);
-		if (async->proc_out >= 0)
-			close(async->proc_out);
+		if (async) {
+			if (async->proc_in >= 0)
+				close(async->proc_in);
+			if (async->proc_out >= 0)
+				close(async->proc_out);
+		}
 		pthread_exit((void *)128);
 	}
 
@@ -668,6 +670,22 @@ int git_atexit(void (*handler)(void))
 
 #endif
 
+void setup_main_thread(void)
+{
+	if (!main_thread_set) {
+		/*
+		 * We assume that the first time that start_async is called
+		 * it is from the main thread.
+		 */
+		main_thread_set = 1;
+		main_thread = pthread_self();
+		pthread_key_create(&async_key, NULL);
+		pthread_key_create(&async_die_counter, NULL);
+		set_die_routine(die_async);
+		set_die_is_recursing_routine(async_die_is_recursing);
+	}
+}
+
 int start_async(struct async *async)
 {
 	int need_in, need_out;
@@ -740,18 +758,7 @@ int start_async(struct async *async)
 	else if (async->out)
 		close(async->out);
 #else
-	if (!main_thread_set) {
-		/*
-		 * We assume that the first time that start_async is called
-		 * it is from the main thread.
-		 */
-		main_thread_set = 1;
-		main_thread = pthread_self();
-		pthread_key_create(&async_key, NULL);
-		pthread_key_create(&async_die_counter, NULL);
-		set_die_routine(die_async);
-		set_die_is_recursing_routine(async_die_is_recursing);
-	}
+	setup_main_thread();
 
 	if (proc_in >= 0)
 		set_cloexec(proc_in);
diff --git a/run-command.h b/run-command.h
index 5b4425a..176a5b2 100644
--- a/run-command.h
+++ b/run-command.h
@@ -119,4 +119,7 @@ struct async {
 int start_async(struct async *async);
 int finish_async(struct async *async);
 
+/* die gracefully from within threads */
+void setup_main_thread(void);
+
 #endif
diff --git a/thread-utils.c b/thread-utils.c
index a2135e0..30ccd79 100644
--- a/thread-utils.c
+++ b/thread-utils.c
@@ -1,5 +1,7 @@
 #include "cache.h"
 #include "thread-utils.h"
+#include "run-command.h"
+#include "git-compat-util.h"
 
 #if defined(hpux) || defined(__hpux) || defined(_hpux)
 #  include <sys/pstat.h>
@@ -75,3 +77,193 @@ int init_recursive_mutex(pthread_mutex_t *m)
 	}
 	return ret;
 }
+
+#ifndef NO_PTHREADS
+struct job_list {
+	int (*fct)(struct task_queue *tq, void *task);
+	void *task;
+	struct job_list *next;
+};
+
+struct task_queue {
+	pthread_mutex_t mutex;
+	pthread_cond_t cond_non_empty;
+
+	int queued_tasks;
+	struct job_list *first;
+	struct job_list *last;
+
+	pthread_t *threads;
+	unsigned max_threads;
+	unsigned max_tasks;
+
+	void (*finish_function)(struct task_queue *tq);
+	int early_return;
+};
+
+static void next_task(struct task_queue *tq,
+		      int (**fct)(struct task_queue *tq, void *task),
+		      void **task,
+		      int *early_return)
+{
+	struct job_list *job = NULL;
+
+	pthread_mutex_lock(&tq->mutex);
+	while (tq->queued_tasks == 0)
+		pthread_cond_wait(&tq->cond_non_empty, &tq->mutex);
+
+	tq->early_return |= *early_return;
+
+	if (!tq->early_return) {
+		job = tq->first;
+		tq->first = job->next;
+		if (!tq->first)
+			tq->last = NULL;
+		tq->queued_tasks--;
+	}
+
+	pthread_mutex_unlock(&tq->mutex);
+
+	if (job) {
+		*fct = job->fct;
+		*task = job->task;
+	} else {
+		*fct = NULL;
+		*task = NULL;
+	}
+
+	free(job);
+}
+
+static void *dispatcher(void *args)
+{
+	void *task;
+	int (*fct)(struct task_queue *tq, void *task);
+	int early_return = 0;
+	struct task_queue *tq = args;
+
+	next_task(tq, &fct, &task, &early_return);
+	while (fct && !early_return) {
+		early_return = fct(tq, task);
+		next_task(tq, &fct, &task, &early_return);
+	}
+
+	if (tq->finish_function)
+		tq->finish_function(tq);
+
+	pthread_exit(0);
+}
+
+struct task_queue *create_task_queue(unsigned max_threads)
+{
+	struct task_queue *tq = xmalloc(sizeof(*tq));
+
+	int i, ret;
+	if (!max_threads)
+		tq->max_threads = online_cpus();
+	else
+		tq->max_threads = max_threads;
+
+	pthread_mutex_init(&tq->mutex, NULL);
+	pthread_cond_init(&tq->cond_non_empty, NULL);
+
+	tq->threads = xmalloc(tq->max_threads * sizeof(pthread_t));
+
+	tq->queued_tasks = 0;
+	tq->first = NULL;
+	tq->last = NULL;
+
+	setup_main_thread();
+
+	for (i = 0; i < tq->max_threads; i++) {
+		ret = pthread_create(&tq->threads[i], 0, &dispatcher, tq);
+		if (ret)
+			die("unable to create thread: %s", strerror(ret));
+	}
+
+	tq->early_return = 0;
+
+	return tq;
+}
+
+void add_task(struct task_queue *tq,
+	      int (*fct)(struct task_queue *tq, void *task),
+	      void *task)
+{
+	struct job_list *job_list;
+
+	job_list = xmalloc(sizeof(*job_list));
+	job_list->task = task;
+	job_list->fct = fct;
+	job_list->next = NULL;
+
+	pthread_mutex_lock(&tq->mutex);
+
+	if (!tq->last) {
+		tq->last = job_list;
+		tq->first = tq->last;
+	} else {
+		tq->last->next = job_list;
+		tq->last = tq->last->next;
+	}
+	tq->queued_tasks++;
+
+	pthread_mutex_unlock(&tq->mutex);
+	pthread_cond_signal(&tq->cond_non_empty);
+}
+
+int finish_task_queue(struct task_queue *tq, void (*fct)(struct task_queue *tq))
+{
+	int ret;
+	int i;
+
+	tq->finish_function = fct;
+
+	for (i = 0; i < tq->max_threads; i++)
+		add_task(tq, NULL, NULL);
+
+	for (i = 0; i < tq->max_threads; i++)
+		pthread_join(tq->threads[i], 0);
+
+	pthread_mutex_destroy(&tq->mutex);
+	pthread_cond_destroy(&tq->cond_non_empty);
+
+	if (tq->first)
+		die("BUG: internal error with queuing jobs for threads");
+
+	free(tq->threads);
+	ret = tq->early_return;
+
+	free(tq);
+	return ret;
+}
+#else /* NO_PTHREADS */
+
+struct task_queue {
+	int early_return;
+};
+
+struct task_queue *create_task_queue(unsigned max_threads)
+{
+	struct task_queue *tq = xmalloc(sizeof(*tq));
+
+	tq->early_return = 0;
+}
+
+void add_task(struct task_queue *tq,
+	      int (*fct)(struct task_queue *tq, void *task),
+	      void *task)
+{
+	if (tq->early_return)
+		return;
+
+	tq->early_return |= fct(tq, task);
+}
+
+int finish_task_queue(struct task_queue *tq, void (*fct)(struct task_queue *tq))
+{
+	int ret = tq->early_return;
+	free(tq);
+	return ret;
+}
+#endif
diff --git a/thread-utils.h b/thread-utils.h
index d9a769d..f41cfb1 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -12,4 +12,39 @@ extern int init_recursive_mutex(pthread_mutex_t*);
 #define online_cpus() 1
 
 #endif
+
+/*
+ * Creates a struct `task_queue`, which holds a list of tasks. Up to
+ * `max_threads` threads are active to process the enqueued tasks
+ * processing the tasks in a first in first out order.
+ *
+ * If `max_threads` is zero the number of cores available will be used.
+ *
+ * Currently this only works in environments with pthreads, in other
+ * environments, the task will be processed sequentially in `add_task`.
+ */
+struct task_queue *create_task_queue(unsigned max_threads);
+
+/*
+ * The function and data are put into the task queue.
+ *
+ * The function `fct` must not be NULL, as that's used internally
+ * in `finish_task_queue` to signal shutdown. If the return code
+ * of `fct` is unequal to 0, the tasks will stop eventually,
+ * the current parallel tasks will be flushed out.
+ */
+void add_task(struct task_queue *tq,
+	      int (*fct)(struct task_queue *tq, void *task),
+	      void *task);
+
+/*
+ * Waits for all tasks to be done and frees the object. The return code
+ * is zero if all enqueued tasks were processed.
+ *
+ * The function `fct` is called once in each thread after the last task
+ * for that thread was processed. If no thread local cleanup needs to be
+ * performed, pass NULL.
+ */
+int finish_task_queue(struct task_queue *tq, void (*fct)(struct task_queue *tq));
+
 #endif /* THREAD_COMPAT_H */
-- 
2.5.0.264.g5e52b0d
Stefan Beller· Aug 28, 2015, 01:14 UTC · re: Stefan Beller · lore

[PATCH 5/9] run-command: add synced output

In the last patch we added an easy way to get a thread pool. Now if we want to run external commands from threads in the thread pool, the output will mix up between the threads.

To solve this problem we protect the output via a mutex from becoming garbled. Each thread will try to acquire and directly pipe their output to the standard output if they are lucky to get the mutex. If they do not have the mutex each thread will buffer their output.

Example: Let's assume we have 5 tasks A,B,C,D,E and each task takes a different amount of time (say `git fetch` for different submodules), then the output of tasks in sequential order might look like this:

 time -->
 output: |---A---|   |-B-|   |----C-----------|   |-D-|   |-E-|

When we schedule these tasks into two threads, a schedule and sample output over time may look like this:

thread 1: |---A---|   |-D-|   |-E-|
thread 2: |-B-|   |----C-----------|
output:   |---A---|B|------C-------|ED

So A will be perceived as it would run normally in the single threaded version of foreach. As B has finished by the time the mutex becomes available, the whole buffer will just be dumped into the standard output. This will be perceived by the user as just a 'very fast' operation of B. Once that is done, C takes the mutex, and flushes the piled up buffer to standard output. In case the it is a git command, we have a nice progress display, which will just look like the first half of C happend really quickly.

Notice how E and D are put out in a different order than the original as the new parallel foreach doesn't care about the order.

So this way of output is really good for human consumption, as it only changes the timing, not the actual output.

For machine consumption the output needs to be prepared in the tasks, by either having a prefix per line or per block to indicate whose tasks output is displayed.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 run-command.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 run-command.h | 18 ++++++++++++++++++
 2 files changed, 78 insertions(+)
Show changes to 2 files +78 −0

run-command.c, run-command.h

diff --git a/run-command.c b/run-command.c
index 3d37f8c..bac4367 100644
--- a/run-command.c
+++ b/run-command.c
@@ -556,17 +556,77 @@ int finish_command(struct child_process *cmd)
 }
 
 int run_command(struct child_process *cmd)
+#ifdef NO_PTHREADS
 {
 	int code;
 
 	if (cmd->out < 0 || cmd->err < 0)
 		die("BUG: run_command with a pipe can cause deadlock");
 
+	if (cmd->sync_buf)
+		xwrite(cmd->out, cmd->sync_buf->buf, cmd->sync_buf->len);
+
 	code = start_command(cmd);
 	if (code)
 		return code;
 	return finish_command(cmd);
 }
+#else
+{
+	int code, lock_acquired;
+
+	if (!cmd->sync_mutex) {
+		if (cmd->out < 0 || cmd->err < 0)
+			die("BUG: run_command with a pipe can cause deadlock");
+
+		if (cmd->sync_buf)
+			xwrite(cmd->out, cmd->sync_buf->buf, cmd->sync_buf->len);
+	} else {
+		if (cmd->out < 0)
+			die("BUG: run_command with a pipe can cause deadlock");
+
+		if (!cmd->stdout_to_stderr)
+			die("BUG: run_command with sync_mutex not supported "
+			    "without stdout_to_stderr set");
+
+		if (!cmd->sync_buf)
+			die("BUG: Must pass a buffer when specifying "
+			    "to sync output");
+	}
+
+	code = start_command(cmd);
+	if (code)
+		return code;
+
+	if (cmd->sync_mutex) {
+		while (1) {
+			char buf[1024];
+			ssize_t len = xread(cmd->err, buf, sizeof(buf));
+			if (len < 0)
+				die("Read from command failed");
+			else if (len == 0)
+				break;
+			else
+				strbuf_add(cmd->sync_buf, buf, len);
+
+			if (!lock_acquired
+			    && !pthread_mutex_trylock(cmd->sync_mutex))
+				lock_acquired = 1;
+			if (lock_acquired) {
+				fputs(cmd->sync_buf->buf, stderr);
+				strbuf_reset(cmd->sync_buf);
+			}
+		}
+		if (!lock_acquired)
+			pthread_mutex_lock(cmd->sync_mutex);
+
+		fputs(cmd->sync_buf->buf, stderr);
+		pthread_mutex_unlock(cmd->sync_mutex);
+	}
+
+	return finish_command(cmd);
+}
+#endif
 
 int run_command_v_opt(const char **argv, int opt)
 {
diff --git a/run-command.h b/run-command.h
index 176a5b2..0df83c9 100644
--- a/run-command.h
+++ b/run-command.h
@@ -43,6 +43,24 @@ struct child_process {
 	unsigned stdout_to_stderr:1;
 	unsigned use_shell:1;
 	unsigned clean_on_exit:1;
+#ifndef NO_PTHREAD
+	/*
+	 * In a threaded environment running multiple commands from different
+	 * threads would lead to garbled output as the output of different
+	 * commands would mix.
+	 * If this mutex is not NULL, the output of `err` is only done when
+	 * holding the mutex. If the mutex cannot be acquired, the output
+	 * will be buffered until the mutex can be acquired.
+	 */
+	pthread_mutex_t *sync_mutex;
+#endif
+	/*
+	 * The sync_buf will be used to buffer the output while the mutex
+	 * is not acquired. It can also contain data before being passed into
+	 * run_command, which will be output together with the output of
+	 * the child.
+	 */
+	struct strbuf *sync_buf;
 };
 
 #define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, ARGV_ARRAY_INIT }
-- 
2.5.0.264.g5e52b0d
Stefan Beller· Aug 28, 2015, 01:14 UTC · re: Stefan Beller · lore

[PATCH 6/9] submodule: helper to run foreach in parallel

Similar to `git submodule foreach` the new command `git submodule foreach_parallel` will run a command on each submodule.

The commands are run in parallel up to the number of cores by default, or you can specify '-j 4' tun just run with 4 threads for example.

One major difference to `git submodule foreach` is the handling of input and output to the commands. Because of the parallel nature of the execution it is not trivial how to schedule the std{in,out,err} channel for submodule the command is run in. So in this patch there is no support for stdin. stdout will be piped to stderr. stderr will make use of the synchronized output feature of run_command.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c  | 133 ++++++++++++++++++++++++++++++++++++++++++-
 git-submodule.sh             |  11 +++-
 t/t7407-submodule-foreach.sh |  11 ++++
 3 files changed, 153 insertions(+), 2 deletions(-)
Show changes to 3 files +153 −2

builtin/submodule--helper.c, git-submodule.sh, t/t7407-submodule-foreach.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d29499c..18b67f0 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -8,6 +8,7 @@
 #include "submodule.h"
 #include "submodule-config.h"
 #include "string-list.h"
+#include "thread-utils.h"
 #include "run-command.h"
 
 static const struct cache_entry **ce_entries;
@@ -266,6 +267,127 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+#ifndef NO_PTHREADS
+struct submodule_args {
+	const char *name;
+	const char *path;
+	const char *sha1;
+	const char *toplevel;
+	const char *prefix;
+	const char **cmd;
+	pthread_mutex_t *sync;
+};
+
+int run_cmd_submodule(struct task_queue *aq, void *task)
+{
+	int i;
+	struct submodule_args *args = task;
+	struct strbuf out = STRBUF_INIT;
+	struct strbuf sb = STRBUF_INIT;
+	struct child_process *cp = xmalloc(sizeof(*cp));
+
+	strbuf_addf(&out, N_("Entering %s\n"), relative_path(args->path, args->prefix, &sb));
+
+	child_process_init(cp);
+	argv_array_pushv(&cp->args, args->cmd);
+
+	argv_array_pushf(&cp->env_array, "name=%s", args->name);
+	argv_array_pushf(&cp->env_array, "path=%s", args->path);
+	argv_array_pushf(&cp->env_array, "sha1=%s", args->sha1);
+	argv_array_pushf(&cp->env_array, "toplevel=%s", args->toplevel);
+
+	for (i = 0; local_repo_env[i]; i++)
+		argv_array_push(&cp->env_array, local_repo_env[i]);
+
+	cp->no_stdin = 1;
+	cp->out = 0;
+	cp->err = -1;
+	cp->dir = args->path;
+	cp->stdout_to_stderr = 1;
+	cp->use_shell = 1;
+	cp->sync_mutex = args->sync;
+	cp->sync_buf = &out;
+
+	return run_command(cp);
+}
+
+int module_foreach_parallel(int argc, const char **argv, const char *prefix)
+{
+	int i, recursive = 0, number_threads = 0, quiet = 0;
+	static struct pathspec pathspec;
+	struct strbuf sb = STRBUF_INIT;
+	struct task_queue *aq;
+	char **cmd;
+	const char **nullargv = {NULL};
+	pthread_mutex_t mutex;
+
+	struct option module_update_options[] = {
+		OPT_STRING(0, "prefix", &alternative_path,
+			   N_("path"),
+			   N_("alternative anchor for relative paths")),
+		OPT_STRING(0, "cmd", &cmd,
+			   N_("string"),
+			   N_("command to run")),
+		OPT_BOOL('r', "--recursive", &recursive,
+			 N_("Recurse into nexted submodules")),
+		OPT_INTEGER('j', "jobs", &number_threads,
+			    N_("Recurse into nexted submodules")),
+		OPT__QUIET(&quiet, N_("Suppress output")),
+		OPT_END()
+	};
+
+	static const char * const git_submodule_helper_usage[] = {
+		N_("git submodule--helper foreach [--prefix=<path>] [<path>...]"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_update_options,
+			     git_submodule_helper_usage, 0);
+
+	if (module_list_compute(0, nullargv, NULL, &pathspec) < 0)
+		return 1;
+
+	gitmodules_config();
+
+	pthread_mutex_init(&mutex, NULL);
+	aq = create_task_queue(number_threads);
+
+	for (i = 0; i < ce_used; i++) {
+		const struct submodule *sub;
+		const struct cache_entry *ce = ce_entries[i];
+		struct submodule_args *args = malloc(sizeof(*args));
+
+		if (ce_stage(ce))
+			args->sha1 = xstrdup(sha1_to_hex(null_sha1));
+		else
+			args->sha1 = xstrdup(sha1_to_hex(ce->sha1));
+
+		strbuf_reset(&sb);
+		strbuf_addf(&sb, "%s/.git", ce->name);
+		if (!file_exists(sb.buf)) {
+			free(args);
+			continue;
+		}
+
+		args->path = ce->name;
+		sub = submodule_from_path(null_sha1, args->path);
+		if (!sub)
+			die("No submodule mapping found in .gitmodules for path '%s'", args->path);
+
+		args->name = sub->name;
+		args->toplevel = xgetcwd();
+		args->cmd = argv;
+		args->sync = &mutex;
+		args->prefix = alternative_path;
+		add_task(aq, run_cmd_submodule, args);
+	}
+
+	finish_task_queue(aq, NULL);
+	pthread_mutex_destroy(&mutex);
+	return 0;
+}
+#endif /* NO_PTHREADS */
+
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
 {
 	if (argc < 2)
@@ -280,7 +402,16 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
 	if (!strcmp(argv[1], "module_clone"))
 		return module_clone(argc - 1, argv + 1, prefix);
 
+#ifndef NO_PTHREADS
+	if (!strcmp(argv[1], "foreach_parallel"))
+		return module_foreach_parallel(argc - 1, argv + 1, prefix);
+#endif
+
 usage:
 	usage("git submodule--helper [module_list | module_name | "
-	      "module_clone]\n");
+	      "module_clone"
+#ifndef NO_PTHREADS
+	      " | foreach_parallel"
+#endif
+	      "]\n");
 }
diff --git a/git-submodule.sh b/git-submodule.sh
index fb5155e..f06488a 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -431,6 +431,15 @@ cmd_foreach()
 }
 
 #
+# Execute an arbitrary command sequence in each checked out
+# submodule in parallel.
+#
+cmd_foreach_parallel()
+{
+	git submodule--helper foreach_parallel --prefix "$wt_prefix" $@
+}
+
+#
 # Register submodules in .git/config
 #
 # $@ = requested paths (default to all)
@@ -1225,7 +1234,7 @@ cmd_sync()
 while test $# != 0 && test -z "$command"
 do
 	case "$1" in
-	add | foreach | init | deinit | update | status | summary | sync)
+	add | foreach | foreach_parallel | init | deinit | update | status | summary | sync)
 		command=$1
 		;;
 	-q|--quiet)
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 7ca10b8..16f6138 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -195,6 +195,17 @@ test_expect_success 'test "foreach --quiet --recursive"' '
 	test_cmp expect actual
 '
 
+test_expect_success 'test "foreach_parallel --quiet"' '
+	(
+		cd clone2 &&
+		git submodule foreach_parallel -q -- "echo \$name-\$path" > ../actual
+	) &&
+	grep nested1-nested1 actual &&
+	grep foo1-sub1 actual &&
+	grep foo2-sub2 actual &&
+	grep foo3-sub3 actual
+'
+
 test_expect_success 'use "update --recursive" to checkout all submodules' '
 	git clone super clone3 &&
 	(
-- 
2.5.0.264.g5e52b0d
Stefan Beller· Aug 28, 2015, 17:08 UTC · re: Stefan Beller · lore

Re: [PATCH 6/9] submodule: helper to run foreach in parallel

On Thu, Aug 27, 2015 at 6:14 PM, Stefan Beller <sbeller@google.com> wrote:
Show 226 quoted lines
> Similar to `git submodule foreach` the new command `git submodule
> foreach_parallel` will run a command on each submodule.
>
> The commands are run in parallel up to the number of cores by default,
> or you can specify '-j 4' tun just run with 4 threads for example.
>
> One major difference to `git submodule foreach` is the handling of input
> and output to the commands. Because of the parallel nature of the execution
> it is not trivial how to schedule the std{in,out,err} channel for submodule
> the command is run in. So in this patch there is no support for stdin.
> stdout will be piped to stderr. stderr will make use of the synchronized
> output feature of run_command.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/submodule--helper.c  | 133 ++++++++++++++++++++++++++++++++++++++++++-
>  git-submodule.sh             |  11 +++-
>  t/t7407-submodule-foreach.sh |  11 ++++
>  3 files changed, 153 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index d29499c..18b67f0 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -8,6 +8,7 @@
>  #include "submodule.h"
>  #include "submodule-config.h"
>  #include "string-list.h"
> +#include "thread-utils.h"
>  #include "run-command.h"
>
>  static const struct cache_entry **ce_entries;
> @@ -266,6 +267,127 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>         return 0;
>  }
>
> +#ifndef NO_PTHREADS
> +struct submodule_args {
> +       const char *name;
> +       const char *path;
> +       const char *sha1;
> +       const char *toplevel;
> +       const char *prefix;
> +       const char **cmd;
> +       pthread_mutex_t *sync;
> +};
> +
> +int run_cmd_submodule(struct task_queue *aq, void *task)
> +{
> +       int i;
> +       struct submodule_args *args = task;
> +       struct strbuf out = STRBUF_INIT;
> +       struct strbuf sb = STRBUF_INIT;
> +       struct child_process *cp = xmalloc(sizeof(*cp));
> +
> +       strbuf_addf(&out, N_("Entering %s\n"), relative_path(args->path, args->prefix, &sb));
> +
> +       child_process_init(cp);
> +       argv_array_pushv(&cp->args, args->cmd);
> +
> +       argv_array_pushf(&cp->env_array, "name=%s", args->name);
> +       argv_array_pushf(&cp->env_array, "path=%s", args->path);
> +       argv_array_pushf(&cp->env_array, "sha1=%s", args->sha1);
> +       argv_array_pushf(&cp->env_array, "toplevel=%s", args->toplevel);
> +
> +       for (i = 0; local_repo_env[i]; i++)
> +               argv_array_push(&cp->env_array, local_repo_env[i]);
> +
> +       cp->no_stdin = 1;
> +       cp->out = 0;
> +       cp->err = -1;
> +       cp->dir = args->path;
> +       cp->stdout_to_stderr = 1;
> +       cp->use_shell = 1;
> +       cp->sync_mutex = args->sync;
> +       cp->sync_buf = &out;
> +
> +       return run_command(cp);
> +}
> +
> +int module_foreach_parallel(int argc, const char **argv, const char *prefix)
> +{
> +       int i, recursive = 0, number_threads = 0, quiet = 0;
> +       static struct pathspec pathspec;
> +       struct strbuf sb = STRBUF_INIT;
> +       struct task_queue *aq;
> +       char **cmd;
> +       const char **nullargv = {NULL};
> +       pthread_mutex_t mutex;
> +
> +       struct option module_update_options[] = {
> +               OPT_STRING(0, "prefix", &alternative_path,
> +                          N_("path"),
> +                          N_("alternative anchor for relative paths")),
> +               OPT_STRING(0, "cmd", &cmd,
> +                          N_("string"),
> +                          N_("command to run")),
> +               OPT_BOOL('r', "--recursive", &recursive,
> +                        N_("Recurse into nexted submodules")),
> +               OPT_INTEGER('j', "jobs", &number_threads,
> +                           N_("Recurse into nexted submodules")),
> +               OPT__QUIET(&quiet, N_("Suppress output")),
> +               OPT_END()
> +       };
> +
> +       static const char * const git_submodule_helper_usage[] = {
> +               N_("git submodule--helper foreach [--prefix=<path>] [<path>...]"),
> +               NULL
> +       };
> +
> +       argc = parse_options(argc, argv, prefix, module_update_options,
> +                            git_submodule_helper_usage, 0);
> +
> +       if (module_list_compute(0, nullargv, NULL, &pathspec) < 0)
> +               return 1;
> +
> +       gitmodules_config();
> +
> +       pthread_mutex_init(&mutex, NULL);
> +       aq = create_task_queue(number_threads);
> +
> +       for (i = 0; i < ce_used; i++) {
> +               const struct submodule *sub;
> +               const struct cache_entry *ce = ce_entries[i];
> +               struct submodule_args *args = malloc(sizeof(*args));
> +
> +               if (ce_stage(ce))
> +                       args->sha1 = xstrdup(sha1_to_hex(null_sha1));
> +               else
> +                       args->sha1 = xstrdup(sha1_to_hex(ce->sha1));
> +
> +               strbuf_reset(&sb);
> +               strbuf_addf(&sb, "%s/.git", ce->name);
> +               if (!file_exists(sb.buf)) {
> +                       free(args);
> +                       continue;
> +               }
> +
> +               args->path = ce->name;
> +               sub = submodule_from_path(null_sha1, args->path);
> +               if (!sub)
> +                       die("No submodule mapping found in .gitmodules for path '%s'", args->path);
> +
> +               args->name = sub->name;
> +               args->toplevel = xgetcwd();
> +               args->cmd = argv;
> +               args->sync = &mutex;
> +               args->prefix = alternative_path;
> +               add_task(aq, run_cmd_submodule, args);
> +       }
> +
> +       finish_task_queue(aq, NULL);
> +       pthread_mutex_destroy(&mutex);
> +       return 0;
> +}
> +#endif /* NO_PTHREADS */
> +
>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>  {
>         if (argc < 2)
> @@ -280,7 +402,16 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>         if (!strcmp(argv[1], "module_clone"))
>                 return module_clone(argc - 1, argv + 1, prefix);
>
> +#ifndef NO_PTHREADS
> +       if (!strcmp(argv[1], "foreach_parallel"))
> +               return module_foreach_parallel(argc - 1, argv + 1, prefix);
> +#endif
> +
>  usage:
>         usage("git submodule--helper [module_list | module_name | "
> -             "module_clone]\n");
> +             "module_clone"
> +#ifndef NO_PTHREADS
> +             " | foreach_parallel"
> +#endif
> +             "]\n");
>  }
> diff --git a/git-submodule.sh b/git-submodule.sh
> index fb5155e..f06488a 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -431,6 +431,15 @@ cmd_foreach()
>  }
>
>  #
> +# Execute an arbitrary command sequence in each checked out
> +# submodule in parallel.
> +#
> +cmd_foreach_parallel()
> +{
> +       git submodule--helper foreach_parallel --prefix "$wt_prefix" $@
> +}
> +
> +#
>  # Register submodules in .git/config
>  #
>  # $@ = requested paths (default to all)
> @@ -1225,7 +1234,7 @@ cmd_sync()
>  while test $# != 0 && test -z "$command"
>  do
>         case "$1" in
> -       add | foreach | init | deinit | update | status | summary | sync)
> +       add | foreach | foreach_parallel | init | deinit | update | status | summary | sync)
>                 command=$1
>                 ;;
>         -q|--quiet)
> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
> index 7ca10b8..16f6138 100755
> --- a/t/t7407-submodule-foreach.sh
> +++ b/t/t7407-submodule-foreach.sh
> @@ -195,6 +195,17 @@ test_expect_success 'test "foreach --quiet --recursive"' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'test "foreach_parallel --quiet"' '
> +       (
> +               cd clone2 &&
> +               git submodule foreach_parallel -q -- "echo \$name-\$path" > ../actual
> +       ) &&
> +       grep nested1-nested1 actual &&
> +       grep foo1-sub1 actual &&
> +       grep foo2-sub2 actual &&
> +       grep foo3-sub3 actual
> +'
> +

This test fails, because the variables don't seem to be resolved. actual reads literally "$name-$path" instead of the actual values replaced.

Using the `git submodule--helper foreach_parallel "echo \$name-\$path"` works as expected, so I think there is an issue in wrapping that in git-submodule.sh

Show 6 quoted lines
>  test_expect_success 'use "update --recursive" to checkout all submodules' '
>         git clone super clone3 &&
>         (
> --
> 2.5.0.264.g5e52b0d
>
Stefan Beller· Aug 28, 2015, 01:14 UTC · re: Stefan Beller · lore

[PATCH 7/9] fetch: fetch submodules in parallel

This makes use of the new task queue and the syncing feature of run-command to fetch a number of submodules at the same time.

The output will look like it would have been run sequential, but faster.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/fetch-options.txt |   7 +++
 builtin/fetch.c                 |   6 ++-
 builtin/pull.c                  |   6 +++
 submodule.c                     | 100 +++++++++++++++++++++++++++++++++-------
 submodule.h                     |   2 +-
 5 files changed, 102 insertions(+), 19 deletions(-)
Show changes to 5 files +102 −19

Documentation/fetch-options.txt, builtin/fetch.c, builtin/pull.c, submodule.c, submodule.h

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 45583d8..e2a59c3 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -100,6 +100,13 @@ ifndef::git-pull[]
 	reference to a commit that isn't already in the local submodule
 	clone.
 
+-j::
+--jobs=<n>::
+	Number of threads to be used for fetching submodules. Each thread
+	will fetch from different submodules, such that fetching many
+	submodules will be faster. By default the number of cpus will
+	be used .
+
 --no-recurse-submodules::
 	Disable recursive fetching of submodules (this has the same effect as
 	using the '--recurse-submodules=no' option).
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ee1f1a9..636707e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -37,6 +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_threads;
 static const char *depth;
 static const char *upload_pack;
 static struct strbuf default_rla = STRBUF_INIT;
@@ -99,6 +100,8 @@ static struct option builtin_fetch_options[] = {
 		    N_("fetch all tags and associated objects"), TAGS_SET),
 	OPT_SET_INT('n', NULL, &tags,
 		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
+	OPT_INTEGER('j', "jobs", &max_threads,
+		    N_("number of threads used for fetching")),
 	OPT_BOOL('p', "prune", &prune,
 		 N_("prune remote-tracking branches no longer on remote")),
 	{ OPTION_CALLBACK, 0, "recurse-submodules", NULL, N_("on-demand"),
@@ -1217,7 +1220,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		result = fetch_populated_submodules(&options,
 						    submodule_prefix,
 						    recurse_submodules,
-						    verbosity < 0);
+						    verbosity < 0,
+						    max_threads);
 		argv_array_clear(&options);
 	}
 
diff --git a/builtin/pull.c b/builtin/pull.c
index 722a83c..fbbda67 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -94,6 +94,7 @@ static int opt_force;
 static char *opt_tags;
 static char *opt_prune;
 static char *opt_recurse_submodules;
+static char *max_threads;
 static int opt_dry_run;
 static char *opt_keep;
 static char *opt_depth;
@@ -177,6 +178,9 @@ static struct option pull_options[] = {
 		N_("on-demand"),
 		N_("control recursive fetching of submodules"),
 		PARSE_OPT_OPTARG),
+	OPT_PASSTHRU('j', "jobs", &max_threads, N_("n"),
+		N_("number of threads used for fetching submodules"),
+		PARSE_OPT_OPTARG),
 	OPT_BOOL(0, "dry-run", &opt_dry_run,
 		N_("dry run")),
 	OPT_PASSTHRU('k', "keep", &opt_keep, NULL,
@@ -524,6 +528,8 @@ static int run_fetch(const char *repo, const char **refspecs)
 		argv_array_push(&args, opt_prune);
 	if (opt_recurse_submodules)
 		argv_array_push(&args, opt_recurse_submodules);
+	if (max_threads)
+		argv_array_push(&args, max_threads);
 	if (opt_dry_run)
 		argv_array_push(&args, "--dry-run");
 	if (opt_keep)
diff --git a/submodule.c b/submodule.c
index 9fcc86f..50266a8 100644
--- a/submodule.c
+++ b/submodule.c
@@ -12,6 +12,7 @@
 #include "sha1-array.h"
 #include "argv-array.h"
 #include "blob.h"
+#include "thread-utils.h"
 
 static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
 static struct string_list changed_submodule_paths;
@@ -615,13 +616,79 @@ static void calculate_changed_submodule_paths(void)
 	initialized_fetch_ref_tips = 0;
 }
 
+struct submodule_parallel_fetch {
+	struct child_process cp;
+	struct argv_array argv;
+	int *result;
+};
+
+#ifndef NO_PTHREADS
+static pthread_mutex_t output_mutex = PTHREAD_MUTEX_INITIALIZER;
+
+static void set_output_mutex(struct submodule_parallel_fetch *spf)
+{
+	spf->cp.sync_mutex = &output_mutex;
+}
+
+#define lock_output_mutex() pthread_mutex_lock(&output_mutex)
+
+#define unlock_output_mutex() pthread_mutex_unlock(&output_mutex)
+
+static void destroy_output_mutex()
+{
+	pthread_mutex_destroy(&output_mutex);
+}
+
+#else
+#define set_output_mutex()
+#define destroy_output_mutex()
+#define lock_output_mutex()
+#define unlock_output_mutex()
+#endif
+
+static struct submodule_parallel_fetch *submodule_parallel_fetch_create()
+{
+	struct submodule_parallel_fetch *spf = xmalloc(sizeof(*spf));
+	child_process_init(&spf->cp);
+	spf->cp.env = local_repo_env;
+	spf->cp.git_cmd = 1;
+	spf->cp.no_stdin = 1;
+	spf->cp.stdout_to_stderr = 1;
+	spf->cp.sync_buf = xmalloc(sizeof(spf->cp.sync_buf));
+	strbuf_init(spf->cp.sync_buf, 0);
+
+	argv_array_init(&spf->argv);
+	return spf;
+}
+
+static int run_command_and_cleanup(struct task_queue *tq, void *arg)
+{
+	int code;
+	struct submodule_parallel_fetch *spf = arg;
+
+	spf->cp.argv = spf->argv.argv;
+
+	code = run_command(&spf->cp);
+	if (code) {
+		lock_output_mutex();
+		*spf->result = code;
+		unlock_output_mutex();
+	}
+
+	argv_array_clear(&spf->argv);
+	free((char*)spf->cp.dir);
+	free(spf);
+	return 0;
+}
+
 int fetch_populated_submodules(const struct argv_array *options,
 			       const char *prefix, int command_line_option,
-			       int quiet)
+			       int quiet, int max_parallel_jobs)
 {
 	int i, result = 0;
-	struct child_process cp = CHILD_PROCESS_INIT;
+	struct task_queue *tq;
 	struct argv_array argv = ARGV_ARRAY_INIT;
+	struct submodule_parallel_fetch *spf;
 	const char *work_tree = get_git_work_tree();
 	if (!work_tree)
 		goto out;
@@ -635,12 +702,9 @@ int fetch_populated_submodules(const struct argv_array *options,
 	argv_array_push(&argv, "--recurse-submodules-default");
 	/* default value, "--submodule-prefix" and its value are added later */
 
-	cp.env = local_repo_env;
-	cp.git_cmd = 1;
-	cp.no_stdin = 1;
-
 	calculate_changed_submodule_paths();
 
+	tq = create_task_queue(max_parallel_jobs);
 	for (i = 0; i < active_nr; i++) {
 		struct strbuf submodule_path = STRBUF_INIT;
 		struct strbuf submodule_git_dir = STRBUF_INIT;
@@ -693,24 +757,26 @@ int fetch_populated_submodules(const struct argv_array *options,
 		if (!git_dir)
 			git_dir = submodule_git_dir.buf;
 		if (is_directory(git_dir)) {
+			spf = submodule_parallel_fetch_create();
+			spf->result = &result;
+			spf->cp.dir = strbuf_detach(&submodule_path, NULL);
 			if (!quiet)
-				printf("Fetching submodule %s%s\n", prefix, ce->name);
-			cp.dir = submodule_path.buf;
-			argv_array_push(&argv, default_argv);
-			argv_array_push(&argv, "--submodule-prefix");
-			argv_array_push(&argv, submodule_prefix.buf);
-			cp.argv = argv.argv;
-			if (run_command(&cp))
-				result = 1;
-			argv_array_pop(&argv);
-			argv_array_pop(&argv);
-			argv_array_pop(&argv);
+				strbuf_addf(spf->cp.sync_buf,
+					    "Fetching submodule %s%s",
+					    prefix, ce->name);
+			argv_array_pushv(&spf->argv, argv.argv);
+			argv_array_push(&spf->argv, default_argv);
+			argv_array_push(&spf->argv, "--submodule-prefix");
+			argv_array_push(&spf->argv, submodule_prefix.buf);
+			set_output_mutex(spf);
+			add_task(tq, run_command_and_cleanup, spf);
 		}
 		strbuf_release(&submodule_path);
 		strbuf_release(&submodule_git_dir);
 		strbuf_release(&submodule_prefix);
 	}
 	argv_array_clear(&argv);
+	destroy_output_mutex();
 out:
 	string_list_clear(&changed_submodule_paths, 1);
 	return result;
diff --git a/submodule.h b/submodule.h
index 5507c3d..cbc0003 100644
--- a/submodule.h
+++ b/submodule.h
@@ -31,7 +31,7 @@ void set_config_fetch_recurse_submodules(int value);
 void check_for_new_submodule_commits(unsigned char new_sha1[20]);
 int fetch_populated_submodules(const struct argv_array *options,
 			       const char *prefix, int command_line_option,
-			       int quiet);
+			       int quiet, int max_parallel_jobs);
 unsigned is_submodule_modified(const char *path, int ignore_untracked);
 int submodule_uses_gitfile(const char *path);
 int ok_to_remove_submodule(const char *path);
-- 
2.5.0.264.g5e52b0d
Stefan Beller· Aug 28, 2015, 17:00 UTC · re: Stefan Beller · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

On Thu, Aug 27, 2015 at 6:14 PM, Stefan Beller <sbeller@google.com> wrote:
Show 5 quoted lines
> This makes use of the new task queue and the syncing feature of
> run-command to fetch a number of submodules at the same time.
>
> The output will look like it would have been run sequential,
> but faster.

And it breaks the tests t5526-fetch-submodules.sh as the output is done on stderr only, instead of putting "Fetching submodule <submodule-path> to stdout. :(

I guess combining stdout and stderr is not a good strategy after all now.
Advantages:
* The order is preserved if everything is in one stream.
Disadvantages:
* It's a change, which may not be expected.
* It's harder to make it machine parsable, as that probably
   relied on having 2 channels.

So now I need to come up with a way to either buffer 2 channels at the same time, or we need to redefine parallel submodule fetch output semantics a bit.

Show 252 quoted lines
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  Documentation/fetch-options.txt |   7 +++
>  builtin/fetch.c                 |   6 ++-
>  builtin/pull.c                  |   6 +++
>  submodule.c                     | 100 +++++++++++++++++++++++++++++++++-------
>  submodule.h                     |   2 +-
>  5 files changed, 102 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index 45583d8..e2a59c3 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -100,6 +100,13 @@ ifndef::git-pull[]
>         reference to a commit that isn't already in the local submodule
>         clone.
>
> +-j::
> +--jobs=<n>::
> +       Number of threads to be used for fetching submodules. Each thread
> +       will fetch from different submodules, such that fetching many
> +       submodules will be faster. By default the number of cpus will
> +       be used .
> +
>  --no-recurse-submodules::
>         Disable recursive fetching of submodules (this has the same effect as
>         using the '--recurse-submodules=no' option).
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ee1f1a9..636707e 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -37,6 +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_threads;
>  static const char *depth;
>  static const char *upload_pack;
>  static struct strbuf default_rla = STRBUF_INIT;
> @@ -99,6 +100,8 @@ static struct option builtin_fetch_options[] = {
>                     N_("fetch all tags and associated objects"), TAGS_SET),
>         OPT_SET_INT('n', NULL, &tags,
>                     N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
> +       OPT_INTEGER('j', "jobs", &max_threads,
> +                   N_("number of threads used for fetching")),
>         OPT_BOOL('p', "prune", &prune,
>                  N_("prune remote-tracking branches no longer on remote")),
>         { OPTION_CALLBACK, 0, "recurse-submodules", NULL, N_("on-demand"),
> @@ -1217,7 +1220,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>                 result = fetch_populated_submodules(&options,
>                                                     submodule_prefix,
>                                                     recurse_submodules,
> -                                                   verbosity < 0);
> +                                                   verbosity < 0,
> +                                                   max_threads);
>                 argv_array_clear(&options);
>         }
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 722a83c..fbbda67 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -94,6 +94,7 @@ static int opt_force;
>  static char *opt_tags;
>  static char *opt_prune;
>  static char *opt_recurse_submodules;
> +static char *max_threads;
>  static int opt_dry_run;
>  static char *opt_keep;
>  static char *opt_depth;
> @@ -177,6 +178,9 @@ static struct option pull_options[] = {
>                 N_("on-demand"),
>                 N_("control recursive fetching of submodules"),
>                 PARSE_OPT_OPTARG),
> +       OPT_PASSTHRU('j', "jobs", &max_threads, N_("n"),
> +               N_("number of threads used for fetching submodules"),
> +               PARSE_OPT_OPTARG),
>         OPT_BOOL(0, "dry-run", &opt_dry_run,
>                 N_("dry run")),
>         OPT_PASSTHRU('k', "keep", &opt_keep, NULL,
> @@ -524,6 +528,8 @@ static int run_fetch(const char *repo, const char **refspecs)
>                 argv_array_push(&args, opt_prune);
>         if (opt_recurse_submodules)
>                 argv_array_push(&args, opt_recurse_submodules);
> +       if (max_threads)
> +               argv_array_push(&args, max_threads);
>         if (opt_dry_run)
>                 argv_array_push(&args, "--dry-run");
>         if (opt_keep)
> diff --git a/submodule.c b/submodule.c
> index 9fcc86f..50266a8 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -12,6 +12,7 @@
>  #include "sha1-array.h"
>  #include "argv-array.h"
>  #include "blob.h"
> +#include "thread-utils.h"
>
>  static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
>  static struct string_list changed_submodule_paths;
> @@ -615,13 +616,79 @@ static void calculate_changed_submodule_paths(void)
>         initialized_fetch_ref_tips = 0;
>  }
>
> +struct submodule_parallel_fetch {
> +       struct child_process cp;
> +       struct argv_array argv;
> +       int *result;
> +};
> +
> +#ifndef NO_PTHREADS
> +static pthread_mutex_t output_mutex = PTHREAD_MUTEX_INITIALIZER;
> +
> +static void set_output_mutex(struct submodule_parallel_fetch *spf)
> +{
> +       spf->cp.sync_mutex = &output_mutex;
> +}
> +
> +#define lock_output_mutex() pthread_mutex_lock(&output_mutex)
> +
> +#define unlock_output_mutex() pthread_mutex_unlock(&output_mutex)
> +
> +static void destroy_output_mutex()
> +{
> +       pthread_mutex_destroy(&output_mutex);
> +}
> +
> +#else
> +#define set_output_mutex()
> +#define destroy_output_mutex()
> +#define lock_output_mutex()
> +#define unlock_output_mutex()
> +#endif
> +
> +static struct submodule_parallel_fetch *submodule_parallel_fetch_create()
> +{
> +       struct submodule_parallel_fetch *spf = xmalloc(sizeof(*spf));
> +       child_process_init(&spf->cp);
> +       spf->cp.env = local_repo_env;
> +       spf->cp.git_cmd = 1;
> +       spf->cp.no_stdin = 1;
> +       spf->cp.stdout_to_stderr = 1;
> +       spf->cp.sync_buf = xmalloc(sizeof(spf->cp.sync_buf));
> +       strbuf_init(spf->cp.sync_buf, 0);
> +
> +       argv_array_init(&spf->argv);
> +       return spf;
> +}
> +
> +static int run_command_and_cleanup(struct task_queue *tq, void *arg)
> +{
> +       int code;
> +       struct submodule_parallel_fetch *spf = arg;
> +
> +       spf->cp.argv = spf->argv.argv;
> +
> +       code = run_command(&spf->cp);
> +       if (code) {
> +               lock_output_mutex();
> +               *spf->result = code;
> +               unlock_output_mutex();
> +       }
> +
> +       argv_array_clear(&spf->argv);
> +       free((char*)spf->cp.dir);
> +       free(spf);
> +       return 0;
> +}
> +
>  int fetch_populated_submodules(const struct argv_array *options,
>                                const char *prefix, int command_line_option,
> -                              int quiet)
> +                              int quiet, int max_parallel_jobs)
>  {
>         int i, result = 0;
> -       struct child_process cp = CHILD_PROCESS_INIT;
> +       struct task_queue *tq;
>         struct argv_array argv = ARGV_ARRAY_INIT;
> +       struct submodule_parallel_fetch *spf;
>         const char *work_tree = get_git_work_tree();
>         if (!work_tree)
>                 goto out;
> @@ -635,12 +702,9 @@ int fetch_populated_submodules(const struct argv_array *options,
>         argv_array_push(&argv, "--recurse-submodules-default");
>         /* default value, "--submodule-prefix" and its value are added later */
>
> -       cp.env = local_repo_env;
> -       cp.git_cmd = 1;
> -       cp.no_stdin = 1;
> -
>         calculate_changed_submodule_paths();
>
> +       tq = create_task_queue(max_parallel_jobs);
>         for (i = 0; i < active_nr; i++) {
>                 struct strbuf submodule_path = STRBUF_INIT;
>                 struct strbuf submodule_git_dir = STRBUF_INIT;
> @@ -693,24 +757,26 @@ int fetch_populated_submodules(const struct argv_array *options,
>                 if (!git_dir)
>                         git_dir = submodule_git_dir.buf;
>                 if (is_directory(git_dir)) {
> +                       spf = submodule_parallel_fetch_create();
> +                       spf->result = &result;
> +                       spf->cp.dir = strbuf_detach(&submodule_path, NULL);
>                         if (!quiet)
> -                               printf("Fetching submodule %s%s\n", prefix, ce->name);
> -                       cp.dir = submodule_path.buf;
> -                       argv_array_push(&argv, default_argv);
> -                       argv_array_push(&argv, "--submodule-prefix");
> -                       argv_array_push(&argv, submodule_prefix.buf);
> -                       cp.argv = argv.argv;
> -                       if (run_command(&cp))
> -                               result = 1;
> -                       argv_array_pop(&argv);
> -                       argv_array_pop(&argv);
> -                       argv_array_pop(&argv);
> +                               strbuf_addf(spf->cp.sync_buf,
> +                                           "Fetching submodule %s%s",
> +                                           prefix, ce->name);
> +                       argv_array_pushv(&spf->argv, argv.argv);
> +                       argv_array_push(&spf->argv, default_argv);
> +                       argv_array_push(&spf->argv, "--submodule-prefix");
> +                       argv_array_push(&spf->argv, submodule_prefix.buf);
> +                       set_output_mutex(spf);
> +                       add_task(tq, run_command_and_cleanup, spf);
>                 }
>                 strbuf_release(&submodule_path);
>                 strbuf_release(&submodule_git_dir);
>                 strbuf_release(&submodule_prefix);
>         }
>         argv_array_clear(&argv);
> +       destroy_output_mutex();
>  out:
>         string_list_clear(&changed_submodule_paths, 1);
>         return result;
> diff --git a/submodule.h b/submodule.h
> index 5507c3d..cbc0003 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -31,7 +31,7 @@ void set_config_fetch_recurse_submodules(int value);
>  void check_for_new_submodule_commits(unsigned char new_sha1[20]);
>  int fetch_populated_submodules(const struct argv_array *options,
>                                const char *prefix, int command_line_option,
> -                              int quiet);
> +                              int quiet, int max_parallel_jobs);
>  unsigned is_submodule_modified(const char *path, int ignore_untracked);
>  int submodule_uses_gitfile(const char *path);
>  int ok_to_remove_submodule(const char *path);
> --
> 2.5.0.264.g5e52b0d
>
Jonathan Nieder· Aug 28, 2015, 17:01 UTC · re: Stefan Beller · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

Stefan Beller wrote:
> On Thu, Aug 27, 2015 at 6:14 PM, Stefan Beller <sbeller@google.com> wrote:
Show 11 quoted lines
>> This makes use of the new task queue and the syncing feature of
>> run-command to fetch a number of submodules at the same time.
>>
>> The output will look like it would have been run sequential,
>> but faster.
>
> And it breaks the tests t5526-fetch-submodules.sh as the output is done
> on stderr only, instead of putting "Fetching submodule <submodule-path>
> to stdout. :(
>
> I guess combining stdout and stderr is not a good strategy after all now.

IMHO the "Fetching submodule <submodule-path>" output always should have gone to stderr. It is not output that scripts would be relying on --- it is just progress output.

So a preliminary patch doing that (and updating tests) would make sense to me.

Thoughts? Jonathan

Junio C Hamano· Aug 28, 2015, 17:12 UTC · re: Jonathan Nieder · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 21 quoted lines
> Stefan Beller wrote:
>> On Thu, Aug 27, 2015 at 6:14 PM, Stefan Beller <sbeller@google.com> wrote:
>
>>> This makes use of the new task queue and the syncing feature of
>>> run-command to fetch a number of submodules at the same time.
>>>
>>> The output will look like it would have been run sequential,
>>> but faster.
>>
>> And it breaks the tests t5526-fetch-submodules.sh as the output is done
>> on stderr only, instead of putting "Fetching submodule <submodule-path>
>> to stdout. :(
>>
>> I guess combining stdout and stderr is not a good strategy after all now.
>
> IMHO the "Fetching submodule <submodule-path>" output always should have
> gone to stderr.  It is not output that scripts would be relying on ---
> it is just progress output.
>
> So a preliminary patch doing that (and updating tests) would make sense
> to me.
Sounds good.

I personally do not think the "we still do all the output from a single process while blocking output from others" buffering implemented in this n-th round (by the way, please use [PATCH v$n N/M]) is worth doing, though. It does not make the output machine parseable, because the reader does not get any signal in what order output of these subprocesses arrive anyway. The payload does not even have "here is the beginning of output from the process that handled the submodule X" to delimit them.

My preference is still (1) leave standard error output all connected to the same fd without multiplexing, and (2) line buffer standard output so that the output is at least readable as a text, in a similar way a log of an irc channel where everybody is talking at the same time.

Stefan Beller· Aug 28, 2015, 17:45 UTC · re: Junio C Hamano · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

On Fri, Aug 28, 2015 at 10:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 32 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Stefan Beller wrote:
>>> On Thu, Aug 27, 2015 at 6:14 PM, Stefan Beller <sbeller@google.com> wrote:
>>
>>>> This makes use of the new task queue and the syncing feature of
>>>> run-command to fetch a number of submodules at the same time.
>>>>
>>>> The output will look like it would have been run sequential,
>>>> but faster.
>>>
>>> And it breaks the tests t5526-fetch-submodules.sh as the output is done
>>> on stderr only, instead of putting "Fetching submodule <submodule-path>
>>> to stdout. :(
>>>
>>> I guess combining stdout and stderr is not a good strategy after all now.
>>
>> IMHO the "Fetching submodule <submodule-path>" output always should have
>> gone to stderr.  It is not output that scripts would be relying on ---
>> it is just progress output.
>>
>> So a preliminary patch doing that (and updating tests) would make sense
>> to me.
>
> Sounds good.
>
> I personally do not think the "we still do all the output from a
> single process while blocking output from others" buffering
> implemented in this n-th round (by the way, please use [PATCH v$n
> N/M]) is worth doing, though.  It does not make the output machine
> parseable, because the reader does not get any signal in what order
> output of these subprocesses arrive anyway.
> The payload does not
> even have "here is the beginning of output from the process that
> handled the submodule X" to delimit them.

Oh it does, but it is not handled by the buffering code, but the application code, so for git-fetch we would have "Fetching submodule <path> " while for git submodule foreach we would have "Entering <submodule path>"

Show 6 quoted lines
>
> My preference is still (1) leave standard error output all connected
> to the same fd without multiplexing, and (2) line buffer standard
> output so that the output is at least readable as a text, in a
> similar way a log of an irc channel where everybody is talking at
> the same time.

In case of fetch passing the quiet flag becomes mandatory then, because the the per-repo progress is put to stderr, which also deletes previous output, to have the nice counters.

Jonathan Nieder· Aug 28, 2015, 18:20 UTC · re: Junio C Hamano · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

Junio C Hamano wrote:
Show 5 quoted lines
> My preference is still (1) leave standard error output all connected
> to the same fd without multiplexing, and (2) line buffer standard
> output so that the output is at least readable as a text, in a
> similar way a log of an irc channel where everybody is talking at
> the same time.

There is something nice about the immediacy of seeing output from all the subprocesses at the same time in that model.

But for commands that show progress like "git clone", "git checkout", and "git fetch", it does not work well at all. They provide output that updates itself by putting a carriage return at the end of each chunk of output, like this:

 remote: Finding sources:  11% (18/155)           \r
 remote: Finding sources:  12% (19/155)           \r

With multiple commands producing such output, they will overwrite each other's lines, producing a mixture that is confusing and unuseful.

Even with --no-progress, there is a similar sense of confusion in the intermixed output. Error messages are hard to find. This is a comment complaint about the current "repo sync -j" implementation.

Ideally what I as a user want to see is something like what "prove" writes, showing progress on the multiple tasks that are taking place at once:

 ===(     103;1  0/?  8/?  3/?  11/?  6/?  16/?  1/?  1/? )==============

That would require more sophisticated inter-process communication than seems necessary for the first version of parallel "git submodule update". For the first version that people use in the wild, showing output from one of the tasks at a time, simulating a sped-up sequential implementation, seems useful to me.

In the degenerate case where only one task is running, it reduces to the current output. The output is familiar and easy to use. I quite like the approach.

My two cents, Jonathan

Junio C Hamano· Aug 28, 2015, 18:27 UTC · re: Jonathan Nieder · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 21 quoted lines
> Junio C Hamano wrote:
>
>> My preference is still (1) leave standard error output all connected
>> to the same fd without multiplexing, and (2) line buffer standard
>> output so that the output is at least readable as a text, in a
>> similar way a log of an irc channel where everybody is talking at
>> the same time.
>
> There is something nice about the immediacy of seeing output from all
> the subprocesses at the same time in that model.
>
> But for commands that show progress like "git clone", "git checkout",
> and "git fetch", it does not work well at all.  They provide output
> that updates itself by putting a carriage return at the end of each
> chunk of output, like this:
>
>  remote: Finding sources:  11% (18/155)           \r
>  remote: Finding sources:  12% (19/155)           \r
>
> With multiple commands producing such output, they will overwrite each
> other's lines, producing a mixture that is confusing and unuseful.

That example also illustrates why it is not a useful to buffer all of these lines and showing them once.

Show 5 quoted lines
> Ideally what I as a user want to see is something like what "prove"
> writes, showing progress on the multiple tasks that are taking place
> at once:
>
>  ===(     103;1  0/?  8/?  3/?  11/?  6/?  16/?  1/?  1/? )==============

Tell me how that "buffer all and show them once" helps us to get near that ideal.

> That would require more sophisticated inter-process communication than
> seems necessary for the first version of parallel "git submodule
> update".

Exactly. Why waste memory to buffer and stall the entire output from other processes in the interim solution, then?

Jeff King· Aug 28, 2015, 18:35 UTC · re: Junio C Hamano · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

On Fri, Aug 28, 2015 at 11:27:04AM -0700, Junio C Hamano wrote:
Show 13 quoted lines
> > But for commands that show progress like "git clone", "git checkout",
> > and "git fetch", it does not work well at all.  They provide output
> > that updates itself by putting a carriage return at the end of each
> > chunk of output, like this:
> >
> >  remote: Finding sources:  11% (18/155)           \r
> >  remote: Finding sources:  12% (19/155)           \r
> >
> > With multiple commands producing such output, they will overwrite each
> > other's lines, producing a mixture that is confusing and unuseful.
> 
> That example also illustrates why it is not a useful to buffer all
> of these lines and showing them once.

I think Jonathan's point is that you could pick _one_ active child to show without buffering, while simultaneously buffering everybody else's output. When that finishes, pick a new active child, show its buffer, and then start showing its output in realtime. And so on.

So to an observer, it would look like a serial operation, but subsequent operations after the first would magically go much faster (because they'd been working and buffering in the background).

And that doesn't require any additional IPC magic (though I am not sure how we get progress in the first place if the child stderr is a pipe...).

-Peff
Junio C Hamano· Aug 28, 2015, 18:41 UTC · re: Jeff King · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> I think Jonathan's point is that you could pick _one_ active child to
> show without buffering, while simultaneously buffering everybody else's
> output. When that finishes, pick a new active child, show its buffer,
> and then start showing its output in realtime. And so on.
>
> So to an observer, it would look like a serial operation, but subsequent
> operations after the first would magically go much faster (because
> they'd been working and buffering in the background).
OK, I can buy that explanation.
> And that doesn't require any additional IPC magic (though I am not sure
> how we get progress in the first place if the child stderr is a
> pipe...).
That is a good point ;-)
Stefan Beller· Aug 28, 2015, 18:41 UTC · re: Jeff King · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

On Fri, Aug 28, 2015 at 11:35 AM, Jeff King <peff@peff.net> wrote:
Show 20 quoted lines
> On Fri, Aug 28, 2015 at 11:27:04AM -0700, Junio C Hamano wrote:
>
>> > But for commands that show progress like "git clone", "git checkout",
>> > and "git fetch", it does not work well at all.  They provide output
>> > that updates itself by putting a carriage return at the end of each
>> > chunk of output, like this:
>> >
>> >  remote: Finding sources:  11% (18/155)           \r
>> >  remote: Finding sources:  12% (19/155)           \r
>> >
>> > With multiple commands producing such output, they will overwrite each
>> > other's lines, producing a mixture that is confusing and unuseful.
>>
>> That example also illustrates why it is not a useful to buffer all
>> of these lines and showing them once.
>
> I think Jonathan's point is that you could pick _one_ active child to
> show without buffering, while simultaneously buffering everybody else's
> output. When that finishes, pick a new active child, show its buffer,
> and then start showing its output in realtime. And so on.

or better yet, pick that child with the most progress (i.e. flush all finished children and then pick the next active child), that would approximate the progress in the output best, as it would reduce the hidden buffered progress.

Show 8 quoted lines
>
> So to an observer, it would look like a serial operation, but subsequent
> operations after the first would magically go much faster (because
> they'd been working and buffering in the background).
>
> And that doesn't require any additional IPC magic (though I am not sure
> how we get progress in the first place if the child stderr is a
> pipe...).

Moving the contents from the pipe to a strbuf buffer which we can grow indefinitely (way larger than pipe limits, but the output of a git fetch should be small enough for that).

>
> -Peff
Jeff King· Aug 28, 2015, 18:44 UTC · re: Stefan Beller · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

On Fri, Aug 28, 2015 at 11:41:17AM -0700, Stefan Beller wrote:
Show 12 quoted lines
> > So to an observer, it would look like a serial operation, but subsequent
> > operations after the first would magically go much faster (because
> > they'd been working and buffering in the background).
> >
> > And that doesn't require any additional IPC magic (though I am not sure
> > how we get progress in the first place if the child stderr is a
> > pipe...).
> 
> Moving the contents from the pipe to a strbuf buffer which we can grow
> indefinitely
> (way larger than pipe limits, but the output of a git fetch should be
> small enough for that).

Right, clearly we can't rely on pipe buffers to be large enough here (though we _may_ want to rely on tempfiles if we aren't sure that the stdout is bounded in a reasonable way).

But what I meant was: the child will only show progress if stderr is a tty, but here it is not.

I wonder if we need to set GIT_STDERR_IS_TTY=1 in the parent process, and then respect it in the children (this is similar to what GIT_PAGER_IN_USE does for stdout).

-Peff
Jonathan Nieder· Aug 28, 2015, 18:50 UTC · re: Jeff King · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

Jeff King wrote:
Show 6 quoted lines
> Right, clearly we can't rely on pipe buffers to be large enough here
> (though we _may_ want to rely on tempfiles if we aren't sure that the
> stdout is bounded in a reasonable way).
>
> But what I meant was: the child will only show progress if stderr is a
> tty, but here it is not.
For clone / fetch, we can pass --progress explicitly.

For some reason 'git checkout' doesn't support a --progress option. I suppose it should. ;-)

Thanks, Jonathan

Jeff King· Aug 28, 2015, 18:53 UTC · re: Jonathan Nieder · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

On Fri, Aug 28, 2015 at 11:50:50AM -0700, Jonathan Nieder wrote:
Show 7 quoted lines
> > But what I meant was: the child will only show progress if stderr is a
> > tty, but here it is not.
> 
> For clone / fetch, we can pass --progress explicitly.
> 
> For some reason 'git checkout' doesn't support a --progress option.  I
> suppose it should. ;-)

Yeah, that will work for those tools, but I thought you could pass arbitrary shell commands. It would be nice if git sub-commands run through those just magically worked, even though we don't have an opportunity to change their command-line parameters.

-Peff
Stefan Beller· Aug 28, 2015, 19:02 UTC · re: Jeff King · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

On Fri, Aug 28, 2015 at 11:53 AM, Jeff King <peff@peff.net> wrote:
Show 14 quoted lines
> On Fri, Aug 28, 2015 at 11:50:50AM -0700, Jonathan Nieder wrote:
>
>> > But what I meant was: the child will only show progress if stderr is a
>> > tty, but here it is not.
>>
>> For clone / fetch, we can pass --progress explicitly.
>>
>> For some reason 'git checkout' doesn't support a --progress option.  I
>> suppose it should. ;-)
>
> Yeah, that will work for those tools, but I thought you could pass
> arbitrary shell commands.  It would be nice if git sub-commands run
> through those just magically worked, even though we don't have an
> opportunity to change their command-line parameters.

Technically speaking this discusses the patch for fetch only and we'd want to have that discussion at the previous patch. ;) But as both of them use the same code, the sync feature of run-command, which is in patch 5/9, we want to have git commands just work anyway.

>
> -Peff
Stefan Beller· Aug 28, 2015, 18:59 UTC · re: Jeff King · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

On Fri, Aug 28, 2015 at 11:44 AM, Jeff King <peff@peff.net> wrote:
Show 21 quoted lines
> On Fri, Aug 28, 2015 at 11:41:17AM -0700, Stefan Beller wrote:
>
>> > So to an observer, it would look like a serial operation, but subsequent
>> > operations after the first would magically go much faster (because
>> > they'd been working and buffering in the background).
>> >
>> > And that doesn't require any additional IPC magic (though I am not sure
>> > how we get progress in the first place if the child stderr is a
>> > pipe...).
>>
>> Moving the contents from the pipe to a strbuf buffer which we can grow
>> indefinitely
>> (way larger than pipe limits, but the output of a git fetch should be
>> small enough for that).
>
> Right, clearly we can't rely on pipe buffers to be large enough here
> (though we _may_ want to rely on tempfiles if we aren't sure that the
> stdout is bounded in a reasonable way).
>
> But what I meant was: the child will only show progress if stderr is a
> tty, but here it is not.
Oh, I forgot about that.
>
> I wonder if we need to set GIT_STDERR_IS_TTY=1 in the parent process,
> and then respect it in the children (this is similar to what
> GIT_PAGER_IN_USE does for stdout).

The use of GIT_PAGER_IN_USE looks straightforward to me. I'll try to add GIT_STDERR_IS_TTY then.

>
> -Peff
Jonathan Nieder· Aug 28, 2015, 18:44 UTC · re: Jeff King · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

Jeff King wrote:
> I think Jonathan's point is that you could pick _one_ active child to
> show without buffering, while simultaneously buffering everybody else's
> output.
Yep.  Thanks for interpreting.
[...]
> So to an observer, it would look like a serial operation, but subsequent
> operations after the first would magically go much faster (because
> they'd been working and buffering in the background).
Yes.

Thanks, Jonathan

Stefan Beller· Aug 28, 2015, 18:36 UTC · re: Junio C Hamano · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

On Fri, Aug 28, 2015 at 11:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 35 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Junio C Hamano wrote:
>>
>>> My preference is still (1) leave standard error output all connected
>>> to the same fd without multiplexing, and (2) line buffer standard
>>> output so that the output is at least readable as a text, in a
>>> similar way a log of an irc channel where everybody is talking at
>>> the same time.
>>
>> There is something nice about the immediacy of seeing output from all
>> the subprocesses at the same time in that model.
>>
>> But for commands that show progress like "git clone", "git checkout",
>> and "git fetch", it does not work well at all.  They provide output
>> that updates itself by putting a carriage return at the end of each
>> chunk of output, like this:
>>
>>  remote: Finding sources:  11% (18/155)           \r
>>  remote: Finding sources:  12% (19/155)           \r
>>
>> With multiple commands producing such output, they will overwrite each
>> other's lines, producing a mixture that is confusing and unuseful.
>
> That example also illustrates why it is not a useful to buffer all
> of these lines and showing them once.
>
>> Ideally what I as a user want to see is something like what "prove"
>> writes, showing progress on the multiple tasks that are taking place
>> at once:
>>
>>  ===(     103;1  0/?  8/?  3/?  11/?  6/?  16/?  1/?  1/? )==============
>
> Tell me how that "buffer all and show them once" helps us to get
> near that ideal.

It doesn't, but it looks better than the irc like everybody speaks at the same time solution IMHO.

At the very the user may not even be interested in the details similar to what prove provides. The client can estimate its own total progress by weighting the progress from all tasks.

Show 9 quoted lines
>
>> That would require more sophisticated inter-process communication than
>> seems necessary for the first version of parallel "git submodule
>> update".
>
> Exactly.  Why waste memory to buffer and stall the entire output
> from other processes in the interim solution, then?
>
>
Jonathan Nieder· Aug 28, 2015, 18:42 UTC · re: Junio C Hamano · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 8 quoted lines
>>  remote: Finding sources:  11% (18/155)           \r
>>  remote: Finding sources:  12% (19/155)           \r
>>
>> With multiple commands producing such output, they will overwrite each
>> other's lines, producing a mixture that is confusing and unuseful.
>
> That example also illustrates why it is not a useful to buffer all
> of these lines and showing them once.

I don't completely follow. Are you referring to the wasted memory to store the line that is going to be written and rewritten or some other aspect?

Today (without parallelism), if I clone a repository with multiple submodules and walk away from the terminal, then when I get back I see

	Cloning into 'plugins/cookbook-plugin'...
	remote: Counting objects: 36, done
	remote: Finding sources: 100% (36/36)
	remote: Total 1192 (delta 196), reused 1192 (delta 196)
	Receiving objects: 100% (1192/1192), 239.46 KiB | 0 bytes/s, done.
	Resolving deltas: 100% (196/196), done.
	Checking connectivity... done.
	Submodule path 'plugins/cookbook-plugin': checked out 'b9d3ca8a65030071e28be19296ba867ab424fbbf'
	Cloning into 'plugins/download-commands'...
	remote: Counting objects: 37, done
	remote: Finding sources: 100% (37/37)
	remote: Total 448 (delta 46), reused 448 (delta 46)
	Receiving objects: 100% (448/448), 96.13 KiB | 0 bytes/s, done.
	Resolving deltas: 100% (46/46), done.
	Checking connectivity... done.
	Submodule path 'plugins/download-commands': checked out '99e61fb06a4505a9558c23a56213cb32ceaa9cca'
	...

The output for each submodule is in one chunk and I can understand what happened in each.

By contrast, with inter-mixing the speed of output tells you something about whether the process is stalled while you stare at the screen and wait for it to finish (good) but the result on the screen is unintelligible when the process finishes (bad).

Showing interactive output for one task still provides the real-time feedback about whether it is completely stuck and needs to be cancelled. It is easier to make sense of even from the point of view of real-time progress than the intermixed output.

What problem is the intermixed output meant to solve?

In other words, what is your objection about? Perhaps there is a way to both satisfy that objection and to have output clumped per task, but it is hard to know without knowing what the objection is.

Hope that helps, Jonathan

Junio C Hamano· Aug 31, 2015, 18:56 UTC · re: Stefan Beller · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

Stefan Beller <sbeller@google.com> writes:
> +static void destroy_output_mutex()
static void destroy_output_mutex(void)
Show 12 quoted lines
> +{
> +	pthread_mutex_destroy(&output_mutex);
> +}
> +
> +#else
> +#define set_output_mutex()
> +#define destroy_output_mutex()
> +#define lock_output_mutex()
> +#define unlock_output_mutex()
> +#endif
> +
> +static struct submodule_parallel_fetch *submodule_parallel_fetch_create()
static struct submodule_parallel_fetch *submodule_parallel_fetch_create(void)
Jeff King· Aug 31, 2015, 19:05 UTC · re: Junio C Hamano · lore

Re: [PATCH 7/9] fetch: fetch submodules in parallel

On Mon, Aug 31, 2015 at 11:56:04AM -0700, Junio C Hamano wrote:
Show 5 quoted lines
> Stefan Beller <sbeller@google.com> writes:
> 
> > +static void destroy_output_mutex()
> 
> static void destroy_output_mutex(void)

Yep. Stefan, you may want to beef up the warning flags in your config.mak. For reference, I use:

  CFLAGS += -Wall -Werror
  CFLAGS += -Wdeclaration-after-statement
  CFLAGS += -Wpointer-arith
  CFLAGS += -Wstrict-prototypes
  CFLAGS += -Wvla
  CFLAGS += -Wold-style-declaration
  CFLAGS += -Wold-style-definition

though note that you will need to relax some of those if compiling older versions of git. My complete config.mak is at:

  https://github.com/peff/git/blob/meta/config/config.mak

which handles this semi-automatically (it's wildly undocumented, but I'd be happy to explain any of it if anybody is interested).

-Peff
Stefan Beller· Aug 28, 2015, 01:14 UTC · re: Stefan Beller · lore

[PATCH 8/9] index-pack: Use the new worker pool

This demonstrates how the new threading API may be used. There is no change in the workflow, just using the new threading API instead of keeping track of the pthreads ourselves.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/index-pack.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)
Show changes to builtin/index-pack.c +12 −11
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 3f10840..187b281 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1075,7 +1075,7 @@ static void resolve_base(struct object_entry *obj)
 }
 
 #ifndef NO_PTHREADS
-static void *threaded_second_pass(void *data)
+static int threaded_second_pass(struct task_queue *tq, void *data)
 {
 	set_thread_data(data);
 	for (;;) {
@@ -1096,7 +1096,7 @@ static void *threaded_second_pass(void *data)
 
 		resolve_base(&objects[i]);
 	}
-	return NULL;
+	return 0;
 }
 #endif
 
@@ -1195,18 +1195,19 @@ static void resolve_deltas(void)
 					  nr_ref_deltas + nr_ofs_deltas);
 
 #ifndef NO_PTHREADS
-	nr_dispatched = 0;
+
 	if (nr_threads > 1 || getenv("GIT_FORCE_THREADS")) {
+		struct task_queue *tq;
+		nr_dispatched = 0;
 		init_thread();
-		for (i = 0; i < nr_threads; i++) {
-			int ret = pthread_create(&thread_data[i].thread, NULL,
-						 threaded_second_pass, thread_data + i);
-			if (ret)
-				die(_("unable to create thread: %s"),
-				    strerror(ret));
-		}
+
+		tq = create_task_queue(nr_threads);
 		for (i = 0; i < nr_threads; i++)
-			pthread_join(thread_data[i].thread, NULL);
+			add_task(tq, threaded_second_pass, thread_data + i);
+
+		if (finish_task_queue(tq, NULL))
+			die("Not all threads have finished");
+
 		cleanup_thread();
 		return;
 	}
-- 
2.5.0.264.g5e52b0d
Stefan Beller· Aug 28, 2015, 01:14 UTC · re: Stefan Beller · lore

[PATCH 9/9] pack-objects: Use new worker pool

Before we had <n> threads doing the delta finding work, and the main thread was load balancing the threads, i.e. moving work from a thread with a large amount left to an idle thread whenever such a situation arose.

This moves the load balancing to the threads themselves. As soon as one thread is done working it will look at its peer threads and will pickup half the work load from the thread with the largest pending load.

By having the load balancing as part of the threads, the locking and communication model becomes easier, such that we don't need so many mutexes any more.

It also demonstrates the usage of the new threading pool being easily applied in different situations.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/pack-objects.c | 175 ++++++++++++++++---------------------------------
 1 file changed, 57 insertions(+), 118 deletions(-)
Show changes to builtin/pack-objects.c +57 −118
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 62cc16d..f46d2df 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -17,6 +17,7 @@
 #include "pack-objects.h"
 #include "progress.h"
 #include "refs.h"
+#include "run-command.h"
 #include "streaming.h"
 #include "thread-utils.h"
 #include "pack-bitmap.h"
@@ -1887,26 +1888,12 @@ static void try_to_free_from_threads(size_t size)
 
 static try_to_free_t old_try_to_free_routine;
 
-/*
- * The main thread waits on the condition that (at least) one of the workers
- * has stopped working (which is indicated in the .working member of
- * struct thread_params).
- * When a work thread has completed its work, it sets .working to 0 and
- * signals the main thread and waits on the condition that .data_ready
- * becomes 1.
- */
-
 struct thread_params {
-	pthread_t thread;
 	struct object_entry **list;
 	unsigned list_size;
 	unsigned remaining;
 	int window;
 	int depth;
-	int working;
-	int data_ready;
-	pthread_mutex_t mutex;
-	pthread_cond_t cond;
 	unsigned *processed;
 };
 
@@ -1933,7 +1920,52 @@ static void cleanup_threaded_search(void)
 	pthread_mutex_destroy(&progress_mutex);
 }
 
-static void *threaded_find_deltas(void *arg)
+static struct thread_params *p;
+
+static void threaded_split_largest_workload(struct thread_params *target)
+{
+	int i;
+
+	struct object_entry **list;
+	struct thread_params *victim = NULL;
+	unsigned sub_size = 0;
+
+	/* Find a victim */
+	progress_lock();
+	for (i = 0; i < delta_search_threads; i++)
+		if (p[i].remaining > 2*window &&
+		    (!victim || victim->remaining < p[i].remaining))
+			victim = &p[i];
+
+	if (victim) {
+		sub_size = victim->remaining / 2;
+		list = victim->list + victim->list_size - sub_size;
+		while (sub_size && list[0]->hash &&
+		       list[0]->hash == list[-1]->hash) {
+			list++;
+			sub_size--;
+		}
+		if (!sub_size) {
+			/*
+			 * It is possible for some "paths" to have
+			 * so many objects that no hash boundary
+			 * might be found.  Let's just steal the
+			 * exact half in that case.
+			 */
+			sub_size = victim->remaining / 2;
+			list -= sub_size;
+		}
+		victim->list_size -= sub_size;
+		victim->remaining -= sub_size;
+
+		target->list = list;
+		target->list_size = sub_size;
+		target->remaining = sub_size;
+	}
+	progress_unlock();
+}
+
+static int threaded_find_deltas(struct task_queue *tq, void *arg)
 {
 	struct thread_params *me = arg;
 
@@ -1941,34 +1973,17 @@ static void *threaded_find_deltas(void *arg)
 		find_deltas(me->list, &me->remaining,
 			    me->window, me->depth, me->processed);
 
-		progress_lock();
-		me->working = 0;
-		pthread_cond_signal(&progress_cond);
-		progress_unlock();
-
-		/*
-		 * We must not set ->data_ready before we wait on the
-		 * condition because the main thread may have set it to 1
-		 * before we get here. In order to be sure that new
-		 * work is available if we see 1 in ->data_ready, it
-		 * was initialized to 0 before this thread was spawned
-		 * and we reset it to 0 right away.
-		 */
-		pthread_mutex_lock(&me->mutex);
-		while (!me->data_ready)
-			pthread_cond_wait(&me->cond, &me->mutex);
-		me->data_ready = 0;
-		pthread_mutex_unlock(&me->mutex);
+		threaded_split_largest_workload(me);
 	}
-	/* leave ->working 1 so that this doesn't get more work assigned */
-	return NULL;
+
+	return 0;
 }
 
 static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 			   int window, int depth, unsigned *processed)
 {
-	struct thread_params *p;
-	int i, ret, active_threads = 0;
+	struct task_queue *tq;
+	int i;
 
 	init_threaded_search();
 
@@ -1980,8 +1995,11 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 	if (progress > pack_to_stdout)
 		fprintf(stderr, "Delta compression using up to %d threads.\n",
 				delta_search_threads);
+
 	p = xcalloc(delta_search_threads, sizeof(*p));
 
+	tq = create_task_queue(delta_search_threads);
+
 	/* Partition the work amongst work threads. */
 	for (i = 0; i < delta_search_threads; i++) {
 		unsigned sub_size = list_size / (delta_search_threads - i);
@@ -1993,8 +2011,6 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 		p[i].window = window;
 		p[i].depth = depth;
 		p[i].processed = processed;
-		p[i].working = 1;
-		p[i].data_ready = 0;
 
 		/* try to split chunks on "path" boundaries */
 		while (sub_size && sub_size < list_size &&
@@ -2008,87 +2024,10 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 
 		list += sub_size;
 		list_size -= sub_size;
+		add_task(tq, threaded_find_deltas, &p[i]);
 	}
 
-	/* Start work threads. */
-	for (i = 0; i < delta_search_threads; i++) {
-		if (!p[i].list_size)
-			continue;
-		pthread_mutex_init(&p[i].mutex, NULL);
-		pthread_cond_init(&p[i].cond, NULL);
-		ret = pthread_create(&p[i].thread, NULL,
-				     threaded_find_deltas, &p[i]);
-		if (ret)
-			die("unable to create thread: %s", strerror(ret));
-		active_threads++;
-	}
-
-	/*
-	 * Now let's wait for work completion.  Each time a thread is done
-	 * with its work, we steal half of the remaining work from the
-	 * thread with the largest number of unprocessed objects and give
-	 * it to that newly idle thread.  This ensure good load balancing
-	 * until the remaining object list segments are simply too short
-	 * to be worth splitting anymore.
-	 */
-	while (active_threads) {
-		struct thread_params *target = NULL;
-		struct thread_params *victim = NULL;
-		unsigned sub_size = 0;
-
-		progress_lock();
-		for (;;) {
-			for (i = 0; !target && i < delta_search_threads; i++)
-				if (!p[i].working)
-					target = &p[i];
-			if (target)
-				break;
-			pthread_cond_wait(&progress_cond, &progress_mutex);
-		}
-
-		for (i = 0; i < delta_search_threads; i++)
-			if (p[i].remaining > 2*window &&
-			    (!victim || victim->remaining < p[i].remaining))
-				victim = &p[i];
-		if (victim) {
-			sub_size = victim->remaining / 2;
-			list = victim->list + victim->list_size - sub_size;
-			while (sub_size && list[0]->hash &&
-			       list[0]->hash == list[-1]->hash) {
-				list++;
-				sub_size--;
-			}
-			if (!sub_size) {
-				/*
-				 * It is possible for some "paths" to have
-				 * so many objects that no hash boundary
-				 * might be found.  Let's just steal the
-				 * exact half in that case.
-				 */
-				sub_size = victim->remaining / 2;
-				list -= sub_size;
-			}
-			target->list = list;
-			victim->list_size -= sub_size;
-			victim->remaining -= sub_size;
-		}
-		target->list_size = sub_size;
-		target->remaining = sub_size;
-		target->working = 1;
-		progress_unlock();
-
-		pthread_mutex_lock(&target->mutex);
-		target->data_ready = 1;
-		pthread_cond_signal(&target->cond);
-		pthread_mutex_unlock(&target->mutex);
-
-		if (!sub_size) {
-			pthread_join(target->thread, NULL);
-			pthread_cond_destroy(&target->cond);
-			pthread_mutex_destroy(&target->mutex);
-			active_threads--;
-		}
-	}
+	finish_task_queue(tq, NULL);
 	cleanup_threaded_search();
 	free(p);
 }
-- 
2.5.0.264.g5e52b0d
Johannes Schindelin· Aug 28, 2015, 10:09 UTC · re: Stefan Beller · lore

Re: [PATCH 0/9] Progress with git submodule

Hi Stefan,
On 2015-08-28 03:14, Stefan Beller wrote:
> Stefan Beller (9):
>   submodule: implement `module_list` as a builtin helper
>   submodule: implement `module_name` as a builtin helper
>   submodule: implement `module_clone` as a builtin helper
Another thing that just hit me: is there any specific reason why the underscore? I think we prefer dashes for subcommands (think: cherry-pick) and maybe we want to do the same for subsubcommands...

Ciao, Dscho

Stefan Beller· Aug 28, 2015, 16:35 UTC · re: Johannes Schindelin · lore

Re: [PATCH 0/9] Progress with git submodule

On Fri, Aug 28, 2015 at 3:09 AM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 14 quoted lines
> Hi Stefan,
>
> On 2015-08-28 03:14, Stefan Beller wrote:
>
>> Stefan Beller (9):
>>   submodule: implement `module_list` as a builtin helper
>>   submodule: implement `module_name` as a builtin helper
>>   submodule: implement `module_clone` as a builtin helper
>
> Another thing that just hit me: is there any specific reason why the underscore? I think we prefer dashes for subcommands (think: cherry-pick) and maybe we want to do the same for subsubcommands...
>
> Ciao,
> Dscho
>
Junio wrote on Aug 3rd:
Show 7 quoted lines
>>>     $ git submodule--helper module_list
>>
>> Why would you use an underscore in here as opposed to a dash?
>
> Simply because the diff would be easier to read; the callers used to
> call module_list shell function, now they call the subcommand with the
> same name of submodule--helper.

I mean no user is expected to use the submodule--helper functions directly and we want to get rid of them eventually anyway (once the whole git-submodule.sh is rewritten in C we can just call the C functions instead of calling out to helpers)

← back to recent threads