[GSoC][RFC/PATCH] submodule: port subcommand foreach from shell to C

[GSoC][RFC/PATCH] submodule: port subcommand foreach from shell to C

[Prathamesh Chavan]

This aims to make git-submodule foreach a builtin. This is the very
first step taken in this direction. Hence, 'foreach' is ported to
submodule--helper, and submodule--helper is called from git-submodule.sh.
The code is split up to have one function to obtain all the list of
submodules and a calling function that takes care of running the command
in that submodule, and recursively perform the same when --recursive is
flagged.

The First function module_foreach first parses the options present in
argv, and then with the help of read_cache, generates the list of
submodules present in the current working tree. Traversing through the
list, foreach_submodule function is called for each entry.

The second function foreach_submodule, generates a submodule struct sub
for $name, $path values and then later prepends name=sub->name;
path=sub-> path; and other value assignment to an argv_array structure.
Also the <command> of submodule-foreach is appended to this structure
and finally, using run_command_v_opt the commands are executed in a
single but separate shell.

The second function also takes care of the recursive flag, by creating
a saperate argv_array structure and prepending "--super-prefix displaypath",
and other required arguments to the structure and then appending the
input <command> of submodule-foreach to the argument's array.


Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---

The build report of this patch is available at: 
https://travis-ci.org/pratham-pc/git/builds/223573936

Clearly, there are still some tests which are failing. I have
submitted this as RFC patch for getting suggestions on debugging these
errors and for reviewing the approach taken for porting submodule
'foreach' subcommand to C.

Also, I have based my branch on gitster/jk/ls-files-recurse-submodules-fix,
since while using --super-prefix in recursively calling the foreach
command, it produced results indicating that a --super-prefix can't
be used from a subdirectory:

  fatal: can't use --super-prefix from a subdirectory

The patch and the discussion related to it can be found at: 
https://public-inbox.org/git/20170412003911.1142-1-jacob.e.keller@intel.com/T/#u

Also, I would like to ask is there are any more changes required in my microproject for getting it merged.
https://public-inbox.org/git/20170403213557.27724-1-pc44800@gmail.com/


 builtin/submodule--helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  40 +-----------
 2 files changed, 154 insertions(+), 39 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 85aafe46a..276ed6025 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -487,6 +487,158 @@ static int module_name(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static void foreach_submodule(int argc, const char **argv, const char *path,
+			      const unsigned char *sha1, const char *prefix,
+			      int quiet, int recursive)
+{
+	const char *toplevel = xgetcwd();
+	const struct submodule *sub;
+	struct strbuf sb = STRBUF_INIT;
+	struct strbuf sub_sha1 = STRBUF_INIT;
+	struct strbuf cmd = STRBUF_INIT;
+	char *displaypath;
+	int i;
+
+	/* Only loads from .gitmodules, no overlay with .git/config */
+	gitmodules_config();
+
+	if (prefix && get_super_prefix()) {
+		die("BUG: cannot have prefix and superprefix");
+	} else if (prefix) {
+		displaypath = xstrdup(relative_path(prefix, path,  &sb));
+	} else if (get_super_prefix()) {
+		strbuf_addf(&sb, "%s/%s", get_super_prefix(), path);
+		displaypath = strbuf_detach(&sb, NULL);
+	} else {
+		displaypath = xstrdup(path);
+	}
+
+	sub = submodule_from_path(null_sha1, path);
+
+	if (!sub)
+		die(_("No url found for submodule path '%s' in .gitmodules"),
+		      displaypath);
+	strbuf_add_unique_abbrev(&sub_sha1, sha1 , 40);
+
+	if (!chdir(path)) {
+		if (!access_or_warn(".git", R_OK, 0)) {
+			if (!quiet)
+				printf(_("Entering '%s'\n"), displaypath);
+
+			if (argc == 1) {
+				struct argv_array argcp1 = ARGV_ARRAY_INIT;
+
+				strbuf_addstr(&cmd, "name=");
+				strbuf_addstr(&cmd, sub->name);
+				strbuf_addstr(&cmd, "; ");
+				strbuf_addstr(&cmd, "toplevel=");
+				strbuf_addstr(&cmd, toplevel);
+				strbuf_addstr(&cmd, "; ");
+				strbuf_addstr(&cmd, "sha1=");
+				strbuf_addstr(&cmd, sub_sha1.buf);
+				strbuf_addstr(&cmd, "; ");
+				strbuf_addstr(&cmd, "path=");
+				strbuf_addstr(&cmd, sub->path);
+				strbuf_addstr(&cmd, "; ");
+				strbuf_addstr(&cmd, argv[0]);
+
+				argv_array_push(&argcp1, cmd.buf);
+				run_command_v_opt(argcp1.argv, RUN_USING_SHELL);
+			} else {
+				run_command_v_opt(argv, RUN_USING_SHELL);
+			}
+
+			if (recursive) {
+				struct argv_array argcp = ARGV_ARRAY_INIT;
+
+				argv_array_push(&argcp, "git");
+				argv_array_push(&argcp, "--super-prefix");
+				argv_array_push(&argcp, displaypath);
+				argv_array_push(&argcp, "submodule--helper");
+
+				if (quiet)
+					argv_array_push(&argcp, "--quiet");
+				argv_array_push(&argcp, "foreach");
+				argv_array_push(&argcp, "--recursive");
+
+				for (i = 0; i < argc; i++)
+					argv_array_push(&argcp, argv[i]);
+
+				run_command_v_opt(argcp.argv, RUN_USING_SHELL);
+			}
+
+			if (chdir(toplevel))
+				die_errno(_("cannot chdir to %s"), toplevel);
+		}
+	} else {
+		die_errno(_("cannot chdir to %s"), path);
+	}
+
+	strbuf_release(&cmd);
+	strbuf_release(&sub_sha1);
+	strbuf_release(&sb);
+	free(displaypath);
+	return;
+}
+
+static int module_foreach(int argc, const char **argv, const char *prefix)
+{
+	struct module_list list = MODULE_LIST_INIT;
+	int quiet = 0;
+	int recursive = 0;
+	int i;
+
+	struct option module_foreach_options[] = {
+		OPT__QUIET(&quiet, N_("Suppress output of Entering each submodule command")),
+		OPT_BOOL(0, "recursive", &recursive,
+			 N_("Traverse submodules ercursively and apply the command for all nested submodules")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper foreach [--recursive] <command>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_foreach_options,
+			     git_submodule_helper_usage, 0);
+
+	if (read_cache() < 0)
+		die(_("index file corrupt"));
+
+	for (i = 0; i < active_nr; i++) {
+		const struct cache_entry *ce = active_cache[i];
+
+		if (!S_ISGITLINK(ce->ce_mode))
+				continue;
+
+		ALLOC_GROW(list.entries, list.nr + 1, list.alloc);
+		list.entries[list.nr++] = 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++;
+	}
+
+	for (i = 0; i < list.nr; i++) {
+		if (prefix) {
+			const char *out = NULL;
+			if (skip_prefix(prefix, list.entries[i]->name, &out)) {
+				if (out && out[0] == '/' && !out + 1) 
+					return 0;
+			}
+		}
+
+		foreach_submodule(argc, argv, list.entries[i]->name,
+				  list.entries[i]->oid.hash, prefix,
+				  quiet, recursive);
+	}
+	return 0;
+}
+
 static int clone_submodule(const char *path, const char *gitdir, const char *url,
 			   const char *depth, struct string_list *reference,
 			   int quiet, int progress)
@@ -1168,6 +1320,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},
+	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
 	{"init", module_init, SUPPORT_SUPER_PREFIX},
 	{"remote-branch", resolve_remote_submodule_branch, 0},
 	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
diff --git a/git-submodule.sh b/git-submodule.sh
index 6ec35e5fc..e2c2b40f4 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -321,46 +321,8 @@ cmd_foreach()
 		esac
 		shift
 	done
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
 
-	toplevel=$(pwd)
-
-	# dup stdin so that it can be restored when running the external
-	# command in the subshell (and a recursive call to this function)
-	exec 3<&0
-
-	{
-		git submodule--helper list --prefix "$wt_prefix" ||
-		echo "#unmatched" $?
-	} |
-	while read mode sha1 stage sm_path
-	do
-		die_if_unmatched "$mode" "$sha1"
-		if test -e "$sm_path"/.git
-		then
-			displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
-			say "$(eval_gettext "Entering '\$displaypath'")"
-			name=$(git submodule--helper name "$sm_path")
-			(
-				prefix="$prefix$sm_path/"
-				sanitize_submodule_env
-				cd "$sm_path" &&
-				sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
-				# we make $path available to scripts ...
-				path=$sm_path &&
-				if test $# -eq 1
-				then
-					eval "$1"
-				else
-					"$@"
-				fi &&
-				if test -n "$recursive"
-				then
-					cmd_foreach "--recursive" "$@"
-				fi
-			) <&3 3<&- ||
-			die "$(eval_gettext "Stopping at '\$displaypath'; script returned non-zero status.")"
-		fi
-	done
 }
 
 #
-- 
2.11.0

Re: [GSoC][RFC/PATCH] submodule: port subcommand foreach from shell to C

[Stefan Beller]

On Wed, Apr 19, 2017 at 10:05 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> This aims to make git-submodule foreach a builtin. This is the very
> first step taken in this direction. Hence, 'foreach' is ported to
> submodule--helper, and submodule--helper is called from git-submodule.sh.

cool :)


> The code is split up to have one function to obtain all the list of
> submodules and a calling function that takes care of running the command
> in that submodule, and recursively perform the same when --recursive is
> flagged.
>
> The First function module_foreach first parses the options present in
> argv, and then with the help of read_cache, generates the list of
> submodules present in the current working tree. Traversing through the
> list, foreach_submodule function is called for each entry.

I wonder if we could re-use module_list here?

> The second function foreach_submodule, generates a submodule struct sub
> for $name, $path values and then later prepends name=sub->name;
> path=sub-> path; and other value assignment to an argv_array structure.
> Also the <command> of submodule-foreach is appended to this structure
> and finally, using run_command_v_opt the commands are executed in a
> single but separate shell.

As noted below, I would use a struct child_process as that seems to make life
easier here.


When applying the patch git-am says:
Applying: submodule: port subcommand foreach from shell to C
.git/rebase-apply/patch:177: trailing whitespace.
                           if (out && out[0] == '/' && !out + 1)
warning: 1 line adds whitespace errors.

>
>  builtin/submodule--helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            |  40 +-----------

cool. :)

> +
> +       /* Only loads from .gitmodules, no overlay with .git/config */

Why would we not overlay the .gitmodules config with .git/config?

> +       gitmodules_config();
> +
> +       if (prefix && get_super_prefix()) {
> +               die("BUG: cannot have prefix and superprefix");
> +       } else if (prefix) {
> +               displaypath = xstrdup(relative_path(prefix, path,  &sb));
> +       } else if (get_super_prefix()) {
> +               strbuf_addf(&sb, "%s/%s", get_super_prefix(), path);
> +               displaypath = strbuf_detach(&sb, NULL);
> +       } else {
> +               displaypath = xstrdup(path);
> +       }
> +
> +       sub = submodule_from_path(null_sha1, path);
> +
> +       if (!sub)
> +               die(_("No url found for submodule path '%s' in .gitmodules"),
> +                     displaypath);
> +       strbuf_add_unique_abbrev(&sub_sha1, sha1 , 40);
> +

