[GSoC][PATCH v1 1/2] submodule: port set_name_rev from shell to C

[GSoC][PATCH v1 1/2] submodule: port set_name_rev from shell to C

[Prathamesh Chavan]

Since later on we want to port submodule subcommand status, and since
set_name_rev is part of cmd_status, hence this function is ported. It
has been ported to function set_name_rev in C, which calls get_name_rev
to get the revname, and after formatting it, set_name_prints it. And
hence in this way, the command `git submodule--helper set-name-rev
"sm_path" "sha1"` sets value of revname in git-submodule.sh

The function get_name_rev returns the stdout of the git describe
commands. Since there are four different git-describe commands used for
generating the name rev, four child_process are introduced, each successive
child process running only when previous has no stdout. The order of these
four git-describe commands is maintained the same as it was in the function
set_name_rev() before porting.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---
This series of patch is based on gitster/jk/bug-to-abort for untilizing its
BUG() macro.

Since submodule subcommand used set_name_rev function, first this function
was ported before porting the subcommand to C.

Complete build report for this patch is available at:
https://travis-ci.org/pratham-pc/git/builds/
Branch: status
Build #64

Also, I have updated my Github and pushed this work. It can be seen at:
https://github.com/pratham-pc/git/commits/status

 builtin/submodule--helper.c | 65 +++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 16 ++---------
 2 files changed, 67 insertions(+), 14 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 566a5b6a6..5f0ddd8ad 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -219,6 +219,70 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
 	return 0;
 }
 
