[PATCHv2 0/3] submodule--helper: Have some refactoring only patches first

[PATCHv2 0/3] submodule--helper: Have some refactoring only patches first

[Stefan Beller]

This patch series is the first part of the larger series I sent daily last week
and contains only those patches which have had good review by Jeff, Dscho
and Junio doing only internal refactoring, not introducing any parallelism yet.
It replaces all patches on top of (5a1ba6b48a62b,
Merge 'hv/submodule-config' to 'sb/submodule-helper')

Changes are only small nits:
 * whitespace correction in free((char *)pointer);
 * break a long line,
 * git submodule--helper -h adheres to the standard help output
 
One of the mistakes I made last week was mixing these quite stable patches
with highly RFC/WIP patches and not pointing out loud which is which.

Thanks,
Stefan

Stefan Beller (3):
  submodule: implement `module_list` as a builtin helper
  submodule: implement `module_name` as a builtin helper
  submodule: implement `module_clone` as a builtin helper

 .gitignore                  |   1 +
 Makefile                    |   1 +
 builtin.h                   |   1 +
 builtin/submodule--helper.c | 286 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 166 +++----------------------
 git.c                       |   1 +
 6 files changed, 306 insertions(+), 150 deletions(-)
 create mode 100644 builtin/submodule--helper.c

-- 
2.5.0.264.geed6e44.dirty

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

[Stefan Beller]

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

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.geed6e44.dirty

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

[Stefan Beller]

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(-)

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.geed6e44.dirty

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

[Stefan Beller]