> +
> +                       if (argc == 1) {
> +                               struct argv_array argcp1 = ARGV_ARRAY_INIT;

Oh the case of argc=1 is interesting. 1c4fb136db (submodule foreach:
skip eval for more than one argument, 2013-09-27) explains why.



> +
> +                               strbuf_addstr(&cmd, "name=");
> +                               strbuf_addstr(&cmd, sub->name);
> +                               strbuf_addstr(&cmd, "; ");
> +                               strbuf_addstr(&cmd, "toplevel=");
> +                               strbuf_addstr(&cmd, toplevel);
> +                               strbuf_addstr(&cmd, "; ");
> +                               strbuf_addstr(&cmd, "sha1=");
> +                               strbuf_addstr(&cmd, sub_sha1.buf);
> +                               strbuf_addstr(&cmd, "; ");
> +                               strbuf_addstr(&cmd, "path=");
> +                               strbuf_addstr(&cmd, sub->path);
> +                               strbuf_addstr(&cmd, "; ");
> +                               strbuf_addstr(&cmd, argv[0]);

Instead of prefixing the command with these variables, we can set them
as environment variables; Then we do not have to add semicolons ourselves as the
environment variable infrastructure does that for us.

    struct child_process cp = CHILD_PROCESS_INIT;
    argv_array_pushf(&cp.env_array, "name=%s", sub->name);
    argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
    ...

> +
> +                               argv_array_push(&argcp1, cmd.buf);
> +                               run_command_v_opt(argcp1.argv, RUN_USING_SHELL);

Oh, you use run_command_v_opt, which is a wrapper around the struct
child_process.
I would suggest to use the struct child_process directly as then we
can set the environment
ourselves in an easier way. To set this flag we'd do

    cp.use_shell = 1;

> +       if (!chdir(path)) {
> +               if (!access_or_warn(".git", R_OK, 0)) {

The same applies to changing directories. Here in this code we chdir
(path) and later
chdir (toplevel), but this process doesn't need to change its
directories but it can stay at
the toplevel directory. Only the child process needs chdir to the
correct path, which can
be done via

    cp.dir = path;


> +                       } else {
> +                               run_command_v_opt(argv, RUN_USING_SHELL);
> +                       }
> +
> +                       if (recursive) {
> +                               struct argv_array argcp = ARGV_ARRAY_INIT;
> +
> +                               argv_array_push(&argcp, "git");
> +                               argv_array_push(&argcp, "--super-prefix");
> +                               argv_array_push(&argcp, displaypath);
> +                               argv_array_push(&argcp, "submodule--helper");

Good call, the recursing still needs to create a new child process of its own
instead of just calling the function recursively, as we do not have
access to the
nested submodule data here.

> +
> +                               if (quiet)
> +                                       argv_array_push(&argcp, "--quiet");
> +                               argv_array_push(&argcp, "foreach");
> +                               argv_array_push(&argcp, "--recursive");
> +
> +                               for (i = 0; i < argc; i++)
> +                                       argv_array_push(&argcp, argv[i]);
> +
> +                               run_command_v_opt(argcp.argv, RUN_USING_SHELL);

I'd also suggest to use the struct child_process directly here instead
of a wrapper
as then we have access to all the knobs ourselves.

> +
> +       struct option module_foreach_options[] = {
> +               OPT__QUIET(&quiet, N_("Suppress output of Entering each submodule command")),
> +               OPT_BOOL(0, "recursive", &recursive,
> +                        N_("Traverse submodules ercursively and apply the command for all nested submodules")),

s/ercursively/recursively/
Also wording: Do you apply a command or do you run a command? Maybe in
a shorter version:
   N_("recurse into nested submodules")


> +       const char *const git_submodule_helper_usage[] = {
> +               N_("git submodule--helper foreach [--recursive] <command>"),

and [--quiet] as well


> +       for (i = 0; i < active_nr; i++) {
> +               const struct cache_entry *ce = active_cache[i];
> +
> +               if (!S_ISGITLINK(ce->ce_mode))
> +                               continue;
> +
> +               ALLOC_GROW(list.entries, list.nr + 1, list.alloc);
> +               list.entries[list.nr++] = 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++;
> +       }
> +
> +       for (i = 0; i < list.nr; i++) {
> +               if (prefix) {
> +                       const char *out = NULL;
> +                       if (skip_prefix(prefix, list.entries[i]->name, &out)) {
> +                               if (out && out[0] == '/' && !out + 1)
> +                                       return 0;
> +                       }
> +               }

The lines up to here are the functional equivalent of
    git submodule--helper list --prefix "$wt_prefix"

Would it make sense to get the list via calling

    int argc = 0;
    char *argv = {NULL}; /* it has to be null terminated, I think */

    struct pathspec pathspec;
    /* no need to init this, as module_list_compute will do that via
      parse_pathspec */

    struct module_list list = MODULE_LIST_INIT

    if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
       die("BUG: module_list_compute should not choke on empty pathspec");

    for (i = 0; i < list.nr; i++)
        foreach_submodule(...);

>  static int clone_submodule(const char *path, const char *gitdir, const char *url,
>                            const char *depth, struct string_list *reference,
>                            int quiet, int progress)
> @@ -1168,6 +1320,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},
> +       {"foreach", module_foreach, SUPPORT_SUPER_PREFIX},

Having SUPPORT_SUPER_PREFIX makes sense as we want to print out
path from outside the repo for example.


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

I think we'd want to drop the "quotes" around the last $@ such that the
arguments are not passed in as one long string, but one by one.
(Then the submodule--helper code can differentiate between the
number of arguments, just as the shell code does below)

Thanks,
Stefan

[GSoC][RFC/PATCH v2] submodule: port subcommand foreach from shell to C

[Prathamesh Chavan]

This aims to make git-submodule foreach a builtin. This is the very
first step taken in this direction. Hence, 'foreach' is ported to
submodule--helper, and submodule--helper is called from git-submodule.sh.
The code is split up to have one function to obtain all the list of
submodules and a calling function that takes care of running the command
in that submodule, and recursively perform the same when --recursive is
flagged.

The First function module_foreach first 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. Traversing through the
list, foreach_submodule function is called for each entry.

The second function foreach_submodule, generates a submodule struct sub
for $name, value and then later prepends name=sub->name; and other
value assignment to the env argv_array structure of a child_process.
Also the <command> of submodule-foreach is push to args argv_array
structure and finally, using run_command the commands are executed
using a shell.

The second function also takes care of the recursive flag, by creating
a saperate child_process structure and prepending "--super-prefix displaypath",
to the args argv_array structure. Other required arguments and the
input <command> of submodule-foreach is also appended to this argv_array.

Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---

In this new version of patch following changes have been added:

module_list_compute is used to generate the list of submodules present
in the current working tree.

Instead of using argv_array structure, a child_process structure
is been used to set the env variable, and run the given command at the 
specified path with the given arguments.

It was suggested that the quotes of "$@" be removed to avoid passing
the <command> arguments as a single long string. But since quoted
$@ doesn't pass the arguments as a single long string, but instead
passes quoted arguments, in this patch I have kept "$@" unchanged.
Information related to this is available on:
www.gnu.org/software/bash/manual/html_node/Special-Parameters.html

Also, it was observed that just after removal of the quotes of "$@"
present in the git-submodule.sh file, there were a number of tests
from t7407 which failed, and otherwise were passing with "$@".

Some additional changes are also mode which weren't suggested.

Added the flag PARSE_OPT_KEEP_UNKNOWN in the parameters of
parse_options to ignore any option passed as a part of <command>
and not as an option for git-submodule foreach.

Previously, display path showed incorrect output when the command
was executed from with a subdirectory. This occurred due to an incorrect
order of parameter being passed and hence has been corrected.

Additional env variable $sm_path was added, since it was used in
test '"submodule foreach" from subdirectory' in t7407.
I preferred adding this, instead of changing the test case, since
in the case of git-submodule.sh, this env variable was accessible.

I checked-out the commit 1c4fb136db (submodule foreach: skip eval for 
more than one argument, 2013-09-27), which explains that why for 
the case when argc>1, we do not use eval. But since, we are calling the
command in a separate shell itself for all values of argc, hence IMO,
this case need not be considered separately.

The build report of this patch is available at: 
travis-ci.org/pratham-pc/git/builds/224744614 
  
There are still 5 tests which are failing. I have submitted this
as RFC patch for getting suggestions on debugging these
errors and for reviewing the approach taken for porting submodule
'foreach' subcommand to C.
  
I have observed that when we recursively run a command foreach
submodule from a subdirectory, the $path variable as finally obtained
by this patch differs with the $path variable as observed by the 
existing git-submodule code for a nested submodule.

I'll again mention that I have based my branch on
gitster/jk/ls-files-recurse-submodules-fix, since while
using --super-prefix in recursively calling the foreach
command, it produced results indicating that a --super-prefix can't
be used from a subdirectory:

  fatal: can't use --super-prefix from a subdirectory

The patch and the discussion related to it can be found at: 
public-inbox.org/git/20170412003911.1142-1-jacob.e.keller@intel.com/T/#u

Also, in the function foreach_submodule, we call gitmodules_config()
to read values from the worktree .gitmodules and then look up
the information (in this case only the sub->name) by using
submodule_from_path funciton. Since we don't want to
overwrite the null_sha1 entry, only loads from .gitmodules
and avoid overlaying with .git/config.
(also, since this whole process is required only to get the value
og submodule's name, is there some other way by which we may obtain
the value so as to avoid this step?)

As currently finally exams are going on in my college, I was unable to
work on it much and the submission got delayed.


 builtin/submodule--helper.c | 130 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  40 +-------------
 2 files changed, 131 insertions(+), 39 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 85aafe46a..fba59c495 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -487,6 +487,135 @@ static int module_name(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static void foreach_submodule(int argc, const char **argv, const char *path,
+			      const unsigned char *sha1, const char *prefix,
+			      int quiet, int recursive)
+{
+	const char *toplevel = xgetcwd();
+	const struct submodule *sub;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf sb = STRBUF_INIT;
+	struct strbuf sub_sha1 = STRBUF_INIT;
+	struct strbuf file = STRBUF_INIT;
+	char* displaypath = NULL;
+	int i;
+
+	/* Only loads from .gitmodules, no overlay with .git/config */
+	gitmodules_config();
+
+	if (prefix && get_super_prefix()) {
+		die("BUG: cannot have prefix and superprefix");
+	} else if (prefix) {
+		displaypath = xstrdup(relative_path(path, prefix,  &sb));
+	} else if (get_super_prefix()) {
+		strbuf_addf(&sb, "%s/%s", get_super_prefix(), path);
+		displaypath = strbuf_detach(&sb, NULL);
+	} else {
+		displaypath = xstrdup(path);
+	}
+
+	sub = submodule_from_path(null_sha1, path);
+
+	if (!sub)
+		die(_("No url found for submodule path '%s' in .gitmodules"),
+		      displaypath);
+	strbuf_add_unique_abbrev(&sub_sha1, sha1 , 40);
+
+	argv_array_pushf(&cp.env_array, "name=%s", sub->name);
+	argv_array_pushf(&cp.env_array, "path=%s", displaypath);
+	argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
+	argv_array_pushf(&cp.env_array, "sha1=%s", sub_sha1.buf);
+	argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
+
+	cp.use_shell = 1;
+	cp.dir = path;
+
+	for (i = 0; i < argc; i++)
+		argv_array_push(&cp.args, argv[i]);
+
+	strbuf_addstr(&file, path);
+	strbuf_addstr(&file, "/.git");
+
+	if (!quiet && !access_or_warn(file.buf, R_OK, 0))
+		printf(_("Entering '%s'\n"), displaypath);
+
+	if (!access_or_warn(file.buf, R_OK, 0))
+		run_command(&cp);
+
+	if(recursive) {
+		struct child_process cpr = CHILD_PROCESS_INIT;
+
+		cpr.use_shell = 1;
+		cpr.dir = path;
+
+		argv_array_pushf(&cpr.args, "git");
+		argv_array_pushf(&cpr.args, "--super-prefix");
+		argv_array_push(&cpr.args, displaypath);
+		argv_array_pushf(&cpr.args, "submodule--helper");
+
+		if (quiet)
+			argv_array_pushf(&cpr.args, "--quiet");
+
+		argv_array_pushf(&cpr.args, "foreach");
+		argv_array_pushf(&cpr.args, "--recursive");
+
+		for (i = 0; i < argc; i++)
+			argv_array_push(&cpr.args, argv[i]);
+
+		run_command(&cpr);
+	}
+
+	strbuf_release(&file);
+	strbuf_release(&sub_sha1);
+	strbuf_release(&sb);
+	free(displaypath);
+
+	return;
+}
+
+static int module_foreach(int argc, const char **argv, const char *prefix)
+{
+	struct pathspec pathspec;
+	struct module_list list = MODULE_LIST_INIT;
+	int quiet = 0;
+	int recursive = 0;
+	int i;
+
+	struct option module_foreach_options[] = {
+		OPT__QUIET(&quiet, N_("Suppress output of entering each submodule command")),
+		OPT_BOOL(0, "recursive", &recursive,
+			 N_("Recurse into nested submodules")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper [--quiet] foreach [--recursive] <command>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_foreach_options,
+			     git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);
+
+	if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)
+			die("BUG: module_list_compute should not choke on empty pathspec");
+
+	for (i = 0; i < list.nr; i++) {
+		if (prefix) {
+			const char *out = NULL;
+			if (skip_prefix(prefix, list.entries[i]->name, &out)) {
+				if (out && out[0] == '/' && !out + 1)
+					return 0;
+			}
+		}
+
+		foreach_submodule(argc, argv, list.entries[i]->name,
+				  list.entries[i]->oid.hash, prefix,
+				  quiet, recursive);
+	}
+
+	return 0;
+}
+
 static int clone_submodule(const char *path, const char *gitdir, const char *url,
 			   const char *depth, struct string_list *reference,
 			   int quiet, int progress)
@@ -1168,6 +1297,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},
+	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
 	{"init", module_init, SUPPORT_SUPER_PREFIX},
 	{"remote-branch", resolve_remote_submodule_branch, 0},
 	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
diff --git a/git-submodule.sh b/git-submodule.sh
index 6ec35e5fc..e2c2b40f4 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -321,46 +321,8 @@ cmd_foreach()
 		esac
 		shift
 	done
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
 
-	toplevel=$(pwd)
-
-	# dup stdin so that it can be restored when running the external
-	# command in the subshell (and a recursive call to this function)
-	exec 3<&0
-
-	{
-		git submodule--helper list --prefix "$wt_prefix" ||
-		echo "#unmatched" $?
-	} |
-	while read mode sha1 stage sm_path
-	do
-		die_if_unmatched "$mode" "$sha1"
-		if test -e "$sm_path"/.git
-		then
-			displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
-			say "$(eval_gettext "Entering '\$displaypath'")"
-			name=$(git submodule--helper name "$sm_path")
-			(
-				prefix="$prefix$sm_path/"
-				sanitize_submodule_env
-				cd "$sm_path" &&
-				sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
-				# we make $path available to scripts ...
-				path=$sm_path &&
-				if test $# -eq 1
-				then
-					eval "$1"
-				else
-					"$@"
-				fi &&
-				if test -n "$recursive"
-				then
-					cmd_foreach "--recursive" "$@"
-				fi
-			) <&3 3<&- ||
-			die "$(eval_gettext "Stopping at '\$displaypath'; script returned non-zero status.")"
-		fi
-	done
 }
 
 #
-- 
2.11.0

Re: [GSoC][RFC/PATCH v2] submodule: port subcommand foreach from shell to C

[Junio C Hamano]

> +static void foreach_submodule(int argc, const char **argv, const char *path,
> +			      const unsigned char *sha1, const char *prefix,
> +			      int quiet, int recursive)
> +{

I think that a reader would expect that a function whose name is
foreach_something() to iterate over some collection and do something
on each of them, but this is not.  It is "do something on one thing"
part in a loop that exists elsewhere.

Do not call such a helper "foreach_<something>".

Would it make more sense to use the "struct object_id" (instead of
uchar[40]) interface for new functions like this?

> +	const char *toplevel = xgetcwd();
> +	const struct submodule *sub;
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf sb = STRBUF_INIT;
> +	struct strbuf sub_sha1 = STRBUF_INIT;
> +	struct strbuf file = STRBUF_INIT;
> +	char* displaypath = NULL;
> +	int i;
> +
> +	/* Only loads from .gitmodules, no overlay with .git/config */
> +	gitmodules_config();
> +
> +	if (prefix && get_super_prefix()) {
> +		die("BUG: cannot have prefix and superprefix");
> +	} else if (prefix) {
> +		displaypath = xstrdup(relative_path(path, prefix,  &sb));

An extra SP after the last comma?

> +	} else if (get_super_prefix()) {
> +		strbuf_addf(&sb, "%s/%s", get_super_prefix(), path);
> +		displaypath = strbuf_detach(&sb, NULL);
> +	} else {
> +		displaypath = xstrdup(path);
> +	}
> +
> +	sub = submodule_from_path(null_sha1, path);
> +
> +	if (!sub)
> +		die(_("No url found for submodule path '%s' in .gitmodules"),
> +		      displaypath);
> +	strbuf_add_unique_abbrev(&sub_sha1, sha1 , 40);

Why add-unique-abbrev if we do not want to abbreviate at all (with
hardcoded constant 40)?  IOW, shouldn't this be

	strbuf_addstr(&sub_sha1, sha1_to_hex(sha1));

> +	argv_array_pushf(&cp.env_array, "name=%s", sub->name);
> +	argv_array_pushf(&cp.env_array, "path=%s", displaypath);
> +	argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);

Why are we sending a value that will always be the same in two
variables?  "git submodule --help" seems to list only name, path,
sha1 and toplevel variables, and not sm_path.  Is it a documentation
bug, or are we clobbering a variable that the end user may be using
for other purposes in her foreach script?

> +	argv_array_pushf(&cp.env_array, "sha1=%s", sub_sha1.buf);
> +	argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
> +
> +	cp.use_shell = 1;
> +	cp.dir = path;
> +
> +	for (i = 0; i < argc; i++)
> +		argv_array_push(&cp.args, argv[i]);
> +
> +	strbuf_addstr(&file, path);
> +	strbuf_addstr(&file, "/.git");
> +
> +	if (!quiet && !access_or_warn(file.buf, R_OK, 0))
> +		printf(_("Entering '%s'\n"), displaypath);
> +
> +	if (!access_or_warn(file.buf, R_OK, 0))
> +		run_command(&cp);
> +
> +	if(recursive) {

Missing SP after "if".

More importantly, if the earlier access-or-warn failed and we didn't
do the run-command for the path, does it even make sense to recurse
into it?

The original scripted version seems to refrain from recursing into
it if the command fails in the submodule in the first place.  This
code discards the exit status from run_command(), which looks wrong.

> +		struct child_process cpr = CHILD_PROCESS_INIT;
> +
> +		cpr.use_shell = 1;
> +		cpr.dir = path;
> +
> +		argv_array_pushf(&cpr.args, "git");
> +		argv_array_pushf(&cpr.args, "--super-prefix");
> +		argv_array_push(&cpr.args, displaypath);
> +		argv_array_pushf(&cpr.args, "submodule--helper");
> +
> +		if (quiet)
> +			argv_array_pushf(&cpr.args, "--quiet");
> +
> +		argv_array_pushf(&cpr.args, "foreach");
> +		argv_array_pushf(&cpr.args, "--recursive");
> +
> +		for (i = 0; i < argc; i++)
> +			argv_array_push(&cpr.args, argv[i]);
> +
> +		run_command(&cpr);

Similarly, the exit status of this invocation is discarded, which
probably is wrong.  The original seems to pay attention to the
failure from the command invoked via "foreach --recursive" and stops
interation when it sees a failure.

> +static int module_foreach(int argc, const char **argv, const char *prefix)
> +{
> +	struct pathspec pathspec;
> +	struct module_list list = MODULE_LIST_INIT;
> +	int quiet = 0;
> +	int recursive = 0;
> +	int i;
> +
> +	struct option module_foreach_options[] = {
> +		OPT__QUIET(&quiet, N_("Suppress output of entering each submodule command")),
> +		OPT_BOOL(0, "recursive", &recursive,
> +			 N_("Recurse into nested submodules")),
> +		OPT_END()
> +	};
> +
> +	const char *const git_submodule_helper_usage[] = {
> +		N_("git submodule--helper [--quiet] foreach [--recursive] <command>"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, module_foreach_options,
> +			     git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);

> +	if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)
> +			die("BUG: module_list_compute should not choke on empty pathspec");

This line would fit within 80-col if it weren't overly indented, I
would think.

One suggestion.

Start by updating "git submodule foreach [--recursive]" tests in t/
directory before writing any more C code.  Arrange to have two
submodules, have your custom command that is run with foreach fail
in the first submodule, and write a test that documents the
behaviour of the scripted version (e.g. your custom command should
not be even run inside the second submodule).  Add a similar test
with two submodules, first of which has a nested submodule, and have
your custom command fail in one of these three places, and document
the behaviour of the scripted version (e.g. your custom command will
run for the first submodule, and if it fails there, does not run in
the nested submodule or in the second submodule; if your custom
command succeeds in the first submodule, it goes to its nested
submodule, and if your custom command fails there, the second
submodule will not visited, etc.).

Also if it is not done in the existing tests, perhaps writing tests
or two with submodules whose names are "submodule 1", "submodule 2",
etc. and making sure that there is no funny splitting of the arguments
would be a healthy thing to do.

After getting that working, then start writing the above C code.
That would catch cases where your "rewrite in C" accidentally
introduces regressions.

Thanks.

Re: [GSoC][RFC/PATCH v2] submodule: port subcommand foreach from shell to C

[Stefan Beller]

> The First function module_foreach first parses the options present in

s/First/first/

> The second function also takes care of the recursive flag, by creating
> a saperate child_process structure and prepending "--super-prefix displaypath",

 s/saperate/separate

> Additional env variable $sm_path was added, since it was used in
> test '"submodule foreach" from subdirectory' in t7407.
> I preferred adding this, instead of changing the test case, since
> in the case of git-submodule.sh, this env variable was accessible.

Makes sense. Though this explanation could go into the commit message as well.

> I checked-out the commit 1c4fb136db (submodule foreach: skip eval for
> more than one argument, 2013-09-27), which explains that why for
> the case when argc>1, we do not use eval. But since, we are calling the
> command in a separate shell itself for all values of argc, hence IMO,
> this case need not be considered separately.

Makes sense. Though this explanation could go into the commit message as well.

> I have observed that when we recursively run a command foreach
> submodule from a subdirectory, the $path variable as finally obtained
> by this patch differs with the $path variable as observed by the
> existing git-submodule code for a nested submodule.

Uh, then the recuring is still a bit broken.

> I'll again mention that I have based my branch on
> gitster/jk/ls-files-recurse-submodules-fix, since while
> using --super-prefix in recursively calling the foreach

Oops, missed that part. (Later in the reply I hint at the patch not
applying correctly, disregard that part.)

> Also, in the function foreach_submodule, we call gitmodules_config()
> to read values from the worktree .gitmodules and then look up
> the information (in this case only the sub->name) by using
> submodule_from_path funciton. Since we don't want to
> overwrite the null_sha1 entry, only loads from .gitmodules
> and avoid overlaying with .git/config.

The null_sha1 entry is there nevertheless.

> (also, since this whole process is required only to get the value
> og submodule's name, is there some other way by which we may obtain
> the value so as to avoid this step?)

Unfortunately this is the only way to get the name. But that's right, for
just getting the name we do not need the .git/config overlay as that
is not supposed to overwrite name/path mappings.

> As currently finally exams are going on in my college, I was unable to
> work on it much and the submission got delayed.

Good luck with the finals. :)

+
+ /* Only loads from .gitmodules, no overlay with .git/config */
+ gitmodules_config();

We should do the config loading/parsing not
in the inner looping function, but just once outside.

+
+ argv_array_pushf(&cp.env_array, "name=%s", sub->name);
+ argv_array_pushf(&cp.env_array, "path=%s", displaypath);
+ argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);

You mention keeping 'sm_path' in the notes after the commit message. I would
add that part to the commit message, to explain why we have multiple variables
that have the same value. Maybe even a comment in the code:

    /* Keep sm_path for historic reasons, see tests in 091a6eb0fee. */
    .. sm_path ..

+ argv_array_pushf(&cp.env_array, "sha1=%s", sub_sha1.buf);
+ argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
+
+ cp.use_shell = 1;
+ cp.dir = path;
+
+ for (i = 0; i < argc; i++)
+ argv_array_push(&cp.args, argv[i]);
+
+ strbuf_addstr(&file, path);
+ strbuf_addstr(&file, "/.git");

This could be oneline as:
        strbuf_addf(&file, "%s/.git", path);
+
+ if (!quiet && !access_or_warn(file.buf, R_OK, 0))
+ printf(_("Entering '%s'\n"), displaypath);
+
+ if (!access_or_warn(file.buf, R_OK, 0))

I do not think we'd want to warn about a missing .git file (which is what the
or_warn part does).

Instead I'd rather use the abstract function 'is_submodule_populated_gently'.

+ run_command(&cp);
+
+ if(recursive) {
+ struct child_process cpr = CHILD_PROCESS_INIT;
+
+ cpr.use_shell = 1;
+ cpr.dir = path;
+
+ argv_array_pushf(&cpr.args, "git");
+ argv_array_pushf(&cpr.args, "--super-prefix");
+ argv_array_push(&cpr.args, displaypath);
+ argv_array_pushf(&cpr.args, "submodule--helper");

No need for the 'f'ormat version of pusing strings if there is no formatting
going on. An alternative would be to push them all at once:

    argv_array_pushl(&cpr.args, "git", "--super-prefix", displaypath,
                     submodule--helper, NULL);

(The 'l' version of pushing accepts an arbitrary number of arguments until the
final NULL)


+ if (quiet)
+ argv_array_pushf(&cpr.args, "--quiet");
+
+ argv_array_pushf(&cpr.args, "foreach");
+ argv_array_pushf(&cpr.args, "--recursive");


+
+ for (i = 0; i < argc; i++)
+ argv_array_push(&cpr.args, argv[i]);
+
+ run_command(&cpr);

I'd think we'd need to react to the exit code here (and above), and even
stop early just as the shell version does. (To find out how it does so,
I used gitk, and followed the error line
  die "$(eval_gettext "Stopping at '\$displaypath'; script returned
non-zero status.")"
and right clicked on that line "Show origin of that line". There is a lot of
noise going on, e.g. i18n, path issues being fixed, and such, until you'd find
19a31f9c1a (git-submodule - Add 'foreach' subcommand, 2008-08-10), which
outlines more clearly its basic functionality:

    cmd_foreach()
    {
        git ls-files --stage | grep '^160000 ' |
        while read mode sha1 stage path
        do
            if test -e "$path"/.git
            then
                say "Entering '$path'"
                (cd "$path" && eval "$@") ||
                die "Stopping at '$path'; script returned non-zero status."
            fi
        done
    }

Note the && || operators, that act as
https://en.wikipedia.org/wiki/Short-circuit_evaluation


I do not think we would need the following block of code:

+ if (prefix) {
+ const char *out = NULL;
+ if (skip_prefix(prefix, list.entries[i]->name, &out)) {
+ if (out && out[0] == '/' && !out + 1)
+ return 0;
+ }
+ }

because 'module_list_compute' was given the 'prefix', such that the list only
contains list submodules within the 'prefix' area. However with the early
return inside, I wonder what this code is about? (If this is not about
scoping to the directory with prefix, maybe a comment could clarify?)

+ foreach_submodule(argc, argv, list.entries[i]->name,
+  list.entries[i]->oid.hash, prefix,
+  quiet, recursive);

As Junio suggests, the name of foreach_submodule can be improved.
Also as I think further about it, we may actually want to have 3
(instead of 2) functions:

* The outer-most function is what we have here except the for-loop.
  (i.e. it is the front-end function for "submodule--helper foreach")
* the actual looping function, as that can be re-used later from within
  C, then we would not need to spawn an extra submodule--helper, but
  could directly call the
    foreach_submodule(<list>, <function ptr>, <data ptr>)
* The inner-most function is what is currently named 'foreach_submodule'
  it "does one thing", in this case runa process in each submodule.

An example of such an approach is seen with the functions:
  'for_each_string_list(list, string_list_each_func_t, cb_data);'
  (not the macro ending in _item)
  'for_each_*' as found in cache.h

    int for_each_string_list(struct string_list *list,
                 string_list_each_func_t fn, void *cb_data)
    {
        int i, ret = 0;
        for (i = 0; i < list->nr; i++)
            if ((ret = fn(&list->items[i], cb_data)))
                break;
        return ret;
    }

That way we'd have a nice helper function in the future to iterate
over submodules with a given list.

--

The following hunk changed in upstream, which is why I could not apply
the patch.
The colliding commit is cf9e55f494 (submodule: prevent backslash expantion in
submodule names, 2017-04-07), which added just a "-r" to the "while
read mode..."

So if you'd rebase your work on top of the latest master, this will produce
a conflict as well for you.

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

- toplevel=$(pwd)
-
- # dup stdin so that it can be restored when running the external
- # command in the subshell (and a recursive call to this function)
- exec 3<&0
-
..

Thanks,
Stefan

Re: [GSoC][RFC/PATCH v2] submodule: port subcommand foreach from shell to C

[Ramsay Jones]

On 24/04/17 21:03, Stefan Beller wrote:
[snip]

> +
> + argv_array_pushf(&cp.env_array, "name=%s", sub->name);
> + argv_array_pushf(&cp.env_array, "path=%s", displaypath);
> + argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
> 
> You mention keeping 'sm_path' in the notes after the commit message. I would
> add that part to the commit message, to explain why we have multiple variables
> that have the same value. Maybe even a comment in the code:
> 
>     /* Keep sm_path for historic reasons, see tests in 091a6eb0fee. */
>     .. sm_path ..

Hmm, you need to be a bit careful with putting 'path' in the
environment (if you then export it to sub-processes) on windows
(cygwin, MinGW, GfW). See commit 64394e3ae9. I would have liked
to remove $path altogether from the 'submodule-foreach api' in
that commit, but users and their scripts were already using it
(so I couldn't just drop it, without some deprecation period).
So long as whatever was being 'eval'-ed in the script didn't
export $path, ...

ATB,
Ramsay Jones

Re: [GSoC][RFC/PATCH v2] submodule: port subcommand foreach from shell to C

[Stefan Beller]

On Mon, Apr 24, 2017 at 3:11 PM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
> On 24/04/17 21:03, Stefan Beller wrote:
> [snip]
>
>> +
>> + argv_array_pushf(&cp.env_array, "name=%s", sub->name);
>> + argv_array_pushf(&cp.env_array, "path=%s", displaypath);
>> + argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
>>
>> You mention keeping 'sm_path' in the notes after the commit message. I would
>> add that part to the commit message, to explain why we have multiple variables
>> that have the same value. Maybe even a comment in the code:
>>
>>     /* Keep sm_path for historic reasons, see tests in 091a6eb0fee. */
>>     .. sm_path ..
>
> Hmm, you need to be a bit careful with putting 'path' in the
> environment (if you then export it to sub-processes) on windows
> (cygwin, MinGW, GfW). See commit 64394e3ae9. I would have liked
> to remove $path altogether from the 'submodule-foreach api' in
> that commit, but users and their scripts were already using it
> (so I couldn't just drop it, without some deprecation period).
> So long as whatever was being 'eval'-ed in the script didn't
> export $path, ...
>

Oh, I misread the comment

     # we make $path available to scripts ...
     path=$sm_path

as it was such a casual friendly thing to say in that context.
So the *real* historic baggage is
    argv_array_pushf(&cp.env_array, "path=%s", displaypath);
whereas
    argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
is considered the correct way to go.

Thanks,
Stefan

Re: [GSoC][RFC/PATCH v2] submodule: port subcommand foreach from shell to C

[Ramsay Jones]

On 24/04/17 23:17, Stefan Beller wrote:
> On Mon, Apr 24, 2017 at 3:11 PM, Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
>>
>>
>> On 24/04/17 21:03, Stefan Beller wrote:
>> [snip]
>>
>>> +
>>> + argv_array_pushf(&cp.env_array, "name=%s", sub->name);
>>> + argv_array_pushf(&cp.env_array, "path=%s", displaypath);
>>> + argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
>>>
>>> You mention keeping 'sm_path' in the notes after the commit message. I would
>>> add that part to the commit message, to explain why we have multiple variables
>>> that have the same value. Maybe even a comment in the code:
>>>
>>>     /* Keep sm_path for historic reasons, see tests in 091a6eb0fee. */
>>>     .. sm_path ..
>>
>> Hmm, you need to be a bit careful with putting 'path' in the
>> environment (if you then export it to sub-processes) on windows
>> (cygwin, MinGW, GfW). See commit 64394e3ae9. I would have liked
>> to remove $path altogether from the 'submodule-foreach api' in
>> that commit, but users and their scripts were already using it
>> (so I couldn't just drop it, without some deprecation period).
>> So long as whatever was being 'eval'-ed in the script didn't
>> export $path, ...
>>
> 
> Oh, I misread the comment
> 
>      # we make $path available to scripts ...
>      path=$sm_path
> 
> as it was such a casual friendly thing to say in that context.
> So the *real* historic baggage is
>     argv_array_pushf(&cp.env_array, "path=%s", displaypath);
> whereas
>     argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
> is considered the correct way to go.

I have to admit that I didn't actually read the code in this
patch. I just saw the subject line and the ass-backward comment
about $sm_path. ;-)

My intention was simply to warn: 'there be dragons'.

ATB,
Ramsay Jones

[GSoC][RFC/PATCH v3 1/2] t7407: test "submodule foreach --recursive" from subdirectory added

[Prathamesh Chavan]

Additional test cases added to the submodule-foreach test suite
to check the submodule foreach --recursive behavior from a
subdirectory as this was missing from the test suite.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---
 t/t7407-submodule-foreach.sh | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6ba5daf42..58a890e31 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -79,7 +79,6 @@ test_expect_success 'test basic "submodule foreach" usage' '
 	) &&
 	test_i18ncmp expect actual
 '
-
 cat >expect <<EOF
 Entering '../sub1'
 $pwd/clone-foo1-../sub1-$sub1sha1
@@ -197,6 +196,40 @@ test_expect_success 'test messages from "foreach --recursive" from subdirectory'
 	test_i18ncmp expect actual
 '
 
+sub1sha1=$(cd clone2/sub1 && git rev-parse HEAD)
+sub2sha1=$(cd clone2/sub2 && git rev-parse HEAD)
+sub3sha1=$(cd clone2/sub3 && git rev-parse HEAD)
+nested1sha1=$(cd clone2/nested1 && git rev-parse HEAD)
+nested2sha1=$(cd clone2/nested1/nested2 && git rev-parse HEAD)
+nested3sha1=$(cd clone2/nested1/nested2/nested3 && git rev-parse HEAD)
+submodulesha1=$(cd clone2/nested1/nested2/nested3/submodule && git rev-parse HEAD)
+
+cat >expect <<EOF
+Entering '../nested1'
+$pwd/clone2-nested1-../nested1-$nested1sha1
+Entering '../nested1/nested2'
+$pwd/clone2/nested1-nested2-../nested2-$nested2sha1
+Entering '../nested1/nested2/nested3'
+$pwd/clone2/nested1/nested2-nested3-../nested3-$nested3sha1
+Entering '../nested1/nested2/nested3/submodule'
+$pwd/clone2/nested1/nested2/nested3-submodule-../submodule-$submodulesha1
+Entering '../sub1'
+$pwd/clone2-foo1-../sub1-$sub1sha1
+Entering '../sub2'
+$pwd/clone2-foo2-../sub2-$sub2sha1
+Entering '../sub3'
+$pwd/clone2-foo3-../sub3-$sub3sha1
+EOF
+
+test_expect_success 'test "submodule foreach --recursive" from subdirectory' '
+	(
+		cd clone2 &&
+		cd untracked &&
+		git submodule foreach --recursive "echo \$toplevel-\$name-\$sm_path-\$sha1" >../../actual
+	) &&
+	test_i18ncmp expect actual
+'
+
 cat > expect <<EOF
 nested1-nested1
 nested2-nested2
-- 
2.11.0

[GSoC][RFC/PATCH v3 2/2] submodule: port subcommand foreach from shell to C

[Prathamesh Chavan]

This aims to make git-submodule foreach a builtin. This is the very
first step taken in this direction. Hence, 'foreach' is ported to
submodule--helper, and submodule--helper is called from git-submodule.sh.
The code is split up to have one function to obtain all the list of
submodules. This function acts as the front-end of git-submodule foreach
subcommand. It calls the function for_each_submodule_list, which basically
loops through the list and calls function fn, which in this case is
runcommand_in_submodule. This third function is a calling function that
takes care of running the command in that submodule, and recursively
perform the same when --recursive is flagged.

The first function module_foreach first 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 case of submodule subcommand
foreach is runcommand_in_submodule) is called for each entry.

The third function runcommand_in_submodule, generates a submodule struct sub
for $name value and then later prepends name=sub->name and other
value assignments to the env argv_array structure of a child_process.
Also the <command> of submodule-foreach is push to args argv_array
structure and finally, using run_command the commands are executed
using a shell.

The third function also takes care of the recursive flag, by creating
a separate child_process structure and prepending "--super-prefix displaypath",
to the args argv_array structure. Other required arguments and the
input <command> of submodule-foreach is also appended to this argv_array.

The commit 1c4fb136db (submodule foreach: skip eval for more than one
argument, 2013-09-27), which explains that why for the case when argc>1,
we do not use eval. But since in this patch, we are calling the
command in a separate shell itself for all values of argc, this case
is not considered separately.

Both env variable $path and $sm_path were added since both are used in
tests in t7407.

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, following changes were added:

foreach_submodule was renamed to runcommand_in_submodule

We module_foreach into two parts, such that, module foreach
generates the list of submodules present in the cwd. And later
calls for_each_submodule_list which basically loops through the
list of submodule and calls the passed function fn.

Additionally, this patch also pass all those test, which it
failed earlier.

Since in the run-command API errors out when child
process with no arguments is passed, for test #10
from t7407-submodule-foreach, additional condition
was added in runcommand_in_submodule funciton to
handle this case instead of modifying the run-command.

Complete build report is available at:
https://travis-ci.org/pratham-pc/git/builds/
branch: foreach
build #52

It can been seen that the patch fails in test #9
of t7407-submodule-foreach, which is the newly added
test to that suite. The main reason of adding this test
was to bring the behavior of $path for the submodule
foreach --recursive case.

The observation made was as follows:

For a project - super containing dir (not a submodule)
and a submodule sub which contains another submodule
subsub. When we run a command from super/dir:

git submodule foreach "echo \$path-\$sm_path"

actual results:
Entering '../sub'
../sub-../sub
Entering '../sub/subsub'
../subsub-../subsub

ported function's result:
Entering '../sub'
sub-../sub
Entering '../sub/subsub'
subsub-../sub/subsub

This is occurring since in cmd_foreach of git-submodule.sh
when we use to recurse, we call cmd_foreach
and hence the process ran in the same shell.
Because of this, the variable $wt_prefix is set only once
which is at the beginning of the submodule foreach execution.
wt_prefix=$(git rev-parse --show-prefix)

And since sm_path and path are set using $wt_prefix as :
sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
path=$sm_path
It differs with the value of displaypath as well.

This make the value of $path confusing and I also feel it
deviates from its documentation:
$path is the name of the submodule directory relative
to the superproject.

But since in refactoring the code, we wish to maintain the
code in same way, we need to pass wt_prefix on every
recursive call, which may result in complex C code.
Another option could be to first correct the $path value
in git-submodule.sh and then port the updated cmd_foreach.

 builtin/submodule--helper.c | 145 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  39 +-----------
 2 files changed, 146 insertions(+), 38 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 566a5b6a6..1a5b26c22 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -13,6 +13,10 @@
 #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;
@@ -331,6 +335,15 @@ 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);
+
+	return;
+}
+
 static void init_submodule(const char *path, const char *prefix, int quiet)
 {
 	const struct submodule *sub;
@@ -487,6 +500,137 @@ static int module_name(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct cb_foreach {
+	int argc;
+	const char **argv;
+	const char *prefix;
+	int quiet;
+	int recursive;
+};
+
+static void runcommand_in_submodule(const struct cache_entry *list_item, void *cb_data)
+{
+	struct cb_foreach *info = cb_data;
+	const char *toplevel = xgetcwd();
+	const struct submodule *sub;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf sb = STRBUF_INIT;
+	struct strbuf sub_sha1 = STRBUF_INIT;
+	char* displaypath = NULL;
+	int i;
+
+	/* Only loads from .gitmodules, no overlay with .git/config */
+	gitmodules_config();
+
+	if (info->prefix && get_super_prefix()) {
+		die("BUG: cannot have prefix '%s' and superprefix '%s'",
+		    info->prefix, get_super_prefix());
+	} else if (info->prefix) {
+		displaypath = xstrdup(relative_path(list_item->name, info->prefix, &sb));
+	} else if (get_super_prefix()) {
+		strbuf_addf(&sb, "%s/%s", get_super_prefix(), list_item->name);
+		displaypath = strbuf_detach(&sb, NULL);
+	} else {
+		displaypath = xstrdup(list_item->name);
+	}
+
+	sub = submodule_from_path(null_sha1, list_item->name);
+
+	if (!sub)
+		die(_("No url found for submodule path '%s' in .gitmodules"),
+		      displaypath);
+
+	strbuf_addstr(&sub_sha1, oid_to_hex(&list_item->oid));
+
+	argv_array_pushf(&cp.env_array, "name=%s", sub->name);
+	argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
+	argv_array_pushf(&cp.env_array, "path=%s", list_item->name); 
+	argv_array_pushf(&cp.env_array, "sha1=%s", sub_sha1.buf);
+	argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
+
+	cp.use_shell = 1;
+	cp.dir = list_item->name;
+	prepare_submodule_repo_env(&cp.env_array);
+	argv_array_pushf(&cp.env_array, "%s", GIT_SUPER_PREFIX_ENVIRONMENT);
+
+	for (i = 0; i < info->argc; i++)
+		argv_array_push(&cp.args, info->argv[i]);
+
+	if (!is_submodule_populated_gently(list_item->name, NULL))
+		return;
+	
+	if (!info->quiet)
+		printf(_("Entering '%s'\n"), displaypath);
+	if (info->argv[0] && run_command(&cp))
+		die(_("run_command returned non-zero status for %s\n."), displaypath);
+
+	if (info->recursive) {
+		struct child_process cpr = CHILD_PROCESS_INIT;
+
+		cpr.use_shell = 1;
+		cpr.dir = list_item->name;
+		prepare_submodule_repo_env(&cpr.env_array);
+
+		argv_array_pushl(&cpr.args, "git", "--super-prefix", displaypath,
+				 "submodule--helper", NULL);
+
+		argv_array_pushl(&cpr.args, "foreach", "--recursive", NULL);
+
+		if (info->quiet)
+			argv_array_push(&cpr.args, "--quiet");
+
+		for (i = 0; i < info->argc; i++)
+			argv_array_push(&cpr.args, info->argv[i]);
+
+		run_command(&cpr);
+	}
+
+	strbuf_release(&sub_sha1);
+	strbuf_release(&sb);
+	free(displaypath);
+
+	return;
+}
+
+static int module_foreach(int argc, const char **argv, const char *prefix)
+{
+	struct cb_foreach info;
+	struct pathspec pathspec;
+	struct module_list list = MODULE_LIST_INIT;
+	int quiet = 0;
+	int recursive = 0;
+
+	struct option module_foreach_options[] = {
+		OPT__QUIET(&quiet, N_("Suppress output of entering each submodule command")),
+		OPT_BOOL(0, "recursive", &recursive,
+			 N_("Recurse into nested submodules")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper foreach [--quiet] [--recursive] <command>"),
+		NULL
+	};
+
+	memset(&info, 0, sizeof(info));
+
+	argc = parse_options(argc, argv, prefix, module_foreach_options,
+			     git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);
+
+	if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)
+			die("BUG: module_list_compute should not choke on empty pathspec");
+
+	info.argc = argc;
+	info.argv = argv;
+	info.prefix = prefix;
+	info.quiet = quiet;
+	info.recursive = recursive;
+
+	for_each_submodule_list(list, runcommand_in_submodule, &info);
+
+	return 0;
+}
+
 static int clone_submodule(const char *path, const char *gitdir, const char *url,
 			   const char *depth, struct string_list *reference,
 			   int quiet, int progress)
@@ -1212,6 +1356,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},
+	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
 	{"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..032fd2540 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -322,45 +322,8 @@ cmd_foreach()
 		shift
 	done
 
-	toplevel=$(pwd)
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
 
-	# dup stdin so that it can be restored when running the external
-	# command in the subshell (and a recursive call to this function)
-	exec 3<&0
-
-	{
-		git submodule--helper list --prefix "$wt_prefix" ||
-		echo "#unmatched" $?
-	} |
-	while read -r mode sha1 stage sm_path
-	do
-		die_if_unmatched "$mode" "$sha1"
-		if test -e "$sm_path"/.git
-		then
-			displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
-			say "$(eval_gettext "Entering '\$displaypath'")"
-			name=$(git submodule--helper name "$sm_path")
-			(
-				prefix="$prefix$sm_path/"
-				sanitize_submodule_env
-				cd "$sm_path" &&
-				sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
-				# we make $path available to scripts ...
-				path=$sm_path &&
-				if test $# -eq 1
-				then
-					eval "$1"
-				else
-					"$@"
-				fi &&
-				if test -n "$recursive"
-				then
-					cmd_foreach "--recursive" "$@"
-				fi
-			) <&3 3<&- ||
-			die "$(eval_gettext "Stopping at '\$displaypath'; script returned non-zero status.")"
-		fi
-	done
 }
 
 #
-- 
2.11.0

Re: [GSoC][RFC/PATCH v3 2/2] submodule: port subcommand foreach from shell to C

[Stefan Beller]

So we're looking for more reviewers for this patch,
one way to do it is e.g. via
$ git shortlog --since 12.month -sne -- ./git-submodule.sh
(so I cc'd Brandon)
Another way to find reviewers is
$ git blame ./git-submodule.sh
(so I cc'd Anders and Johan)

> It can been seen that the patch fails in test #9
> of t7407-submodule-foreach, which is the newly added
> test to that suite. The main reason of adding this test
> was to bring the behavior of $path for the submodule
> foreach --recursive case.
>
> The observation made was as follows:
>
> For a project - super containing dir (not a submodule)
> and a submodule sub which contains another submodule
> subsub. When we run a command from super/dir:
>
> git submodule foreach "echo \$path-\$sm_path"
>
> actual results:
> Entering '../sub'
> ../sub-../sub
> Entering '../sub/subsub'
> ../subsub-../subsub
>
> ported function's result:
> Entering '../sub'
> sub-../sub
> Entering '../sub/subsub'
> subsub-../sub/subsub
>
> This is occurring since in cmd_foreach of git-submodule.sh
> when we use to recurse, we call cmd_foreach
> and hence the process ran in the same shell.
> Because of this, the variable $wt_prefix is set only once
> which is at the beginning of the submodule foreach execution.
> wt_prefix=$(git rev-parse --show-prefix)
>
> And since sm_path and path are set using $wt_prefix as :
> sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
> path=$sm_path
> It differs with the value of displaypath as well.
>
> This make the value of $path confusing and I also feel it
> deviates from its documentation:
> $path is the name of the submodule directory relative
> to the superproject.
>
> But since in refactoring the code, we wish to maintain the
> code in same way, we need to pass wt_prefix on every
> recursive call, which may result in complex C code.
> Another option could be to first correct the $path value
> in git-submodule.sh and then port the updated cmd_foreach.
>

We discussed this patch off list before and I think the code is fine,
two minor nits below. But this observation is something we'd want
to talk about here on list. Sorry for dropping the ball for 3 days.

If we do not apply patch 1, the test suite passes, as far as I observe
the situation and patch 1 introduces a test that passes the shell
foreach, but breaks here, IIUC.

Specifically the $sm_path breaks the test in the sense that it is
actually correct now for nested submodules when the command
in the superproject is run from within a directory.

I think this is a rare bug, that we can just fix along the way, i.e.
mention in the commit message that we fix it and adapt the test
such that it passes with the rewrite in C, here.

> +       argv_array_pushf(&cp.env_array, "path=%s", list_item->name);

git-am claims there are trailing white spaces.
$ git am <patch>
Applying: submodule: port subcommand foreach from shell to C
.git/rebase-apply/patch:158: trailing whitespace.
argv_array_pushf(&cp.env_array, "path=%s", list_item->name);
.git/rebase-apply/patch:172: trailing whitespace.
warning: 2 lines add whitespace errors.

Not sure which editor you use, but I could configure my editor to strip
trailing whitespaces on save, and had never any issues with them
since.

> +       struct cb_foreach info;
..
> +
> +       memset(&info, 0, sizeof(info));

Instead of this memset, you could also have a #define CB_FOREACH_INIT
similar to e.g. MODULE_LIST_INIT, STRBUF_INIT, STRING_LIST_INIT and so on.
This memset works, too.

Thanks,
Stefan

Re: [GSoC][RFC/PATCH v3 2/2] submodule: port subcommand foreach from shell to C

[Brandon Williams]

On 05/12, Prathamesh Chavan wrote:
> This aims to make git-submodule foreach a builtin. This is the very
> first step taken in this direction. Hence, 'foreach' is ported to
> submodule--helper, and submodule--helper is called from git-submodule.sh.
> The code is split up to have one function to obtain all the list of
> submodules. This function acts as the front-end of git-submodule foreach
> subcommand. It calls the function for_each_submodule_list, which basically
> loops through the list and calls function fn, which in this case is
> runcommand_in_submodule. This third function is a calling function that
> takes care of running the command in that submodule, and recursively
> perform the same when --recursive is flagged.
> 
> The first function module_foreach first 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 case of submodule subcommand
> foreach is runcommand_in_submodule) is called for each entry.
> 
> The third function runcommand_in_submodule, generates a submodule struct sub
> for $name value and then later prepends name=sub->name and other
> value assignments to the env argv_array structure of a child_process.
> Also the <command> of submodule-foreach is push to args argv_array
> structure and finally, using run_command the commands are executed
> using a shell.
> 
> The third function also takes care of the recursive flag, by creating
> a separate child_process structure and prepending "--super-prefix displaypath",
> to the args argv_array structure. Other required arguments and the
> input <command> of submodule-foreach is also appended to this argv_array.
> 
> The commit 1c4fb136db (submodule foreach: skip eval for more than one
> argument, 2013-09-27), which explains that why for the case when argc>1,
> we do not use eval. But since in this patch, we are calling the
> command in a separate shell itself for all values of argc, this case
> is not considered separately.
> 
> Both env variable $path and $sm_path were added since both are used in
> tests in t7407.
> 
> 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, following changes were added:
> 
> foreach_submodule was renamed to runcommand_in_submodule
> 
> We module_foreach into two parts, such that, module foreach
> generates the list of submodules present in the cwd. And later
> calls for_each_submodule_list which basically loops through the
> list of submodule and calls the passed function fn.
> 
> Additionally, this patch also pass all those test, which it
> failed earlier.
> 
> Since in the run-command API errors out when child
> process with no arguments is passed, for test #10
> from t7407-submodule-foreach, additional condition
> was added in runcommand_in_submodule funciton to
> handle this case instead of modifying the run-command.
> 
> Complete build report is available at:
> https://travis-ci.org/pratham-pc/git/builds/
> branch: foreach
> build #52
> 
> It can been seen that the patch fails in test #9
> of t7407-submodule-foreach, which is the newly added
> test to that suite. The main reason of adding this test
> was to bring the behavior of $path for the submodule
> foreach --recursive case.
> 
> The observation made was as follows:
> 
> For a project - super containing dir (not a submodule)
> and a submodule sub which contains another submodule
> subsub. When we run a command from super/dir:
> 
> git submodule foreach "echo \$path-\$sm_path"
> 
> actual results:
> Entering '../sub'
> ../sub-../sub
> Entering '../sub/subsub'
> ../subsub-../subsub
> 
> ported function's result:
> Entering '../sub'
> sub-../sub
> Entering '../sub/subsub'
> subsub-../sub/subsub
> 
> This is occurring since in cmd_foreach of git-submodule.sh
> when we use to recurse, we call cmd_foreach
> and hence the process ran in the same shell.
> Because of this, the variable $wt_prefix is set only once
> which is at the beginning of the submodule foreach execution.
> wt_prefix=$(git rev-parse --show-prefix)
> 
> And since sm_path and path are set using $wt_prefix as :
> sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
> path=$sm_path
> It differs with the value of displaypath as well.
> 
> This make the value of $path confusing and I also feel it
> deviates from its documentation:
> $path is the name of the submodule directory relative
> to the superproject.
> 
> But since in refactoring the code, we wish to maintain the
> code in same way, we need to pass wt_prefix on every
> recursive call, which may result in complex C code.
> Another option could be to first correct the $path value
> in git-submodule.sh and then port the updated cmd_foreach.
> 
>  builtin/submodule--helper.c | 145 ++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            |  39 +-----------
>  2 files changed, 146 insertions(+), 38 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 566a5b6a6..1a5b26c22 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -13,6 +13,10 @@
>  #include "refs.h"
>  #include "connect.h"
>  
> +typedef void (*submodule_list_func_t)(const struct cache_entry *list_item, void *cb_data);
> +
> +
> +

nit: You could probably get rid of 2 of these empty lines.

>  static char *get_default_remote(void)
>  {
>  	char *dest = NULL, *ret;
> @@ -331,6 +335,15 @@ 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);
> +
> +	return;
> +}
> +
>  static void init_submodule(const char *path, const char *prefix, int quiet)
>  {
>  	const struct submodule *sub;
> @@ -487,6 +500,137 @@ static int module_name(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +struct cb_foreach {
> +	int argc;
> +	const char **argv;
> +	const char *prefix;
> +	int quiet;
> +	int recursive;
> +};
> +
> +static void runcommand_in_submodule(const struct cache_entry *list_item, void *cb_data)
> +{
> +	struct cb_foreach *info = cb_data;
> +	const char *toplevel = xgetcwd();

xgetcwd() returns a 'char *' so you own the memory for the string that
was returned.  This declaration should be changed to 'char *' and be
freed at the end before returning from this function.

> +	const struct submodule *sub;
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf sb = STRBUF_INIT;
> +	struct strbuf sub_sha1 = STRBUF_INIT;
> +	char* displaypath = NULL;
> +	int i;
> +
> +	/* Only loads from .gitmodules, no overlay with .git/config */
> +	gitmodules_config();
> +
> +	if (info->prefix && get_super_prefix()) {
> +		die("BUG: cannot have prefix '%s' and superprefix '%s'",
> +		    info->prefix, get_super_prefix());
> +	} else if (info->prefix) {
> +		displaypath = xstrdup(relative_path(list_item->name, info->prefix, &sb));

I would move the declaration of the strbuf to be inside this if block
since its not really used else where (except in the next if block but
I'll get to that).

> +	} else if (get_super_prefix()) {
> +		strbuf_addf(&sb, "%s/%s", get_super_prefix(), list_item->name);
> +		displaypath = strbuf_detach(&sb, NULL);

You don't really need to use a strbuf for this, you could just use
xfmtstr() instead and avoid needing to call detach.  Also with the
number of times you call 'get_super_prefix()' it may be worth while to
have a local 'const char *super_prefix' which you assign a single time
and reuse so that you don't waste time calling that function over and
over again.

> +	} else {
> +		displaypath = xstrdup(list_item->name);
> +	}
> +
> +	sub = submodule_from_path(null_sha1, list_item->name);
> +
> +	if (!sub)
> +		die(_("No url found for submodule path '%s' in .gitmodules"),
> +		      displaypath);
> +
> +	strbuf_addstr(&sub_sha1, oid_to_hex(&list_item->oid));
> +
> +	argv_array_pushf(&cp.env_array, "name=%s", sub->name);
> +	argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
> +	argv_array_pushf(&cp.env_array, "path=%s", list_item->name); 

Trailing white space                                            ^ here

> +	argv_array_pushf(&cp.env_array, "sha1=%s", sub_sha1.buf);

You can remove the 'sub_sha1' strbuf.  Since  you are just using it this
once it makes more sense just substitute 'sub_sha1.buf' for the call to
'oid_to_hex()'

> +	argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
> +
> +	cp.use_shell = 1;
> +	cp.dir = list_item->name;
> +	prepare_submodule_repo_env(&cp.env_array);

I would probably hoist this call up before you start setting 'name',
'sm_path', 'path', etc.  That way you are certain that the call to
prepare_submodule_repo_env() doesn't mess with anything you've already
set.  Now in this context it doesn't and  you would be fine but this is
just to future proof any changes to that function.

> +	argv_array_pushf(&cp.env_array, "%s", GIT_SUPER_PREFIX_ENVIRONMENT);

I assume you are doing this to ensure that the super prefix environment
var is cleared in the child process?  Though you shouldn't have to
because this env var is covered in 'local_repo_env' and should already
be taken care of my the call to 'prepare_submodule_repo_env()'

> +
> +	for (i = 0; i < info->argc; i++)
> +		argv_array_push(&cp.args, info->argv[i]);
> +
> +	if (!is_submodule_populated_gently(list_item->name, NULL))
> +		return;
> +	

And trailing white space here ^

> +	if (!info->quiet)
> +		printf(_("Entering '%s'\n"), displaypath);
> +	if (info->argv[0] && run_command(&cp))
> +		die(_("run_command returned non-zero status for %s\n."), displaypath);
> +
> +	if (info->recursive) {
> +		struct child_process cpr = CHILD_PROCESS_INIT;

You don't need to use a new child_process struct here, you could reuse
'cp' since the call to run_command will free all the memory at the end
of the call....oh but you're not unconditionally running run_command
because of the check to argv[0]...

Maybe we want to do an early return if argv[0] is empty?  Not sure how
that would break tests though...

> +
> +		cpr.use_shell = 1;
> +		cpr.dir = list_item->name;
> +		prepare_submodule_repo_env(&cpr.env_array);
> +
> +		argv_array_pushl(&cpr.args, "git", "--super-prefix", displaypath,
> +				 "submodule--helper", NULL);
> +
> +		argv_array_pushl(&cpr.args, "foreach", "--recursive", NULL);

since you're already using pushl, just combine these two function calls.

> +
> +		if (info->quiet)
> +			argv_array_push(&cpr.args, "--quiet");
> +
> +		for (i = 0; i < info->argc; i++)
> +			argv_array_push(&cpr.args, info->argv[i]);
> +
> +		run_command(&cpr);

Are you not going to do anything if the exit status was non-zero?

> +	}
> +
> +	strbuf_release(&sub_sha1);
> +	strbuf_release(&sb);
> +	free(displaypath);
> +
> +	return;

You don't need this return here.

> +}
> +
> +static int module_foreach(int argc, const char **argv, const char *prefix)
> +{
> +	struct cb_foreach info;
> +	struct pathspec pathspec;
> +	struct module_list list = MODULE_LIST_INIT;
> +	int quiet = 0;
> +	int recursive = 0;
> +
> +	struct option module_foreach_options[] = {
> +		OPT__QUIET(&quiet, N_("Suppress output of entering each submodule command")),
> +		OPT_BOOL(0, "recursive", &recursive,
> +			 N_("Recurse into nested submodules")),
> +		OPT_END()
> +	};
> +
> +	const char *const git_submodule_helper_usage[] = {
> +		N_("git submodule--helper foreach [--quiet] [--recursive] <command>"),
> +		NULL
> +	};
> +
> +	memset(&info, 0, sizeof(info));
> +
> +	argc = parse_options(argc, argv, prefix, module_foreach_options,
> +			     git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);
> +
> +	if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)
> +			die("BUG: module_list_compute should not choke on empty pathspec");

nit: Indention looks off here.

> +
> +	info.argc = argc;
> +	info.argv = argv;
> +	info.prefix = prefix;
> +	info.quiet = quiet;
> +	info.recursive = recursive;
> +
> +	for_each_submodule_list(list, runcommand_in_submodule, &info);
> +
> +	return 0;
> +}
> +
>  static int clone_submodule(const char *path, const char *gitdir, const char *url,
>  			   const char *depth, struct string_list *reference,
>  			   int quiet, int progress)
> @@ -1212,6 +1356,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},
> +	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
>  	{"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..032fd2540 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -322,45 +322,8 @@ cmd_foreach()
>  		shift
>  	done
>  
> -	toplevel=$(pwd)
> +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
>  
> -	# dup stdin so that it can be restored when running the external
> -	# command in the subshell (and a recursive call to this function)
> -	exec 3<&0
> -
> -	{
> -		git submodule--helper list --prefix "$wt_prefix" ||
> -		echo "#unmatched" $?
> -	} |
> -	while read -r mode sha1 stage sm_path
> -	do
> -		die_if_unmatched "$mode" "$sha1"
> -		if test -e "$sm_path"/.git
> -		then
> -			displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
> -			say "$(eval_gettext "Entering '\$displaypath'")"
> -			name=$(git submodule--helper name "$sm_path")
> -			(
> -				prefix="$prefix$sm_path/"
> -				sanitize_submodule_env
> -				cd "$sm_path" &&
> -				sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
> -				# we make $path available to scripts ...
> -				path=$sm_path &&
> -				if test $# -eq 1
> -				then
> -					eval "$1"
> -				else
> -					"$@"
> -				fi &&
> -				if test -n "$recursive"
> -				then
> -					cmd_foreach "--recursive" "$@"
> -				fi
> -			) <&3 3<&- ||
> -			die "$(eval_gettext "Stopping at '\$displaypath'; script returned non-zero status.")"
> -		fi
> -	done
>  }
>  
>  #
> -- 
> 2.11.0
> 

-- 
Brandon Williams

[GSoC][PATCH v4 1/2] t7407: test "submodule foreach --recursive" from subdirectory added

[Prathamesh Chavan]

Additional test cases added to the submodule-foreach test suite
to check the submodule foreach --recursive behavior from a
subdirectory as this was missing from the test suite.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---
It was observed that after porting the submodule subcommand to
C, it passed all the test from the existing test-suite.
But since there was some observation made, where the output of
the orignal submodule foreach subcommand wasn't matching to that
of the newly ported function, this test has been added.

After which, it can been seen that the patch fails in test #9
of t7407-submodule-foreach, which is the newly added
test to that suite. The main reason of adding this test
was to bring the behavior of $path for the submodule
foreach --recursive case.

The observation made was as follows:

For a project - super containing dir (not a submodule)
and a submodule sub which contains another submodule
subsub. When we run a command from super/dir:

git submodule foreach "echo \$path-\$sm_path"

actual results:
Entering '../sub'
../sub-../sub
Entering '../sub/subsub'
../subsub-../subsub

ported function's result:
Entering '../sub'
sub-../sub
Entering '../sub/subsub'
subsub-../sub/subsub

This is occurring since in cmd_foreach of git-submodule.sh
when we use to recurse, we call cmd_foreach
and hence the process ran in the same shell.
Because of this, the variable $wt_prefix is set only once
which is at the beginning of the submodule foreach execution.
wt_prefix=$(git rev-parse --show-prefix)

And since sm_path and path are set using $wt_prefix as :
sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
path=$sm_path
It differs with the value of displaypath as well.

This make the value of $path confusing and I also feel it
deviates from its documentation:
$path is the name of the submodule directory relative
to the superproject.

But since in refactoring the code, we wish to maintain the
code in same way, we need to pass wt_prefix on every
recursive call, which may result in complex C code.
Another option could be to first correct the $path value
in git-submodule.sh and then port the updated cmd_foreach.

 t/t7407-submodule-foreach.sh | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6ba5daf42..58a890e31 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -79,7 +79,6 @@ test_expect_success 'test basic "submodule foreach" usage' '
 	) &&
 	test_i18ncmp expect actual
 '
-
 cat >expect <<EOF
 Entering '../sub1'
 $pwd/clone-foo1-../sub1-$sub1sha1
@@ -197,6 +196,40 @@ test_expect_success 'test messages from "foreach --recursive" from subdirectory'
 	test_i18ncmp expect actual
 '
 
+sub1sha1=$(cd clone2/sub1 && git rev-parse HEAD)
+sub2sha1=$(cd clone2/sub2 && git rev-parse HEAD)
+sub3sha1=$(cd clone2/sub3 && git rev-parse HEAD)
+nested1sha1=$(cd clone2/nested1 && git rev-parse HEAD)
+nested2sha1=$(cd clone2/nested1/nested2 && git rev-parse HEAD)
+nested3sha1=$(cd clone2/nested1/nested2/nested3 && git rev-parse HEAD)
+submodulesha1=$(cd clone2/nested1/nested2/nested3/submodule && git rev-parse HEAD)
+
+cat >expect <<EOF
+Entering '../nested1'
+$pwd/clone2-nested1-../nested1-$nested1sha1
+Entering '../nested1/nested2'
+$pwd/clone2/nested1-nested2-../nested2-$nested2sha1
+Entering '../nested1/nested2/nested3'
+$pwd/clone2/nested1/nested2-nested3-../nested3-$nested3sha1
+Entering '../nested1/nested2/nested3/submodule'
+$pwd/clone2/nested1/nested2/nested3-submodule-../submodule-$submodulesha1
+Entering '../sub1'
+$pwd/clone2-foo1-../sub1-$sub1sha1
+Entering '../sub2'
+$pwd/clone2-foo2-../sub2-$sub2sha1
+Entering '../sub3'
+$pwd/clone2-foo3-../sub3-$sub3sha1
+EOF
+
+test_expect_success 'test "submodule foreach --recursive" from subdirectory' '
+	(
+		cd clone2 &&
+		cd untracked &&
+		git submodule foreach --recursive "echo \$toplevel-\$name-\$sm_path-\$sha1" >../../actual
+	) &&
+	test_i18ncmp expect actual
+'
+
 cat > expect <<EOF
 nested1-nested1
 nested2-nested2
-- 
2.11.0

[GSoC][PATCH v4 2/2] submodule: port subcommand foreach from shell to C

[Prathamesh Chavan]

This aims to make git-submodule foreach a builtin. This is the very
first step taken in this direction. Hence, 'foreach' is ported to
submodule--helper, and submodule--helper is called from git-submodule.sh.
The code is split up to have one function to obtain all the list of
submodules. This function acts as the front-end of git-submodule foreach
subcommand. It calls the function for_each_submodule_list, which basically
loops through the list and calls function fn, which in this case is
runcommand_in_submodule. This third function is a calling function that
takes care of running the command in that submodule, and recursively
perform the same when --recursive is flagged.

The first function module_foreach first 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 case of submodule subcommand
foreach is runcommand_in_submodule) is called for each entry.

The third function runcommand_in_submodule, generates a submodule struct sub
for $name, value and then later prepends name=sub->name; and other
value assignment to the env argv_array structure of a child_process.
Also the <command> of submodule-foreach is push to args argv_array
structure and finally, using run_command the commands are executed
using a shell.

The third function also takes care of the recursive flag, by creating
a separate child_process structure and prepending "--super-prefix displaypath",
to the args argv_array structure. Other required arguments and the
input <command> of submodule-foreach is also appended to this argv_array.

The commit 1c4fb136db (submodule foreach: skip eval for more than one
argument, 2013-09-27), which explains that why for the case when argc>1,
we do not use eval. But since in this patch, we are calling the
command in a separate shell itself for all values of argc, this case
is not considered separately.

Both env variable $path and $sm_path were added since both are used in
tests in t7407.

Helped-by: Brandon Williams <bmwill@google.com>
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.

In this new version of patch, a new function
get_submodule_displaypath is introduced, which is the same one
as that in the patch series for porting of submodule subcommand
status. I had to again introduce this in this patch as well as
I am working on two separate branches for parting of each function.
Also, the function for_each_submodule_list repeats for the same
reason.

I have pushed this work on Github at:
https://github.com/pratham-pc/git/commits/foreach

Its build report is available at:
https://travis-ci.org/pratham-pc/git/builds/
Branch: foreach
Build #67

I have also made some changes in git-submodule.sh for correcting
the $path variable. And hence made the corresponding changes in
the new test introduced in t7407-submodule-foreach as well.
I have push this work at:
https://github.com/pratham-pc/git/commits/foreach-bug-fixed

Its build report is available at:
https://travis-ci.org/pratham-pc/git/builds/
Branch: foreach-bug-fixed
Build #66

 builtin/submodule--helper.c | 142 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  39 +-----------
 2 files changed, 143 insertions(+), 38 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 566a5b6a6..4e19beaff 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);
+	}
+}
+
 struct module_list {
 	const struct cache_entry **entries;
 	int alloc, nr;
@@ -331,6 +350,15 @@ 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);
+
+	return;
+}
+
 static void init_submodule(const char *path, const char *prefix, int quiet)
 {
 	const struct submodule *sub;
@@ -487,6 +515,119 @@ static int module_name(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct cb_foreach {
+	int argc;
+	const char **argv;
+	const char *prefix;
+	unsigned int quiet: 1;
+	unsigned int recursive: 1;
+};
+#define CB_FOREACH_INIT { 0, NULL, 0, 0 }
+
+static void runcommand_in_submodule(const struct cache_entry *list_item, void *cb_data)
+{
+	struct cb_foreach *info = cb_data;
+	char *toplevel = xgetcwd();
+	const struct submodule *sub;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	char* displaypath = NULL;
+	int i;
+
+	/* Only loads from .gitmodules, no overlay with .git/config */
+	gitmodules_config();
+
+	displaypath = get_submodule_displaypath(list_item->name, info->prefix);
+
+	sub = submodule_from_path(null_sha1, list_item->name);
+
+	if (!sub)
+		die(_("No url found for submodule path '%s' in .gitmodules"),
+		      displaypath);
+
+	prepare_submodule_repo_env(&cp.env_array);
+	cp.use_shell = 1;
+	cp.dir = list_item->name;
+
+	argv_array_pushf(&cp.env_array, "name=%s", sub->name);
+	argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
+	argv_array_pushf(&cp.env_array, "path=%s", list_item->name);
+	argv_array_pushf(&cp.env_array, "sha1=%s", oid_to_hex(&list_item->oid));
+	argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
+
+	for (i = 0; i < info->argc; i++)
+		argv_array_push(&cp.args, info->argv[i]);
+
+	if (!is_submodule_populated_gently(list_item->name, NULL))
+		return;
+
+	if (!info->quiet)
+		printf(_("Entering '%s'\n"), displaypath);
+	if (info->argv[0] && run_command(&cp))
+		die(_("run_command returned non-zero status for %s\n."), displaypath);
+
+	if (info->recursive) {
+		struct child_process cpr = CHILD_PROCESS_INIT;
+
+		cpr.use_shell = 1;
+		cpr.dir = list_item->name;
+		prepare_submodule_repo_env(&cpr.env_array);
+
+		argv_array_pushl(&cpr.args, "git", "--super-prefix", displaypath,
+				 "submodule--helper", "foreach", "--recursive", NULL);
+
+		if (info->quiet)
+			argv_array_push(&cpr.args, "--quiet");
+
+		for (i = 0; i < info->argc; i++)
+			argv_array_push(&cpr.args, info->argv[i]);
+
+		if (run_command(&cpr))
+			die(_("run_command returned non-zero status while \
+			      recuring in the nested submodules of %s\n."),
+			      displaypath);
+	}
+
+	free(displaypath);
+	free(toplevel);
+}
+
+static int module_foreach(int argc, const char **argv, const char *prefix)
+{
+	struct cb_foreach info = CB_FOREACH_INIT;
+	struct pathspec pathspec;
+	struct module_list list = MODULE_LIST_INIT;
+	int quiet = 0;
+	int recursive = 0;
+
+	struct option module_foreach_options[] = {
+		OPT__QUIET(&quiet, N_("Suppress output of entering each submodule command")),
+		OPT_BOOL(0, "recursive", &recursive,
+			 N_("Recurse into nested submodules")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper foreach [--quiet] [--recursive] <command>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_foreach_options,
+			     git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);
+
+	if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)
+		die("BUG: module_list_compute should not choke on empty pathspec");
+
+	info.argc = argc;
+	info.argv = argv;
+	info.prefix = prefix;
+	info.quiet = !!quiet;
+	info.recursive = !!recursive;
+
+	for_each_submodule_list(list, runcommand_in_submodule, &info);
+
+	return 0;
+}
+
 static int clone_submodule(const char *path, const char *gitdir, const char *url,
 			   const char *depth, struct string_list *reference,
 			   int quiet, int progress)
@@ -1212,6 +1353,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},
+	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
 	{"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..032fd2540 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -322,45 +322,8 @@ cmd_foreach()
 		shift
 	done
 
-	toplevel=$(pwd)
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
 
-	# dup stdin so that it can be restored when running the external
-	# command in the subshell (and a recursive call to this function)
-	exec 3<&0
-
-	{
-		git submodule--helper list --prefix "$wt_prefix" ||
-		echo "#unmatched" $?
-	} |
-	while read -r mode sha1 stage sm_path
-	do
-		die_if_unmatched "$mode" "$sha1"
-		if test -e "$sm_path"/.git
-		then
-			displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
-			say "$(eval_gettext "Entering '\$displaypath'")"
-			name=$(git submodule--helper name "$sm_path")
-			(
-				prefix="$prefix$sm_path/"
-				sanitize_submodule_env
-				cd "$sm_path" &&
-				sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
-				# we make $path available to scripts ...
-				path=$sm_path &&
-				if test $# -eq 1
-				then
-					eval "$1"
-				else
-					"$@"
-				fi &&
-				if test -n "$recursive"
-				then
-					cmd_foreach "--recursive" "$@"
-				fi
-			) <&3 3<&- ||
-			die "$(eval_gettext "Stopping at '\$displaypath'; script returned non-zero status.")"
-		fi
-	done
 }
 
 #
-- 
2.11.0

Re: [GSoC][PATCH v4 2/2] submodule: port subcommand foreach from shell to C

[Stefan Beller]

On Sun, May 21, 2017 at 5:58 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:

> I have also made some changes in git-submodule.sh for correcting
> the $path variable. And hence made the corresponding changes in
> the new test introduced in t7407-submodule-foreach as well.
> I have push this work at:
> https://github.com/pratham-pc/git/commits/foreach-bug-fixed

This one seems to pass the test suite by having the bug fixed.
(The patches posted here seems to be
https://github.com/pratham-pc/git/commits/foreach
which does not pass tests? These two series seem to only differ in
the bug fix commit, which I think is a good idea to include, as then we
have a bug fixed and the tests pass.)

> +static void for_each_submodule_list(const struct module_list list, submodule_list_func_t fn, void *cb_data)
..
> +       return;

no need for an explicit return in a void function.

> +struct cb_foreach {
> +       int argc;
> +       const char **argv;
> +       const char *prefix;
> +       unsigned int quiet: 1;
> +       unsigned int recursive: 1;
> +};
> +#define CB_FOREACH_INIT { 0, NULL, 0, 0 }

This static initializer doesn't quite match the struct,
(I would expect two NULLs as we have two const char pointers).

> +
> +       info.argc = argc;
> +       info.argv = argv;
> +       info.prefix = prefix;
> +       info.quiet = !!quiet;
> +       info.recursive = !!recursive;

as you assign all fields of the struct yourself, you could also omit the
static initialization via _INIT above.


Apart from these two minor nits the code looks good to me.
However we'd really want to have the bug fix patch as well.
(At the time of submission of a patch we should not be aware
of any tests failing, which we are without said bug fix patch)

Thanks,
Stefan

Re: [GSoC][PATCH v4 1/2] t7407: test "submodule foreach --recursive" from subdirectory added

[Brandon Williams]

On 05/21, Prathamesh Chavan wrote:
> Additional test cases added to the submodule-foreach test suite
> to check the submodule foreach --recursive behavior from a
> subdirectory as this was missing from the test suite.
> 
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
> It was observed that after porting the submodule subcommand to
> C, it passed all the test from the existing test-suite.
> But since there was some observation made, where the output of
> the orignal submodule foreach subcommand wasn't matching to that
> of the newly ported function, this test has been added.
> 
> After which, it can been seen that the patch fails in test #9
> of t7407-submodule-foreach, which is the newly added
> test to that suite. The main reason of adding this test
> was to bring the behavior of $path for the submodule
> foreach --recursive case.
> 
> The observation made was as follows:
> 
> For a project - super containing dir (not a submodule)
> and a submodule sub which contains another submodule
> subsub. When we run a command from super/dir:
> 
> git submodule foreach "echo \$path-\$sm_path"
> 
> actual results:
> Entering '../sub'
> ../sub-../sub
> Entering '../sub/subsub'
> ../subsub-../subsub
> 
> ported function's result:
> Entering '../sub'
> sub-../sub
> Entering '../sub/subsub'
> subsub-../sub/subsub
> 
> This is occurring since in cmd_foreach of git-submodule.sh
> when we use to recurse, we call cmd_foreach
> and hence the process ran in the same shell.
> Because of this, the variable $wt_prefix is set only once
> which is at the beginning of the submodule foreach execution.
> wt_prefix=$(git rev-parse --show-prefix)
> 
> And since sm_path and path are set using $wt_prefix as :
> sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
> path=$sm_path
> It differs with the value of displaypath as well.
> 
> This make the value of $path confusing and I also feel it
> deviates from its documentation:
> $path is the name of the submodule directory relative
> to the superproject.
> 
> But since in refactoring the code, we wish to maintain the
> code in same way, we need to pass wt_prefix on every
> recursive call, which may result in complex C code.
> Another option could be to first correct the $path value
> in git-submodule.sh and then port the updated cmd_foreach.
> 
>  t/t7407-submodule-foreach.sh | 35 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
> index 6ba5daf42..58a890e31 100755
> --- a/t/t7407-submodule-foreach.sh
> +++ b/t/t7407-submodule-foreach.sh
> @@ -79,7 +79,6 @@ test_expect_success 'test basic "submodule foreach" usage' '
>  	) &&
>  	test_i18ncmp expect actual
>  '
> -