+enum describe_step {
+	step_bare = 0,
+	step_tags,
+	step_contains,
+	step_all_always,
+	step_end
+};
+
+static char *get_name_rev(int argc, const char **argv, const char *prefix)
+{
+	struct child_process cp;
+	struct strbuf sb = STRBUF_INIT;
+	enum describe_step cur_step;
+
+	for (cur_step = step_bare; cur_step < step_end; cur_step++) {
+		child_process_init(&cp);
+		prepare_submodule_repo_env(&cp.env_array);
+		cp.dir = argv[1];
+		cp.no_stderr = 1;
+
+		switch (cur_step) {
+			case step_bare:
+				argv_array_pushl(&cp.args, "git", "describe", argv[2], NULL);
+				break;
+			case step_tags:
+				argv_array_pushl(&cp.args, "git", "describe", "--tags",
+						 argv[2], NULL);
+				break;
+			case step_contains:
+				argv_array_pushl(&cp.args, "git", "describe",
+						 "--contains", argv[2], NULL);
+				break;
+			case step_all_always:
+				argv_array_pushl(&cp.args, "git", "describe", "--all",
+						 "--always", argv[2], NULL);
+				break;
+			default:
+				BUG("unknown describe step '%d'", cur_step);
+		}
+
+		if (!capture_command(&cp, &sb, 0) && sb.len) {
+			strbuf_strip_suffix(&sb, "\n");
+			return strbuf_detach(&sb, NULL);
+		}
+	}
+
+	strbuf_release(&sb);
+	return NULL;
+}
+
+
+static int set_name_rev(int argc, const char **argv, const char *prefix)
+{
+	char *namerev;
+	if (argc != 3)
+		die("set-name-rev only accepts two arguments: <path> <sha1>");
+
+	namerev = get_name_rev(argc, argv, prefix);
+	if (namerev[0])
+		printf(" (%s)\n", namerev);
+
+	return 0;
+}
+
 struct module_list {
 	const struct cache_entry **entries;
 	int alloc, nr;
@@ -1212,6 +1276,7 @@ static struct cmd_struct commands[] = {
 	{"relative-path", resolve_relative_path, 0},
 	{"resolve-relative-url", resolve_relative_url, 0},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
+	{"set-name-rev", set_name_rev, 0},
 	{"init", module_init, SUPPORT_SUPER_PREFIX},
 	{"remote-branch", resolve_remote_submodule_branch, 0},
 	{"push-check", push_check, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index c0d0e9a4c..b6eb5bcce 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -758,18 +758,6 @@ cmd_update()
 	}
 }
 
-set_name_rev () {
-	revname=$( (
-		sanitize_submodule_env
-		cd "$1" && {
-			git describe "$2" 2>/dev/null ||
-			git describe --tags "$2" 2>/dev/null ||
-			git describe --contains "$2" 2>/dev/null ||
-			git describe --all --always "$2"
-		}
-	) )
-	test -z "$revname" || revname=" ($revname)"
-}
 #
 # Show commit summary for submodules in index or working tree
 #
@@ -1041,14 +1029,14 @@ cmd_status()
 		fi
 		if git diff-files --ignore-submodules=dirty --quiet -- "$sm_path"
 		then
-			set_name_rev "$sm_path" "$sha1"
+			revname=$(git submodule--helper set-name-rev "$sm_path" "$sha1")
 			say " $sha1 $displaypath$revname"
 		else
 			if test -z "$cached"
 			then
 				sha1=$(sanitize_submodule_env; cd "$sm_path" && git rev-parse --verify HEAD)
 			fi
-			set_name_rev "$sm_path" "$sha1"
+			revname=$(git submodule--helper set-name-rev "$sm_path" "$sha1")
 			say "+$sha1 $displaypath$revname"
 		fi
 
-- 
2.11.0

[GSoC][PATCH v1 2/2] submodule: port submodule subcommand status

[Prathamesh Chavan]

This aims to make git-submodule status a builtin. 'status' is ported
to submodule--helper, and submodule--helper is called from
git-submodule.sh.

For the purpose of porting cmd_status, the code is split up such that
one function obtains all the list of submodules, acting as the
front-end of git-submodule status. This function later calls the
second function for_each_submodule_list,it which basically loops
through the list of submodules and calls function fn, which in this
case is status_submodule. The third function, status submodule returns
the status of submodule and also takes care of the recursive flag.

The first function module_status parses the options present in argv,
and then with the help of module_list_compute, generates the list of
submodules present in the current working tree.

The second function for_each_submodule_list traverses through the list,
and calls function fn (which in the case of submodule subcommand
foreach is status_submodule) is called for each entry.

The third function status_foreach checks for the various conditions,
and prints the status of the submodule accordingly. Also, this
function takes care of the recursive flag by creating a separate
child_process and running it inside the submodule.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---
A new function, get_submodule_displaypath is also introduced for getting
the displaypath of the submodule while taking care of its prefix and
superprefix.

 builtin/submodule--helper.c | 162 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  48 +------------
 2 files changed, 163 insertions(+), 47 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5f0ddd8ad..7c040a375 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -13,6 +13,8 @@
 #include "refs.h"
 #include "connect.h"
 
+typedef void (*submodule_list_func_t)(const struct cache_entry *list_item, void *cb_data);
+
 static char *get_default_remote(void)
 {
 	char *dest = NULL, *ret;
@@ -219,6 +221,23 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
 	return 0;
 }
 
+static char *get_submodule_displaypath(const char *path, const char *prefix)
+{
+	const char *super_prefix = get_super_prefix();
+
+	if (prefix && super_prefix) {
+		BUG("cannot have prefix '%s' and superprefix '%s'",
+		    prefix, super_prefix);
+	} else if (prefix) {
+		struct strbuf sb = STRBUF_INIT;
+		return xstrdup(relative_path(path, prefix, &sb));
+	} else if (super_prefix) {
+		return xstrfmt("%s/%s", super_prefix, path);
+	} else {
+		return xstrdup(path);
+	}
+}
+
 enum describe_step {
 	step_bare = 0,
 	step_tags,
@@ -395,6 +414,13 @@ static int module_list(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static void for_each_submodule_list(const struct module_list list, submodule_list_func_t fn, void *cb_data)
+{
+	int i;
+	for (i = 0; i < list.nr; i++)
+		fn(list.entries[i], cb_data);
+}
+
 static void init_submodule(const char *path, const char *prefix, int quiet)
 {
 	const struct submodule *sub;
@@ -532,6 +558,141 @@ static int module_init(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct cb_status {
+	const char *prefix;
+	unsigned int quiet: 1;
+	unsigned int cached: 1;
+	unsigned int recursive: 1;
+};
+#define CB_STATUS_INIT { NULL, 0, 0, 0 }
+
+static void status_submodule(const struct cache_entry *list_item, void *cb_data)
+{
+	struct cb_status *info = cb_data;
+	struct strbuf sub_sha1 = STRBUF_INIT;
+	char* displaypath = NULL;
+	struct argv_array diff_files_arg = ARGV_ARRAY_INIT;
+
+	argv_array_pushl(&diff_files_arg, "diff-files", "--ignore-submodules=dirty",
+			"--quiet", "--", list_item->name, NULL);
+
+	gitmodules_config();
+
+	displaypath = get_submodule_displaypath(list_item->name, info->prefix);
+
+	if (!submodule_from_path(null_sha1, list_item->name))
+		die(_("no submodule mapping found in .gitmodules for path '%s'"), list_item->name);
+
+	strbuf_addstr(&sub_sha1, oid_to_hex(&list_item->oid));
+
+	if (list_item->ce_flags) {
+		printf(_("U%s %s\n"), sha1_to_hex(null_sha1), displaypath);
+		return;
+	}
+
+	if (!is_submodule_initialized(list_item->name)) {
+		printf(_("-%s %s\n"), sub_sha1.buf, displaypath);
+		return;
+	}
+
+	if (!cmd_diff_files(diff_files_arg.argc, diff_files_arg.argv , info->prefix)) {
+		struct argv_array name_rev_args = ARGV_ARRAY_INIT;
+		argv_array_pushl(&name_rev_args, "set-name-rev", list_item->name,
+				sub_sha1.buf, NULL);
+		printf(_(" %s %s"), sub_sha1.buf, displaypath);
+		set_name_rev(name_rev_args.argc, name_rev_args.argv, info->prefix);
+	} else {
+		struct argv_array name_rev_args = ARGV_ARRAY_INIT;
+
+		if (info->cached == 0) {
+			struct child_process cp = CHILD_PROCESS_INIT;
+			struct strbuf sb = STRBUF_INIT;
+
+			prepare_submodule_repo_env(&cp.env_array);
+			cp.dir = list_item->name;
+
+			argv_array_pushl(&cp.args, "git", "rev-parse", "--verify", "HEAD", NULL);
+
+			if (capture_command(&cp, &sb, 0))
+				die(_("Could not run 'git rev-parse --verify HEAD' in submodule %s"), list_item->name);
+
+			strbuf_strip_suffix(&sb, "\n");
+			argv_array_pushl(&name_rev_args, "set-name-rev", list_item->name,
+					sb.buf, NULL);
+
+			printf(_("+%s %s"), sb.buf, displaypath);
+			set_name_rev(name_rev_args.argc, name_rev_args.argv, info->prefix);
+
+			strbuf_release(&sb);
+		} else {
+
+			argv_array_pushl(&name_rev_args, "set-name-rev", list_item->name,
+					sub_sha1.buf, NULL);
+
+			printf(_("+%s %s"), sub_sha1.buf, displaypath);
+			set_name_rev(name_rev_args.argc, name_rev_args.argv, info->prefix);
+		}
+	}
+
+	if (info->recursive) {
+		struct child_process cpr = CHILD_PROCESS_INIT;
+
+		cpr.git_cmd = 1;
+		cpr.dir = list_item->name;
+		prepare_submodule_repo_env(&cpr.env_array);
+
+		argv_array_pushl(&cpr.args, "--super-prefix", displaypath,
+				 "submodule--helper", "status", "--recursive", NULL);
+
+		if (info->cached)
+			argv_array_push(&cpr.args, "--cached");
+
+		if (info->quiet)
+			argv_array_push(&cpr.args, "--quiet");
+
+		if (run_command(&cpr))
+			die(_("Failed to recurse into submodule path %s"), list_item->name);
+	}
+
+}
+
+static int module_status(int argc, const char **argv, const char *prefix)
+{
+	struct cb_status info = CB_STATUS_INIT;
+	struct pathspec pathspec;
+	struct module_list list = MODULE_LIST_INIT;
+	int quiet = 0;
+	int cached = 0;
+	int recursive = 0;
+
+	struct option module_status_options[] = {
+		OPT__QUIET(&quiet, N_("Suppress output for initializing a submodule")),
+		OPT_BOOL(0, "cached", &cached, N_("Use commit stored in the index instead of the one stored in the submodule HEAD")),
+		OPT_BOOL(0, "recursive", &recursive, N_("Recurse into nested submodules")),
+		OPT_END(),
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule status [--quiet] [--cached] [--recursive] [<path>]"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_status_options,
+			     git_submodule_helper_usage, 0);
+
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
+		return 1;
+
+	info.prefix = prefix;
+	info.quiet = !!quiet;
+	info.cached = !!cached;
+	info.recursive = !!recursive;
+
+	for_each_submodule_list(list, status_submodule, &info);
+
+	return 0;
+}
+
 static int module_name(int argc, const char **argv, const char *prefix)
 {
 	const struct submodule *sub;
@@ -1278,6 +1439,7 @@ static struct cmd_struct commands[] = {
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"set-name-rev", set_name_rev, 0},
 	{"init", module_init, SUPPORT_SUPER_PREFIX},
+	{"status", module_status, SUPPORT_SUPER_PREFIX},
 	{"remote-branch", resolve_remote_submodule_branch, 0},
 	{"push-check", push_check, 0},
 	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
diff --git a/git-submodule.sh b/git-submodule.sh
index b6eb5bcce..a142f9c04 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1004,54 +1004,8 @@ cmd_status()
 		shift
 	done
 
-	{
-		git submodule--helper list --prefix "$wt_prefix" "$@" ||
-		echo "#unmatched" $?
-	} |
-	while read -r mode sha1 stage sm_path
-	do
-		die_if_unmatched "$mode" "$sha1"
-		name=$(git submodule--helper name "$sm_path") || exit
-		displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
-		if test "$stage" = U
-		then
-			say "U$sha1 $displaypath"
-			continue
-		fi
-		if ! git submodule--helper is-active "$sm_path" ||
-		{
-			! test -d "$sm_path"/.git &&
-			! test -f "$sm_path"/.git
-		}
-		then
-			say "-$sha1 $displaypath"
-			continue;
-		fi
-		if git diff-files --ignore-submodules=dirty --quiet -- "$sm_path"
-		then
-			revname=$(git submodule--helper set-name-rev "$sm_path" "$sha1")
-			say " $sha1 $displaypath$revname"
-		else
-			if test -z "$cached"
-			then
-				sha1=$(sanitize_submodule_env; cd "$sm_path" && git rev-parse --verify HEAD)
-			fi
-			revname=$(git submodule--helper set-name-rev "$sm_path" "$sha1")
-			say "+$sha1 $displaypath$revname"
-		fi
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} "$@"
 
-		if test -n "$recursive"
-		then
-			(
-				prefix="$displaypath/"
-				sanitize_submodule_env
-				wt_prefix=
-				cd "$sm_path" &&
-				eval cmd_status
-			) ||
-			die "$(eval_gettext "Failed to recurse into submodule path '\$sm_path'")"
-		fi
-	done
 }
 #
 # Sync remote urls for submodules
-- 
2.11.0

Re: [GSoC][PATCH v1 2/2] submodule: port submodule subcommand status

[Stefan Beller]

On Sun, May 21, 2017 at 5:27 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> This aims to make git-submodule status a builtin. 'status' is ported
> to submodule--helper, and submodule--helper is called from
> git-submodule.sh.
>
> For the purpose of porting cmd_status, the code is split up such that
> one function obtains all the list of submodules, acting as the
> front-end of git-submodule status. This function later calls the
> second function for_each_submodule_list,it which basically loops
> through the list of submodules and calls function fn, which in this
> case is status_submodule. The third function, status submodule returns
> the status of submodule and also takes care of the recursive flag.
>
> The first function module_status parses the options present in argv,
> and then with the help of module_list_compute, generates the list of
> submodules present in the current working tree.
>
> The second function for_each_submodule_list traverses through the list,
> and calls function fn (which in the case of submodule subcommand
> foreach is status_submodule) is called for each entry.
>
> The third function status_foreach checks for the various conditions,
> and prints the status of the submodule accordingly. Also, this
> function takes care of the recursive flag by creating a separate
> child_process and running it inside the submodule.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
> A new function, get_submodule_displaypath is also introduced for getting
> the displaypath of the submodule while taking care of its prefix and
> superprefix.
>
>  builtin/submodule--helper.c | 162 ++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            |  48 +------------
>  2 files changed, 163 insertions(+), 47 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 5f0ddd8ad..7c040a375 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -13,6 +13,8 @@
>  #include "refs.h"
>  #include "connect.h"
>
> +typedef void (*submodule_list_func_t)(const struct cache_entry *list_item, void *cb_data);
> +
>  static char *get_default_remote(void)
>  {
>         char *dest = NULL, *ret;
> @@ -219,6 +221,23 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
>         return 0;
>  }
>
> +static char *get_submodule_displaypath(const char *path, const char *prefix)
> +{
> +       const char *super_prefix = get_super_prefix();
> +
> +       if (prefix && super_prefix) {
> +               BUG("cannot have prefix '%s' and superprefix '%s'",
> +                   prefix, super_prefix);
> +       } else if (prefix) {
> +               struct strbuf sb = STRBUF_INIT;
> +               return xstrdup(relative_path(path, prefix, &sb));
> +       } else if (super_prefix) {
> +               return xstrfmt("%s/%s", super_prefix, path);
> +       } else {
> +               return xstrdup(path);
> +       }
> +}
> +
>  enum describe_step {
>         step_bare = 0,
>         step_tags,
> @@ -395,6 +414,13 @@ static int module_list(int argc, const char **argv, const char *prefix)
>         return 0;
>  }
>
> +static void for_each_submodule_list(const struct module_list list, submodule_list_func_t fn, void *cb_data)
> +{
> +       int i;
> +       for (i = 0; i < list.nr; i++)
> +               fn(list.entries[i], cb_data);
> +}

Up to here it looks like the patch in
https://public-inbox.org/git/20170521125814.26255-2-pc44800@gmail.com/
(without the nit of having an extra void return)

Maybe it is worth it to combine the two patch series, such that we'd need
to review the common parts only once?

> +
>  static void init_submodule(const char *path, const char *prefix, int quiet)
>  {
>         const struct submodule *sub;
> @@ -532,6 +558,141 @@ static int module_init(int argc, const char **argv, const char *prefix)
>         return 0;
>  }
>
> +struct cb_status {
> +       const char *prefix;
> +       unsigned int quiet: 1;
> +       unsigned int cached: 1;
> +       unsigned int recursive: 1;
> +};
> +#define CB_STATUS_INIT { NULL, 0, 0, 0 }



> +
> +               if (run_command(&cpr))
> +                       die(_("Failed to recurse into submodule path %s"), list_item->name);

I thought this is a badly worded error message, but it turns out it
is just as in the shell code, which is good for a direct translation.

Maybe we can adapt the error message in a later follow up to be more
aligned to other submodule error messages. (dropping "path" and putting
single quotes around %s, also un-capitalize the first letter)


> +static int module_status(int argc, const char **argv, const char *prefix)
> +{
> +       struct cb_status info = CB_STATUS_INIT;
> +       struct pathspec pathspec;
> +       struct module_list list = MODULE_LIST_INIT;
> +       int quiet = 0;
> +       int cached = 0;
> +       int recursive = 0;
> +
> +       struct option module_status_options[] = {
> +               OPT__QUIET(&quiet, N_("Suppress output for initializing a submodule")),
> +               OPT_BOOL(0, "cached", &cached, N_("Use commit stored in the index instead of the one stored in the submodule HEAD")),
> +               OPT_BOOL(0, "recursive", &recursive, N_("Recurse into nested submodules")),
> +               OPT_END(),
> +       };
> +
> +       const char *const git_submodule_helper_usage[] = {
> +               N_("git submodule status [--quiet] [--cached] [--recursive] [<path>]"),
> +               NULL
> +       };
> +
> +       argc = parse_options(argc, argv, prefix, module_status_options,
> +                            git_submodule_helper_usage, 0);
> +
> +       if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
> +               return 1;
> +
> +       info.prefix = prefix;
> +       info.quiet = !!quiet;
> +       info.cached = !!cached;
> +       info.recursive = !!recursive;
> +
> +       for_each_submodule_list(list, status_submodule, &info);
> +
> +       return 0;
> +}

This function looks good. Though my gut reaction was to suggest to
add another layer of abstraction. Then I checked wt-status.c, but that
makes use of "submodule summary" and not "submodule status". So all is good.


> +       git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} "$@"

I'd think we would not need to pass down superprefix here as we do not call
"submodule status" in a recursive way. The recursion works on
the submodule helper itself, so we could simplify it to just

    git ${wt_prefix:+-C "$wt_prefix"} submodule--helper status
${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive}
"$@"

Another idea that I just had:
Maybe we could drop --cached, --recursive as well,
as they are just command line options, which could
be just part of "$@".

For --quiet this is a bit more complicated as it may come in
via an environment variable (which we could also check for
in C in theory. I know I omitted that when writing some
submodule--helper code a couple months ago, but the reason
escaped me)

Thanks,
Stefan

[GSoC][PATCH v2 1/2] submodule: port set_name_rev from shell to C

[Prathamesh Chavan]

Since later on we want to port submodule subcommand status, and since
set_name_rev is part of cmd_status, hence this function is ported. It
has been ported to function print_name_rev in C, which calls get_name_rev
to get the revname, and after formatting it, print_name_rev prints it.
And hence in this way, the command `git submodule--helper print-name-rev
"sm_path" "sha1"` sets value of revname in git-submodule.sh

The function get_name_rev returns the stdout of the git describe
commands. Since there are four different git-describe commands used for
generating the name rev, four child_process are introduced, each successive
child process running only when previous has no stdout. The order of these
four git-describe commands is maintained the same as it was in the function
set_name_rev() in shell script.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---
 builtin/submodule--helper.c | 67 +++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 16 ++---------
 2 files changed, 69 insertions(+), 14 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 566a5b6a6..3022118d1 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -219,6 +219,72 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
 	return 0;
 }
 
+enum describe_step {
+	step_bare = 0,
+	step_tags,
+	step_contains,
+	step_all_always,
+	step_end
+};
+
+static char *get_name_rev(int argc, const char **argv, const char *prefix)
+{
+	struct child_process cp;
+	struct strbuf sb = STRBUF_INIT;
+	enum describe_step cur_step;
+
+	for (cur_step = step_bare; cur_step < step_end; cur_step++) {
+		child_process_init(&cp);
+		prepare_submodule_repo_env(&cp.env_array);
+		cp.dir = argv[1];
+		cp.no_stderr = 1;
+
+		switch (cur_step) {
+			case step_bare:
+				argv_array_pushl(&cp.args, "git", "describe",
+						 argv[2], NULL);
+				break;
+			case step_tags:
+				argv_array_pushl(&cp.args, "git", "describe",
+						 "--tags", argv[2], NULL);
+				break;
+			case step_contains:
+				argv_array_pushl(&cp.args, "git", "describe",
+						 "--contains", argv[2], NULL);
+				break;
+			case step_all_always:
+				argv_array_pushl(&cp.args, "git", "describe",
+						 "--all", "--always", argv[2],
+						 NULL);
+				break;
+			default:
+				BUG("unknown describe step '%d'", cur_step);
+		}
+
+		if (!capture_command(&cp, &sb, 0) && sb.len) {
+			strbuf_strip_suffix(&sb, "\n");
+			return strbuf_detach(&sb, NULL);
+		}
+	}
+
+	strbuf_release(&sb);
+	return NULL;
+}
+
+static int print_name_rev(int argc, const char **argv, const char *prefix)
+{
+	char *namerev;
+	if (argc != 3)
+		die("print-name-rev only accepts two arguments: <path> <sha1>");
+
+	namerev = get_name_rev(argc, argv, prefix);
+	if (namerev && namerev[0])
+		printf(" (%s)", namerev);
+	printf("\n");
+
+	return 0;
+}
+
 struct module_list {
 	const struct cache_entry **entries;
 	int alloc, nr;
@@ -1212,6 +1278,7 @@ static struct cmd_struct commands[] = {
 	{"relative-path", resolve_relative_path, 0},
 	{"resolve-relative-url", resolve_relative_url, 0},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
+	{"print-name-rev", print_name_rev, 0},
 	{"init", module_init, SUPPORT_SUPER_PREFIX},
 	{"remote-branch", resolve_remote_submodule_branch, 0},
 	{"push-check", push_check, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index c0d0e9a4c..091051891 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -758,18 +758,6 @@ cmd_update()
 	}
 }
 
-set_name_rev () {
-	revname=$( (
-		sanitize_submodule_env
-		cd "$1" && {
-			git describe "$2" 2>/dev/null ||
-			git describe --tags "$2" 2>/dev/null ||
-			git describe --contains "$2" 2>/dev/null ||
-			git describe --all --always "$2"
-		}
-	) )
-	test -z "$revname" || revname=" ($revname)"
-}
 #
 # Show commit summary for submodules in index or working tree
 #
@@ -1041,14 +1029,14 @@ cmd_status()
 		fi
 		if git diff-files --ignore-submodules=dirty --quiet -- "$sm_path"
 		then
-			set_name_rev "$sm_path" "$sha1"
+			revname=$(git submodule--helper print-name-rev "$sm_path" "$sha1")
 			say " $sha1 $displaypath$revname"
 		else
 			if test -z "$cached"
 			then
 				sha1=$(sanitize_submodule_env; cd "$sm_path" && git rev-parse --verify HEAD)
 			fi
-			set_name_rev "$sm_path" "$sha1"
+			revname=$(git submodule--helper print-name-rev "$sm_path" "$sha1")
 			say "+$sha1 $displaypath$revname"
 		fi
 
-- 
2.13.0

[GSoC][PATCH v2 2/2] submodule: port submodule subcommand status

[Prathamesh Chavan]

This aims to make git-submodule subcommand status a builtin. Here
'status' is ported to submodule--helper, and submodule--helper is
called from git-submodule.sh.

For the purpose of porting cmd_status, the code is split up such that
one function obtains all the list of submodules, acting as the front-end
of git-submodule status. This function later calls the second function
for_each_submodule_list,it which basically loops through the list of
submodules and calls function fn, which in this case is status_submodule.
The third function, status submodule returns the status of submodule and
also takes care of the recursive flag.

The first function module_status parses the options present in argv,
and then with the help of module_list_compute, generates the list of
submodules present in the current working tree.

The second function for_each_submodule_list traverses through the list,
and calls function fn (which in the case of submodule subcommand status
is status_submodule) is called for each entry.

The third function status_submodule checks for the various conditions,
and prints the status of the submodule accordingly. Also, this function
takes care of the recursive flag by creating a separate child_process
and running it inside the submodule. The function print_status handles the
printing of submodule's status.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---
In this new version of patch, function print_status is introduced.

The functions for_each_submodule_list and get_submodule_displaypath
are found to be the same as those in the ported submodule subcommand
foreach's patches. The reason for doing so is to keep both the patches
independant and on separate branches. Also this patch is build on
the branch gitster/jk/bug-to-abort for utilizing its BUG() macro.
 
Complete build report is available at:
https://travis-ci.org/pratham-pc/git/builds/
Branch: submodule-status-new
Build #91

 builtin/submodule--helper.c | 181 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  49 +-----------
 2 files changed, 182 insertions(+), 48 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 3022118d1..85da05550 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -13,6 +13,9 @@
 #include "refs.h"
 #include "connect.h"
 
+typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,
+				      void *cb_data);
+
 static char *get_default_remote(void)
 {
 	char *dest = NULL, *ret;
@@ -219,6 +222,25 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
 	return 0;
 }
 
+static char *get_submodule_displaypath(const char *path, const char *prefix)
+{
+	const char *super_prefix = get_super_prefix();
+
+	if (prefix && super_prefix) {
+		BUG("cannot have prefix '%s' and superprefix '%s'",
+		    prefix, super_prefix);
+	} else if (prefix) {
+		struct strbuf sb = STRBUF_INIT;
+		char *displaypath = xstrdup(relative_path(path, prefix, &sb));
+		strbuf_release(&sb);
+		return displaypath;
+	} else if (super_prefix) {
+		return xstrfmt("%s/%s", super_prefix, path);
+	} else {
+		return xstrdup(path);
+	}
+}
+
 enum describe_step {
 	step_bare = 0,
 	step_tags,
@@ -397,6 +419,13 @@ static int module_list(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static void for_each_submodule_list(const struct module_list list, submodule_list_func_t fn, void *cb_data)
+{
+	int i;
+	for (i = 0; i < list.nr; i++)
+		fn(list.entries[i], cb_data);
+}
+
 static void init_submodule(const char *path, const char *prefix, int quiet)
 {
 	const struct submodule *sub;
@@ -534,6 +563,157 @@ static int module_init(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct status_cb {
+	const char *prefix;
+	unsigned int quiet: 1;
+	unsigned int recursive: 1;
+	unsigned int cached: 1;
+};
+#define STATUS_CB_INIT { NULL, 0, 0, 0 }
+
+static void print_status(struct status_cb *info, char state, const char *path,
+			 char *sub_sha1, char *displaypath)
+{
+	if (info->quiet)
+		return;
+
+	printf("%c%s %s", state, sub_sha1, displaypath);
+
+	if (state == ' ' || state == '+') {
+		struct argv_array name_rev_args = ARGV_ARRAY_INIT;
+
+		argv_array_pushl(&name_rev_args, "print-name-rev",
+				 path, sub_sha1, NULL);
+		print_name_rev(name_rev_args.argc, name_rev_args.argv,
+			       info->prefix);
+	} else {
+		printf("\n");
+	}
+}
+
+static void status_submodule(const struct cache_entry *list_item, void *cb_data)
+{
+	struct status_cb *info = cb_data;
+	char *sub_sha1 = xstrdup(oid_to_hex(&list_item->oid));
+	char *displaypath;
+	struct argv_array diff_files_args = ARGV_ARRAY_INIT;
+
+	if (!submodule_from_path(null_sha1, list_item->name))
+		die(_("no submodule mapping found in .gitmodules for path '%s'"),
+		      list_item->name);
+
+	displaypath = get_submodule_displaypath(list_item->name, info->prefix);
+
+	if (list_item->ce_flags) {
+		print_status(info, 'U', list_item->name,
+			     sha1_to_hex(null_sha1), displaypath);
+		goto cleanup;
+	}
+
+	if (!is_submodule_initialized(list_item->name)) {
+		print_status(info, '-', list_item->name, sub_sha1, displaypath);
+		goto cleanup;
+	}
+
+	argv_array_pushl(&diff_files_args, "diff-files",
+			 "--ignore-submodules=dirty", "--quiet", "--",
+			 list_item->name, NULL);
+
+	if (!cmd_diff_files(diff_files_args.argc, diff_files_args.argv,
+			    info->prefix)) {
+		print_status(info, ' ', list_item->name, sub_sha1, displaypath);
+	} else {
+		if (!info->cached) {
+			struct child_process cp = CHILD_PROCESS_INIT;
+			struct strbuf sb = STRBUF_INIT;
+
+			prepare_submodule_repo_env(&cp.env_array);
+			cp.git_cmd = 1;
+			cp.dir = list_item->name;
+
+			argv_array_pushl(&cp.args, "rev-parse",
+					 "--verify", "HEAD", NULL);
+
+			if (capture_command(&cp, &sb, 0))
+				die(_("could not run 'git rev-parse --verify"
+				      "HEAD' in submodule %s"),
+				      list_item->name);
+
+			strbuf_strip_suffix(&sb, "\n");
+			print_status(info, '+', list_item->name, sb.buf,
+				     displaypath);
+			strbuf_release(&sb);
+		} else {
+			print_status(info, '+', list_item->name, sub_sha1,
+				     displaypath);
+		}
+	}
+
+	if (info->recursive) {
+		struct child_process cpr = CHILD_PROCESS_INIT;
+
+		cpr.git_cmd = 1;
+		cpr.dir = list_item->name;
+		prepare_submodule_repo_env(&cpr.env_array);
+
+		argv_array_pushl(&cpr.args, "--super-prefix", displaypath,
+				 "submodule--helper", "status", "--recursive",
+				 NULL);
+
+		if (info->cached)
+			argv_array_push(&cpr.args, "--cached");
+
+		if (info->quiet)
+			argv_array_push(&cpr.args, "--quiet");
+
+		if (run_command(&cpr))
+			die(_("failed to recurse into submodule '%s'"),
+			      list_item->name);
+	}
+
+cleanup:
+	free(displaypath);
+	free(sub_sha1);
+}
+
+static int module_status(int argc, const char **argv, const char *prefix)
+{
+	struct status_cb info = STATUS_CB_INIT;
+	struct pathspec pathspec;
+	struct module_list list = MODULE_LIST_INIT;
+	int quiet = 0;
+	int cached = 0;
+	int recursive = 0;
+
+	struct option module_status_options[] = {
+		OPT__QUIET(&quiet, N_("Suppress submodule status output")),
+		OPT_BOOL(0, "cached", &cached, N_("Use commit stored in the index instead of the one stored in the submodule HEAD")),
+		OPT_BOOL(0, "recursive", &recursive, N_("Recurse into nested submodules")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule status [--quiet] [--cached] [--recursive] [<path>]"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_status_options,
+			     git_submodule_helper_usage, 0);
+
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
+		return 1;
+
+	info.prefix = prefix;
+	info.quiet = !!quiet;
+	info.recursive = !!recursive;
+	info.cached = !!cached;
+
+	gitmodules_config();
+	for_each_submodule_list(list, status_submodule, &info);
+
+	return 0;
+}
+
 static int module_name(int argc, const char **argv, const char *prefix)
 {
 	const struct submodule *sub;
@@ -1280,6 +1460,7 @@ static struct cmd_struct commands[] = {
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"print-name-rev", print_name_rev, 0},
 	{"init", module_init, SUPPORT_SUPER_PREFIX},
+	{"status", module_status, SUPPORT_SUPER_PREFIX},
 	{"remote-branch", resolve_remote_submodule_branch, 0},
 	{"push-check", push_check, 0},
 	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
diff --git a/git-submodule.sh b/git-submodule.sh
index 091051891..a24b1b91b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1004,54 +1004,7 @@ cmd_status()
 		shift
 	done
 
-	{
-		git submodule--helper list --prefix "$wt_prefix" "$@" ||
-		echo "#unmatched" $?
-	} |
-	while read -r mode sha1 stage sm_path
-	do
-		die_if_unmatched "$mode" "$sha1"
-		name=$(git submodule--helper name "$sm_path") || exit
-		displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
-		if test "$stage" = U
-		then
-			say "U$sha1 $displaypath"
-			continue
-		fi
-		if ! git submodule--helper is-active "$sm_path" ||
-		{
-			! test -d "$sm_path"/.git &&
-			! test -f "$sm_path"/.git
-		}
-		then
-			say "-$sha1 $displaypath"
-			continue;
-		fi
-		if git diff-files --ignore-submodules=dirty --quiet -- "$sm_path"
-		then
-			revname=$(git submodule--helper print-name-rev "$sm_path" "$sha1")
-			say " $sha1 $displaypath$revname"
-		else
-			if test -z "$cached"
-			then
-				sha1=$(sanitize_submodule_env; cd "$sm_path" && git rev-parse --verify HEAD)
-			fi
-			revname=$(git submodule--helper print-name-rev "$sm_path" "$sha1")
-			say "+$sha1 $displaypath$revname"
-		fi
-
-		if test -n "$recursive"
-		then
-			(
-				prefix="$displaypath/"
-				sanitize_submodule_env
-				wt_prefix=
-				cd "$sm_path" &&
-				eval cmd_status
-			) ||
-			die "$(eval_gettext "Failed to recurse into submodule path '\$sm_path'")"
-		fi
-	done
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} "$@"
 }
 #
 # Sync remote urls for submodules
-- 
2.13.0

Re: [GSoC][PATCH v2 2/2] submodule: port submodule subcommand status

[Stefan Beller]

On Mon, Jun 5, 2017 at 1:25 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> This aims to make git-submodule subcommand status a builtin. Here
> 'status' is ported to submodule--helper, and submodule--helper is
> called from git-submodule.sh.
>
> For the purpose of porting cmd_status, the code is split up such that
> one function obtains all the list of submodules, acting as the front-end
> of git-submodule status. This function later calls the second function
> for_each_submodule_list,it which basically loops through the list of
> submodules and calls function fn, which in this case is status_submodule.
> The third function, status submodule returns the status of submodule and
> also takes care of the recursive flag.
>
> The first function module_status parses the options present in argv,
> and then with the help of module_list_compute, generates the list of
> submodules present in the current working tree.
>
> The second function for_each_submodule_list traverses through the list,
> and calls function fn (which in the case of submodule subcommand status
> is status_submodule) is called for each entry.
>
> The third function status_submodule checks for the various conditions,
> and prints the status of the submodule accordingly. Also, this function
> takes care of the recursive flag by creating a separate child_process
> and running it inside the submodule. The function print_status handles the
> printing of submodule's status.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
> In this new version of patch, function print_status is introduced.
>
> The functions for_each_submodule_list and get_submodule_displaypath
> are found to be the same as those in the ported submodule subcommand
> foreach's patches. The reason for doing so is to keep both the patches
> independant and on separate branches.


Maybe keep it even in a separate patch, such that
the status series becomes:
  patch 1: introduce for_each_submodule_list and get_submodule_displaypath
  patch 2: port print_name_rev
  patch 3: port status

whereas the foreach series (and other series later) could
re-use patch 1, and build on top of it.

For reviewing patches, it is fine to have the
get_submodule_displaypath is both series, though for applying
patches it for less complication/deduplication from the maintainer
I would think.

> +
> +static void print_status(struct status_cb *info, char state, const char *path,
> +                        char *sub_sha1, char *displaypath)
> +{
> +       if (info->quiet)
> +               return;
> +
> +       printf("%c%s %s", state, sub_sha1, displaypath);
> +
> +       if (state == ' ' || state == '+') {
> +               struct argv_array name_rev_args = ARGV_ARRAY_INIT;
> +
> +               argv_array_pushl(&name_rev_args, "print-name-rev",
> +                                path, sub_sha1, NULL);
> +               print_name_rev(name_rev_args.argc, name_rev_args.argv,
> +                              info->prefix);

... with the suggestion given in the print_name_rev patch, this would
become a one liner. :)


The rest looks good to me :)

Re: [GSoC][PATCH v2 2/2] submodule: port submodule subcommand status

[Christian Couder]

On Mon, Jun 5, 2017 at 10:25 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:

> ---
> In this new version of patch, function print_status is introduced.
>
> The functions for_each_submodule_list and get_submodule_displaypath
> are found to be the same as those in the ported submodule subcommand
> foreach's patches. The reason for doing so is to keep both the patches
> independant and on separate branches. Also this patch is build on
> the branch gitster/jk/bug-to-abort for utilizing its BUG() macro.

The BUG() macro is now in master.

Re: [GSoC][PATCH v2 1/2] submodule: port set_name_rev from shell to C

[Stefan Beller]

On Mon, Jun 5, 2017 at 1:25 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> Since later on we want to port submodule subcommand status, and since
> set_name_rev is part of cmd_status, hence this function is ported. It
> has been ported to function print_name_rev in C, which calls get_name_rev
> to get the revname, and after formatting it, print_name_rev prints it.
> And hence in this way, the command `git submodule--helper print-name-rev
> "sm_path" "sha1"` sets value of revname in git-submodule.sh
>
> The function get_name_rev returns the stdout of the git describe
> commands. Since there are four different git-describe commands used for
> generating the name rev, four child_process are introduced, each successive
> child process running only when previous has no stdout. The order of these
> four git-describe commands is maintained the same as it was in the function
> set_name_rev() in shell script.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
>  builtin/submodule--helper.c | 67 +++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            | 16 ++---------
>  2 files changed, 69 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 566a5b6a6..3022118d1 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -219,6 +219,72 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
>         return 0;
>  }
>
> +enum describe_step {
> +       step_bare = 0,

Do we rely on step_bare to be equal to 0?
(This is the hint I am reading from '=0' here.
If we do not, please omit.)

> +       step_tags,
> +       step_contains,
> +       step_all_always,
> +       step_end
> +};
> +
> +static char *get_name_rev(int argc, const char **argv, const char *prefix)

So we split up the functionality into two functions.
get_name_rev, which does the heavy lifting work, and
print_name_rev, that is a wrapper around having to deal with
going from shell to C and back.

One of C strength' compared to shell is type safety,
so maybe we can tighten the contract that get_name_rev
offers to its callers and make it

  get_name_rev(const char *sub_path, const char *object_id / sha1)

and then have print_name_rev call it via

  get_name_rev (argv[1], argv[2])

(which coincidentally is right after checking for
argc != 3, which reinforces that the contract of the
wrapper is "just making sure we have valid input" and
this function "just does heavy lifting, assuming input
is valid".

> +{
> +       struct child_process cp;
> +       struct strbuf sb = STRBUF_INIT;
> +       enum describe_step cur_step;
> +
> +       for (cur_step = step_bare; cur_step < step_end; cur_step++) {
> +               child_process_init(&cp);

(minor nit, personal opinion, feel free to ignore:)
Alternatively, you could declare cp inside the loop assigned to
CHILD_PROCESS_INIT.
Same for strbuf sb as well, such that you only declare the iterator
variable outside the loop.

> +               prepare_submodule_repo_env(&cp.env_array);
> +               cp.dir = argv[1];
> +               cp.no_stderr = 1;

cp.git_cmd = 1; as well?

Thanks,
Stefan

Re: [GSoC][PATCH v2 1/2] submodule: port set_name_rev from shell to C

[Brandon Williams]

On 06/06, Prathamesh Chavan wrote:
> Since later on we want to port submodule subcommand status, and since
> set_name_rev is part of cmd_status, hence this function is ported. It
> has been ported to function print_name_rev in C, which calls get_name_rev
> to get the revname, and after formatting it, print_name_rev prints it.
> And hence in this way, the command `git submodule--helper print-name-rev
> "sm_path" "sha1"` sets value of revname in git-submodule.sh
> 
> The function get_name_rev returns the stdout of the git describe
> commands. Since there are four different git-describe commands used for
> generating the name rev, four child_process are introduced, each successive
> child process running only when previous has no stdout. The order of these
> four git-describe commands is maintained the same as it was in the function
> set_name_rev() in shell script.
> 
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
>  builtin/submodule--helper.c | 67 +++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            | 16 ++---------
>  2 files changed, 69 insertions(+), 14 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 566a5b6a6..3022118d1 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -219,6 +219,72 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
>  	return 0;
>  }
>  
> +enum describe_step {
> +	step_bare = 0,
> +	step_tags,
> +	step_contains,
> +	step_all_always,
> +	step_end
> +};
> +
> +static char *get_name_rev(int argc, const char **argv, const char *prefix)
> +{
> +	struct child_process cp;
> +	struct strbuf sb = STRBUF_INIT;
> +	enum describe_step cur_step;
> +
> +	for (cur_step = step_bare; cur_step < step_end; cur_step++) {
> +		child_process_init(&cp);
> +		prepare_submodule_repo_env(&cp.env_array);
> +		cp.dir = argv[1];
> +		cp.no_stderr = 1;

set cp.git = 1 so that you can avoid pushing "git" onto the arg array.

> +
> +		switch (cur_step) {
> +			case step_bare:
> +				argv_array_pushl(&cp.args, "git", "describe",
> +						 argv[2], NULL);
> +				break;
> +			case step_tags:
> +				argv_array_pushl(&cp.args, "git", "describe",
> +						 "--tags", argv[2], NULL);
> +				break;
> +			case step_contains:
> +				argv_array_pushl(&cp.args, "git", "describe",
> +						 "--contains", argv[2], NULL);
> +				break;
> +			case step_all_always:
> +				argv_array_pushl(&cp.args, "git", "describe",
> +						 "--all", "--always", argv[2],
> +						 NULL);
> +				break;
> +			default:
> +				BUG("unknown describe step '%d'", cur_step);
> +		}
> +
> +		if (!capture_command(&cp, &sb, 0) && sb.len) {
> +			strbuf_strip_suffix(&sb, "\n");
> +			return strbuf_detach(&sb, NULL);
> +		}
> +	}
> +
> +	strbuf_release(&sb);
> +	return NULL;
> +}
> +
> +static int print_name_rev(int argc, const char **argv, const char *prefix)
> +{
> +	char *namerev;
> +	if (argc != 3)
> +		die("print-name-rev only accepts two arguments: <path> <sha1>");
> +
> +	namerev = get_name_rev(argc, argv, prefix);
> +	if (namerev && namerev[0])
> +		printf(" (%s)", namerev);
> +	printf("\n");
> +
> +	return 0;
> +}
> +
>  struct module_list {
>  	const struct cache_entry **entries;
>  	int alloc, nr;
> @@ -1212,6 +1278,7 @@ static struct cmd_struct commands[] = {
>  	{"relative-path", resolve_relative_path, 0},
>  	{"resolve-relative-url", resolve_relative_url, 0},
>  	{"resolve-relative-url-test", resolve_relative_url_test, 0},
> +	{"print-name-rev", print_name_rev, 0},
>  	{"init", module_init, SUPPORT_SUPER_PREFIX},
>  	{"remote-branch", resolve_remote_submodule_branch, 0},
>  	{"push-check", push_check, 0},
> diff --git a/git-submodule.sh b/git-submodule.sh
> index c0d0e9a4c..091051891 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -758,18 +758,6 @@ cmd_update()
>  	}
>  }
>  
> -set_name_rev () {
> -	revname=$( (
> -		sanitize_submodule_env
> -		cd "$1" && {
> -			git describe "$2" 2>/dev/null ||
> -			git describe --tags "$2" 2>/dev/null ||
> -			git describe --contains "$2" 2>/dev/null ||
> -			git describe --all --always "$2"
> -		}
> -	) )
> -	test -z "$revname" || revname=" ($revname)"
> -}
>  #
>  # Show commit summary for submodules in index or working tree
>  #
> @@ -1041,14 +1029,14 @@ cmd_status()
>  		fi
>  		if git diff-files --ignore-submodules=dirty --quiet -- "$sm_path"
>  		then
> -			set_name_rev "$sm_path" "$sha1"
> +			revname=$(git submodule--helper print-name-rev "$sm_path" "$sha1")
>  			say " $sha1 $displaypath$revname"
>  		else
>  			if test -z "$cached"
>  			then
>  				sha1=$(sanitize_submodule_env; cd "$sm_path" && git rev-parse --verify HEAD)
>  			fi
> -			set_name_rev "$sm_path" "$sha1"
> +			revname=$(git submodule--helper print-name-rev "$sm_path" "$sha1")
>  			say "+$sha1 $displaypath$revname"
>  		fi
>  
> -- 
> 2.13.0
> 

-- 
Brandon Williams