submodule: port subcommand foreach from shell to C

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.

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

builtin/submodule--helper.c +14 -0

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

builtin/submodule--helper.c +4 -0

+
+                               strbuf_addstr(&cmd, "name=");
+                               strbuf_addstr(&cmd, sub->name);
+                               strbuf_addstr(&cmd, "; ");

builtin/submodule--helper.c +15 -0

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

builtin/submodule--helper.c +6 -0

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

builtin/submodule--helper.c +9 -0

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

builtin/submodule--helper.c +10 -0

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

builtin/submodule--helper.c +15 -0

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

The First function module_foreach first parses the options present in

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

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.

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

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.

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

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

Quoted 11 lines

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

Quoted 17 lines · Ramsay Jones

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

Quoted 11 lines

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

Quoted 9 lines

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.

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.

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

+ struct cb_foreach info;

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 <[email protected]>

Mentored-by Stefan Beller <[email protected]>

Signed-off-by Prathamesh Chavan <[email protected]>

---

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

builtin/submodule--helper.c +0 -0

index 566a5b6a6..1a5b26c22 100644

builtin/submodule--helper.c +11 -0

 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;

builtin/submodule--helper.c +5 -0

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

builtin/submodule--helper.c +5 -0

+	} else {
+		displaypath = xstrdup(list_item->name);
+	}
+
+	sub = submodule_from_path(null_sha1, list_item->name);

builtin/submodule--helper.c +19 -0

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

builtin/submodule--helper.c +13 -0

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

git-submodule.sh +1 -38

index c0d0e9a4c..032fd2540 100755
@@ -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
 }
 
 #

Signature · 1 line

2.11.0

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

+static void for_each_submodule_list(const struct module_list list, submodule_list_func_t fn, void *cb_data)

+ return;

Quoted 5 lines · Prathamesh Chavan

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

Quoted 1 line

+static void for_each_submodule_list(const struct module_list list, submodule_list_func_t fn, void *cb_data)

..

Quoted 1 line

+ return;

no need for an explicit return in a void function.

builtin/submodule--helper.c static int module_name(int argc, const char **argv, const char *prefix) · line 525

Quoted 8 lines

builtin/submodule--helper.c +8 -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 }

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

builtin/submodule--helper.c static int module_name(int argc, const char **argv, const char *prefix) · line 624

Quoted 7 lines

builtin/submodule--helper.c +6 -0

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

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 <[email protected]>

Mentored-by Christian Couder <[email protected]>

Mentored-by Stefan Beller <[email protected]>

Signed-off-by Prathamesh Chavan <[email protected]>

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

builtin/submodule--helper.c +2 -0

index 566a5b6a6..4e19beaff 100644
@@ -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;
 }
 

builtin/submodule--helper.c +7 -0

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

builtin/submodule--helper.c +1 -0

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

builtin/submodule--helper.c +26 -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);

builtin/submodule--helper.c +22 -0

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

builtin/submodule--helper.c +7 -0

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

git-submodule.sh +1 -38

index c0d0e9a4c..032fd2540 100755
@@ -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
 }
 
 #

Signature · 1 line

2.11.0

You can set .git_cmd = 1 instead.

builtin/submodule--helper.c static int module_name(int argc, const char **argv, const char *prefix) · line 575

Quoted 4 lines

builtin/submodule--helper.c +4 -0

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

builtin/submodule--helper.c static int module_name(int argc, const char **argv, const char *prefix) · line 624

Quoted 6 lines

builtin/submodule--helper.c +6 -0

+
+     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 '!!'?

Quoted 8 lines · Brandon Williams

You can set .git_cmd = 1 instead.

Quoted 4 lines

builtin/submodule--helper.c +4 -0

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

Quoted 1 line

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)

Quoted 9 lines

builtin/submodule--helper.c static int module_name(int argc, const char **argv, const char *prefix) · line 624

Quoted 7 lines

builtin/submodule--helper.c +6 -0

+
+     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 \!\!)

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.

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 <[email protected]>

Mentored-by Christian Couder <[email protected]>

Mentored-by Stefan Beller <[email protected]>

Signed-off-by Prathamesh Chavan <[email protected]>

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

t/t7407-submodule-foreach.sh +21 -0

index 6ba5daf42..1c8d132d8 100755
@@ -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

t/t7407-submodule-foreach.sh +3 -0

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

Signature · 1 line

2.11.0

This aims to make git-submodule foreach a builtin.

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 <[email protected]>

Mentored-by Christian Couder <[email protected]>

Mentored-by Stefan Beller <[email protected]>

Signed-off-by Prathamesh Chavan <[email protected]>

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

builtin/submodule--helper.c +3 -0

index 566a5b6a6..343b6269c 100644
@@ -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)

builtin/submodule--helper.c +37 -0

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

builtin/submodule--helper.c +88 -0

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

git-submodule.sh +1 -38

index ea6f56337..032fd2540 100755
@@ -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
 }
 
 #

Signature · 1 line

2.11.0

Am 26.05.2017 um 17:17 schrieb Prathamesh Chavan:

Quoted 1 line

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

Quoted 5 lines · Johannes Sixt

Am 26.05.2017 um 17:17 schrieb Prathamesh Chavan:

Quoted 1 line

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

Quoted 3 lines

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

t/t7407-submodule-foreach.sh +19 -0

index 6ba5daf42..c2d66bab7 100755
@@ -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'

Am 26.05.2017 um 17:17 schrieb Prathamesh Chavan:

builtin/submodule--helper.c static int module_name(int argc, const char **argv, const char *prefix) · line 559

Quoted 1 line

builtin/submodule--helper.c +1 -0

+	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

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 <[email protected]>

Mentored-by Stefan Beller <[email protected]>

Signed-off-by Prathamesh Chavan <[email protected]>

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

git-submodule.sh +0 -0

index c0d0e9a4c..ea6f56337 100755

Quoted 51 lines · 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 <[email protected]>

Mentored-by Stefan Beller <[email protected]>

Signed-off-by Prathamesh Chavan <[email protected]>

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

git-submodule.sh +0 -1

index c0d0e9a4c..ea6f56337 100755
@@ -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 ...

git-submodule.sh cmd_foreach() · line 352

Quoted 6 lines

git-submodule.sh +1 -0

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

Quoted 1 line · Ramsay Jones

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

Quoted 3 lines

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.

Quoted 6 lines · Stefan Beller

Quoted 1 line · Ramsay Jones

Hmm, I'm not sure which documentation you are referring to,

Quite likely our fine manual pages. ;)

Quoted 15 lines

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

Quoted 3 lines

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

Quoted 7 lines

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

Quoted 5 lines

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