The removal of this line seems unrelated to the rest of this patch.  Was
this intended?

>  cat >expect <<EOF
>  Entering '../sub1'
>  $pwd/clone-foo1-../sub1-$sub1sha1
> @@ -197,6 +196,40 @@ test_expect_success 'test messages from "foreach --recursive" from subdirectory'
>  	test_i18ncmp expect actual
>  '
>  
> +sub1sha1=$(cd clone2/sub1 && git rev-parse HEAD)
> +sub2sha1=$(cd clone2/sub2 && git rev-parse HEAD)
> +sub3sha1=$(cd clone2/sub3 && git rev-parse HEAD)
> +nested1sha1=$(cd clone2/nested1 && git rev-parse HEAD)
> +nested2sha1=$(cd clone2/nested1/nested2 && git rev-parse HEAD)
> +nested3sha1=$(cd clone2/nested1/nested2/nested3 && git rev-parse HEAD)
> +submodulesha1=$(cd clone2/nested1/nested2/nested3/submodule && git rev-parse HEAD)
> +
> +cat >expect <<EOF
> +Entering '../nested1'
> +$pwd/clone2-nested1-../nested1-$nested1sha1
> +Entering '../nested1/nested2'
> +$pwd/clone2/nested1-nested2-../nested2-$nested2sha1
> +Entering '../nested1/nested2/nested3'
> +$pwd/clone2/nested1/nested2-nested3-../nested3-$nested3sha1
> +Entering '../nested1/nested2/nested3/submodule'
> +$pwd/clone2/nested1/nested2/nested3-submodule-../submodule-$submodulesha1
> +Entering '../sub1'
> +$pwd/clone2-foo1-../sub1-$sub1sha1
> +Entering '../sub2'
> +$pwd/clone2-foo2-../sub2-$sub2sha1
> +Entering '../sub3'
> +$pwd/clone2-foo3-../sub3-$sub3sha1
> +EOF
> +
> +test_expect_success 'test "submodule foreach --recursive" from subdirectory' '
> +	(
> +		cd clone2 &&
> +		cd untracked &&
> +		git submodule foreach --recursive "echo \$toplevel-\$name-\$sm_path-\$sha1" >../../actual
> +	) &&
> +	test_i18ncmp expect actual
> +'
> +
>  cat > expect <<EOF
>  nested1-nested1
>  nested2-nested2
> -- 
> 2.11.0
> 