`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(-)

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.geed6e44.dirty

Re: [PATCHv2 0/3] submodule--helper: Have some refactoring only patches first

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> One of the mistakes I made last week was mixing these quite stable patches
> with highly RFC/WIP patches and not pointing out loud which is which.

I think it was very clear which is which to people who have been
following (which are the only people you would care about anyway),
but I agree that it would be easier to follow by others if you made
two series, i.e. solid bits like this one, marked with "PATCH v$n",
and the WIP part as a separate series, described as building on the
v$n reroll of the other series in its own cover letter.

Thanks, will replace.  Let's move this part to 'next' and start
cooking.

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>  usage:
> -	usage("git submodule--helper module_list\n");
> +	usage("git submodule--helper [module_list | module_name]\n");

To me, the above reads as if saying:

    The command takes one of the two subcommands at this stage,
    module_list that does not take any parameter, and module_name
    that does not take any parameter.

which is not what you intended.

I think that the help for individual options and arguments are
sufficiently given in the implementation of each subcommand
(e.g. module_list does its own parse_options() thing), so there is
no need to duplicate them here.  The only purpose of this usage serves
is to tell the user that the subcommand name was not understood, and
give the list of available subcommands.  For that, I wonder if the
usual single-liner "usage" is the best way to do so.

    $ git submodule--helper frotz
    fatal: 'frotz' is not a valid submodule--helper subcommand, which are
           module_list, module_name.

or something along that line, perhaps, may be more appropriate?

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

[Stefan Beller]

On Mon, Aug 31, 2015 at 1:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>  usage:
>> -     usage("git submodule--helper module_list\n");
>> +     usage("git submodule--helper [module_list | module_name]\n");
>
> To me, the above reads as if saying:
>
>     The command takes one of the two subcommands at this stage,
>     module_list that does not take any parameter, and module_name
>     that does not take any parameter.
>
> which is not what you intended.
>
> I think that the help for individual options and arguments are
> sufficiently given in the implementation of each subcommand
> (e.g. module_list does its own parse_options() thing), so there is
> no need to duplicate them here.  The only purpose of this usage serves
> is to tell the user that the subcommand name was not understood, and
> give the list of available subcommands.  For that, I wonder if the
> usual single-liner "usage" is the best way to do so.
>
>     $ git submodule--helper frotz
>     fatal: 'frotz' is not a valid submodule--helper subcommand, which are
>            module_list, module_name.
>
> or something along that line, perhaps, may be more appropriate?

As this is something that *should* not happen in the wild, (but it
will of course),
it sounds like a good idea to have a clear error message here. I'll send a patch
for that. (either one on top of 3/3 to improve the message, or a
reroll of the series,
as you'd like)

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

[Eric Sunshine]

On Mon, Aug 31, 2015 at 3:19 PM, Stefan Beller <sbeller@google.com> wrote:
> 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.

ECANNOTPARSE

Did you mean
    s/git-submodule.sh/&,/
    s/commands/&,/
    s/top most/top-most/
    s/keeping/keep/
    s/subdirectories/subdirectory/
?

> 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

s/stil/still/

> 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.

s/where to/at which to/

More below.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 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;

Why is 'alternative_path' declared at file scope?

> +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;

Does 'ps_matched' need to be freed before these two 'return's?

> +}
> +
> +static int module_list(int argc, const char **argv, const char *prefix)
> +{
> +       int i;
> +       static struct pathspec pathspec;

Drop 'static'.

> +
> +       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[] = {

You can drop this 'static' too.

Style: *const

> +               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));
> +               }

Style: drop unnecessary braces.

> +               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");

Aside:
[bikeshedding on]

I agree with Dscho that these subcommands would be better spelled with
a hyphen than an underscore. If I recall correctly, the arguments for
using underscore were (1) a less noisy diff, (2) these are internal
commands nobody will be typing anyhow. However, (1) the diff noise
will be the same with hyphens, and (2) people will want to test these
commands manually anyhow, and its easier to type hyphens and easier to
remember them since the precedent for hyphens in command-names is
already well established.

Also, the reason that the original shell code used underscores was
because hyphens are not valid characters in shell function names, but
that's an implementation detail which shouldn't be allowed to bleed
over to user-facing interface design (and these subcommands are
user-facing).

[bikeshedding off]

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

[Eric Sunshine]

On Mon, Aug 31, 2015 at 3:19 PM, Stefan Beller <sbeller@google.com> wrote:
> This implements the helper `module_name` in C instead of shell,
> yielding a nice performance boost.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 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
> @@ -101,6 +104,26 @@ static int module_list(int argc, const char **argv, const char *prefix)
> +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]);

In the original shell code, this error message went through
eval_gettext(), so don't you want:

    die(_("No ..."), ...);

?

> +       name = sub->name;
> +       printf("%s\n", name);

Why the useless assignment to 'name'? Instead:

    printf("%s\n", sub->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");
>  }

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

[Eric Sunshine]

On Mon, Aug 31, 2015 at 3:19 PM, Stefan Beller <sbeller@google.com> wrote:
> `module_clone` is part of the update command,
> which I want to convert to C next.

Hmm, place commentary below "---".

> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 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
> @@ -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, "")) {

More idiomatic:

    if (depth && *depth) {

> +               argv_array_push(&cp.args, "--depth");
> +               argv_array_push(&cp.args, depth);

These would be easier to read if you consolidate them with argv_array_pushl():

    argv_array_pushl(&cp.args, "--depth", depth, NULL);

and it would allow you to drop the noisy braces.

> +       }
> +       if (reference && strcmp(reference, "")) {

    if (reference && *reference) {

> +               argv_array_push(&cp.args, "--reference");
> +               argv_array_push(&cp.args, reference);

    argv_array_pushl(&cp.args, "--reference", reference, NULL);

> +       }
> +       if (gitdir) {

Why is this case different than the others, in that it doesn't check
for the zero-length string ""?

> +               argv_array_push(&cp.args, "--separate-git-dir");
> +               argv_array_push(&cp.args, gitdir);

    argv_array_pushl(&cp.args, "--separate-git-dir", gitdir, NULL);

> +       }
> +       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;

Why is 'sm_gitdir' declared const even though it's always pointing at
memory which this function owns and needs to free? If you drop the
'const', then you won't have to cast it to non-const in a couple
places below.

> +       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[] = {

You can drop this 'static'.

> +               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;

I understand that you're simply replicating the behavior of the shell
code, but this is "yuck".

'module_clone' is only called from two places in git-submodule.sh, so
how about a cleanup patch which makes 'module_clone' accept a --quiet
flag and have the two callers pass it explicitly? Finally, have this C
replacement accept --quiet as a proper command-line option rather than
via this hidden backdoor method.

> +       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);

Why is it that the return value of
safe_create_leading_directories_const() is sometimes checked in this
function and sometimes not?

> +               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);

Check the return value of unlink()?

> +       }
> +
> +       /* Write a .git file in the submodule to redirect to the superproject. */
> +       if (alternative_path && !strcmp(alternative_path, "")) {

Maybe?

    if (alternative_path && !*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);

Why the mix of capitalized and lowercase-only error messages in this function?

Also, the original shell code consistently uses eval_gettext(), so
_("...") seems warranted.

> +       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 */

s/superprojects/superproject's/

> +       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);

Why squirrel-away 'sm_gitdir' in 's' unnecessarily? Equivalent:

    if (strbuf_getcwd(&sb))
        die_errno(...);
    strbuf_addf(&sb, "/%s, sm_gitdir);
    free(sm_gitdir);
    sm_gitdir = strbuf_detach(&sb, NULL);

> +       }
> +
> +       if (strbuf_getcwd(&sb))
> +               die_errno("unable to get current working directory");

The conditional block just above here also gets 'cwd'. If you move
this code above the !is_absolute_path(sm_gitdir) conditional block,
then you can avoid the duplication.

> +       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}"
>  }

[PATCHv3 0/3] submodule--helper: Have some refactoring only patches first

[Stefan Beller]

* Implemented different error messages as suggested by Junio
* Implemented all of Erics suggestions,
  including renaming to module-with-dash-now

Stefan Beller (3):
  submodule: implement `module-list` as a builtin helper
  submodule: implement `module-name` as a builtin helper
  submodule: implement `module-clone` as a builtin helper

 .gitignore                  |   1 +
 Makefile                    |   1 +
 builtin.h                   |   1 +
 builtin/submodule--helper.c | 282 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 166 +++-----------------------
 git.c                       |   1 +
 6 files changed, 302 insertions(+), 150 deletions(-)
 create mode 100644 builtin/submodule--helper.c

-- 
2.5.0.256.gb62b30d.dirty

[PATCHv3 1/3] submodule: implement `module-list` as a builtin helper

[Stefan Beller]

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 keep the
subdirectory 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 still 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 which to anchor relative path arguments.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 .gitignore                  |   1 +
 Makefile                    |   1 +
 builtin.h                   |   1 +
 builtin/submodule--helper.c | 117 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  54 +++-----------------
 git.c                       |   1 +
 6 files changed, 127 insertions(+), 48 deletions(-)
 create mode 100644 builtin/submodule--helper.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..44310f5
--- /dev/null
+++ b/builtin/submodule--helper.c
@@ -0,0 +1,117 @@
+#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 int module_list_compute(int argc, const char **argv,
+				const char *prefix,
+				struct pathspec *pathspec)
+{
+	int i, result = 0;
+	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))
+		result = -1;
+
+	free(ps_matched);
+
+	return result;
+}
+
+static int module_list(int argc, const char **argv, const char *prefix)
+{
+	int i;
+	struct pathspec pathspec;
+	const char *alternative_path;
+
+	struct option module_list_options[] = {
+		OPT_STRING(0, "prefix", &alternative_path,
+			   N_("path"),
+			   N_("alternative anchor for relative paths")),
+		OPT_END()
+	};
+
+	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)
+		die(N_("fatal: submodule--helper subcommand must be called with "
+		       "a subcommand, which is module-list\n"));
+
+	if (!strcmp(argv[1], "module-list"))
+		return module_list(argc - 1, argv + 1, prefix);
+
+	die(N_("fatal: '%s' is not a valid submodule--helper subcommand, "
+	       "which is module-list\n"),
+	    argv[1]);
+}
diff --git a/git-submodule.sh b/git-submodule.sh
index 36797c3..d4d710c 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.256.gb62b30d.dirty

[PATCHv3 2/3] submodule: implement `module-name` as a builtin helper

[Stefan Beller]

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>
---
 builtin/submodule--helper.c | 29 +++++++++++++++++++++++++++--
 git-submodule.sh            | 32 +++++++-------------------------
 2 files changed, 34 insertions(+), 27 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 44310f5..91bd420 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;
@@ -102,16 +105,38 @@ 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 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(N_("No submodule mapping found in .gitmodules for path '%s'"),
+		    argv[0]);
+
+	printf("%s\n", sub->name);
+
+	return 0;
+}
+
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
 {
 	if (argc < 2)
 		die(N_("fatal: submodule--helper subcommand must be called with "
-		       "a subcommand, which is module-list\n"));
+		       "a subcommand, which are module-list, module-name\n"));
 
 	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);
+
 	die(N_("fatal: '%s' is not a valid submodule--helper subcommand, "
-	       "which is module-list\n"),
+	       "which are module-list, module-name\n"),
 	    argv[1]);
 }
diff --git a/git-submodule.sh b/git-submodule.sh
index d4d710c..5f3dfc5 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.256.gb62b30d.dirty

[PATCHv3 3/3] submodule: implement `module-clone` as a builtin helper

[Stefan Beller]

This converts implements the helper `module_clone`. This functionality is
needed for converting `git submodule update` later on, which we want to
add threading to.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 144 +++++++++++++++++++++++++++++++++++++++++++-
 git-submodule.sh            |  80 +-----------------------
 2 files changed, 145 insertions(+), 79 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 91bd420..0669641 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,11 +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 && *depth)
+		argv_array_pushl(&cp.args, "--depth", depth, NULL);
+	if (reference && *reference)
+		argv_array_pushl(&cp.args, "--reference", reference, NULL);
+	if (gitdir && *gitdir)
+		argv_array_pushl(&cp.args, "--separate-git-dir", gitdir, NULL);
+
+	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;
+	const char *alternative_path = NULL, *p;
+	int quiet = 0;
+	FILE *submodule_dot_git;
+	char *sm_gitdir;
+	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__QUIET(&quiet, "Suppress output for cloning a submodule"),
+		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);
+
+	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(sm_gitdir);
+	return 0;
+}
+
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
 {
 	if (argc < 2)
 		die(N_("fatal: submodule--helper subcommand must be called with "
-		       "a subcommand, which are module-list, module-name\n"));
+		       "a subcommand, which are module-list, module-name, "
+		       "module-clone\n"));
 
 	if (!strcmp(argv[1], "module-list"))
 		return module_list(argc - 1, argv + 1, prefix);
@@ -136,7 +273,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);
+
 	die(N_("fatal: '%s' is not a valid submodule--helper subcommand, "
-	       "which are module-list, module-name\n"),
+	       "which are module-list, module-name, module-clone\n"),
 	    argv[1]);
 }
diff --git a/git-submodule.sh b/git-submodule.sh
index 5f3dfc5..d1523ea 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 ${GIT_QUIET:+--quiet} --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 ${GIT_QUIET:+--quiet} --prefix "$prefix" --path "$sm_path" --name "$name" --url "$url" "$reference" "$depth" || exit
 			cloned_modules="$cloned_modules;$name"
 			subsha1=
 		else
-- 
2.5.0.256.gb62b30d.dirty

Re: [PATCHv3 0/3] submodule--helper: Have some refactoring only patches first

[Eric Sunshine]

On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller <sbeller@google.com> wrote:
> * Implemented different error messages as suggested by Junio
> * Implemented all of Erics suggestions,
>   including renaming to module-with-dash-now

In the future, please include an interdiff showing changes between
versions. Thanks.

> Stefan Beller (3):
>   submodule: implement `module-list` as a builtin helper
>   submodule: implement `module-name` as a builtin helper
>   submodule: implement `module-clone` as a builtin helper

Re: [PATCHv3 1/3] submodule: implement `module-list` as a builtin helper

[Eric Sunshine]

On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller <sbeller@google.com> wrote:
> 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 keep the
> subdirectory 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 still 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 which to anchor relative path arguments.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> new file mode 100644
> index 0000000..44310f5
> --- /dev/null
> +++ b/builtin/submodule--helper.c
> +static int module_list_compute(int argc, const char **argv,
> +                               const char *prefix,
> +                               struct pathspec *pathspec)
> +{
> +       int i, result = 0;
> +       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");

die(_("..."));

> +       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))
> +               result = -1;
> +
> +       free(ps_matched);
> +
> +       return result;
> +}
> +
> +static int module_list(int argc, const char **argv, const char *prefix)
> +{
> +       int i;
> +       struct pathspec pathspec;
> +       const char *alternative_path;
> +
> +       struct option module_list_options[] = {
> +               OPT_STRING(0, "prefix", &alternative_path,
> +                          N_("path"),
> +                          N_("alternative anchor for relative paths")),
> +               OPT_END()
> +       };
> +
> +       const char *const git_submodule_helper_usage[] = {
> +               N_("git submodule--helper module_list [--prefix=<path>] [<path>...]"),

s/module_list/module-list/

> +               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)
> +               die(N_("fatal: submodule--helper subcommand must be called with "
> +                      "a subcommand, which is module-list\n"));
> +
> +       if (!strcmp(argv[1], "module-list"))

Can we drop the "module-" prefix altogether from these subcommands,
please? Considering that the parent name is already
"submodule--helper", the "module-" prefix is probably pure redundancy.
Instead:

    submodule--helper list
    submodule--helper name
    submodule--helper clone

> +               return module_list(argc - 1, argv + 1, prefix);
> +
> +       die(N_("fatal: '%s' is not a valid submodule--helper subcommand, "
> +              "which is module-list\n"),
> +           argv[1]);
> +}

Re: [PATCHv3 2/3] submodule: implement `module-name` as a builtin helper

[Eric Sunshine]

On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller <sbeller@google.com> wrote:
> 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>
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 44310f5..91bd420 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -102,16 +105,38 @@ static int module_list(int argc, const char **argv, const char *prefix)
> +static int module_name(int argc, const char **argv, const char *prefix)
> +{
> +       const struct submodule *sub;
> +
> +       if (argc != 1)
> +               usage("git submodule--helper module_name <path>\n");

usage(_("..."));

s/module_name/module-name/

I think you also need to drop the newline from the usage() argument.

> +       gitmodules_config();
> +       sub = submodule_from_path(null_sha1, argv[0]);
> +
> +       if (!sub)
> +               die(N_("No submodule mapping found in .gitmodules for path '%s'"),
> +                   argv[0]);
> +
> +       printf("%s\n", sub->name);
> +
> +       return 0;
> +}
> +
>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>  {
>         if (argc < 2)
>                 die(N_("fatal: submodule--helper subcommand must be called with "
> -                      "a subcommand, which is module-list\n"));
> +                      "a subcommand, which are module-list, module-name\n"));

Manually maintaining this list is likely to become a maintenance issue
pretty quickly. Isn't there an OPT_CMDMODE for this sort of thing?

Also, it would like be more readable if the possible commands were
printed one per line rather than all squashed together.

>         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);
> +
>         die(N_("fatal: '%s' is not a valid submodule--helper subcommand, "
> -              "which is module-list\n"),
> +              "which are module-list, module-name\n"),

And, it's duplicated here, making it even more of a maintenance problem.

>             argv[1]);
>  }

Re: [PATCHv3 3/3] submodule: implement `module-clone` as a builtin helper

[Eric Sunshine]

On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller <sbeller@google.com> wrote:
> This converts implements the helper `module_clone`. This functionality is

"converts implements"?

> needed for converting `git submodule update` later on, which we want to
> add threading to.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> +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;
> +       const char *alternative_path = NULL, *p;
> +       int quiet = 0;
> +       FILE *submodule_dot_git;
> +       char *sm_gitdir;
> +       struct strbuf rel_path = STRBUF_INIT;
> +       struct strbuf sb = STRBUF_INIT;
> +
> +       struct option module_update_options[] = {

s/update/clone/ maybe?

> +               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__QUIET(&quiet, "Suppress output for cloning a submodule"),
> +               OPT_END()
> +       };
> +
> +       static const char * const git_submodule_helper_usage[] = {

You can still drop 'static'.

> +               N_("git submodule--helper update [--prefix=<path>] [--quiet] [--remote] [-N|--no-fetch]"

"update"?

> +                  "[-f|--force] [--rebase|--merge] [--reference <repository>]"
> +                  "[--depth <depth>] [--recursive] [--] [<path>...]"),

Also, what's all this "--force", "--rebase", "--remote", "--recursive" stuff?

> +               NULL
> +       };
> +
> +       argc = parse_options(argc, argv, prefix, module_update_options,
> +                            git_submodule_helper_usage, 0);
> +
> +       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);

Still inconsistent checking of return value of
safe_create_leading_directories_const() in this function.

In fact, it looks like pretty much all of my v2 review comments are
still relevant to the remainder of this function. Rather than
repeating them, I'll just give a reference[1].

[1]: http://article.gmane.org/gmane.comp.version-control.git/276952

> +               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(sm_gitdir);
> +       return 0;
> +}

Re: [PATCHv3 2/3] submodule: implement `module-name` as a builtin helper

[Stefan Beller]

On Mon, Aug 31, 2015 at 7:31 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller <sbeller@google.com> wrote:
>> 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>
>> ---
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 44310f5..91bd420 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -102,16 +105,38 @@ static int module_list(int argc, const char **argv, const char *prefix)
>> +static int module_name(int argc, const char **argv, const char *prefix)
>> +{
>> +       const struct submodule *sub;
>> +
>> +       if (argc != 1)
>> +               usage("git submodule--helper module_name <path>\n");
>
> usage(_("..."));
>
> s/module_name/module-name/
>
> I think you also need to drop the newline from the usage() argument.
>
>> +       gitmodules_config();
>> +       sub = submodule_from_path(null_sha1, argv[0]);
>> +
>> +       if (!sub)
>> +               die(N_("No submodule mapping found in .gitmodules for path '%s'"),
>> +                   argv[0]);
>> +
>> +       printf("%s\n", sub->name);
>> +
>> +       return 0;
>> +}
>> +
>>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>>  {
>>         if (argc < 2)
>>                 die(N_("fatal: submodule--helper subcommand must be called with "
>> -                      "a subcommand, which is module-list\n"));
>> +                      "a subcommand, which are module-list, module-name\n"));
>
> Manually maintaining this list is likely to become a maintenance issue
> pretty quickly. Isn't there an OPT_CMDMODE for this sort of thing?

If we were using OPT_CMDMODE, we'd need to have all the options in
one array, passing it to the option parser. I don't think we could have
2 layers of option parsing here. If the first layer only contains the
check for argv[1] (list, name, clone), then it would fail not knowing the
arguments for each of these sub commands.

If we parse all the options together, then we would need to have lots of

 if (cmd == name)
    die ("it makes no sense to give --reference or --depth");

also the variables where the options are passed into would not be
function specific, but file global.

>
> Also, it would like be more readable if the possible commands were
> printed one per line rather than all squashed together.
>
>>         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);
>> +
>>         die(N_("fatal: '%s' is not a valid submodule--helper subcommand, "
>> -              "which is module-list\n"),
>> +              "which are module-list, module-name\n"),
>
> And, it's duplicated here, making it even more of a maintenance problem.
>
>>             argv[1]);
>>  }

Maybe we can do it similar to git.c to have our own array with name
and function pointers,
which can be used both in the help text as well as the actual function call.

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

[Stefan Beller]

took all suggestions except the one below.

>
>     if (strbuf_getcwd(&sb))
>         die_errno(...);
>     strbuf_addf(&sb, "/%s, sm_gitdir);
>     free(sm_gitdir);
>     sm_gitdir = strbuf_detach(&sb, NULL);
>
>> +       }
>> +
>> +       if (strbuf_getcwd(&sb))
>> +               die_errno("unable to get current working directory");
>
> The conditional block just above here also gets 'cwd'. If you move
> this code above the !is_absolute_path(sm_gitdir) conditional block,
> then you can avoid the duplication.

I don't get it. We need to have strbuf_getcwd twice no matter how we
arrange the code
as we strbuf_detach will empty the strbuf.

Do you mean to call strbuf_getcwd just once and then xstrdup the value,
then reset the strbuf to just contain the cwd and append the other string ?

>
>> +       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;
>> +}

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

[Eric Sunshine]

On Tue, Sep 1, 2015 at 1:49 PM, Stefan Beller <sbeller@google.com> wrote:
> took all suggestions except the one below.
>
>>
>>     if (strbuf_getcwd(&sb))
>>         die_errno(...);
>>     strbuf_addf(&sb, "/%s, sm_gitdir);
>>     free(sm_gitdir);
>>     sm_gitdir = strbuf_detach(&sb, NULL);
>>
>>> +       }
>>> +
>>> +       if (strbuf_getcwd(&sb))
>>> +               die_errno("unable to get current working directory");
>>
>> The conditional block just above here also gets 'cwd'. If you move
>> this code above the !is_absolute_path(sm_gitdir) conditional block,
>> then you can avoid the duplication.
>
> I don't get it. We need to have strbuf_getcwd twice no matter how we
> arrange the code
> as we strbuf_detach will empty the strbuf.
>
> Do you mean to call strbuf_getcwd just once and then xstrdup the value,
> then reset the strbuf to just contain the cwd and append the other string ?

Sorry, I have no idea what you are asking.

All I am saying is that you're unnecessarily invoking getcwd() twice.
It's value doesn't change between invocations, so the second call is
entirely redundant and wasteful. You can instead call it just once and
use the result in both places.