-- 
Brandon Williams

Re: [GSoC][PATCH v4 2/2] submodule: port subcommand foreach from shell to C

[Brandon Williams]

On 05/22, Stefan Beller wrote:
> On Sun, May 21, 2017 at 5:58 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> 
> > I have also made some changes in git-submodule.sh for correcting
> > the $path variable. And hence made the corresponding changes in
> > the new test introduced in t7407-submodule-foreach as well.
> > I have push this work at:
> > https://github.com/pratham-pc/git/commits/foreach-bug-fixed
> 
> This one seems to pass the test suite by having the bug fixed.
> (The patches posted here seems to be
> https://github.com/pratham-pc/git/commits/foreach
> which does not pass tests? These two series seem to only differ in
> the bug fix commit, which I think is a good idea to include, as then we
> have a bug fixed and the tests pass.)
> 
> > +static void for_each_submodule_list(const struct module_list list, submodule_list_func_t fn, void *cb_data)
> ..
> > +       return;
> 
> no need for an explicit return in a void function.
> 
> > +struct cb_foreach {
> > +       int argc;
> > +       const char **argv;
> > +       const char *prefix;
> > +       unsigned int quiet: 1;
> > +       unsigned int recursive: 1;
> > +};
> > +#define CB_FOREACH_INIT { 0, NULL, 0, 0 }
> 
> This static initializer doesn't quite match the struct,
> (I would expect two NULLs as we have two const char pointers).

If we ever move to a new version of C, these initializers would be much
more readable as we could assign values to the fields themselves.  But
that is unrelated to this change.

> 
> > +
> > +       info.argc = argc;
> > +       info.argv = argv;
> > +       info.prefix = prefix;
> > +       info.quiet = !!quiet;
> > +       info.recursive = !!recursive;
> 
> as you assign all fields of the struct yourself, you could also omit the
> static initialization via _INIT above.
> 
> 
> Apart from these two minor nits the code looks good to me.
> However we'd really want to have the bug fix patch as well.
> (At the time of submission of a patch we should not be aware
> of any tests failing, which we are without said bug fix patch)
> 
> Thanks,
> Stefan

-- 
Brandon Williams

Re: [GSoC][PATCH v4 2/2] submodule: port subcommand foreach from shell to C

[Brandon Williams]

On 05/21, Prathamesh Chavan wrote:
> This aims to make git-submodule foreach a builtin. This is the very
> first step taken in this direction. Hence, 'foreach' is ported to
> submodule--helper, and submodule--helper is called from git-submodule.sh.
> The code is split up to have one function to obtain all the list of
> submodules. This function acts as the front-end of git-submodule foreach
> subcommand. It calls the function for_each_submodule_list, which basically
> loops through the list and calls function fn, which in this case is
> runcommand_in_submodule. This third function is a calling function that
> takes care of running the command in that submodule, and recursively
> perform the same when --recursive is flagged.
> 
> The first function module_foreach first 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 case of submodule subcommand
> foreach is runcommand_in_submodule) is called for each entry.
> 
> The third function runcommand_in_submodule, generates a submodule struct sub
> for $name, value and then later prepends name=sub->name; and other
> value assignment to the env argv_array structure of a child_process.
> Also the <command> of submodule-foreach is push to args argv_array
> structure and finally, using run_command the commands are executed
> using a shell.
> 
> The third function also takes care of the recursive flag, by creating
> a separate child_process structure and prepending "--super-prefix displaypath",
> to the args argv_array structure. Other required arguments and the
> input <command> of submodule-foreach is also appended to this argv_array.
> 
> The commit 1c4fb136db (submodule foreach: skip eval for more than one
> argument, 2013-09-27), which explains that why for the case when argc>1,
> we do not use eval. But since in this patch, we are calling the
> command in a separate shell itself for all values of argc, this case
> is not considered separately.
> 
> Both env variable $path and $sm_path were added since both are used in
> tests in t7407.
> 
> Helped-by: Brandon Williams <bmwill@google.com>
> 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.
> 
> In this new version of patch, a new function
> get_submodule_displaypath is introduced, which is the same one
> as that in the patch series for porting of submodule subcommand
> status. I had to again introduce this in this patch as well as
> I am working on two separate branches for parting of each function.
> Also, the function for_each_submodule_list repeats for the same
> reason.
> 
> I have pushed this work on Github at:
> https://github.com/pratham-pc/git/commits/foreach
> 
> Its build report is available at:
> https://travis-ci.org/pratham-pc/git/builds/
> Branch: foreach
> Build #67
> 
> I have also made some changes in git-submodule.sh for correcting
> the $path variable. And hence made the corresponding changes in
> the new test introduced in t7407-submodule-foreach as well.
> I have push this work at:
> https://github.com/pratham-pc/git/commits/foreach-bug-fixed
> 
> Its build report is available at:
> https://travis-ci.org/pratham-pc/git/builds/
> Branch: foreach-bug-fixed
> Build #66
> 
>  builtin/submodule--helper.c | 142 ++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            |  39 +-----------
>  2 files changed, 143 insertions(+), 38 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 566a5b6a6..4e19beaff 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));

You have a potential memory leak here, you need to release the strbuf
before returning.

> +	} else if (super_prefix) {
> +		return xstrfmt("%s/%s", super_prefix, path);
> +	} else {
> +		return xstrdup(path);
> +	}
> +}
> +
>  struct module_list {
>  	const struct cache_entry **entries;
>  	int alloc, nr;
> @@ -331,6 +350,15 @@ 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)

nit: You could probably break this line so its not longer than 80 chars.

> +{
> +	int i;
> +	for (i = 0; i < list.nr; i++)
> +		fn(list.entries[i], cb_data);
> +
> +	return;

No return needed.

> +}

small nit, and not that important, but could this function potentially
be moved closer to where it is used? What as the rational for placing it
here?

> +
>  static void init_submodule(const char *path, const char *prefix, int quiet)
>  {
>  	const struct submodule *sub;
> @@ -487,6 +515,119 @@ static int module_name(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +struct cb_foreach {
> +	int argc;
> +	const char **argv;
> +	const char *prefix;
> +	unsigned int quiet: 1;
> +	unsigned int recursive: 1;
> +};
> +#define CB_FOREACH_INIT { 0, NULL, 0, 0 }

Need an extra NULL as Stefan pointed out:
  { 0, NULL, NULL, 0, 0 }

> +
> +static void runcommand_in_submodule(const struct cache_entry *list_item, void *cb_data)
> +{
> +	struct cb_foreach *info = cb_data;
> +	char *toplevel = xgetcwd();
> +	const struct submodule *sub;
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	char* displaypath = NULL;
> +	int i;
> +
> +	/* Only loads from .gitmodules, no overlay with .git/config */
> +	gitmodules_config();
> +
> +	displaypath = get_submodule_displaypath(list_item->name, info->prefix);
> +
> +	sub = submodule_from_path(null_sha1, list_item->name);
> +
> +	if (!sub)
> +		die(_("No url found for submodule path '%s' in .gitmodules"),
> +		      displaypath);
> +
> +	prepare_submodule_repo_env(&cp.env_array);
> +	cp.use_shell = 1;
> +	cp.dir = list_item->name;
> +
> +	argv_array_pushf(&cp.env_array, "name=%s", sub->name);
> +	argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
> +	argv_array_pushf(&cp.env_array, "path=%s", list_item->name);
> +	argv_array_pushf(&cp.env_array, "sha1=%s", oid_to_hex(&list_item->oid));
> +	argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
> +
> +	for (i = 0; i < info->argc; i++)
> +		argv_array_push(&cp.args, info->argv[i]);
> +
> +	if (!is_submodule_populated_gently(list_item->name, NULL))
> +		return;

This check needs to be hoisted up probably before calculating the
display path, otherwise you have a bunch of memory leaks that need to be
plugged. Something like:

    +	sub = submodule_from_path(null_sha1, list_item->name);
    +
    +	if (!sub)
    +		die(_("No url found for submodule path '%s' in .gitmodules"),
    +		      displaypath);
    +
    +	if (!is_submodule_populated_gently(list_item->name, NULL))
    +		return;
    +
    +	displaypath = get_submodule_displaypath(list_item->name, info->prefix);

> +
> +	if (!info->quiet)
> +		printf(_("Entering '%s'\n"), displaypath);
> +	if (info->argv[0] && run_command(&cp))
> +		die(_("run_command returned non-zero status for %s\n."), displaypath);
> +
> +	if (info->recursive) {
> +		struct child_process cpr = CHILD_PROCESS_INIT;
> +
> +		cpr.use_shell = 1;

You can set .git_cmd = 1 instead.

> +		cpr.dir = list_item->name;
> +		prepare_submodule_repo_env(&cpr.env_array);
> +
> +		argv_array_pushl(&cpr.args, "git", "--super-prefix", displaypath,

And then you don't need to include "git" here.

> +				 "submodule--helper", "foreach", "--recursive", NULL);
> +
> +		if (info->quiet)
> +			argv_array_push(&cpr.args, "--quiet");
> +
> +		for (i = 0; i < info->argc; i++)
> +			argv_array_push(&cpr.args, info->argv[i]);
> +
> +		if (run_command(&cpr))
> +			die(_("run_command returned non-zero status while \
> +			      recuring in the nested submodules of %s\n."),

If you're going to split these two lines up then it may make more sense
to use concatenation instead of a continuation '\'.  I'm not sure how
the spaces at the beginning of the line will look when printed.
Something like this:

    +			die(_("run_command returned non-zero status while"
    +			      "recursing in the nested submodules of %s\n."),


Also s/recuring/recursing

> +			      displaypath);
> +	}
> +
> +	free(displaypath);
> +	free(toplevel);
> +}
> +
> +static int module_foreach(int argc, const char **argv, const char *prefix)
> +{
> +	struct cb_foreach info = CB_FOREACH_INIT;
> +	struct pathspec pathspec;
> +	struct module_list list = MODULE_LIST_INIT;
> +	int quiet = 0;
> +	int recursive = 0;
> +
> +	struct option module_foreach_options[] = {
> +		OPT__QUIET(&quiet, N_("Suppress output of entering each submodule command")),
> +		OPT_BOOL(0, "recursive", &recursive,
> +			 N_("Recurse into nested submodules")),
> +		OPT_END()
> +	};
> +
> +	const char *const git_submodule_helper_usage[] = {
> +		N_("git submodule--helper foreach [--quiet] [--recursive] <command>"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, module_foreach_options,
> +			     git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);
> +
> +	if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)
> +		die("BUG: module_list_compute should not choke on empty pathspec");

You mentioned using the 'BUG()' function call, and you use it up above,
so why not use it here too.

> +
> +	info.argc = argc;
> +	info.argv = argv;
> +	info.prefix = prefix;
> +	info.quiet = !!quiet;
> +	info.recursive = !!recursive;

If these values are boolean why do we need to do the extra '!!'?

> +
> +	for_each_submodule_list(list, runcommand_in_submodule, &info);
> +
> +	return 0;
> +}
> +
>  static int clone_submodule(const char *path, const char *gitdir, const char *url,
>  			   const char *depth, struct string_list *reference,
>  			   int quiet, int progress)
> @@ -1212,6 +1353,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},
> +	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
>  	{"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..032fd2540 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -322,45 +322,8 @@ cmd_foreach()
>  		shift
>  	done
>  
> -	toplevel=$(pwd)
> +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
>  
> -	# dup stdin so that it can be restored when running the external
> -	# command in the subshell (and a recursive call to this function)
> -	exec 3<&0
> -
> -	{
> -		git submodule--helper list --prefix "$wt_prefix" ||
> -		echo "#unmatched" $?
> -	} |
> -	while read -r mode sha1 stage sm_path
> -	do
> -		die_if_unmatched "$mode" "$sha1"
> -		if test -e "$sm_path"/.git
> -		then
> -			displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
> -			say "$(eval_gettext "Entering '\$displaypath'")"
> -			name=$(git submodule--helper name "$sm_path")
> -			(
> -				prefix="$prefix$sm_path/"
> -				sanitize_submodule_env
> -				cd "$sm_path" &&
> -				sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
> -				# we make $path available to scripts ...
> -				path=$sm_path &&
> -				if test $# -eq 1
> -				then
> -					eval "$1"
> -				else
> -					"$@"
> -				fi &&
> -				if test -n "$recursive"
> -				then
> -					cmd_foreach "--recursive" "$@"
> -				fi
> -			) <&3 3<&- ||
> -			die "$(eval_gettext "Stopping at '\$displaypath'; script returned non-zero status.")"
> -		fi
> -	done
>  }
>  
>  #
> -- 
> 2.11.0
> 

-- 
Brandon Williams

Re: [GSoC][PATCH v4 2/2] submodule: port subcommand foreach from shell to C

[Stefan Beller]

On Tue, May 23, 2017 at 12:36 PM, Brandon Williams <bmwill@google.com> wrote:
>
> You can set .git_cmd = 1 instead.
>
>> +             cpr.dir = list_item->name;
>> +             prepare_submodule_repo_env(&cpr.env_array);
>> +
>> +             argv_array_pushl(&cpr.args, "git", "--super-prefix", displaypath,
>
> And then you don't need to include "git" here.

even if git_cmd = 1 is set, you'd need a first dummy argument?
cf. find_unpushed_submodules, See comment in 9cfa1c260f
(serialize collection of refs that contain submodule changes, 2016-11-16)

>> +
>> +     info.argc = argc;
>> +     info.argv = argv;
>> +     info.prefix = prefix;
>> +     info.quiet = !!quiet;
>> +     info.recursive = !!recursive;
>
> If these values are boolean why do we need to do the extra '!!'?

Actually that was my advice. As we only have a limited space in a single
bit, strange things happen when you were to do:

    quiet = 2; /* be extra quiet */
    info.quiet = quiet;

This is not the case here, but other commands have evolved over time
to first take a OPT_BOOL, and then in a later patch an OPT_INT.
(some commands take a "-v -v -v")

And by having the double negative we'd have some defensive programming
right here. (To prove I am not telling crazy stories, $ git log -S \!\!)

Re: [GSoC][PATCH v4 2/2] submodule: port subcommand foreach from shell to C

[Brandon Williams]

On 05/23, Stefan Beller wrote:
> On Tue, May 23, 2017 at 12:36 PM, Brandon Williams <bmwill@google.com> wrote:
> >
> > You can set .git_cmd = 1 instead.
> >
> >> +             cpr.dir = list_item->name;
> >> +             prepare_submodule_repo_env(&cpr.env_array);
> >> +
> >> +             argv_array_pushl(&cpr.args, "git", "--super-prefix", displaypath,
> >
> > And then you don't need to include "git" here.
> 
> even if git_cmd = 1 is set, you'd need a first dummy argument?
> cf. find_unpushed_submodules, See comment in 9cfa1c260f
> (serialize collection of refs that contain submodule changes, 2016-11-16)

Different subsystem, you don't need a dummy first argument.  The
revision walking code does (for some reason) need a dummy first
argument.

> 
> >> +
> >> +     info.argc = argc;
> >> +     info.argv = argv;
> >> +     info.prefix = prefix;
> >> +     info.quiet = !!quiet;
> >> +     info.recursive = !!recursive;
> >
> > If these values are boolean why do we need to do the extra '!!'?
> 
> Actually that was my advice. As we only have a limited space in a single
> bit, strange things happen when you were to do:
> 
>     quiet = 2; /* be extra quiet */
>     info.quiet = quiet;
> 
> This is not the case here, but other commands have evolved over time
> to first take a OPT_BOOL, and then in a later patch an OPT_INT.
> (some commands take a "-v -v -v")
> 
> And by having the double negative we'd have some defensive programming
> right here. (To prove I am not telling crazy stories, $ git log -S \!\!)

All good, I didn't notice that they were bit fields.

-- 
Brandon Williams

[GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Prathamesh Chavan]

According to the documentation about git-submodule foreach subcommand's
$path variable:
$path is the name of the submodule directory relative to the superproject

But it was observed when the value of the $path value deviates from this
for the nested submodules when the <command> is run from a subdirectory.
This patch aims for its correction.

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.

The observation made was as follows:
For a project - super containing dir (not a submodule) and a submodule sub 
which contains another submodule subsub. When we run a command from super/dir:

git submodule foreach "echo \$path-\$sm_path"

actual results:
Entering '../sub'
../sub-../sub
Entering '../sub/subsub'
../subsub-../subsub

expected result wrt documentation and current test suite:
Entering '../sub'
sub-../sub
Entering '../sub/subsub'
subsub-../sub/subsub

This make the value of $path confusing and I also feel it deviates from its 
documentation:
$path is the name of the submodule directory relative to the superproject.
Hence, this patch corrects the value assigned to the $path and $sm_path.

 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index c0d0e9a4c..ea6f56337 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -344,9 +344,9 @@ cmd_foreach()
 				prefix="$prefix$sm_path/"
 				sanitize_submodule_env
 				cd "$sm_path" &&
-				sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
 				# we make $path available to scripts ...
 				path=$sm_path &&
+				sm_path=$displaypath &&
 				if test $# -eq 1
 				then
 					eval "$1"
-- 
2.11.0

[GSoC][PATCH v5 2/3] t7407: test "submodule foreach --recursive" from subdirectory added

[Prathamesh Chavan]

Additional test cases added to the submodule-foreach test suite
to check the submodule foreach --recursive behavior from a
subdirectory as this was missing from the test suite.

Helped-by: Brandon Williams <bmwill@google.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---
Additional test added to check the bug fixed in the [PATCH v5 1/3] of
this patch series.

 t/t7407-submodule-foreach.sh | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6ba5daf42..1c8d132d8 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -197,6 +197,40 @@ test_expect_success 'test messages from "foreach --recursive" from subdirectory'
 	test_i18ncmp expect actual
 '
 
+sub1sha1=$(cd clone2/sub1 && git rev-parse HEAD)
+sub2sha1=$(cd clone2/sub2 && git rev-parse HEAD)
+sub3sha1=$(cd clone2/sub3 && git rev-parse HEAD)
+nested1sha1=$(cd clone2/nested1 && git rev-parse HEAD)
+nested2sha1=$(cd clone2/nested1/nested2 && git rev-parse HEAD)
+nested3sha1=$(cd clone2/nested1/nested2/nested3 && git rev-parse HEAD)
+submodulesha1=$(cd clone2/nested1/nested2/nested3/submodule && git rev-parse HEAD)
+
+cat >expect <<EOF
+Entering '../nested1'
+$pwd/clone2-nested1-../nested1-$nested1sha1
+Entering '../nested1/nested2'
+$pwd/clone2/nested1-nested2-../nested1/nested2-$nested2sha1
+Entering '../nested1/nested2/nested3'
+$pwd/clone2/nested1/nested2-nested3-../nested1/nested2/nested3-$nested3sha1
+Entering '../nested1/nested2/nested3/submodule'
+$pwd/clone2/nested1/nested2/nested3-submodule-../nested1/nested2/nested3/submodule-$submodulesha1
+Entering '../sub1'
+$pwd/clone2-foo1-../sub1-$sub1sha1
+Entering '../sub2'
+$pwd/clone2-foo2-../sub2-$sub2sha1
+Entering '../sub3'
+$pwd/clone2-foo3-../sub3-$sub3sha1
+EOF
+
+test_expect_success 'test "submodule foreach --recursive" from subdirectory' '
+	(
+		cd clone2 &&
+		cd untracked &&
+		git submodule foreach --recursive "echo \$toplevel-\$name-\$sm_path-\$sha1" >../../actual
+	) &&
+	test_i18ncmp expect actual
+'
+
 cat > expect <<EOF
 nested1-nested1
 nested2-nested2
-- 
2.11.0

[GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C

[Prathamesh Chavan]

This aims to make git-submodule foreach a builtin. This is the very
first step taken in this direction. Hence, 'foreach' is ported to
submodule--helper, and submodule--helper is called from git-submodule.sh.
The code is split up to have one function to obtain all the list of
submodules. This function acts as the front-end of git-submodule foreach
subcommand. It calls the function for_each_submodule_list, which basically
loops through the list and calls function fn, which in this case is
runcommand_in_submodule. This third function is a calling function that
takes care of running the command in that submodule, and recursively
perform the same when --recursive is flagged.

The first function module_foreach first 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 case of submodule subcommand
foreach is runcommand_in_submodule) is called for each entry.

The third function runcommand_in_submodule, generates a submodule struct sub
for $name, value and then later prepends name=sub->name; and other
value assignment to the env argv_array structure of a child_process.
Also the <command> of submodule-foreach is push to args argv_array
structure and finally, using run_command the commands are executed
using a shell.

The third function also takes care of the recursive flag, by creating
a separate child_process structure and prepending "--super-prefix displaypath",
to the args argv_array structure. Other required arguments and the
input <command> of submodule-foreach is also appended to this argv_array.

The commit 1c4fb136db (submodule foreach: skip eval for more than one
argument, 2013-09-27), which explains that why for the case when argc>1,
we do not use eval. But since in this patch, we are calling the
command in a separate shell itself for all values of argc, this case
is not considered separately.

Both env variable $path and $sm_path were added since both are used in
tests in t7407.

Helped-by: Brandon Williams <bmwill@google.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---
These series of patches passes the complete test suite.
Its build report is available at:
https://travis-ci.org/pratham-pc/git/builds
Branch: submodule-foreach
Build #71

 builtin/submodule--helper.c | 148 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  39 +-----------
 2 files changed, 149 insertions(+), 38 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 566a5b6a6..343b6269c 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,26 @@ 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;
+		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);
+	}
+}
+
 struct module_list {
 	const struct cache_entry **entries;
 	int alloc, nr;
@@ -331,6 +353,14 @@ 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;
@@ -487,6 +517,123 @@ static int module_name(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct cb_foreach {
+	int argc;
+	const char **argv;
+	const char *prefix;
+	unsigned int quiet: 1;
+	unsigned int recursive: 1;
+};
+#define CB_FOREACH_INIT { 0, NULL, NULL, 0, 0 }
+
+static void runcommand_in_submodule(const struct cache_entry *list_item,
+				    void *cb_data)
+{
+	struct cb_foreach *info = cb_data;
+	char *toplevel = xgetcwd();
+	const struct submodule *sub;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	char* displaypath = NULL;
+	int i;
+
+	/* Only loads from .gitmodules, no overlay with .git/config */
+	gitmodules_config();
+
+	sub = submodule_from_path(null_sha1, list_item->name);
+
+	if (!sub)
+		die(_("No url found for submodule path '%s' in .gitmodules"),
+		      displaypath);
+
+	if (!is_submodule_populated_gently(list_item->name, NULL))
+		return;
+
+	displaypath = get_submodule_displaypath(list_item->name, info->prefix);
+
+	prepare_submodule_repo_env(&cp.env_array);
+	cp.use_shell = 1;
+	cp.dir = list_item->name;
+
+	argv_array_pushf(&cp.env_array, "name=%s", sub->name);
+	argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
+	argv_array_pushf(&cp.env_array, "path=%s", list_item->name);
+	argv_array_pushf(&cp.env_array, "sha1=%s", oid_to_hex(&list_item->oid));
+	argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
+
+	for (i = 0; i < info->argc; i++)
+		argv_array_push(&cp.args, info->argv[i]);
+
+	if (!info->quiet)
+		printf(_("Entering '%s'\n"), displaypath);
+
+	if (info->argv[0] && run_command(&cp))
+		die(_("run_command returned non-zero status for %s\n."),
+		      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", "foreach", "--recursive",
+				 NULL);
+
+		if (info->quiet)
+			argv_array_push(&cpr.args, "--quiet");
+
+		for (i = 0; i < info->argc; i++)
+			argv_array_push(&cpr.args, info->argv[i]);
+
+		if (run_command(&cpr))
+			die(_("run_command returned non-zero status while"
+			      "recursing in the nested submodules of %s\n."),
+			      displaypath);
+	}
+
+	free(displaypath);
+	free(toplevel);
+}
+
+static int module_foreach(int argc, const char **argv, const char *prefix)
+{
+	struct cb_foreach info;
+	struct pathspec pathspec;
+	struct module_list list = MODULE_LIST_INIT;
+	int quiet = 0;
+	int recursive = 0;
+
+	struct option module_foreach_options[] = {
+		OPT__QUIET(&quiet, N_("Suppress output of entering each submodule command")),
+		OPT_BOOL(0, "recursive", &recursive,
+			 N_("Recurse into nested submodules")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper foreach [--quiet] [--recursive] <command>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_foreach_options,
+			     git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);
+
+	if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)
+		BUG("module_list_compute should not choke on empty pathspec");
+
+	info.argc = argc;
+	info.argv = argv;
+	info.prefix = prefix;
+	info.quiet = quiet;
+	info.recursive = recursive;
+
+	for_each_submodule_list(list, runcommand_in_submodule, &info);
+
+	return 0;
+}
+
 static int clone_submodule(const char *path, const char *gitdir, const char *url,
 			   const char *depth, struct string_list *reference,
 			   int quiet, int progress)
@@ -1212,6 +1359,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},
+	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
 	{"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 ea6f56337..032fd2540 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -322,45 +322,8 @@ cmd_foreach()
 		shift
 	done
 
-	toplevel=$(pwd)
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
 
-	# dup stdin so that it can be restored when running the external
-	# command in the subshell (and a recursive call to this function)
-	exec 3<&0
-
-	{
-		git submodule--helper list --prefix "$wt_prefix" ||
-		echo "#unmatched" $?
-	} |
-	while read -r mode sha1 stage sm_path
-	do
-		die_if_unmatched "$mode" "$sha1"
-		if test -e "$sm_path"/.git
-		then
-			displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
-			say "$(eval_gettext "Entering '\$displaypath'")"
-			name=$(git submodule--helper name "$sm_path")
-			(
-				prefix="$prefix$sm_path/"
-				sanitize_submodule_env
-				cd "$sm_path" &&
-				# we make $path available to scripts ...
-				path=$sm_path &&
-				sm_path=$displaypath &&
-				if test $# -eq 1
-				then
-					eval "$1"
-				else
-					"$@"
-				fi &&
-				if test -n "$recursive"
-				then
-					cmd_foreach "--recursive" "$@"
-				fi
-			) <&3 3<&- ||
-			die "$(eval_gettext "Stopping at '\$displaypath'; script returned non-zero status.")"
-		fi
-	done
 }
 
 #
-- 
2.11.0

Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C

[Stefan Beller]

On Fri, May 26, 2017 at 8:17 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> This aims to make git-submodule foreach a builtin.

Cool. I reviewed the code and only have one minor nit.

> +static void runcommand_in_submodule(const struct cache_entry *list_item,
> +                                   void *cb_data)
> +{

> +       /* Only loads from .gitmodules, no overlay with .git/config */
> +       gitmodules_config();

Performance nit: We only need to load the gitmodules file once instead
of foreach submodule separately, so we could move this to module_foreach().

Thanks,
Stefan

Re: [GSoC][PATCH v5 2/3] t7407: test "submodule foreach --recursive" from subdirectory added

[Stefan Beller]

On Fri, May 26, 2017 at 8:17 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> Additional test cases added to the submodule-foreach test suite
> to check the submodule foreach --recursive behavior from a
> subdirectory as this was missing from the test suite.

As this demonstrates the fixture of the first patch,
this could be squashed into the first commit.

Reason: When someone is looking at that first commit, they
may wonder if there is no test that demonstrates the fix. (as fixing
a bug with no test is bad style. ;) And given the data structures of
Git it is only easy to find the previous commit, but hard to find the
next commit (this one) later on.

I think with only the minor nit in patch 3, the foreach is tackled. :)

Thanks,
Stefan

Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Ramsay Jones]

On 26/05/17 16:17, Prathamesh Chavan wrote:
> According to the documentation about git-submodule foreach subcommand's
> $path variable:
> $path is the name of the submodule directory relative to the superproject
> 
> But it was observed when the value of the $path value deviates from this
> for the nested submodules when the <command> is run from a subdirectory.
> This patch aims for its correction.
> 
> 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.
> 
> The observation made was as follows:
> For a project - super containing dir (not a submodule) and a submodule sub 
> which contains another submodule subsub. When we run a command from super/dir:
> 
> git submodule foreach "echo \$path-\$sm_path"
> 
> actual results:
> Entering '../sub'
> ../sub-../sub
> Entering '../sub/subsub'
> ../subsub-../subsub
> 
> expected result wrt documentation and current test suite:
> Entering '../sub'
> sub-../sub
> Entering '../sub/subsub'
> subsub-../sub/subsub
> 
> This make the value of $path confusing and I also feel it deviates from its 
> documentation:
> $path is the name of the submodule directory relative to the superproject.
> Hence, this patch corrects the value assigned to the $path and $sm_path.
> 
>  git-submodule.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/git-submodule.sh b/git-submodule.sh
> index c0d0e9a4c..ea6f56337 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -344,9 +344,9 @@ cmd_foreach()
>  				prefix="$prefix$sm_path/"
>  				sanitize_submodule_env
>  				cd "$sm_path" &&
> -				sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
>  				# we make $path available to scripts ...
>  				path=$sm_path &&
> +				sm_path=$displaypath &&
>  				if test $# -eq 1
>  				then
>  					eval "$1"
> 

Hmm, I'm not sure which documentation you are referring to, but if
$path != $sm_path then something is wrong. (unless their definition
has changed, of course). commit 091a6eb0fe may have muddied the water
a little by using $sm_path in the test in t7407, since (as far as I
know) $path is the user-facing variable (NOT $sm_path).

ATB,
Ramsay Jones

Re: [GSoC][PATCH v5 2/3] t7407: test "submodule foreach --recursive" from subdirectory added

[Brandon Williams]

On 05/26, Prathamesh Chavan wrote:
> Additional test cases added to the submodule-foreach test suite
> to check the submodule foreach --recursive behavior from a
> subdirectory as this was missing from the test suite.
> 
> Helped-by: Brandon Williams <bmwill@google.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
> Additional test added to check the bug fixed in the [PATCH v5 1/3] of
> this patch series.
> 
>  t/t7407-submodule-foreach.sh | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
> index 6ba5daf42..1c8d132d8 100755
> --- a/t/t7407-submodule-foreach.sh
> +++ b/t/t7407-submodule-foreach.sh
> @@ -197,6 +197,40 @@ test_expect_success 'test messages from "foreach --recursive" from subdirectory'
>  	test_i18ncmp expect actual
>  '
>  
> +sub1sha1=$(cd clone2/sub1 && git rev-parse HEAD)
> +sub2sha1=$(cd clone2/sub2 && git rev-parse HEAD)
> +sub3sha1=$(cd clone2/sub3 && git rev-parse HEAD)
> +nested1sha1=$(cd clone2/nested1 && git rev-parse HEAD)
> +nested2sha1=$(cd clone2/nested1/nested2 && git rev-parse HEAD)
> +nested3sha1=$(cd clone2/nested1/nested2/nested3 && git rev-parse HEAD)
> +submodulesha1=$(cd clone2/nested1/nested2/nested3/submodule && git rev-parse HEAD)
> +
> +cat >expect <<EOF
> +Entering '../nested1'
> +$pwd/clone2-nested1-../nested1-$nested1sha1
> +Entering '../nested1/nested2'
> +$pwd/clone2/nested1-nested2-../nested1/nested2-$nested2sha1
> +Entering '../nested1/nested2/nested3'
> +$pwd/clone2/nested1/nested2-nested3-../nested1/nested2/nested3-$nested3sha1
> +Entering '../nested1/nested2/nested3/submodule'
> +$pwd/clone2/nested1/nested2/nested3-submodule-../nested1/nested2/nested3/submodule-$submodulesha1
> +Entering '../sub1'
> +$pwd/clone2-foo1-../sub1-$sub1sha1
> +Entering '../sub2'
> +$pwd/clone2-foo2-../sub2-$sub2sha1
> +Entering '../sub3'
> +$pwd/clone2-foo3-../sub3-$sub3sha1
> +EOF
> +
> +test_expect_success 'test "submodule foreach --recursive" from subdirectory' '
> +	(
> +		cd clone2 &&
> +		cd untracked &&
> +		git submodule foreach --recursive "echo \$toplevel-\$name-\$sm_path-\$sha1" >../../actual
> +	) &&

small nit: You can either merge the two cd commands to 'cd clone2/untracked' or
better you can even avoid the subshell entirely by doing the following:

  git -C clone2/untracked submodule foreach --recursive \
    "echo \$toplevel-\$name-\$sm_path-\$sha1" >actual

Or something akin to that.

> +	test_i18ncmp expect actual
> +'
> +
>  cat > expect <<EOF
>  nested1-nested1
>  nested2-nested2
> -- 
> 2.11.0
> 

-- 
Brandon Williams

Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C

[Brandon Williams]

On 05/26, Prathamesh Chavan wrote:
> This aims to make git-submodule foreach a builtin. This is the very
> first step taken in this direction. Hence, 'foreach' is ported to
> submodule--helper, and submodule--helper is called from git-submodule.sh.
> The code is split up to have one function to obtain all the list of
> submodules. This function acts as the front-end of git-submodule foreach
> subcommand. It calls the function for_each_submodule_list, which basically
> loops through the list and calls function fn, which in this case is
> runcommand_in_submodule. This third function is a calling function that
> takes care of running the command in that submodule, and recursively
> perform the same when --recursive is flagged.
> 
> The first function module_foreach first 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 case of submodule subcommand
> foreach is runcommand_in_submodule) is called for each entry.
> 
> The third function runcommand_in_submodule, generates a submodule struct sub
> for $name, value and then later prepends name=sub->name; and other
> value assignment to the env argv_array structure of a child_process.
> Also the <command> of submodule-foreach is push to args argv_array
> structure and finally, using run_command the commands are executed
> using a shell.
> 
> The third function also takes care of the recursive flag, by creating
> a separate child_process structure and prepending "--super-prefix displaypath",
> to the args argv_array structure. Other required arguments and the
> input <command> of submodule-foreach is also appended to this argv_array.
> 
> The commit 1c4fb136db (submodule foreach: skip eval for more than one
> argument, 2013-09-27), which explains that why for the case when argc>1,
> we do not use eval. But since in this patch, we are calling the
> command in a separate shell itself for all values of argc, this case
> is not considered separately.
> 
> Both env variable $path and $sm_path were added since both are used in
> tests in t7407.
> 
> Helped-by: Brandon Williams <bmwill@google.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
> These series of patches passes the complete test suite.
> Its build report is available at:
> https://travis-ci.org/pratham-pc/git/builds
> Branch: submodule-foreach
> Build #71
> 
>  builtin/submodule--helper.c | 148 ++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            |  39 +-----------
>  2 files changed, 149 insertions(+), 38 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 566a5b6a6..343b6269c 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,26 @@ 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;
> +		displaypath = xstrdup(relative_path(path, prefix, &sb));

These can probably go on the same line:
  
  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);
> +	}
> +}
> +
>  struct module_list {
>  	const struct cache_entry **entries;
>  	int alloc, nr;
> @@ -331,6 +353,14 @@ 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;
> @@ -487,6 +517,123 @@ static int module_name(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +struct cb_foreach {
> +	int argc;
> +	const char **argv;
> +	const char *prefix;
> +	unsigned int quiet: 1;
> +	unsigned int recursive: 1;
> +};
> +#define CB_FOREACH_INIT { 0, NULL, NULL, 0, 0 }
> +
> +static void runcommand_in_submodule(const struct cache_entry *list_item,
> +				    void *cb_data)
> +{
> +	struct cb_foreach *info = cb_data;
> +	char *toplevel = xgetcwd();
> +	const struct submodule *sub;
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	char* displaypath = NULL;
> +	int i;
> +
> +	/* Only loads from .gitmodules, no overlay with .git/config */
> +	gitmodules_config();
> +
> +	sub = submodule_from_path(null_sha1, list_item->name);
> +
> +	if (!sub)
> +		die(_("No url found for submodule path '%s' in .gitmodules"),
> +		      displaypath);
> +
> +	if (!is_submodule_populated_gently(list_item->name, NULL))
> +		return;

I missed one other memory leak from the last round.  You should probably
call xgetcwd() to fill 'toplevel' here to avoid leaking the memory if
you do an early return.

> +
> +	displaypath = get_submodule_displaypath(list_item->name, info->prefix);
> +
> +	prepare_submodule_repo_env(&cp.env_array);
> +	cp.use_shell = 1;
> +	cp.dir = list_item->name;
> +
> +	argv_array_pushf(&cp.env_array, "name=%s", sub->name);
> +	argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
> +	argv_array_pushf(&cp.env_array, "path=%s", list_item->name);
> +	argv_array_pushf(&cp.env_array, "sha1=%s", oid_to_hex(&list_item->oid));
> +	argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
> +
> +	for (i = 0; i < info->argc; i++)
> +		argv_array_push(&cp.args, info->argv[i]);
> +
> +	if (!info->quiet)
> +		printf(_("Entering '%s'\n"), displaypath);
> +
> +	if (info->argv[0] && run_command(&cp))
> +		die(_("run_command returned non-zero status for %s\n."),
> +		      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", "foreach", "--recursive",
> +				 NULL);
> +
> +		if (info->quiet)
> +			argv_array_push(&cpr.args, "--quiet");
> +
> +		for (i = 0; i < info->argc; i++)
> +			argv_array_push(&cpr.args, info->argv[i]);
> +
> +		if (run_command(&cpr))
> +			die(_("run_command returned non-zero status while"
> +			      "recursing in the nested submodules of %s\n."),
> +			      displaypath);
> +	}
> +
> +	free(displaypath);
> +	free(toplevel);
> +}
> +
> +static int module_foreach(int argc, const char **argv, const char *prefix)
> +{
> +	struct cb_foreach info;
> +	struct pathspec pathspec;
> +	struct module_list list = MODULE_LIST_INIT;
> +	int quiet = 0;
> +	int recursive = 0;
> +
> +	struct option module_foreach_options[] = {
> +		OPT__QUIET(&quiet, N_("Suppress output of entering each submodule command")),
> +		OPT_BOOL(0, "recursive", &recursive,
> +			 N_("Recurse into nested submodules")),
> +		OPT_END()
> +	};
> +
> +	const char *const git_submodule_helper_usage[] = {
> +		N_("git submodule--helper foreach [--quiet] [--recursive] <command>"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, module_foreach_options,
> +			     git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);
> +
> +	if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)
> +		BUG("module_list_compute should not choke on empty pathspec");
> +
> +	info.argc = argc;
> +	info.argv = argv;
> +	info.prefix = prefix;
> +	info.quiet = quiet;
> +	info.recursive = recursive;
> +
> +	for_each_submodule_list(list, runcommand_in_submodule, &info);
> +
> +	return 0;
> +}
> +
>  static int clone_submodule(const char *path, const char *gitdir, const char *url,
>  			   const char *depth, struct string_list *reference,
>  			   int quiet, int progress)
> @@ -1212,6 +1359,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},
> +	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
>  	{"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 ea6f56337..032fd2540 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -322,45 +322,8 @@ cmd_foreach()
>  		shift
>  	done
>  
> -	toplevel=$(pwd)
> +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
>  
> -	# dup stdin so that it can be restored when running the external
> -	# command in the subshell (and a recursive call to this function)
> -	exec 3<&0
> -
> -	{
> -		git submodule--helper list --prefix "$wt_prefix" ||
> -		echo "#unmatched" $?
> -	} |
> -	while read -r mode sha1 stage sm_path
> -	do
> -		die_if_unmatched "$mode" "$sha1"
> -		if test -e "$sm_path"/.git
> -		then
> -			displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
> -			say "$(eval_gettext "Entering '\$displaypath'")"
> -			name=$(git submodule--helper name "$sm_path")
> -			(
> -				prefix="$prefix$sm_path/"
> -				sanitize_submodule_env
> -				cd "$sm_path" &&
> -				# we make $path available to scripts ...
> -				path=$sm_path &&
> -				sm_path=$displaypath &&
> -				if test $# -eq 1
> -				then
> -					eval "$1"
> -				else
> -					"$@"
> -				fi &&
> -				if test -n "$recursive"
> -				then
> -					cmd_foreach "--recursive" "$@"
> -				fi
> -			) <&3 3<&- ||
> -			die "$(eval_gettext "Stopping at '\$displaypath'; script returned non-zero status.")"
> -		fi
> -	done
>  }
>  
>  #
> -- 
> 2.11.0
> 

Looking good!

-- 
Brandon Williams

Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Stefan Beller]

On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
> On 26/05/17 16:17, Prathamesh Chavan wrote:
>> According to the documentation about git-submodule foreach subcommand's
>> $path variable:
>> $path is the name of the submodule directory relative to the superproject
>>
>> But it was observed when the value of the $path value deviates from this
>> for the nested submodules when the <command> is run from a subdirectory.
>> This patch aims for its correction.
>>
>> 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.
>>
>> The observation made was as follows:
>> For a project - super containing dir (not a submodule) and a submodule sub
>> which contains another submodule subsub. When we run a command from super/dir:
>>
>> git submodule foreach "echo \$path-\$sm_path"
>>
>> actual results:
>> Entering '../sub'
>> ../sub-../sub
>> Entering '../sub/subsub'
>> ../subsub-../subsub
>>
>> expected result wrt documentation and current test suite:
>> Entering '../sub'
>> sub-../sub
>> Entering '../sub/subsub'
>> subsub-../sub/subsub
>>
>> This make the value of $path confusing and I also feel it deviates from its
>> documentation:
>> $path is the name of the submodule directory relative to the superproject.
>> Hence, this patch corrects the value assigned to the $path and $sm_path.
>>
>>  git-submodule.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index c0d0e9a4c..ea6f56337 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -344,9 +344,9 @@ cmd_foreach()
>>                               prefix="$prefix$sm_path/"
>>                               sanitize_submodule_env
>>                               cd "$sm_path" &&
>> -                             sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
>>                               # we make $path available to scripts ...
>>                               path=$sm_path &&
>> +                             sm_path=$displaypath &&
>>                               if test $# -eq 1
>>                               then
>>                                       eval "$1"
>>
>
> Hmm, I'm not sure which documentation you are referring to,

Quite likely our fine manual pages. ;)

       foreach [--recursive] <command>
           Evaluates an arbitrary shell command in each checked out submodule.
           The command has access to the variables $name, $path, $sha1 and
           $toplevel: $name is the name of the relevant submodule section in
           .gitmodules, $path is the name of the submodule directory relative
           to the superproject, $sha1 is the commit as recorded in the
           superproject, and $toplevel is the absolute path to the top-level
           of the superproject. Any submodules defined in the superproject but
           not checked out are ignored by this command. Unless given --quiet,
           foreach prints the name of each submodule before evaluating the
           command. If --recursive is given, submodules are traversed
           recursively (i.e. the given shell command is evaluated in nested
           submodules as well). A non-zero return from the command in any
           submodule causes the processing to terminate. This can be
           overridden by adding || : to the end of the command.

As $path is documented and $sm_path is not, we should care about
$path first to be correct and either fix the documentation or the implementation
such that we have a consistent world view. :)

> but if
> $path != $sm_path then something is wrong. (unless their definition
> has changed, of course).

I would lean in doing so (changing their definition):

    $path (as documented) is the name of the submodule directory
    relative to the direct superproject (so in nested submodules you
    go up only one level).

$sm_path on the other hand is not documented at all and yields
non-sense results in corner cases.

With this patch it becomes less non-sensey and could be documented as:

    $sm_path is the relative path from the current working directory
    to the submodule (ignoring relations to the superproject or nesting
    of submodules). This documentation also fits into the narrative of
    the test in t7407.

Thanks,
Stefan

Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C

[Johannes Sixt]

Am 26.05.2017 um 17:17 schrieb Prathamesh Chavan:
> +	argv_array_pushf(&cp.env_array, "path=%s", list_item->name);

Not good! On Windows, environment variables are case insensitive. The 
environment variable "path" has a very special purpose, although it is 
generally spelled "PATH" (actually "Path" on Windows).

Lowercase "path" may have worked as long as it was only used in a shell 
script (and perhaps only by lucky coincidence), but this I can pretty 
much guarantee to fail. (I haven't tested it, though.)

The correct fix can only be to rename this variable here and in shell 
scripts that need the value that is set here.

-- Hannes

Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C

[Brandon Williams]

On 05/26, Johannes Sixt wrote:
> Am 26.05.2017 um 17:17 schrieb Prathamesh Chavan:
> >+	argv_array_pushf(&cp.env_array, "path=%s", list_item->name);
> 
> Not good! On Windows, environment variables are case insensitive.
> The environment variable "path" has a very special purpose, although
> it is generally spelled "PATH" (actually "Path" on Windows).
> 
> Lowercase "path" may have worked as long as it was only used in a
> shell script (and perhaps only by lucky coincidence), but this I can
> pretty much guarantee to fail. (I haven't tested it, though.)
> 
> The correct fix can only be to rename this variable here and in
> shell scripts that need the value that is set here.
> 
> -- Hannes

Nice catch, the only reason why this would have worked before was
because it was a shell script before...

-- 
Brandon Williams

Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Ramsay Jones]

On 26/05/17 18:07, Stefan Beller wrote:
> On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
>> Hmm, I'm not sure which documentation you are referring to,
> 
> Quite likely our fine manual pages. ;)
> 
>        foreach [--recursive] <command>
>            Evaluates an arbitrary shell command in each checked out submodule.
>            The command has access to the variables $name, $path, $sha1 and
>            $toplevel: $name is the name of the relevant submodule section in
>            .gitmodules, $path is the name of the submodule directory relative
>            to the superproject, $sha1 is the commit as recorded in the
>            superproject, and $toplevel is the absolute path to the top-level
>            of the superproject. Any submodules defined in the superproject but
>            not checked out are ignored by this command. Unless given --quiet,
>            foreach prints the name of each submodule before evaluating the
>            command. If --recursive is given, submodules are traversed
>            recursively (i.e. the given shell command is evaluated in nested
>            submodules as well). A non-zero return from the command in any
>            submodule causes the processing to terminate. This can be
>            overridden by adding || : to the end of the command.

I suspected as much, but I was wondering specifically if $sm_path
had been documented anywhere. I didn't think so, but ...

> As $path is documented and $sm_path is not, we should care about
> $path first to be correct and either fix the documentation or the implementation
> such that we have a consistent world view. :)

Sure, but what is that world view? :-D

I suspect that commit 091a6eb0fe did not intend (should not have)
used $sm_path in that test. If we were to 'fix' that test, would
it still work?

Back in 2012, the submodule list was generated by filtering the
output of 'git ls-files --error-unmatch --stage --'; but I don't
recall if (at that time) git-ls-files required being at the top
of the working tree, or if it would execute fine in a sub-directory.
So, it's possible that the documentation of $path was wrong all along.
;-)

At that time, by definition, $path == $sm_path. However, you know this
stuff much better than me (I don't use git-submodule), so ...

>> but if
>> $path != $sm_path then something is wrong. (unless their definition
>> has changed, of course).
> 
> I would lean in doing so (changing their definition):
> 
>     $path (as documented) is the name of the submodule directory
>     relative to the direct superproject (so in nested submodules you
>     go up only one level).
> 
> $sm_path on the other hand is not documented at all and yields
> non-sense results in corner cases.

Hmm, at what point did '$sm_path yields non-sense results' start
being the case? (perhaps the corner cases need to be fixed first).

> With this patch it becomes less non-sensey and could be documented as:
> 
>     $sm_path is the relative path from the current working directory
>     to the submodule (ignoring relations to the superproject or nesting
>     of submodules). 

OK.

>                      This documentation also fits into the narrative of
>     the test in t7407.

Hmm, does it?

ATB,
Ramsay Jones

Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C

[Ramsay Jones]

On 26/05/17 22:54, Johannes Sixt wrote:
> Am 26.05.2017 um 17:17 schrieb Prathamesh Chavan:
>> +    argv_array_pushf(&cp.env_array, "path=%s", list_item->name);
> 
> Not good! On Windows, environment variables are case insensitive. The environment variable "path" has a very special purpose, although it is generally spelled "PATH" (actually "Path" on Windows).
> 
> Lowercase "path" may have worked as long as it was only used in a shell script (and perhaps only by lucky coincidence), but this I can pretty much guarantee to fail. (I haven't tested it, though.)
> 
> The correct fix can only be to rename this variable here and in shell scripts that need the value that is set here.

Yeah, I already pointed to commit 64394e3ae9 (but it seems not
to have registered!), but ...

I tried provoking a failure on cygwin, and I couldn't get it to fail!
Since Johannes told me about Gfw fork of the msys-runtime, I didn't
even attempt to try and provoke a failure on MSYS2/MinGW.

So, maybe it's fixed (no I'm not convinced either) ...

ATB,
Ramsay Jones

Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C

[Ramsay Jones]

On 27/05/17 02:20, Ramsay Jones wrote:
> 
> 
> On 26/05/17 22:54, Johannes Sixt wrote:
>> Am 26.05.2017 um 17:17 schrieb Prathamesh Chavan:
>>> +    argv_array_pushf(&cp.env_array, "path=%s", list_item->name);
>>
>> Not good! On Windows, environment variables are case insensitive. The environment variable "path" has a very special purpose, although it is generally spelled "PATH" (actually "Path" on Windows).
>>
>> Lowercase "path" may have worked as long as it was only used in a shell script (and perhaps only by lucky coincidence), but this I can pretty much guarantee to fail. (I haven't tested it, though.)
>>
>> The correct fix can only be to rename this variable here and in shell scripts that need the value that is set here.
> 
> Yeah, I already pointed to commit 64394e3ae9 (but it seems not
> to have registered!), but ...
> 
> I tried provoking a failure on cygwin, and I couldn't get it to fail!

To be more explicit, last Sunday I hacked into t7407 to show an
example failure on cygwin (see patch below), but it passes on both
Linux (expected) and cygwin! :( Perhaps you can see what I'm doing
wrong?

ATB,
Ramsay Jones

-- >8 --
Date: Sun, 21 May 2017 16:23:58 +0100
Subject: [PATCH] submodule: foreach $path munging on cygwin

---
 t/t7407-submodule-foreach.sh | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6ba5daf42..c2d66bab7 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -68,17 +68,36 @@ Entering 'sub3'
 $pwd/clone-foo3-sub3-$sub3sha1
 EOF
 
+cat >expect-func <<EOF
+Entering 'sub1'
+running from TRASH
+path is <<sub1>>
+Entering 'sub3'
+running from TRASH
+path is <<sub3>>
+EOF
+
 test_expect_success 'test basic "submodule foreach" usage' '
+	PATH="$PWD:$PATH" &&
+	write_script foreach-func <<-\EOF &&
+	echo "running from TRASH"
+	echo "path is <<$1>>"
+	EOF
 	git clone super clone &&
 	(
 		cd clone &&
 		git submodule update --init -- sub1 sub3 &&
 		git submodule foreach "echo \$toplevel-\$name-\$path-\$sha1" > ../actual &&
+		git submodule foreach "foreach-func \$path" > ../actual-func1 &&
+		git submodule foreach "export path; foreach-func \$path" > ../actual-func2 &&
 		git config foo.bar zar &&
 		git submodule foreach "git config --file \"\$toplevel/.git/config\" foo.bar"
 	) &&
+	test_i18ncmp expect-func actual-func1 &&
+	test_i18ncmp expect-func actual-func2 &&
 	test_i18ncmp expect actual
 '
+test_done
 
 cat >expect <<EOF
 Entering '../sub1'
-- 
2.13.0

Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C

[Johannes Sixt]

Am 27.05.2017 um 16:06 schrieb Ramsay Jones:
> To be more explicit, last Sunday I hacked into t7407 to show an
> example failure on cygwin (see patch below), but it passes on both
> Linux (expected) and cygwin! :( Perhaps you can see what I'm doing
> wrong?

As long as the git.exe you are using here is Cygwin's, Windows 
conventions do not apply. I think, the environment is transfered across 
the exec boundaries using Cygwin's own tools, and Windows's 
case-insensitive environment does not enter the game at all. But that's 
just a shot in the dark.

> 
> ATB,
> Ramsay Jones
> 
> -- >8 --
> Date: Sun, 21 May 2017 16:23:58 +0100
> Subject: [PATCH] submodule: foreach $path munging on cygwin
> 
> ---
>   t/t7407-submodule-foreach.sh | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 
> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
> index 6ba5daf42..c2d66bab7 100755
> --- a/t/t7407-submodule-foreach.sh
> +++ b/t/t7407-submodule-foreach.sh
> @@ -68,17 +68,36 @@ Entering 'sub3'
>   $pwd/clone-foo3-sub3-$sub3sha1
>   EOF
>   
> +cat >expect-func <<EOF
> +Entering 'sub1'
> +running from TRASH
> +path is <<sub1>>
> +Entering 'sub3'
> +running from TRASH
> +path is <<sub3>>
> +EOF
> +
>   test_expect_success 'test basic "submodule foreach" usage' '
> +	PATH="$PWD:$PATH" &&
> +	write_script foreach-func <<-\EOF &&
> +	echo "running from TRASH"
> +	echo "path is <<$1>>"
> +	EOF
>   	git clone super clone &&
>   	(
>   		cd clone &&
>   		git submodule update --init -- sub1 sub3 &&
>   		git submodule foreach "echo \$toplevel-\$name-\$path-\$sha1" > ../actual &&
> +		git submodule foreach "foreach-func \$path" > ../actual-func1 &&
> +		git submodule foreach "export path; foreach-func \$path" > ../actual-func2 &&
>   		git config foo.bar zar &&
>   		git submodule foreach "git config --file \"\$toplevel/.git/config\" foo.bar"
>   	) &&
> +	test_i18ncmp expect-func actual-func1 &&
> +	test_i18ncmp expect-func actual-func2 &&
>   	test_i18ncmp expect actual
>   '
> +test_done
>   
>   cat >expect <<EOF
>   Entering '../sub1'
> 

Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Stefan Beller]

On Fri, May 26, 2017 at 6:10 PM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
> On 26/05/17 18:07, Stefan Beller wrote:
>> On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones
>> <ramsay@ramsayjones.plus.com> wrote:
>>> Hmm, I'm not sure which documentation you are referring to,
>>
>> Quite likely our fine manual pages. ;)
>>
>>        foreach [--recursive] <command>
>>            Evaluates an arbitrary shell command in each checked out submodule.
>>            The command has access to the variables $name, $path, $sha1 and
>>            $toplevel: $name is the name of the relevant submodule section in
>>            .gitmodules, $path is the name of the submodule directory relative
>>            to the superproject, $sha1 is the commit as recorded in the
>>            superproject, and $toplevel is the absolute path to the top-level
>>            of the superproject. Any submodules defined in the superproject but
>>            not checked out are ignored by this command. Unless given --quiet,
>>            foreach prints the name of each submodule before evaluating the
>>            command. If --recursive is given, submodules are traversed
>>            recursively (i.e. the given shell command is evaluated in nested
>>            submodules as well). A non-zero return from the command in any
>>            submodule causes the processing to terminate. This can be
>>            overridden by adding || : to the end of the command.
>
> I suspected as much, but I was wondering specifically if $sm_path
> had been documented anywhere. I didn't think so, but ...
>
>> As $path is documented and $sm_path is not, we should care about
>> $path first to be correct and either fix the documentation or the implementation
>> such that we have a consistent world view. :)
>
> Sure, but what is that world view? :-D
>
> I suspect that commit 091a6eb0fe did not intend (should not have)
> used $sm_path in that test. If we were to 'fix' that test, would
> it still work?
>
> Back in 2012, the submodule list was generated by filtering the
> output of 'git ls-files --error-unmatch --stage --'; but I don't
> recall if (at that time) git-ls-files required being at the top
> of the working tree, or if it would execute fine in a sub-directory.
> So, it's possible that the documentation of $path was wrong all along.
> ;-)
>
> At that time, by definition, $path == $sm_path. However, you know this
> stuff much better than me (I don't use git-submodule), so ...

Don't take that stance. Sometimes I shoot quickly from the hip without
considering consequences (Figuratively).

In a foreach command I can see value both in the 'displaypath'
(what sm_path would become here) and the 'submodule path'
from the superproject. The naming of 'sm_path' hints at that it ought
to be the 'submodule path'.

>>
>>     $path (as documented) is the name of the submodule directory
>>     relative to the direct superproject (so in nested submodules you
>>     go up only one level).
>>
>> $sm_path on the other hand is not documented at all and yields
>> non-sense results in corner cases.
>
> Hmm, at what point did '$sm_path yields non-sense results' start
> being the case? (perhaps the corner cases need to be fixed first).

Well the corner case is described in the patchs notes.
So that patch would fix it to be consistent with the new world view
(that I have in mind) as I do not know about the 2012 ideas how submodules
ought to behave correctly.

>> With this patch it becomes less non-sensey and could be documented as:
>>
>>     $sm_path is the relative path from the current working directory
>>     to the submodule (ignoring relations to the superproject or nesting
>>     of submodules).
>
> OK.
>
>>                      This documentation also fits into the narrative of
>>     the test in t7407.
>
> Hmm, does it?

After rereading that test, I would think so?

Thanks for keeping discussing this.

So maybe we want to
* keep path=sm_path
* fix the documentation via s/$path/$sm_path/g in that section quoted above.
* Introduce a new variable sm_display_path that contains what this patch
  proposes sm_path to be.
* fix the test in t7407 by checking both sm_path (fixed) as well
  as sm_display_path (what is currently recorded in sm_path)
---
In the next patch:
* Additionally in the rewrite in C, we would do an

    #ifndef WINDOWS /* need to lookup the exact macro */
        argv_array_push(env_vars, "path=%s", sm_path);
    #endif

such that Windows users are forced to migrate to sm_path
as path/Path is case sensitive there. sm_path being documented
value, so it should work fine?

Thanks,
Stefan

Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Ramsay Jones]

On 30/05/17 22:53, Stefan Beller wrote:
> On Fri, May 26, 2017 at 6:10 PM, Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
>> On 26/05/17 18:07, Stefan Beller wrote:
>>> On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones
>>> <ramsay@ramsayjones.plus.com> wrote:

>> Back in 2012, the submodule list was generated by filtering the
>> output of 'git ls-files --error-unmatch --stage --'; but I don't
>> recall if (at that time) git-ls-files required being at the top
>> of the working tree, or if it would execute fine in a sub-directory.
>> So, it's possible that the documentation of $path was wrong all along.
>> ;-)
>>
>> At that time, by definition, $path == $sm_path. However, you know this
>> stuff much better than me (I don't use git-submodule), so ...
> 
> Don't take that stance. Sometimes I shoot quickly from the hip without
> considering consequences (Figuratively).
> 
> In a foreach command I can see value both in the 'displaypath'
> (what sm_path would become here) and the 'submodule path'
> from the superproject. The naming of 'sm_path' hints at that it ought
> to be the 'submodule path'.

Well, since I introduced it, I can confidently proclaim that it is
indeed the 'submodule path'. :-D

As I said above, I can't remember how git-ls-files worked back then,
but it seems that I thought of it as the path to the submodule from
the root of the working tree. Again, by definition, $sm_path == $path
(as documented). Of course, that may have changed since then.

>>> With this patch it becomes less non-sensey and could be documented as:
>>>
>>>     $sm_path is the relative path from the current working directory
>>>     to the submodule (ignoring relations to the superproject or nesting
>>>     of submodules).
>>
>> OK.
>>
>>>                      This documentation also fits into the narrative of
>>>     the test in t7407.
>>
>> Hmm, does it?
> 
> After rereading that test, I would think so?

Really? So, if it differs from $path, then something changed between
commit 64394e3ae9 and commit 091a6eb0fe. I haven't really read that
commit/test, so take what I say with a pinch of salt ...

> Thanks for keeping discussing this.
> 
> So maybe we want to
> * keep path=sm_path

As I said in commit 64394e3ae9, $path was part of the API, so I could
not just rename it, without a deprecation period, etc ... So, I was
'crossing my fingers' that nobody would export $path in their user
scripts (not very likely, after all).

> * fix the documentation via s/$path/$sm_path/g in that section quoted above.

So, "$path is the name of the submodule directory relative to the
superproject", as currently documented in the man page, yes?

So, $sm_path == $path, at least for some period?

> * Introduce a new variable sm_display_path that contains what this patch
>   proposes sm_path to be.

So, this would be the path from the cwd to the submodule, yes?

> * fix the test in t7407 by checking both sm_path (fixed) as well
>   as sm_display_path (what is currently recorded in sm_path)

Hmm, ...

> In the next patch:
> * Additionally in the rewrite in C, we would do an
> 
>     #ifndef WINDOWS /* need to lookup the exact macro */
>         argv_array_push(env_vars, "path=%s", sm_path);
>     #endif
> 
> such that Windows users are forced to migrate to sm_path
> as path/Path is case sensitive there. sm_path being documented
> value, so it should work fine?

Well, as you saw in a separate thread, I can no longer get
cygwin to fail, so something (probably in the cygwin runtime)
has changed since 2012 to make this work now, despite the
case insensitive win32 environment block. (This may also be
true of MSYS2, but I haven't tested it).

I have not tested this on MYSY2/MinGW/Git-for-windows, but
Johannes Sixt was concerned about this, so I guess it may
still be a problem there.

I don't know how windows folks will feel about simply
removing $path, ...


ATB,
Ramsay Jones

Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Stefan Beller]

On Tue, May 30, 2017 at 4:07 PM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
> On 30/05/17 22:53, Stefan Beller wrote:
>> On Fri, May 26, 2017 at 6:10 PM, Ramsay Jones
>> <ramsay@ramsayjones.plus.com> wrote:
>>> On 26/05/17 18:07, Stefan Beller wrote:
>>>> On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones
>>>> <ramsay@ramsayjones.plus.com> wrote:
>
>>> Back in 2012, the submodule list was generated by filtering the
>>> output of 'git ls-files --error-unmatch --stage --'; but I don't
>>> recall if (at that time) git-ls-files required being at the top
>>> of the working tree, or if it would execute fine in a sub-directory.
>>> So, it's possible that the documentation of $path was wrong all along.
>>> ;-)
>>>
>>> At that time, by definition, $path == $sm_path. However, you know this
>>> stuff much better than me (I don't use git-submodule), so ...
>>
>> Don't take that stance. Sometimes I shoot quickly from the hip without
>> considering consequences (Figuratively).
>>
>> In a foreach command I can see value both in the 'displaypath'
>> (what sm_path would become here) and the 'submodule path'
>> from the superproject. The naming of 'sm_path' hints at that it ought
>> to be the 'submodule path'.
>
> Well, since I introduced it, I can confidently proclaim that it is
> indeed the 'submodule path'. :-D

ok. :)

> As I said above, I can't remember how git-ls-files worked back then,
> but it seems that I thought of it as the path to the submodule from
> the root of the working tree. Again, by definition, $sm_path == $path
> (as documented). Of course, that may have changed since then.

Documented in 64394e3 (git-submodule.sh: Don't use $path variable in
eval_gettext string, by yourself)

What I intended to say above was "documented to the end user",
and I do not count our commit messages as such. The end user facing
documentation only talks about path, not mentioning sm_path.

>>>> With this patch it becomes less non-sensey and could be documented as:
>>>>
>>>>     $sm_path is the relative path from the current working directory
>>>>     to the submodule (ignoring relations to the superproject or nesting
>>>>     of submodules).
>>>
>>> OK.
>>>
>>>>                      This documentation also fits into the narrative of
>>>>     the test in t7407.
>>>
>>> Hmm, does it?
>>
>> After rereading that test, I would think so?
>
> Really? So, if it differs from $path, then something changed between
> commit 64394e3ae9 and commit 091a6eb0fe. I haven't really read that
> commit/test, so take what I say with a pinch of salt ...

Well yes. I am specifically reading 091a6eb0fe, the changes to t7407.

In that test sm_path contains the relative path from $PWD to the
submodule. (It does NOT: "$[sm_]path is the name of the submodule
directory relative to the superproject" as documented but rather
... relative to the $PWD)

>
>> Thanks for keeping discussing this.
>>
>> So maybe we want to
>> * keep path=sm_path
>
> As I said in commit 64394e3ae9, $path was part of the API, so I could
> not just rename it, without a deprecation period, etc ... So, I was
> 'crossing my fingers' that nobody would export $path in their user
> scripts (not very likely, after all).

Ok. So another approach to get away in the C conversion:
* export the sm_path as all other environment variables
* for "$path" we do not export it into the environment, but
  prefix the command with it, i.e. we'd ask our shell to run
  "path=%s; %s", sm_path, argv[0]
  to preserve the historic behavior.

>
>> * fix the documentation via s/$path/$sm_path/g in that section quoted above.
>
> So, "$path is the name of the submodule directory relative to the
> superproject", as currently documented in the man page, yes?

No, the documentation does not match reality. The reality is that
both sm_path as well as path give the display path.

> So, $sm_path == $path, at least for some period?

yes that is current reality.

>
>> * Introduce a new variable sm_display_path that contains what this patch
>>   proposes sm_path to be.
>
> So, this would be the path from the cwd to the submodule, yes?

yes.

>
> I don't know how windows folks will feel about simply
> removing $path, ...

I agree that this is a bad idea now. As said above we'd
just not export the path as an env variable and should be
"fine" in the sense that we do not break historical expectations,
but have to deal with slightly messy code.

Just digging further, there was ea2fa1040d (submodule foreach:
correct path display in recursive submodules, 2016-03-29), which
is tangent to the issue of whether we think it is a display path
or the superproject->submodule path.

Thanks,
Stefan

Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Ramsay Jones]

On 31/05/17 00:29, Stefan Beller wrote:
 
>> As I said above, I can't remember how git-ls-files worked back then,
>> but it seems that I thought of it as the path to the submodule from
>> the root of the working tree. Again, by definition, $sm_path == $path
>> (as documented). Of course, that may have changed since then.
> 
> Documented in 64394e3 (git-submodule.sh: Don't use $path variable in
> eval_gettext string, by yourself)
> 
> What I intended to say above was "documented to the end user",
> and I do not count our commit messages as such. The end user facing
> documentation only talks about path, not mentioning sm_path.

Correct, and that is exactly what I was saying. ie. $path as
'documented to the end user'. (again by _definition_ $sm_path
_is_ $path).

>>> After rereading that test, I would think so?
>>
>> Really? So, if it differs from $path, then something changed between
>> commit 64394e3ae9 and commit 091a6eb0fe. I haven't really read that
>> commit/test, so take what I say with a pinch of salt ...
> 
> Well yes. I am specifically reading 091a6eb0fe, the changes to t7407.
> 
> In that test sm_path contains the relative path from $PWD to the
> submodule. (It does NOT: "$[sm_]path is the name of the submodule
> directory relative to the superproject" as documented but rather
> ... relative to the $PWD)

In that case, the current user documentation does not agree with
the current implementation, yes?

So, was the user documentation always wrong? (did git-ls-files work
from a sub-directory, limiting its output to the cwd, or did it
chdir() to the top of the worktree first?).

>> As I said in commit 64394e3ae9, $path was part of the API, so I could
>> not just rename it, without a deprecation period, etc ... So, I was
>> 'crossing my fingers' that nobody would export $path in their user
>> scripts (not very likely, after all).
> 
> Ok. So another approach to get away in the C conversion:
> * export the sm_path as all other environment variables
> * for "$path" we do not export it into the environment, but
>   prefix the command with it, i.e. we'd ask our shell to run
>   "path=%s; %s", sm_path, argv[0]
>   to preserve the historic behavior.

Yes, that would probably work.

ATB,
Ramsay Jones

Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Ramsay Jones]

On 31/05/17 01:13, Ramsay Jones wrote:

> On 31/05/17 00:29, Stefan Beller wrote:
>  
>> In that test sm_path contains the relative path from $PWD to the
>> submodule. (It does NOT: "$[sm_]path is the name of the submodule
>> directory relative to the superproject" as documented but rather
>> ... relative to the $PWD)
> 
> In that case, the current user documentation does not agree with
> the current implementation, yes?
> 
> So, was the user documentation always wrong? (did git-ls-files work
> from a sub-directory, limiting its output to the cwd, or did it
> chdir() to the top of the worktree first?).

To answer my own question, I rebuilt git and tried directly:

  $ pwd
  /home/ramsay/git
  $ 
  $ ./git version
  git version 1.7.10.1.g64394
  $ 
  $ cd git_remote_helpers/
  $ ../git-ls-files --error-unmatch --stage --
  100644 2247d5f95a7193c7221b9464debe167763b4fae3 0	.gitignore
  100644 74b05dc91e42414147d5f3dc7b4fc66fb86c0eca 0	Makefile
  100644 00f69cbeda277b24e8ab35cb7db2c25cc0cc122e 0	__init__.py
  100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	git/__init__.py
  100644 9ee5f96d4ce313f4f94505ff65b560943bfd21cb 0	git/exporter.py
  100644 007a1bfdf37d231470f69d9d0cffa46e80127f34 0	git/git.py
  100644 5c6b595e16665bc508625ab0e96c95776bacba1a 0	git/importer.py
  100644 e70025095dcfb31d3944e72ac1f83dd7d4109103 0	git/non_local.py
  100644 acbf8d7785e2253777456f8910e2352992dda474 0	git/repo.py
  100644 4bff8878d14ccaf02c552073ef55d519df0b4cad 0	setup.cfg
  100644 4d434b65cbf5c42a455d5cd3bced030bfb51a245 0	setup.py
  100644 fbbb01b14619c1d2ed6bcc8f304f019fbe98697f 0	util.py
  $ 
  
Hmm, so it looks like $path was always relative to cwd!

(got to get some sleep now ...)

ATB,
Ramsay Jones

[GSoC][PATCH v6 1/2] submodule: fix buggy $path and $sm_path variable's value

[Prathamesh Chavan]

According to the documentation about git-submodule foreach subcommand's
$path variable:
$path is the name of the submodule directory relative to the superproject

But it was observed when the value of the $path value deviates from this
for the nested submodules when the <command> is run from a subdirectory.
This patch aims for its correction.

Additional test cases added to the submodule-foreach test suite in t7407,
to check the submodule foreach --recursive behavior from a subdirectory
as this was missing from the test suite.

Helped-by: Brandon Williams <bmwill@google.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---
 git-submodule.sh             |  2 +-
 t/t7407-submodule-foreach.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index c0d0e9a4c..ea6f56337 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -344,9 +344,9 @@ cmd_foreach()
 				prefix="$prefix$sm_path/"
 				sanitize_submodule_env
 				cd "$sm_path" &&
-				sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
 				# we make $path available to scripts ...
 				path=$sm_path &&
+				sm_path=$displaypath &&
 				if test $# -eq 1
 				then
 					eval "$1"
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6ba5daf42..f4e07eee3 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -197,6 +197,39 @@ test_expect_success 'test messages from "foreach --recursive" from subdirectory'
 	test_i18ncmp expect actual
 '
 
+sub1sha1=$(cd clone2/sub1 && git rev-parse HEAD)
+sub2sha1=$(cd clone2/sub2 && git rev-parse HEAD)
+sub3sha1=$(cd clone2/sub3 && git rev-parse HEAD)
+nested1sha1=$(cd clone2/nested1 && git rev-parse HEAD)
+nested2sha1=$(cd clone2/nested1/nested2 && git rev-parse HEAD)
+nested3sha1=$(cd clone2/nested1/nested2/nested3 && git rev-parse HEAD)
+submodulesha1=$(cd clone2/nested1/nested2/nested3/submodule && git rev-parse HEAD)
+
+cat >expect <<EOF
+Entering '../nested1'
+$pwd/clone2-nested1-../nested1-$nested1sha1
+Entering '../nested1/nested2'
+$pwd/clone2/nested1-nested2-../nested1/nested2-$nested2sha1
+Entering '../nested1/nested2/nested3'
+$pwd/clone2/nested1/nested2-nested3-../nested1/nested2/nested3-$nested3sha1
+Entering '../nested1/nested2/nested3/submodule'
+$pwd/clone2/nested1/nested2/nested3-submodule-../nested1/nested2/nested3/submodule-$submodulesha1
+Entering '../sub1'
+$pwd/clone2-foo1-../sub1-$sub1sha1
+Entering '../sub2'
+$pwd/clone2-foo2-../sub2-$sub2sha1
+Entering '../sub3'
+$pwd/clone2-foo3-../sub3-$sub3sha1
+EOF
+
+test_expect_success 'test "submodule foreach --recursive" from subdirectory' '
+	(
+		cd clone2/untracked &&
+		git submodule foreach --recursive "echo \$toplevel-\$name-\$sm_path-\$sha1" >../../actual
+	) &&
+	test_i18ncmp expect actual
+'
+
 cat > expect <<EOF
 nested1-nested1
 nested2-nested2
-- 
2.13.0

[GSoC][PATCH v6 2/2] submodule: port subcommand foreach from shell to C

[Prathamesh Chavan]

This aims to make git-submodule foreach a builtin. This is the very
first step taken in this direction. Hence, 'foreach' is ported to
submodule--helper, and submodule--helper is called from git-submodule.sh.
The code is split up to have one function to obtain all the list of
submodules. This function acts as the front-end of git-submodule foreach
subcommand. It calls the function for_each_submodule_list, which basically
loops through the list and calls function fn, which in this case is
runcommand_in_submodule. This third function is a calling function that
takes care of running the command in that submodule, and recursively
perform the same when --recursive is flagged.

The first function module_foreach first 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 case of submodule subcommand
foreach is runcommand_in_submodule) is called for each entry.

The third function runcommand_in_submodule, generates a submodule struct sub
for $name, value and then later prepends name=sub->name; and other
value assignment to the env argv_array structure of a child_process.
Also the <command> of submodule-foreach is push to args argv_array
structure and finally, using run_command the commands are executed
using a shell.

The third function also takes care of the recursive flag, by creating
a separate child_process structure and prepending "--super-prefix displaypath",
to the args argv_array structure. Other required arguments and the
input <command> of submodule-foreach is also appended to this argv_array.

Helped-by: Brandon Williams <bmwill@google.com>
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, we avoid exporting the $path variable to
the environment, but instead prefix the <command> with it.
In this way, we avoid the issue it creates with the env variable
$PATH in windows.

Other than that, additionally the case of no. of arugments in <command>
being equal to 1 is also considered separetly.
THe reason of having this change in the shell script was given in the
commit 1c4fb136db.
According to my understanding, eval "$1" executes $1 in same shell,
whereas "$@" gets executed in a separate shell, which doesn't allow
"$@" to access the env variables $name, $path, etc.
Hence, to keep the ported function similar, this condition is also
added. 

Apart from this, other suggested changes are also implemented.

Complete build report is available at:
https://travis-ci.org/pratham-pc/git/builds
Branch: submodule-foreach
Build #87
 
 builtin/submodule--helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  39 +----------
 2 files changed, 154 insertions(+), 38 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 566a5b6a6..d08a02ad9 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);
+	}
+}
+
 struct module_list {
 	const struct cache_entry **entries;
 	int alloc, nr;
@@ -331,6 +353,14 @@ 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;
@@ -487,6 +517,128 @@ static int module_name(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct cb_foreach {
+	int argc;
+	const char **argv;
+	const char *prefix;
+	unsigned int quiet: 1;
+	unsigned int recursive: 1;
+};
+#define CB_FOREACH_INIT { 0, NULL, NULL, 0, 0 }
+
+static void runcommand_in_submodule(const struct cache_entry *list_item,
+				    void *cb_data)
+{
+	struct cb_foreach *info = cb_data;
+	char *toplevel;
+	const struct submodule *sub;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	char *displaypath;
+	int i;
+
+	sub = submodule_from_path(null_sha1, list_item->name);
+
+	displaypath = get_submodule_displaypath(list_item->name, info->prefix);
+
+	if (!sub)
+		die(_("No url found for submodule path '%s' in .gitmodules"),
+		      displaypath);
+
+	if (!is_submodule_populated_gently(list_item->name, NULL))
+		return;
+
+	toplevel = xgetcwd();
+
+	prepare_submodule_repo_env(&cp.env_array);
+	cp.use_shell = 1;
+	cp.dir = list_item->name;
+
+	if (info->argc == 1) {
+		argv_array_pushf(&cp.env_array, "name=%s", sub->name);
+		argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
+		argv_array_pushf(&cp.env_array, "sha1=%s",
+				 oid_to_hex(&list_item->oid));
+		argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
+
+		argv_array_pushf(&cp.args, "path=%s; %s", list_item->name,
+				 info->argv[0]);
+	} else {
+		for (i = 0; i < info->argc; i++)
+			argv_array_push(&cp.args, info->argv[i]);
+	}
+
+	if (!info->quiet)
+		printf(_("Entering '%s'\n"), displaypath);
+
+	if (info->argv[0] && run_command(&cp))
+		die(_("run_command returned non-zero status for %s\n."),
+		      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", "foreach", "--recursive",
+				 NULL);
+
+		if (info->quiet)
+			argv_array_push(&cpr.args, "--quiet");
+
+		for (i = 0; i < info->argc; i++)
+			argv_array_push(&cpr.args, info->argv[i]);
+
+		if (run_command(&cpr))
+			die(_("run_command returned non-zero status while"
+			      "recursing in the nested submodules of %s\n."),
+			      displaypath);
+	}
+
+	free(displaypath);
+	free(toplevel);
+}
+
+static int module_foreach(int argc, const char **argv, const char *prefix)
+{
+	struct cb_foreach info;
+	struct pathspec pathspec;
+	struct module_list list = MODULE_LIST_INIT;
+	int quiet = 0;
+	int recursive = 0;
+
+	struct option module_foreach_options[] = {
+		OPT__QUIET(&quiet, N_("Suppress output of entering each submodule command")),
+		OPT_BOOL(0, "recursive", &recursive,
+			 N_("Recurse into nested submodules")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper foreach [--quiet] [--recursive] <command>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_foreach_options,
+			     git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);
+
+	if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)
+		BUG("module_list_compute should not choke on empty pathspec");
+
+	info.argc = argc;
+	info.argv = argv;
+	info.prefix = prefix;
+	info.quiet = !!quiet;
+	info.recursive = !!recursive;
+
+	gitmodules_config();
+	for_each_submodule_list(list, runcommand_in_submodule, &info);
+
+	return 0;
+}
+
 static int clone_submodule(const char *path, const char *gitdir, const char *url,
 			   const char *depth, struct string_list *reference,
 			   int quiet, int progress)
@@ -1212,6 +1364,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},
+	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
 	{"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 ea6f56337..032fd2540 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -322,45 +322,8 @@ cmd_foreach()
 		shift
 	done
 
-	toplevel=$(pwd)
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
 
-	# dup stdin so that it can be restored when running the external
-	# command in the subshell (and a recursive call to this function)
-	exec 3<&0
-
-	{
-		git submodule--helper list --prefix "$wt_prefix" ||
-		echo "#unmatched" $?
-	} |
-	while read -r mode sha1 stage sm_path
-	do
-		die_if_unmatched "$mode" "$sha1"
-		if test -e "$sm_path"/.git
-		then
-			displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
-			say "$(eval_gettext "Entering '\$displaypath'")"
-			name=$(git submodule--helper name "$sm_path")
-			(
-				prefix="$prefix$sm_path/"
-				sanitize_submodule_env
-				cd "$sm_path" &&
-				# we make $path available to scripts ...
-				path=$sm_path &&
-				sm_path=$displaypath &&
-				if test $# -eq 1
-				then
-					eval "$1"
-				else
-					"$@"
-				fi &&
-				if test -n "$recursive"
-				then
-					cmd_foreach "--recursive" "$@"
-				fi
-			) <&3 3<&- ||
-			die "$(eval_gettext "Stopping at '\$displaypath'; script returned non-zero status.")"
-		fi
-	done
 }
 
 #
-- 
2.13.0

[PATCH] submodule foreach: correct $sm_path in nested submodules from a dir

[Stefan Beller]

When running 'git submodule foreach' from a subdirectory of your
repository, nested submodules get a bogus value for $sm_path:
For a submodule 'sub' that contains a nested submodule 'nested',
running 'git -C dir submodule foreach echo $path' would report
path='../nested' for the nested submodule. The first part '../' is
derived from the logic computing the relative path from $pwd to the
root of the superproject. The second part is the submodule path inside
the submodule. This value is of little use and is hard to document.

There are two different possible solutions that have more value:
(a) The path value is documented as the path from the toplevel of the
    superproject to the mount point of the submodule.
    In this case we would want to have path='sub/nested'.

(b) As Ramsay noticed the documented value is wrong. For the non-nested
    case the path is equal to the relative path from $pwd to the
    submodules working directory. When following this model,
    the expected value would be path='../sub/nested'.

The behavior for (b) was introduced in 091a6eb0fe (submodule: drop the
top-level requirement, 2013-06-16) the intent for $path seemed to be
relative to $cwd to the submodule worktree, but that did not work for
nested submodules, as the intermittent submodules were not included in
the path.

If we were to fix the meaning of the $path using (a) such that "path"
is "the path from the toplevel of the superproject to the mount point
of the submodule", we would break any existing submodule user that runs
foreach from non-root of the superproject as the non-nested submodule
'../sub' would change its path to 'sub'.

If we would fix the meaning of the $path using (b), such that "path"
is "the relative path from $pwd to the submodule", then we would break
any user that uses nested submodules (even from the root directory) as
the 'nested' would become 'sub/nested'.

Both groups can be found in the wild.  The author has no data if one group
outweighs the other by large margin, and offending each one seems equally
bad at first.  However in the authors imagination it is better to go with
(a) as running from a sub directory sounds like it is carried out
by a human rather than by some automation task.  With a human on
the keyboard the feedback loop is short and the changed behavior can be
adapted to quickly unlike some automation that can break silently.

To ameliorate the situation, perform these changes
* Document 'sm_path' instead of 'path'.
  As using a variable '$path' may be harmful to users due to
  capitalization issues, see 64394e3ae9 (git-submodule.sh: Don't
  use $path variable in eval_gettext string, 2012-04-17). Adjust
  the documentation to advocate for using $sm_path,  which contains
  the same value. We still make the 'path' variable available,
  though not documented.

* Clarify the 'toplevel' variable documentation.
  It does not contain the topmost superproject as the author assumed,
  but the direct superproject, such that $toplevel/$sm_path is the
  actual absolute path of the submodule.

* The variable '$displaypath' was accessible but undocumented.
  Rename it '$displaypath' to '$dpath'. Document what it contains.
  Users that are broken by the behavior change of 'sm_path' introduced
  in this commit, can switch from '$path' to '$dpath'.

Discussed-with: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

Ramsay,
I would think this would be least offensive to all parties involved.
With my current understanding of the situation
I think this is the best fix for now.

Prathamesh, sorry for the missleading suggestions earlier
on how to approach this bug. Let's see how the discussion turns out on this
one before rebasing the rewrite in C on this one.

Thanks,
Stefan

 Documentation/git-submodule.txt | 32 ++++++++++++++++++--------------
 git-submodule.sh                |  7 +++----
 t/t7407-submodule-foreach.sh    | 39 ++++++++++++++++++++++++++++++++++++---
 3 files changed, 57 insertions(+), 21 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 74bc6200d5..52e3ef1325 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -218,20 +218,24 @@ information too.
 
 foreach [--recursive] <command>::
 	Evaluates an arbitrary shell command in each checked out submodule.
-	The command has access to the variables $name, $path, $sha1 and
-	$toplevel:
-	$name is the name of the relevant submodule section in .gitmodules,
-	$path is the name of the submodule directory relative to the
-	superproject, $sha1 is the commit as recorded in the superproject,
-	and $toplevel is the absolute path to the top-level of the superproject.
-	Any submodules defined in the superproject but not checked out are
-	ignored by this command. Unless given `--quiet`, foreach prints the name
-	of each submodule before evaluating the command.
-	If `--recursive` is given, submodules are traversed recursively (i.e.
-	the given shell command is evaluated in nested submodules as well).
-	A non-zero return from the command in any submodule causes
-	the processing to terminate. This can be overridden by adding '|| :'
-	to the end of the command.
+	The command has access to the following variables:
++
+* `$name` is the name of the relevant submodule section in .gitmodules,
+* `$sha1` is the commit as recorded in the superproject.
+* `$sm_path` is the path recorded in the superproject.
+* `$toplevel` is the absolute path to its superproject, such that
+  `$toplevel/$sm_path` is the absolute path of the submodule.
+* `$dpath` contains the relative path from the current working directory
+   to the submodules root directory.
++
+Any submodules defined in the superproject but not checked out are
+ignored by this command. Unless given `--quiet`, foreach prints the name
+of each submodule before evaluating the command.
+If `--recursive` is given, submodules are traversed recursively (i.e.
+the given shell command is evaluated in nested submodules as well).
+A non-zero return from the command in any submodule causes
+the processing to terminate. This can be overridden by adding '|| :'
+to the end of the command.
 +
 As an example, the command below will show the path and currently
 checked out commit for each submodule:
diff --git a/git-submodule.sh b/git-submodule.sh
index c0d0e9a4c6..8133640fa1 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -337,14 +337,13 @@ cmd_foreach()
 		die_if_unmatched "$mode" "$sha1"
 		if test -e "$sm_path"/.git
 		then
-			displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
-			say "$(eval_gettext "Entering '\$displaypath'")"
+			dpath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
+			say "$(eval_gettext "Entering '\$dpath'")"
 			name=$(git submodule--helper name "$sm_path")
 			(
 				prefix="$prefix$sm_path/"
 				sanitize_submodule_env
 				cd "$sm_path" &&
-				sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
 				# we make $path available to scripts ...
 				path=$sm_path &&
 				if test $# -eq 1
@@ -358,7 +357,7 @@ cmd_foreach()
 					cmd_foreach "--recursive" "$@"
 				fi
 			) <&3 3<&- ||
-			die "$(eval_gettext "Stopping at '\$displaypath'; script returned non-zero status.")"
+			die "$(eval_gettext "Stopping at '\$dpath'; script returned non-zero status.")"
 		fi
 	done
 }
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6ba5daf42e..a082ca75aa 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -82,16 +82,16 @@ test_expect_success 'test basic "submodule foreach" usage' '
 
 cat >expect <<EOF
 Entering '../sub1'
-$pwd/clone-foo1-../sub1-$sub1sha1
+$pwd/clone-foo1-sub1-../sub1-$sub1sha1
 Entering '../sub3'
-$pwd/clone-foo3-../sub3-$sub3sha1
+$pwd/clone-foo3-sub3-../sub3-$sub3sha1
 EOF
 
 test_expect_success 'test "submodule foreach" from subdirectory' '
 	mkdir clone/sub &&
 	(
 		cd clone/sub &&
-		git submodule foreach "echo \$toplevel-\$name-\$sm_path-\$sha1" >../../actual
+		git submodule foreach "echo \$toplevel-\$name-\$sm_path-\$dpath-\$sha1" >../../actual
 	) &&
 	test_i18ncmp expect actual
 '
@@ -197,6 +197,39 @@ test_expect_success 'test messages from "foreach --recursive" from subdirectory'
 	test_i18ncmp expect actual
 '
 
+sub1sha1=$(cd clone2/sub1 && git rev-parse HEAD)
+sub2sha1=$(cd clone2/sub2 && git rev-parse HEAD)
+sub3sha1=$(cd clone2/sub3 && git rev-parse HEAD)
+nested1sha1=$(cd clone2/nested1 && git rev-parse HEAD)
+nested2sha1=$(cd clone2/nested1/nested2 && git rev-parse HEAD)
+nested3sha1=$(cd clone2/nested1/nested2/nested3 && git rev-parse HEAD)
+submodulesha1=$(cd clone2/nested1/nested2/nested3/submodule && git rev-parse HEAD)
+
+cat >expect <<EOF
+Entering '../nested1'
+$pwd/clone2-nested1-nested1-../nested1-$nested1sha1
+Entering '../nested1/nested2'
+$pwd/clone2/nested1-nested2-nested2-../nested1/nested2-$nested2sha1
+Entering '../nested1/nested2/nested3'
+$pwd/clone2/nested1/nested2-nested3-nested3-../nested1/nested2/nested3-$nested3sha1
+Entering '../nested1/nested2/nested3/submodule'
+$pwd/clone2/nested1/nested2/nested3-submodule-submodule-../nested1/nested2/nested3/submodule-$submodulesha1
+Entering '../sub1'
+$pwd/clone2-foo1-sub1-../sub1-$sub1sha1
+Entering '../sub2'
+$pwd/clone2-foo2-sub2-../sub2-$sub2sha1
+Entering '../sub3'
+$pwd/clone2-foo3-sub3-../sub3-$sub3sha1
+EOF
+
+test_expect_success 'test "submodule foreach --recursive" from subdirectory' '
+	(
+		cd clone2/untracked &&
+		git submodule foreach --recursive "echo \$toplevel-\$name-\$sm_path-\$dpath-\$sha1" >../../actual
+	) &&
+	test_i18ncmp expect actual
+'
+
 cat > expect <<EOF
 nested1-nested1
 nested2-nested2
-- 
2.13.0.17.gab62347cd9

Re: [GSoC][PATCH v6 2/2] submodule: port subcommand foreach from shell to C

[Stefan Beller]

On Fri, Jun 2, 2017 at 4:24 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> This aims to make git-submodule foreach a builtin. This is the very
> first step taken in this direction. Hence, 'foreach' is ported to
> submodule--helper, and submodule--helper is called from git-submodule.sh.
> The code is split up to have one function to obtain all the list of
> submodules. This function acts as the front-end of git-submodule foreach
> subcommand. It calls the function for_each_submodule_list, which basically
> loops through the list and calls function fn, which in this case is
> runcommand_in_submodule. This third function is a calling function that
> takes care of running the command in that submodule, and recursively
> perform the same when --recursive is flagged.
>
> The first function module_foreach first 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 case of submodule subcommand
> foreach is runcommand_in_submodule) is called for each entry.
>
> The third function runcommand_in_submodule, generates a submodule struct sub
> for $name, value and then later prepends name=sub->name; and other
> value assignment to the env argv_array structure of a child_process.
> Also the <command> of submodule-foreach is push to args argv_array
> structure and finally, using run_command the commands are executed
> using a shell.
>
> The third function also takes care of the recursive flag, by creating
> a separate child_process structure and prepending "--super-prefix displaypath",
> to the args argv_array structure. Other required arguments and the
> input <command> of submodule-foreach is also appended to this argv_array.
>

Is the commit message still accurate?
You describe the changes between the versions below the --- line,
that is not recorded in the permanent commit history.

In the commit message is less important to write "what" is happening,
because that can easily be read from the patch/commit itself, but rather
"why" things happen, such as design choices, maybe:

    This aims to make git-submodule foreach a builtin. This is the very
    first step taken in this direction. Hence, 'foreach' is ported to
    submodule--helper, and submodule--helper is called from git-submodule.sh.

    We'll introduce 3 functions, one that is exposed to the command line
    and handles command line arguments, one to iterate over a set of
    submodules, and finally one to execute an arbitrary shell command
    in the submodule.

    Attention must be paid to the 'path' variable, see 64394e3ae9
    (git-submodule.sh: Don't use $path variable in eval_gettext string,
    2012-04-17) details. The path varialbe is not exposed into the environment
    of the invoked shell, but we just give "path=%s;" as the first argument.

    We do not need to condition on the number of arguments as in 1c4fb136db
    (submodule foreach: skip eval for more than one argument, 2013-09-27)
    as we will run exactly one shell in the submodules directory.

    Sign-off-...

>
> Other than that, additionally the case of no. of arugments in <command>
> being equal to 1 is also considered separetly.
> THe reason of having this change in the shell script was given in the
> commit 1c4fb136db.
> According to my understanding, eval "$1" executes $1 in same shell,
> whereas "$@" gets executed in a separate shell, which doesn't allow
> "$@" to access the env variables $name, $path, etc.
> Hence, to keep the ported function similar, this condition is also
> added.

This paragraph would be a good candidate for the commit message, too.
However as we rewrite it in C, we will spawn exactly one shell no matter
how many arguments we have (well for 0 we have no shell, but for 1 or more
arguments we'll spawn exactly one shell?)

> +       } else if (prefix) {
> +               struct strbuf sb = STRBUF_INIT;
> +               char *displaypath = xstrdup(relative_path(path, prefix, &sb));
> +               strbuf_release(&sb);
> +               return displaypath;

Note to self (or any other that is interested in long term clean code):
    I have seen this pattern a couple of times, a strbuf just to appease
    the argument list of relative_path.
    (init_submodule, prepare_to_clone_next_submodule,
    resolve_relative_path in submodule--helper
    cmd_rev_parse in builtin/rev-parse
    connect_work_tree_and_git_dir in dir.c
    write_name_quoted_relative in quote.c
    get_superproject_working_tree in submodule.c
    cmd_main in test-path-utils;
    actually all uses of this function :( )
    We should really make a relative_path function that can work
    without the need of a strbuf, maybe just wrap the 3 lines into a new
    function, or remove the strbuf from the argument list.
    (The potential memleak is horrible to fix though. But as seen here
    we could just always return an allocated string and
    mandate the caller to free it)

> +struct cb_foreach {
> +       int argc;
> +       const char **argv;
> +       const char *prefix;
> +       unsigned int quiet: 1;
> +       unsigned int recursive: 1;
> +};
> +#define CB_FOREACH_INIT { 0, NULL, NULL, 0, 0 }
> +
> +static void runcommand_in_submodule(const struct cache_entry *list_item,
> +                                   void *cb_data)

As we only ever use list_item->name, we could also change
the argument list to take a "const char *[submodule_]path".

> +       prepare_submodule_repo_env(&cp.env_array);
> +       cp.use_shell = 1;
> +       cp.dir = list_item->name;
> +
> +       if (info->argc == 1) {
> +               argv_array_pushf(&cp.env_array, "name=%s", sub->name);
> +               argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
> +               argv_array_pushf(&cp.env_array, "sha1=%s",
> +                                oid_to_hex(&list_item->oid));
> +               argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
> +
> +               argv_array_pushf(&cp.args, "path=%s; %s", list_item->name,
> +                                info->argv[0]);

In the argc != 1 case we also want to have the env_array filled with
useful variables. (It seems we do not have tests for that?)
To test for that we'd need a test similar as in 1c4fb136d,
just with
    git submodule foreach echo \$sm_path \$dpath
for example.

So I think you can move the pushes to the env array outside this condition.
As we set cp.use_shell unconditionally, we do not need to construct
the first argument specially with path preset, but instead we can just prepend
it in the array:

    argv_array_pushf(&cp.env_array, "name=%s", sub->name);
    ... // more env stuff

    argv_array_pushf(&cp.args, "path=%s;", list_item->name);
    for (i = 0; i < info->argc; i++)
        argv_array_push(&cp.args, info->argv[i]);

should do?


> +
> +       if (info->argv[0] && run_command(&cp))
> +               die(_("run_command returned non-zero status for %s\n."),

This would rather be
    die(_("Stopping at '%s'; script returned non-zero status."), displaypath);
to imitate the shell version faithfully?


> +
> +               if (run_command(&cpr))
> +                       die(_("run_command returned non-zero status while"
> +                             "recursing in the nested submodules of %s\n."),
> +                             displaypath);

same here. As the inner process would have already printed the "Stopping..."
we would not need to repeat it, though.

So maybe

    /* no need to report error, child does: */
    run_command(&cpr);

The rest below looks good. :)

Thanks,
Stefan

Re: [PATCH] submodule foreach: correct $sm_path in nested submodules from a dir

[Ramsay Jones]

On 03/06/17 01:37, Stefan Beller wrote:
> When running 'git submodule foreach' from a subdirectory of your
> repository, nested submodules get a bogus value for $sm_path:
> For a submodule 'sub' that contains a nested submodule 'nested',
> running 'git -C dir submodule foreach echo $path' would report
> path='../nested' for the nested submodule. The first part '../' is
> derived from the logic computing the relative path from $pwd to the
> root of the superproject. The second part is the submodule path inside
> the submodule. This value is of little use and is hard to document.
> 
> There are two different possible solutions that have more value:
> (a) The path value is documented as the path from the toplevel of the
>     superproject to the mount point of the submodule.
>     In this case we would want to have path='sub/nested'.
> 
> (b) As Ramsay noticed the documented value is wrong. For the non-nested
>     case the path is equal to the relative path from $pwd to the
>     submodules working directory. When following this model,
>     the expected value would be path='../sub/nested'.
> 
> The behavior for (b) was introduced in 091a6eb0fe (submodule: drop the

Ah, so prior to 091a6eb0fe the documentation of $path was actually
correct - you could not run the command from a sub-directory, so
$path _was_ the 'name of the submodule directory relative to the
superproject'. (The fact that git-ls-files worked from a subdirectory
is not in the least relevant - it never was!) ;-)

> top-level requirement, 2013-06-16) the intent for $path seemed to be
> relative to $cwd to the submodule worktree, but that did not work for
> nested submodules, as the intermittent submodules were not included in
                            ^^^^^^^^^^^^
intermediate?

Hmm, so nobody noticed the change in behaviour (and, of course, that
the documentation hadn't been updated to match) since 2013!

> the path.
> 
> If we were to fix the meaning of the $path using (a) such that "path"
> is "the path from the toplevel of the superproject to the mount point
> of the submodule", we would break any existing submodule user that runs
> foreach from non-root of the superproject as the non-nested submodule
> '../sub' would change its path to 'sub'.
> 
> If we would fix the meaning of the $path using (b), such that "path"
> is "the relative path from $pwd to the submodule", then we would break
> any user that uses nested submodules (even from the root directory) as
> the 'nested' would become 'sub/nested'.
> 
> Both groups can be found in the wild.  The author has no data if one group
> outweighs the other by large margin, and offending each one seems equally
> bad at first.  However in the authors imagination it is better to go with
> (a) as running from a sub directory sounds like it is carried out
> by a human rather than by some automation task.  With a human on
> the keyboard the feedback loop is short and the changed behavior can be
> adapted to quickly unlike some automation that can break silently.

I do not use submodules (I have absolutely no interest in them, except
in a general wish to improve git sense). So, I have no idea what kind
of impact either change will have. However, FWIW, I agree with your
reasoning here. (Not that I actually get a vote, but I vote for a!)

> To ameliorate the situation, perform these changes
> * Document 'sm_path' instead of 'path'.
>   As using a variable '$path' may be harmful to users due to
>   capitalization issues, see 64394e3ae9 (git-submodule.sh: Don't
>   use $path variable in eval_gettext string, 2012-04-17). Adjust
>   the documentation to advocate for using $sm_path,  which contains
>   the same value. We still make the 'path' variable available,
>   though not documented.
> 
> * Clarify the 'toplevel' variable documentation.
>   It does not contain the topmost superproject as the author assumed,
>   but the direct superproject, such that $toplevel/$sm_path is the
>   actual absolute path of the submodule.
> 
> * The variable '$displaypath' was accessible but undocumented.
>   Rename it '$displaypath' to '$dpath'. Document what it contains.
>   Users that are broken by the behavior change of 'sm_path' introduced
>   in this commit, can switch from '$path' to '$dpath'.
> 
> Discussed-with: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 

  Documentation/git-submodule.txt | 32 ++++++++++++++++++--------------
>  git-submodule.sh                |  7 +++----
>  t/t7407-submodule-foreach.sh    | 39 ++++++++++++++++++++++++++++++++++++---
>  3 files changed, 57 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index 74bc6200d5..52e3ef1325 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -218,20 +218,24 @@ information too.
>  
>  foreach [--recursive] <command>::
>  	Evaluates an arbitrary shell command in each checked out submodule.
> -	The command has access to the variables $name, $path, $sha1 and
> -	$toplevel:
> -	$name is the name of the relevant submodule section in .gitmodules,
> -	$path is the name of the submodule directory relative to the
> -	superproject, $sha1 is the commit as recorded in the superproject,
> -	and $toplevel is the absolute path to the top-level of the superproject.
> -	Any submodules defined in the superproject but not checked out are
> -	ignored by this command. Unless given `--quiet`, foreach prints the name
> -	of each submodule before evaluating the command.
> -	If `--recursive` is given, submodules are traversed recursively (i.e.
> -	the given shell command is evaluated in nested submodules as well).
> -	A non-zero return from the command in any submodule causes
> -	the processing to terminate. This can be overridden by adding '|| :'
> -	to the end of the command.
> +	The command has access to the following variables:
> ++
> +* `$name` is the name of the relevant submodule section in .gitmodules,
> +* `$sha1` is the commit as recorded in the superproject.
> +* `$sm_path` is the path recorded in the superproject.

Just to be sure, the 'path recorded in the superproject' means the
same thing as the 'name of the submodule directory relative to the
superproject'. Yes?

> +* `$toplevel` is the absolute path to its superproject, such that
> +  `$toplevel/$sm_path` is the absolute path of the submodule.
> +* `$dpath` contains the relative path from the current working directory
> +   to the submodules root directory.

Subject to the above, this all looks good to me. (I can't comment
on the implementation - I just assume that it correctly implements
the above).

Thanks!

ATB,
Ramsay Jones

Re: [GSoC][PATCH v6 2/2] submodule: port subcommand foreach from shell to C

[Prathamesh Chavan]

On Sat, Jun 3, 2017 at 7:43 AM, Stefan Beller <sbeller@google.com> wrote:
> On Fri, Jun 2, 2017 at 4:24 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:
>> This aims to make git-submodule foreach a builtin. This is the very
>> first step taken in this direction. Hence, 'foreach' is ported to
>> submodule--helper, and submodule--helper is called from git-submodule.sh.
>> The code is split up to have one function to obtain all the list of
>> submodules. This function acts as the front-end of git-submodule foreach
>> subcommand. It calls the function for_each_submodule_list, which basically
>> loops through the list and calls function fn, which in this case is
>> runcommand_in_submodule. This third function is a calling function that
>> takes care of running the command in that submodule, and recursively
>> perform the same when --recursive is flagged.
>>
>> The first function module_foreach first 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 case of submodule subcommand
>> foreach is runcommand_in_submodule) is called for each entry.
>>
>> The third function runcommand_in_submodule, generates a submodule struct sub
>> for $name, value and then later prepends name=sub->name; and other
>> value assignment to the env argv_array structure of a child_process.
>> Also the <command> of submodule-foreach is push to args argv_array
>> structure and finally, using run_command the commands are executed
>> using a shell.
>>
>> The third function also takes care of the recursive flag, by creating
>> a separate child_process structure and prepending "--super-prefix displaypath",
>> to the args argv_array structure. Other required arguments and the
>> input <command> of submodule-foreach is also appended to this argv_array.
>>
>
> Is the commit message still accurate?
> You describe the changes between the versions below the --- line,
> that is not recorded in the permanent commit history.
>
> In the commit message is less important to write "what" is happening,
> because that can easily be read from the patch/commit itself, but rather
> "why" things happen, such as design choices, maybe:
>
>     This aims to make git-submodule foreach a builtin. This is the very
>     first step taken in this direction. Hence, 'foreach' is ported to
>     submodule--helper, and submodule--helper is called from git-submodule.sh.
>
>     We'll introduce 3 functions, one that is exposed to the command line
>     and handles command line arguments, one to iterate over a set of
>     submodules, and finally one to execute an arbitrary shell command
>     in the submodule.
>
>     Attention must be paid to the 'path' variable, see 64394e3ae9
>     (git-submodule.sh: Don't use $path variable in eval_gettext string,
>     2012-04-17) details. The path varialbe is not exposed into the environment
>     of the invoked shell, but we just give "path=%s;" as the first argument.
>
>     We do not need to condition on the number of arguments as in 1c4fb136db
>     (submodule foreach: skip eval for more than one argument, 2013-09-27)
>     as we will run exactly one shell in the submodules directory.
>
>     Sign-off-...
>
>>
>> Other than that, additionally the case of no. of arugments in <command>
>> being equal to 1 is also considered separetly.
>> THe reason of having this change in the shell script was given in the
>> commit 1c4fb136db.
>> According to my understanding, eval "$1" executes $1 in same shell,
>> whereas "$@" gets executed in a separate shell, which doesn't allow
>> "$@" to access the env variables $name, $path, etc.
>> Hence, to keep the ported function similar, this condition is also
>> added.
>
> This paragraph would be a good candidate for the commit message, too.
> However as we rewrite it in C, we will spawn exactly one shell no matter
> how many arguments we have (well for 0 we have no shell, but for 1 or more
> arguments we'll spawn exactly one shell?)
>

I was trying to explaing the condition of the code in git-submodule.sh,
before porting. To be more clear, I meant that when we run the command
eval "$1", it runs in the same shell in which the cmd_foreach has been running,
unlike in the case of "$@", in which case, the command in executed in a separate
shell.

>> +       } else if (prefix) {
>> +               struct strbuf sb = STRBUF_INIT;
>> +               char *displaypath = xstrdup(relative_path(path, prefix, &sb));
>> +               strbuf_release(&sb);
>> +               return displaypath;
>
> Note to self (or any other that is interested in long term clean code):
>     I have seen this pattern a couple of times, a strbuf just to appease
>     the argument list of relative_path.
>     (init_submodule, prepare_to_clone_next_submodule,
>     resolve_relative_path in submodule--helper
>     cmd_rev_parse in builtin/rev-parse
>     connect_work_tree_and_git_dir in dir.c
>     write_name_quoted_relative in quote.c
>     get_superproject_working_tree in submodule.c
>     cmd_main in test-path-utils;
>     actually all uses of this function :( )
>     We should really make a relative_path function that can work
>     without the need of a strbuf, maybe just wrap the 3 lines into a new
>     function, or remove the strbuf from the argument list.
>     (The potential memleak is horrible to fix though. But as seen here
>     we could just always return an allocated string and
>     mandate the caller to free it)
>
>> +struct cb_foreach {
>> +       int argc;
>> +       const char **argv;
>> +       const char *prefix;
>> +       unsigned int quiet: 1;
>> +       unsigned int recursive: 1;
>> +};
>> +#define CB_FOREACH_INIT { 0, NULL, NULL, 0, 0 }
>> +
>> +static void runcommand_in_submodule(const struct cache_entry *list_item,
>> +                                   void *cb_data)
>
> As we only ever use list_item->name, we could also change
> the argument list to take a "const char *[submodule_]path".
>
>> +       prepare_submodule_repo_env(&cp.env_array);
>> +       cp.use_shell = 1;
>> +       cp.dir = list_item->name;
>> +
>> +       if (info->argc == 1) {
>> +               argv_array_pushf(&cp.env_array, "name=%s", sub->name);
>> +               argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
>> +               argv_array_pushf(&cp.env_array, "sha1=%s",
>> +                                oid_to_hex(&list_item->oid));
>> +               argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
>> +
>> +               argv_array_pushf(&cp.args, "path=%s; %s", list_item->name,
>> +                                info->argv[0]);
>
> In the argc != 1 case we also want to have the env_array filled with
> useful variables. (It seems we do not have tests for that?)
> To test for that we'd need a test similar as in 1c4fb136d,
> just with
>     git submodule foreach echo \$sm_path \$dpath
> for example.
>
> So I think you can move the pushes to the env array outside this condition.
> As we set cp.use_shell unconditionally, we do not need to construct
> the first argument specially with path preset, but instead we can just prepend
> it in the array:
>
>     argv_array_pushf(&cp.env_array, "name=%s", sub->name);
>     ... // more env stuff
>
>     argv_array_pushf(&cp.args, "path=%s;", list_item->name);
>     for (i = 0; i < info->argc; i++)
>         argv_array_push(&cp.args, info->argv[i]);
>
> should do?
>
>
Yes, even I think this is what should be done. But for the given code,

if test $# -eq 1
then
    eval "$1"
else
    "$@"
fi &&

in case when $# is not equal to 1,
the <command> "$@" gets executed in a separate shell and hence could not
access the $name, $path, $toplevel, $sha1 variables.
Infact, it can be observed, that the output of the <command>
echo \$name is something bogus and not the submodule's name.

Also, in the given test suite, the env variables are used only in those
cases where no. of arguments is one.

Hence, to keep the ported code similar to the privious one, such an
additional case of argc == 1 needs to be considered.

Thanks,
Prathamesh Chavan

Re: [PATCH] submodule foreach: correct $sm_path in nested submodules from a dir

[Ramsay Jones]

On 03/06/17 15:07, Ramsay Jones wrote:
[snip]

>> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
>> index 74bc6200d5..52e3ef1325 100644
>> --- a/Documentation/git-submodule.txt
>> +++ b/Documentation/git-submodule.txt
>> @@ -218,20 +218,24 @@ information too.
>>  
>>  foreach [--recursive] <command>::
>>  	Evaluates an arbitrary shell command in each checked out submodule.
>> -	The command has access to the variables $name, $path, $sha1 and
>> -	$toplevel:
>> -	$name is the name of the relevant submodule section in .gitmodules,
>> -	$path is the name of the submodule directory relative to the
>> -	superproject, $sha1 is the commit as recorded in the superproject,
>> -	and $toplevel is the absolute path to the top-level of the superproject.
>> -	Any submodules defined in the superproject but not checked out are
>> -	ignored by this command. Unless given `--quiet`, foreach prints the name
>> -	of each submodule before evaluating the command.
>> -	If `--recursive` is given, submodules are traversed recursively (i.e.
>> -	the given shell command is evaluated in nested submodules as well).
>> -	A non-zero return from the command in any submodule causes
>> -	the processing to terminate. This can be overridden by adding '|| :'
>> -	to the end of the command.
>> +	The command has access to the following variables:
>> ++
>> +* `$name` is the name of the relevant submodule section in .gitmodules,
>> +* `$sha1` is the commit as recorded in the superproject.
>> +* `$sm_path` is the path recorded in the superproject.
> 
> Just to be sure, the 'path recorded in the superproject' means the
> same thing as the 'name of the submodule directory relative to the
> superproject'. Yes?
> 
>> +* `$toplevel` is the absolute path to its superproject, such that
>> +  `$toplevel/$sm_path` is the absolute path of the submodule.
>> +* `$dpath` contains the relative path from the current working directory
>> +   to the submodules root directory.

BTW, I have not given any thought to what happens to these
variables/paths if you run '--recursive'. I assume the new
'recursed' submodule takes on the role of the 'superproject'
in the definitions above. Hmm, but what does $dpath show?
(having 'recursed', is the cwd now at the top level of the
'new' superproject?)

Hmm, you have changed several command to recurse into
submodule(s) (I haven't followed that effort closely), so
I suppose it should have some measure of consistency with
those commands. (whatever that means ;-)

ATB,
Ramsay Jones

Re: [PATCH] submodule foreach: correct $sm_path in nested submodules from a dir

[Jonathan Nieder]

Hi,

This patch seems to aim to do multiple things.  Initial thoughts:

Stefan Beller wrote:

[...]
> To ameliorate the situation, perform these changes
> * Document 'sm_path' instead of 'path'.
>   As using a variable '$path' may be harmful to users due to
>   capitalization issues, see 64394e3ae9 (git-submodule.sh: Don't
>   use $path variable in eval_gettext string, 2012-04-17). Adjust
>   the documentation to advocate for using $sm_path,  which contains
>   the same value. We still make the 'path' variable available,
>   though not documented.

Making sm_path part of the public API as described here sounds like a
good idea (as a separate patch), to avoid conflicting with $PATH on
Windows.  It's convenient that scripts have access to the private
variable 'sm_path'.  The 'path' variable would still need to be
documented as a deprecated synonym so people working with existing
scripts can know how to update them.

> * Clarify the 'toplevel' variable documentation.
>   It does not contain the topmost superproject as the author assumed,
>   but the direct superproject, such that $toplevel/$sm_path is the
>   actual absolute path of the submodule.

This is very confusing.  I suspect it's a bug.  Can we make 'toplevel'
point to the topmost superproject (as a separate path)?

> * The variable '$displaypath' was accessible but undocumented.
>   Rename it '$displaypath' to '$dpath'. Document what it contains.
>   Users that are broken by the behavior change of 'sm_path' introduced
>   in this commit, can switch from '$path' to '$dpath'.

What does dpath stand for?  Renaming the variable to $dpath means that
scripts trying to adapt to this change would not work with previous
versions of git.  Would it make sense to use $displaypath for this for
compatibility?

What is the intent behind the sm_path behavior change in this patch?
Stepping back, what kind of scripts is this interface meant to support
(e.g., what is an example script that used this interface that would
be affected), and is there a straightforward way to support those use
cases without breaking existing scripts except where necessary?

To summarize, the patch leaves me a bit confused.  I think it would be
best to have multiple patches that solve one problem at a time, which
would hopefully make the story clearer.

Thanks and hope that helps,
Jonathan