Rebooting pc/submodule-helper-foreach

---
 git-submodule.sh             |  1 -
 t/t7407-submodule-foreach.sh | 36 ++++++++++++++++++++++++++++++++++--
 2 files changed, 34 insertions(+), 3 deletions(-)

git-submodule.sh +0 -1

index 24914963ca2..331d71c908b 100755
@@ -345,7 +345,6 @@ 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 &&
 				if test $# -eq 1

t/t7407-submodule-foreach.sh +34 -2

index 6ba5daf42ee..5144cc6926b 100755
@@ -82,9 +82,9 @@ test_expect_success 'test basic "submodule foreach" usage' '
 
 cat >expect <<EOF
 Entering '../sub1'
-$pwd/clone-foo1-../sub1-$sub1sha1
+$pwd/clone-foo1-sub1-$sub1sha1
 Entering '../sub3'
-$pwd/clone-foo3-../sub3-$sub3sha1
+$pwd/clone-foo3-sub3-$sub3sha1
 EOF
 
 test_expect_success 'test "submodule foreach" from subdirectory' '

@@ -196,6 +196,38 @@ 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'
+toplevel: $pwd/clone2 name: nested1 path: nested1 hash: $nested1sha1
+Entering '../nested1/nested2'
+toplevel: $pwd/clone2/nested1 name: nested2 path: nested2 hash: $nested2sha1
+Entering '../nested1/nested2/nested3'
+toplevel: $pwd/clone2/nested1/nested2 name: nested3 path: nested3 hash: $nested3sha1
+Entering '../nested1/nested2/nested3/submodule'
+toplevel: $pwd/clone2/nested1/nested2/nested3 name: submodule path: submodule hash: $submodulesha1
+Entering '../sub1'
+toplevel: $pwd/clone2 name: foo1 path: sub1 hash: $sub1sha1
+Entering '../sub2'
+toplevel: $pwd/clone2 name: foo2 path: sub2 hash: $sub2sha1
+Entering '../sub3'
+toplevel: $pwd/clone2 name: foo3 path: sub3 hash: $sub3sha1
+EOF
+
+test_expect_success 'test "submodule foreach --recursive" from subdirectory' '
+	(
+		cd clone2/untracked &&
+		git submodule foreach --recursive "echo toplevel: \$toplevel name: \$name path: \$sm_path hash: \$sha1" >../../actual
+	) &&
+	test_i18ncmp expect actual
+'
 
 cat > expect <<EOF
 nested1-nested1

Signature · 1 line

2.17.0.441.gb46fe60e1d-goog

Ramsay Jones

2018-05-03 13:29 UTC #2 lore raw

Re: [PATCH 1/5] submodule foreach: correct '$path' in nested submodules from a subdirectory

Quoted 30 lines · Stefan Beller

From: Prathamesh Chavan <[email protected]>
When running 'git submodule foreach --recursive' from a subdirectory of your repository, nested submodules get a bogus value for $path:
For a submodule 'sub' that contains a nested submodule 'nested', running 'git -C dir submodule foreach echo $path' from the root of the superproject 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 three 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. If 'the' refers to the superproject holding this submodule ('sub' holding 'nested'), the path would be expected to be path='nested'.
(b) In case 'the' superproject is referring to the toplevel, which is the superproject in which the original command was invoked, then path is expected to be path='sub/nested'.
(c) The documentation explains $path as [...] "relative to the superproject", following 091a6eb0fe (submodule: drop the top-level requirement, 2013-06-16), such that the nested submodule would be expected as path='../sub/nested', when "the" superproject is the superproject, where the command was run from
(d) or the value of path='nested' is expected if we take the intermediate superproject into account. [This is the same as (a); it highlights that the documentation is not clear, but technically correct if we were to revert 091a6eb0fe.]

Quoted 4 lines

The behavior for (c) 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

----------------------------^^^^^^^^^^^^
intermediate

ATB,
Ramsay Jones

Jonathan Tan

2018-05-03 17:48 UTC #3 lore raw

Re: [PATCH 1/5] submodule foreach: correct '$path' in nested submodules from a subdirectory

[1] https://public-inbox.org/git/[email protected]/

Quoted 4 lines · On Wed, 2 May 2018 17:53:54 -0700 Stefan Beller <[email protected]> wrote:

From: Prathamesh Chavan <[email protected]>
When running 'git submodule foreach --recursive' from a subdirectory of your repository, nested submodules get a bogus value for $path:

I know I said in a previous e-mail [1] that we should use $path instead of $sm_path, but now I got confused because the test shows a difference in $sm_path, not $path. Maybe add "(and $sm_path, which is an alias of $path)".

Quoted 5 lines

There are three 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. If 'the' refers to the superproject holding this submodule ('sub' holding 'nested'), the path would be expected to be path='nested'.

What is "the", and why is it quoted?

Also, in (c), you use the same indicative present tense as "The path value is documented" to describe the current situation, whereas this seems like a situation you're proposing. It would be clearer to use the imperative here ("Document $path to be the path from the toplevel..."). Do the same for the others.

Also, whenever you mention "superproject", make it clear which superproject you're referring to ("outermost superproject" and "immediate superproject" seem like good terms to me).

Quoted 3 lines

(b) In case 'the' superproject is referring to the toplevel, which is the superproject in which the original command was invoked, then path is expected to be path='sub/nested'.

Same comment about "the", and I think s/toplevel, which is the superproject in which the original command was invoked/outermost superproject/.

Quoted 5 lines

(c) The documentation explains $path as [...] "relative to the superproject", following 091a6eb0fe (submodule: drop the top-level requirement, 2013-06-16), such that the nested submodule would be expected as path='../sub/nested', when "the" superproject is the superproject, where the command was run from

How does "relative to the superproject" result in "../" appearing in the path? I would expect "../" to appear only if a path is relative to $pwd.

Quoted 4 lines

(d) or the value of path='nested' is expected if we take the intermediate superproject into account. [This is the same as (a); it highlights that the documentation is not clear, but technically correct if we were to revert 091a6eb0fe.]

How exactly are we taking the intermediate superproject into account?

Quoted 5 lines

The behavior for (c) 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.

I think "pwd" is more used in the Git project than "cwd", so maybe use $pwd here.

Quoted 16 lines

If we were to fix the meaning of the $path using (a), (d) such that "path" is "the path from the intermediate 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 were to fix the meaning of $path using (b) such that "path" is "the path from the topmost superproject to the mount point of the submodule", then we would break any user that uses nested submodules (even from the root directory) as the 'nested' would become 'sub/nested'.
If we were to fix the meaning of $path using (c) such that "path" is "the display path from where the original command was invoked to the submodule", then we would break users that rely on the assumption that "$toplevel / $path" is the absolute path of the submodule.

Quoted 7 lines

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

As I said in my previous e-mail, this is a good analysis.

Quoted 4 lines

Another argument in favor of (a) is the consistency of the variables provided, "$toplevel / $path" gives the absolute path of the submodule, with 'toplevel' (both the variable as well as the documentation) referring to the immediate superproject of the submodule.

It's confusing to me that $toplevel is not the path to the outermost superproject, but the path to the immediate superproject, so I'm not sure that the goodness of "$toplevel/$path" exists. I would omit this paragraph.

Quoted 1 line

Documentation of the variable is adjusted in a follow-up patch.

By "the variable", I assume you mean $toplevel? If yes, this doesn't seem relevant to this patch, since this patch does not change the meaning of $toplevel at all.

Stefan Beller

author

2018-05-03 18:12 UTC #4 lore raw

Re: [PATCH 1/5] submodule foreach: correct '$path' in nested submodules from a subdirectory

Quoted 15 lines · Jonathan Tan

Quoted 4 lines · On Wed, 2 May 2018 17:53:54 -0700 Stefan Beller <[email protected]> wrote:
From: Prathamesh Chavan <[email protected]>
When running 'git submodule foreach --recursive' from a subdirectory of your repository, nested submodules get a bogus value for $path:
I know I said in a previous e-mail [1] that we should use $path instead of $sm_path, but now I got confused because the test shows a difference in $sm_path, not $path. Maybe add "(and $sm_path, which is an alias of $path)".
[1] https://public-inbox.org/git/[email protected]/

Quoted 7 lines

Quoted 5 lines
There are three 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. If 'the' refers to the superproject holding this submodule ('sub' holding 'nested'), the path would be expected to be path='nested'.
What is "the", and why is it quoted?

because it is unclear what is emphasizes.
It could be the intermediate (direct) superproject, or it could be the topmost superproject (where the command was run from).

Just having "the", makes it unclear which of both it refers to.

Quoted 9 lines

Also, in (c), you use the same indicative present tense as "The path value is documented" to describe the current situation, whereas this seems like a situation you're proposing. It would be clearer to use the imperative here ("Document $path to be the path from the toplevel..."). Do the same for the others.
Also, whenever you mention "superproject", make it clear which superproject you're referring to ("outermost superproject" and "immediate superproject" seem like good terms to me).

ok, I'll rewrite the commit message with clearer superproject annotations.

Quoted 7 lines

Quoted 3 lines
(b) In case 'the' superproject is referring to the toplevel, which is the superproject in which the original command was invoked, then path is expected to be path='sub/nested'.
Same comment about "the", and I think s/toplevel, which is the superproject in which the original command was invoked/outermost superproject/.

The outermost superproject may not be the one you invoke the command in.

We have
* the direct superproject. You can access it currently via $toplevel, which is misleading
* the superproject, where the command was invoked from, Currently only the undocumented $displaypath gives hints at this
* the outermost superproject, which may be even further out than the previous superproject in this list; Consider a layout with 4 git repositories nested as follows:

outmost/invoked/direct/submodule

Submodule is part of the superproject "direct", but the command could have been invoked from outmost/invoked/dir which has "direct" as a submodule at '../direct' and itself is a submodule of outmost.

IMHO 'outmost' is of no relevance to the discussion. If you care about it, discover it yourself via repeated calls to
'git rev-parse --show-superproject-working-tree'

'invoked' is only interesting for $displaypath, but apart from that there is no benefit in knowing that at the time of processing 'submodule'. (It doesn't contain information for submodule, as all those configs are in 'direct')

'direct' is a better name than 'toplevel', which is confusing as it could be understood as any of 'direct', 'invoked' or 'outmost'.

Quoted 8 lines

Quoted 5 lines
(c) The documentation explains $path as [...] "relative to the superproject", following 091a6eb0fe (submodule: drop the top-level requirement, 2013-06-16), such that the nested submodule would be expected as path='../sub/nested', when "the" superproject is the superproject, where the command was run from
How does "relative to the superproject" result in "../" appearing in the path? I would expect "../" to appear only if a path is relative to $pwd.

Gah. I messed that up. I wanted to emphasize *relative* and not the superproject that is merely mentioned to form a sentence there.

Quoted 7 lines

Quoted 5 lines
(d) or the value of path='nested' is expected if we take the intermediate superproject into account. [This is the same as (a); it highlights that the documentation is not clear, but technically correct if we were to revert 091a6eb0fe.]
How exactly are we taking the intermediate superproject into account?

'nested' is the full in-tree path from the intermediate (direct) superproject to that submodule.

Quoted 8 lines

Quoted 5 lines
The behavior for (c) 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.
I think "pwd" is more used in the Git project than "cwd", so maybe use $pwd here.

ok.

Quoted 27 lines

Quoted 17 lines
If we were to fix the meaning of the $path using (a), (d) such that "path" is "the path from the intermediate 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 were to fix the meaning of $path using (b) such that "path" is "the path from the topmost superproject to the mount point of the submodule", then we would break any user that uses nested submodules (even from the root directory) as the 'nested' would become 'sub/nested'.
If we were to fix the meaning of $path using (c) such that "path" is "the display path from where the original command was invoked to the submodule", then we would break users that rely on the assumption that "$toplevel / $path" is the absolute path of the submodule.
Quoted 7 lines
All 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.
As I said in my previous e-mail, this is a good analysis.

Quoted 7 lines

Quoted 4 lines
Another argument in favor of (a) is the consistency of the variables provided, "$toplevel / $path" gives the absolute path of the submodule, with 'toplevel' (both the variable as well as the documentation) referring to the immediate superproject of the submodule.
It's confusing to me that $toplevel is not the path to the outermost superproject,

yes. That confused me for a while, too. Maybe deprecate that (just like we deprecate $path) and introduce $superproject to mean the direct superproject that holds the submodule currently being processed?

Quoted 3 lines

but the path to the immediate superproject, so I'm not sure that the goodness of "$toplevel/$path" exists. I would omit this paragraph.

This exists for all nested submodules currently as these are run from its root tree. For non-nested submodules, you still have the part that is relative from pwd to the submodule.

Quoted 5 lines

Quoted 1 line
Documentation of the variable is adjusted in a follow-up patch.
By "the variable", I assume you mean $toplevel? If yes, this doesn't seem relevant to this patch, since this patch does not change the meaning of $toplevel at all.

ok.

Jonathan Tan

2018-05-04 21:03 UTC #5 lore raw

Re: [PATCH 1/5] submodule foreach: correct '$path' in nested submodules from a subdirectory

Quoted 6 lines · On Thu, 3 May 2018 11:12:27 -0700 Stefan Beller <[email protected]> wrote:

Quoted 6 lines
Quoted 5 lines
There are three 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. If 'the' refers to the superproject holding this submodule ('sub' holding 'nested'), the path would be expected to be path='nested'.

Quoted 7 lines

Quoted 1 line
What is "the", and why is it quoted?
because it is unclear what is emphasizes.
It could be the intermediate (direct) superproject, or it could be the topmost superproject (where the command was run from).
Just having "the", makes it unclear which of both it refers to.

Ah, I see - so s/'the'/'the superproject'/

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
(b) In case 'the' superproject is referring to the toplevel, which is the superproject in which the original command was invoked, then path is expected to be path='sub/nested'.

And here, s/'the' superproject/'the superproject'/

Quoted 6 lines

Quoted 3 lines
Same comment about "the", and I think s/toplevel, which is the superproject in which the original command was invoked/outermost superproject/.
The outermost superproject may not be the one you invoke the command in.

Good point. Maybe "the superproject the original command was run from", but I'm open to a better name. So I would write the beginning as follows:

Also, in git-submodule.txt, $path is documented to be the "name of the submodule directory relative to the superproject", but "the superproject" is ambiguous.

To resolve both these issues, we could:
(a) Change "the superproject" to "its immediate superproject", so $path would be "nested" instead of "../nested".
(b) Change "the superproject" to "the superproject the original command was run from", so $path would be "sub/nested" instead of "../nested".
(c) Change "the superproject" to "the directory the original command was run from", so $path would be "../sub/nested" instead of "../nested".

Going back to the original patch:

Quoted 5 lines

The behavior for (c) 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.

The (c) behavior was never really introduced, right? 091a6eb0fe attempted to introduce (c), but it didn't work when --recursive was specified.

Stefan Beller

author

2018-05-03 00:54 UTC #6 lore raw

PATCH 2/5 submodule foreach: document '$sm_path' instead of '$path'

From: Prathamesh Chavan <[email protected]>

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 and document it as a deprecated synonym of 'sm_path'.

Discussed-with: Ramsay Jones <[email protected]>

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

---
 Documentation/git-submodule.txt | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Documentation/git-submodule.txt +6 -4

index 71c5618e82a..755ed695f08 100644
@@ -183,12 +183,14 @@ 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
+	The command has access to the variables $name, $sm_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.
+	$sm_path is the path of the submodule as recorded in the superproject,
+	$sha1 is the commit as recorded in the superproject, and
+	$toplevel is the absolute path to the top-level of the superproject.
+	Note that to avoid conflicts with '$PATH' on Windows, the '$path'
+	variable is now a deprecated synonym of '$sm_path' variable.
 	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.

Signature · 1 line

2.17.0.441.gb46fe60e1d-goog

Jonathan Tan

2018-05-03 17:50 UTC #7 lore raw

Re: [PATCH 2/5] submodule foreach: document '$sm_path' instead of '$path'

Quoted 7 lines · On Wed, 2 May 2018 17:53:55 -0700 Stefan Beller <[email protected]> wrote:

foreach [--recursive] <command>::

Documentation/git-submodule.txt +1 -2

 	Evaluates an arbitrary shell command in each checked out submodule.
-	The command has access to the variables $name, $path, $sha1 and
+	The command has access to the variables $name, $sm_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

Documentation/git-submodule.txt information too. · line 196

Quoted 10 lines

Documentation/git-submodule.txt +5 -2

-	superproject, $sha1 is the commit as recorded in the superproject,
-	and $toplevel is the absolute path to the top-level of the superproject.
+	$sm_path is the path of the submodule as recorded in the superproject,
+	$sha1 is the commit as recorded in the superproject, and
+	$toplevel is the absolute path to the top-level of the superproject.
+	Note that to avoid conflicts with '$PATH' on Windows, the '$path'
+	variable is now a deprecated synonym of '$sm_path' variable.
 	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.

This patch is fine as-is. I would go further and replace all mentions of "the superproject" to "its immediate superproject", but that is optional.

Stefan Beller

author

2018-05-03 00:54 UTC #8 lore raw

PATCH 3/5 submodule foreach: clarify the '$toplevel' variable documentation

From: Prathamesh Chavan <[email protected]>

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.

Discussed-with: Ramsay Jones <[email protected]>

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

---
 Documentation/git-submodule.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Documentation/git-submodule.txt +2 -1

index 755ed695f08..408d5a0387f 100644
@@ -188,7 +188,8 @@ foreach [--recursive] <command>::
 	$name is the name of the relevant submodule section in `.gitmodules`,
 	$sm_path is the path of the submodule as recorded in the superproject,
 	$sha1 is the commit as recorded in the superproject, and
-	$toplevel is the absolute path to the top-level of the superproject.
+	$toplevel is the absolute path to its direct superproject, such that
+	$toplevel/$sm_path is the absolute path of the submodule.
 	Note that to avoid conflicts with '$PATH' on Windows, the '$path'
 	variable is now a deprecated synonym of '$sm_path' variable.
 	Any submodules defined in the superproject but not checked out are

Signature · 1 line

2.17.0.441.gb46fe60e1d-goog

Jonathan Tan

2018-05-03 17:51 UTC #9 lore raw

Re: [PATCH 3/5] submodule foreach: clarify the '$toplevel' variable documentation

Documentation/git-submodule.txt foreach [--recursive] <command>:: · line 195

Quoted 9 lines · On Wed, 2 May 2018 17:53:56 -0700 Stefan Beller <[email protected]> wrote:

Documentation/git-submodule.txt +2 -1

 	$name is the name of the relevant submodule section in `.gitmodules`,
 	$sm_path is the path of the submodule as recorded in the superproject,
 	$sha1 is the commit as recorded in the superproject, and
-	$toplevel is the absolute path to the top-level of the superproject.
+	$toplevel is the absolute path to its direct superproject, such that
+	$toplevel/$sm_path is the absolute path of the submodule.
 	Note that to avoid conflicts with '$PATH' on Windows, the '$path'
 	variable is now a deprecated synonym of '$sm_path' variable.
 	Any submodules defined in the superproject but not checked out are

Ah, I commented on patch 2 before reading patch 3 properly. I think that if you follow my suggestions on patch 2, it will be obvious that $toplevel/$sm_path is the absolute path of the submodule, so that this patch is no longer needed.

Stefan Beller

author

2018-05-03 00:54 UTC #10 lore raw

PATCH 4/5 submodule foreach: document variable '$displaypath'

From: Prathamesh Chavan <[email protected]>

It was observed that the variable '$displaypath' was accessible but undocumented. Hence, document it.

Discussed-with: Ramsay Jones <[email protected]>

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

---
 Documentation/git-submodule.txt |  6 ++++--
 t/t7407-submodule-foreach.sh    | 22 +++++++++++-----------
 2 files changed, 15 insertions(+), 13 deletions(-)

Documentation/git-submodule.txt +4 -2

index 408d5a0387f..4372d00c42e 100644
@@ -183,10 +183,12 @@ information too.
 
 foreach [--recursive] <command>::
 	Evaluates an arbitrary shell command in each checked out submodule.
-	The command has access to the variables $name, $sm_path, $sha1 and
-	$toplevel:
+	The command has access to the variables $name, $sm_path, $displaypath,
+	$sha1 and $toplevel:
 	$name is the name of the relevant submodule section in `.gitmodules`,
 	$sm_path is the path of the submodule as recorded in the superproject,
+	$displaypath contains the relative path from the current working
+	directory to the submodules root directory,
 	$sha1 is the commit as recorded in the superproject, and
 	$toplevel is the absolute path to its direct superproject, such that
 	$toplevel/$sm_path is the absolute path of the submodule.

t/t7407-submodule-foreach.sh +11 ~~-11~~

index 5144cc6926b..77729ac4aa1 100755
@@ -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-\$displaypath-\$sha1" >../../actual
 	) &&
 	test_i18ncmp expect actual
 '

@@ -206,25 +206,25 @@ submodulesha1=$(cd clone2/nested1/nested2/nested3/submodule && git rev-parse HEA
 
 cat >expect <<EOF
 Entering '../nested1'
-toplevel: $pwd/clone2 name: nested1 path: nested1 hash: $nested1sha1
+toplevel: $pwd/clone2 name: nested1 path: nested1 displaypath: ../nested1 hash: $nested1sha1
 Entering '../nested1/nested2'
-toplevel: $pwd/clone2/nested1 name: nested2 path: nested2 hash: $nested2sha1
+toplevel: $pwd/clone2/nested1 name: nested2 path: nested2 displaypath: ../nested1/nested2 hash: $nested2sha1
 Entering '../nested1/nested2/nested3'
-toplevel: $pwd/clone2/nested1/nested2 name: nested3 path: nested3 hash: $nested3sha1
+toplevel: $pwd/clone2/nested1/nested2 name: nested3 path: nested3 displaypath: ../nested1/nested2/nested3 hash: $nested3sha1
 Entering '../nested1/nested2/nested3/submodule'
-toplevel: $pwd/clone2/nested1/nested2/nested3 name: submodule path: submodule hash: $submodulesha1
+toplevel: $pwd/clone2/nested1/nested2/nested3 name: submodule path: submodule displaypath: ../nested1/nested2/nested3/submodule hash: $submodulesha1
 Entering '../sub1'
-toplevel: $pwd/clone2 name: foo1 path: sub1 hash: $sub1sha1
+toplevel: $pwd/clone2 name: foo1 path: sub1 displaypath: ../sub1 hash: $sub1sha1
 Entering '../sub2'
-toplevel: $pwd/clone2 name: foo2 path: sub2 hash: $sub2sha1
+toplevel: $pwd/clone2 name: foo2 path: sub2 displaypath: ../sub2 hash: $sub2sha1
 Entering '../sub3'
-toplevel: $pwd/clone2 name: foo3 path: sub3 hash: $sub3sha1
+toplevel: $pwd/clone2 name: foo3 path: sub3 displaypath: ../sub3 hash: $sub3sha1
 EOF
 
 test_expect_success 'test "submodule foreach --recursive" from subdirectory' '
 	(
 		cd clone2/untracked &&
-		git submodule foreach --recursive "echo toplevel: \$toplevel name: \$name path: \$sm_path hash: \$sha1" >../../actual
+		git submodule foreach --recursive "echo toplevel: \$toplevel name: \$name path: \$sm_path displaypath: \$displaypath hash: \$sha1" >../../actual
 	) &&
 	test_i18ncmp expect actual
 '

Signature · 1 line

2.17.0.441.gb46fe60e1d-goog

Stefan Beller

author

2018-05-03 00:54 UTC #11 lore raw

PATCH 5/5 submodule: port submodule subcommand 'foreach' from shell to C

From: Prathamesh Chavan <[email protected]>

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

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

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

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

index a404df3ea49..bbbea5868de 100644
@@ -439,6 +439,153 @@ static void for_each_listed_submodule(const struct module_list *list,
 		fn(list->entries[i], cb_data);
 }
 
+struct cb_foreach {
+	int argc;
+	const char **argv;
+	const char *prefix;
+	char *toplevel;
+	int quiet;
+	int recursive;
+};
+#define CB_FOREACH_INIT { 0 }
+
+static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
+				       void *cb_data)
+{
+	struct cb_foreach *info = cb_data;
+	const char *path = list_item->name;
+	const struct object_id *ce_oid = &list_item->oid;
+
+	const struct submodule *sub;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	char *displaypath;
+
+	displaypath = get_submodule_displaypath(path, info->prefix);
+
+	sub = submodule_from_path(&null_oid, path);
+
+	if (!sub)
+		die(_("No url found for submodule path '%s' in .gitmodules"),
+			displaypath);
+
+	if (!is_submodule_populated_gently(path, NULL))
+		goto cleanup;
+
+	prepare_submodule_repo_env(&cp.env_array);
+
+	/*
+	* For the purpose of executing <command> in the submodule,
+	* separate shell is used for the purpose of running the
+	* child process.
+	*/
+	cp.use_shell = 1;
+	cp.dir = path;
+
+	/*
+	* NEEDSWORK: the command currently has access to the variables $name,
+	* $sm_path, $displaypath, $sha1 and $toplevel only when the command
+	* contains a single argument. This is done for maintaining a faithful
+	* translation from shell script.
+	*/
+	if (info->argc == 1) {
+		char *toplevel = xgetcwd();
+
+		argv_array_pushf(&cp.env_array, "name=%s", sub->name);
+		argv_array_pushf(&cp.env_array, "sm_path=%s", path);
+		argv_array_pushf(&cp.env_array, "displaypath=%s", displaypath);
+		argv_array_pushf(&cp.env_array, "sha1=%s",
+				oid_to_hex(ce_oid));
+		argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
+
+		/*
+		* Since the path variable was accessible from the script
+		* before porting, it is also made available after porting.
+		* The environment variable "PATH" has a very special purpose
+		* on windows. And since environment variables are
+		* case-insensitive in windows, it interferes with the
+		* existing PATH variable. Hence, to avoid that, we expose
+		* path via the args argv_array and not via env_array.
+		*/
+		argv_array_pushf(&cp.args, "path=%s; %s",
+				path, info->argv[0]);
+
+		free(toplevel);
+	} else {
+		argv_array_pushv(&cp.args, info->argv);
+	}
+
+	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 = path;
+		prepare_submodule_repo_env(&cpr.env_array);
+
+		argv_array_pushl(&cpr.args, "--super-prefix", NULL);
+		argv_array_pushf(&cpr.args, "%s/", displaypath);
+		argv_array_pushl(&cpr.args, "submodule--helper", "foreach", "--recursive",
+				NULL);
+
+		if (info->quiet)
+			argv_array_push(&cpr.args, "--quiet");
+
+		if (info->toplevel)
+			argv_array_pushf(&cpr.args, "--toplevel=%s", info->toplevel);
+
+		argv_array_pushv(&cpr.args, info->argv);
+
+		if (run_command(&cpr))
+			die(_("run_command returned non-zero status while"
+				"recursing in the nested submodules of %s\n."),
+				displaypath);
+	}
+
+cleanup:
+	free(displaypath);
+}
+
+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;
+
+	struct option module_foreach_options[] = {
+		OPT__QUIET(&info.quiet, N_("Suppress output of entering each submodule command")),
+		OPT_BOOL(0, "recursive", &info.recursive,
+			 N_("Recurse into nested submodules")),
+		OPT_STRING(0, "toplevel", &info.toplevel, N_("path"),
+			   N_("path from the top level of the invocation")),
+		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)
+		return 1;
+
+	info.argc = argc;
+	info.argv = argv;
+	info.prefix = prefix;
+
+	for_each_listed_submodule(&list, runcommand_in_submodule_cb, &info);
+
+	return 0;
+}
+
 struct init_cb {
 	const char *prefix;
 	unsigned int flags;

@@ -1838,6 +1985,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},
 	{"status", module_status, SUPPORT_SUPER_PREFIX},
 	{"print-default-remote", print_default_remote, 0},

git-submodule.sh +1 ~~-38~~

index 331d71c908b..cba585f0754 100755
@@ -323,44 +323,7 @@ cmd_foreach()
 		shift
 	done
 
-	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 -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 &&
-				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
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
 }
 
 #

Signature · 1 line

2.17.0.441.gb46fe60e1d-goog

Stefan Beller

author

2018-05-03 01:06 UTC #12 lore raw

Re: [PATCH 5/5] submodule: port submodule subcommand 'foreach' from shell to C

Quoted 2 lines · Stefan Beller

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

+struct cb_foreach {
+       char *toplevel;

...

builtin/submodule--helper.c static void for_each_listed_submodule(const struct module_list *list, · line 565

Quoted 2 lines

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

+               OPT_STRING(0, "toplevel", &info.toplevel, N_("path"),
+                          N_("path from the top level of the invocation")),

This is a leftover from my experimentation that I hinted at in the cover letter that would be used to implement $topmost. I'll remove this in a reroll.

Jonathan Tan

2018-05-03 18:05 UTC #13 lore raw

Re: [PATCH 5/5] submodule: port submodule subcommand 'foreach' from shell to C

builtin/submodule--helper.c static void for_each_listed_submodule(const struct module_list *list, · line 510

Quoted 2 lines · On Wed, 2 May 2018 17:53:58 -0700 Stefan Beller <[email protected]> wrote:

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

+		argv_array_pushf(&cp.args, "path=%s; %s",
+				path, info->argv[0]);

Do we need to quote the path here? (For example, what if path has a quotation mark?)

Also, would it be useful to have a test testing a submodule with a "weird" character (e.g. greater-than or single quote) in the name and the path?

Stefan Beller

author

2018-05-09 00:30 UTC #14 lore raw

v2 cover Rebooting pc/submodule-helper-foreach

v2:
* rebased onto origin/master
* dropped leftover "toplevel" variable from experimentation
* reworded the commit message for the first patch extensively
* dropped the third patch
* see "branch-diff" below.

v1:
The "What's cooking" email carried this series for some time now:

Quoted 9 lines

* pc/submodule-helper-foreach (2018-02-02) 5 commits
- submodule: port submodule subcommand 'foreach' from shell to C
- submodule foreach: document variable '$displaypath'
- submodule foreach: clarify the '$toplevel' variable documentation
- submodule foreach: document '$sm_path' instead of '$path'
- submodule foreach: correct '$path' in nested submodules from a subdirectory
Expecting a response to review comments
e.g. cf. <[email protected]>

I reworded the commit message of the first patch and nearly confused myself again, as "toplevel" doesn't refer to the "topmost" superproject, just the direct superproject of the submodule.

However I think the code of the first patch is correct as originally presented. Just the wording of the commit message was changed to explain the reasoning more extensively.

Thanks,
Stefan

Prathamesh Chavan (4):
submodule foreach: correct '$path' in nested submodules from a
subdirectory
submodule foreach: document '$sm_path' instead of '$path' submodule foreach: document variable '$displaypath'
submodule: port submodule subcommand 'foreach' from shell to C

 Documentation/git-submodule.txt |  15 ++--
 builtin/submodule--helper.c     | 144 ++++++++++++++++++++++++++++++++
 git-submodule.sh                |  40 +--------
 t/t7407-submodule-foreach.sh    |  38 ++++++++-
 4 files changed, 190 insertions(+), 47 deletions(-)

Signature · 187 lines

2.17.0.255.g8bfb7c0704

1:  0c5f405db24 ! 1:  85f91b48379 submodule foreach: correct '$path' in nested submodules from a subdirectory
    @@ -12,45 +12,38 @@
         submodule path inside the submodule. This value is of little use and is
         hard to document.
     
    -    There are three 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. If 'the' refers to
    -        the superproject holding this submodule ('sub' holding 'nested'),
    -        the path would be expected to be path='nested'.
    -    (b) In case 'the' superproject is referring to the toplevel, which
    -        is the superproject in which the original command was invoked,
    -        then path is expected to be path='sub/nested'.
    -    (c) The documentation explains $path as [...] "relative to the
    -        superproject", following 091a6eb0fe (submodule: drop the
    -        top-level requirement, 2013-06-16), such that the nested submodule
    -        would be expected as path='../sub/nested', when "the" superproject
    -        is the superproject, where the command was run from
    -    (d) or the value of path='nested' is expected if we take the
    -        intermediate superproject into account. [This is the same as
    -        (a); it highlights that the documentation is not clear, but
    -        technically correct if we were to revert 091a6eb0fe.]
    +    Also, in git-submodule.txt, $path is documented to be the "name of the
    +    submodule directory relative to the superproject", but "the
    +    superproject" is ambiguous.
     
    -    The behavior for (c) 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.
    +    To resolve both these issues, we could:
    +    (a) Change "the superproject" to "its immediate superproject", so
    +        $path would be "nested" instead of "../nested".
    +    (b) Change "the superproject" to "the superproject the original
    +        command was run from", so $path would be "sub/nested" instead of
    +        "../nested".
    +    (c) Change "the superproject" to "the directory the original command
    +        was run from", so $path would be "../sub/nested" instead of
    +        "../nested".
     
    -    If we were to fix the meaning of the $path using (a), (d) such that "path"
    -    is "the path from the intermediate 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'.
    +    The behavior for (c) was attempted to be introduced in 091a6eb0fe
    +    (submodule: drop the top-level requirement, 2013-06-16) with the intent
    +    for $path to be relative from $pwd 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 $path using (b) such that "path"
    -    is "the path from the topmost superproject to the mount point of the
    -    submodule", then we would break any user that uses nested submodules
    -    (even from the root directory) as the 'nested' would become 'sub/nested'.
    +    If we were to fix the meaning of the $path using (a), 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 were to fix the meaning of $path using (c) such that "path"
    -    is "the display path from where the original command was invoked to the
    -    submodule", then we would break users that rely on the assumption that
    -    "$toplevel / $path" is the absolute path of the submodule.
    +    If we were to fix the meaning of $path using (b), then we would break
    +    any user that uses nested submodules (even from the root directory)
    +    as the 'nested' would become 'sub/nested'.
    +
    +    If we were to fix the meaning of $path using (c), then we would break
    +    the same users as in (b) as 'nested' would become 'sub/nested' from
    +    the root directory of the superproject.
     
         All 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
    @@ -60,13 +53,6 @@
         the feedback loop is short and the changed behavior can be adapted to
         quickly unlike some automation that can break silently.
     
    -    Another argument in favor of (a) is the consistency of the variables
    -    provided, "$toplevel / $path" gives the absolute path of the submodule,
    -    with 'toplevel' (both the variable as well as the documentation) referring
    -    to the immediate superproject of the submodule.
    -
    -    Documentation of the variable is adjusted in a follow-up patch.
    -
         Discussed-with: Ramsay Jones <[email protected]>
         Signed-off-by: Prathamesh Chavan <[email protected]>
         Signed-off-by: Stefan Beller <[email protected]>
2:  ae5e280a8d3 ! 2:  3a2cb4e0b01 submodule foreach: document '$sm_path' instead of '$path'
    @@ -28,9 +28,10 @@
     -	$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.
    -+	$sm_path is the path of the submodule as recorded in the superproject,
    -+	$sha1 is the commit as recorded in the superproject, and
    -+	$toplevel is the absolute path to the top-level of the superproject.
    ++	$sm_path is the path of the submodule as recorded in the immediate
    ++	superproject, $sha1 is the commit as recorded in the immediate
    ++	superproject, and $toplevel is the absolute path to the top-level
    ++	of the immediate superproject.
     +	Note that to avoid conflicts with '$PATH' on Windows, the '$path'
     +	variable is now a deprecated synonym of '$sm_path' variable.
      	Any submodules defined in the superproject but not checked out are
3:  23a5f04f846 < -:  ----------- submodule foreach: clarify the '$toplevel' variable documentation
4:  3efed1cdb80 ! 3:  3f4686e8395 submodule foreach: document variable '$displaypath'
    @@ -22,12 +22,14 @@
     +	The command has access to the variables $name, $sm_path, $displaypath,
     +	$sha1 and $toplevel:
      	$name is the name of the relevant submodule section in `.gitmodules`,
    - 	$sm_path is the path of the submodule as recorded in the superproject,
    -+	$displaypath contains the relative path from the current working
    -+	directory to the submodules root directory,
    - 	$sha1 is the commit as recorded in the superproject, and
    - 	$toplevel is the absolute path to its direct superproject, such that
    - 	$toplevel/$sm_path is the absolute path of the submodule.
    + 	$sm_path is the path of the submodule as recorded in the immediate
    +-	superproject, $sha1 is the commit as recorded in the immediate
    ++	superproject, $displaypath contains the relative path from the
    ++	current working directory to the submodules root directory,
    ++	$sha1 is the commit as recorded in the immediate
    + 	superproject, and $toplevel is the absolute path to the top-level
    + 	of the immediate superproject.
    + 	Note that to avoid conflicts with '$PATH' on Windows, the '$path'
     
     diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
     --- a/t/t7407-submodule-foreach.sh
5:  2b17d6891b3 ! 4:  0c677680928 submodule: port submodule subcommand 'foreach' from shell to C
    @@ -23,7 +23,6 @@
     +	int argc;
     +	const char **argv;
     +	const char *prefix;
    -+	char *toplevel;
     +	int quiet;
     +	int recursive;
     +};
    @@ -42,7 +41,7 @@
     +
     +	displaypath = get_submodule_displaypath(path, info->prefix);
     +
    -+	sub = submodule_from_path(&null_oid, path);
    ++	sub = submodule_from_path(the_repository, &null_oid, path);
     +
     +	if (!sub)
     +		die(_("No url found for submodule path '%s' in .gitmodules"),
    @@ -69,6 +68,7 @@
     +	*/
     +	if (info->argc == 1) {
     +		char *toplevel = xgetcwd();
    ++		struct strbuf sb = STRBUF_INIT;
     +
     +		argv_array_pushf(&cp.env_array, "name=%s", sub->name);
     +		argv_array_pushf(&cp.env_array, "sm_path=%s", path);
    @@ -86,9 +86,10 @@
     +		* existing PATH variable. Hence, to avoid that, we expose
     +		* path via the args argv_array and not via env_array.
     +		*/
    ++		sq_quote_buf(&sb, path);
     +		argv_array_pushf(&cp.args, "path=%s; %s",
    -+				path, info->argv[0]);
    -+
    ++				 sb.buf, info->argv[0]);
    ++		strbuf_release(&sb);
     +		free(toplevel);
     +	} else {
     +		argv_array_pushv(&cp.args, info->argv);
    @@ -116,9 +117,6 @@
     +		if (info->quiet)
     +			argv_array_push(&cpr.args, "--quiet");
     +
    -+		if (info->toplevel)
    -+			argv_array_pushf(&cpr.args, "--toplevel=%s", info->toplevel);
    -+
     +		argv_array_pushv(&cpr.args, info->argv);
     +
     +		if (run_command(&cpr))
    @@ -141,8 +139,6 @@
     +		OPT__QUIET(&info.quiet, N_("Suppress output of entering each submodule command")),
     +		OPT_BOOL(0, "recursive", &info.recursive,
     +			 N_("Recurse into nested submodules")),
    -+		OPT_STRING(0, "toplevel", &info.toplevel, N_("path"),
    -+			   N_("path from the top level of the invocation")),
     +		OPT_END()
     +	};
     +

Stefan Beller

author

2018-05-09 00:30 UTC #15 lore raw

PATCH v2 1/4 submodule foreach: correct '$path' in nested submodules from a subdirectory

From: Prathamesh Chavan <[email protected]>

Also, in git-submodule.txt, $path is documented to be the "name of the submodule directory relative to the superproject", but "the superproject" is ambiguous.

The behavior for (c) was attempted to be introduced in 091a6eb0fe (submodule: drop the top-level requirement, 2013-06-16) with the intent for $path to be relative from $pwd 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), 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 were to fix the meaning of $path using (b), then we would break any user that uses nested submodules (even from the root directory) as the 'nested' would become 'sub/nested'.

If we were to fix the meaning of $path using (c), then we would break the same users as in (b) as 'nested' would become 'sub/nested' from the root directory of the superproject.

Discussed-with: Ramsay Jones <[email protected]>

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

---
 git-submodule.sh             |  1 -
 t/t7407-submodule-foreach.sh | 36 ++++++++++++++++++++++++++++++++++--
 2 files changed, 34 insertions(+), 3 deletions(-)

git-submodule.sh +0 -1

index 24914963ca2..331d71c908b 100755
@@ -345,7 +345,6 @@ 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 &&
 				if test $# -eq 1

t/t7407-submodule-foreach.sh +34 -2

index 6ba5daf42ee..5144cc6926b 100755
@@ -82,9 +82,9 @@ test_expect_success 'test basic "submodule foreach" usage' '
 
 cat >expect <<EOF
 Entering '../sub1'
-$pwd/clone-foo1-../sub1-$sub1sha1
+$pwd/clone-foo1-sub1-$sub1sha1
 Entering '../sub3'
-$pwd/clone-foo3-../sub3-$sub3sha1
+$pwd/clone-foo3-sub3-$sub3sha1
 EOF
 
 test_expect_success 'test "submodule foreach" from subdirectory' '

@@ -196,6 +196,38 @@ 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'
+toplevel: $pwd/clone2 name: nested1 path: nested1 hash: $nested1sha1
+Entering '../nested1/nested2'
+toplevel: $pwd/clone2/nested1 name: nested2 path: nested2 hash: $nested2sha1
+Entering '../nested1/nested2/nested3'
+toplevel: $pwd/clone2/nested1/nested2 name: nested3 path: nested3 hash: $nested3sha1
+Entering '../nested1/nested2/nested3/submodule'
+toplevel: $pwd/clone2/nested1/nested2/nested3 name: submodule path: submodule hash: $submodulesha1
+Entering '../sub1'
+toplevel: $pwd/clone2 name: foo1 path: sub1 hash: $sub1sha1
+Entering '../sub2'
+toplevel: $pwd/clone2 name: foo2 path: sub2 hash: $sub2sha1
+Entering '../sub3'
+toplevel: $pwd/clone2 name: foo3 path: sub3 hash: $sub3sha1
+EOF
+
+test_expect_success 'test "submodule foreach --recursive" from subdirectory' '
+	(
+		cd clone2/untracked &&
+		git submodule foreach --recursive "echo toplevel: \$toplevel name: \$name path: \$sm_path hash: \$sha1" >../../actual
+	) &&
+	test_i18ncmp expect actual
+'
 
 cat > expect <<EOF
 nested1-nested1

Signature · 1 line

2.17.0.255.g8bfb7c0704

Stefan Beller

author

2018-05-09 00:30 UTC #16 lore raw

PATCH v2 2/4 submodule foreach: document '$sm_path' instead of '$path'

From: Prathamesh Chavan <[email protected]>

Discussed-with: Ramsay Jones <[email protected]>

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

---
 Documentation/git-submodule.txt | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Documentation/git-submodule.txt +7 -4

index 630999f41a9..066c7b6c18e 100644
@@ -183,12 +183,15 @@ 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
+	The command has access to the variables $name, $sm_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.
+	$sm_path is the path of the submodule as recorded in the immediate
+	superproject, $sha1 is the commit as recorded in the immediate
+	superproject, and $toplevel is the absolute path to the top-level
+	of the immediate superproject.
+	Note that to avoid conflicts with '$PATH' on Windows, the '$path'
+	variable is now a deprecated synonym of '$sm_path' variable.
 	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.

Signature · 1 line

2.17.0.255.g8bfb7c0704

Stefan Beller

author

2018-05-09 00:30 UTC #17 lore raw

PATCH v2 3/4 submodule foreach: document variable '$displaypath'

From: Prathamesh Chavan <[email protected]>

It was observed that the variable '$displaypath' was accessible but undocumented. Hence, document it.

Discussed-with: Ramsay Jones <[email protected]>

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

---
 Documentation/git-submodule.txt |  8 +++++---
 t/t7407-submodule-foreach.sh    | 22 +++++++++++-----------
 2 files changed, 16 insertions(+), 14 deletions(-)

Documentation/git-submodule.txt +5 -3

index 066c7b6c18e..500dfdafd16 100644
@@ -183,11 +183,13 @@ information too.
 
 foreach [--recursive] <command>::
 	Evaluates an arbitrary shell command in each checked out submodule.
-	The command has access to the variables $name, $sm_path, $sha1 and
-	$toplevel:
+	The command has access to the variables $name, $sm_path, $displaypath,
+	$sha1 and $toplevel:
 	$name is the name of the relevant submodule section in `.gitmodules`,
 	$sm_path is the path of the submodule as recorded in the immediate
-	superproject, $sha1 is the commit as recorded in the immediate
+	superproject, $displaypath contains the relative path from the
+	current working directory to the submodules root directory,
+	$sha1 is the commit as recorded in the immediate
 	superproject, and $toplevel is the absolute path to the top-level
 	of the immediate superproject.
 	Note that to avoid conflicts with '$PATH' on Windows, the '$path'

t/t7407-submodule-foreach.sh +11 ~~-11~~

index 5144cc6926b..77729ac4aa1 100755
@@ -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-\$displaypath-\$sha1" >../../actual
 	) &&
 	test_i18ncmp expect actual
 '

@@ -206,25 +206,25 @@ submodulesha1=$(cd clone2/nested1/nested2/nested3/submodule && git rev-parse HEA
 
 cat >expect <<EOF
 Entering '../nested1'
-toplevel: $pwd/clone2 name: nested1 path: nested1 hash: $nested1sha1
+toplevel: $pwd/clone2 name: nested1 path: nested1 displaypath: ../nested1 hash: $nested1sha1
 Entering '../nested1/nested2'
-toplevel: $pwd/clone2/nested1 name: nested2 path: nested2 hash: $nested2sha1
+toplevel: $pwd/clone2/nested1 name: nested2 path: nested2 displaypath: ../nested1/nested2 hash: $nested2sha1
 Entering '../nested1/nested2/nested3'
-toplevel: $pwd/clone2/nested1/nested2 name: nested3 path: nested3 hash: $nested3sha1
+toplevel: $pwd/clone2/nested1/nested2 name: nested3 path: nested3 displaypath: ../nested1/nested2/nested3 hash: $nested3sha1
 Entering '../nested1/nested2/nested3/submodule'
-toplevel: $pwd/clone2/nested1/nested2/nested3 name: submodule path: submodule hash: $submodulesha1
+toplevel: $pwd/clone2/nested1/nested2/nested3 name: submodule path: submodule displaypath: ../nested1/nested2/nested3/submodule hash: $submodulesha1
 Entering '../sub1'
-toplevel: $pwd/clone2 name: foo1 path: sub1 hash: $sub1sha1
+toplevel: $pwd/clone2 name: foo1 path: sub1 displaypath: ../sub1 hash: $sub1sha1
 Entering '../sub2'
-toplevel: $pwd/clone2 name: foo2 path: sub2 hash: $sub2sha1
+toplevel: $pwd/clone2 name: foo2 path: sub2 displaypath: ../sub2 hash: $sub2sha1
 Entering '../sub3'
-toplevel: $pwd/clone2 name: foo3 path: sub3 hash: $sub3sha1
+toplevel: $pwd/clone2 name: foo3 path: sub3 displaypath: ../sub3 hash: $sub3sha1
 EOF
 
 test_expect_success 'test "submodule foreach --recursive" from subdirectory' '
 	(
 		cd clone2/untracked &&
-		git submodule foreach --recursive "echo toplevel: \$toplevel name: \$name path: \$sm_path hash: \$sha1" >../../actual
+		git submodule foreach --recursive "echo toplevel: \$toplevel name: \$name path: \$sm_path displaypath: \$displaypath hash: \$sha1" >../../actual
 	) &&
 	test_i18ncmp expect actual
 '

Signature · 1 line

2.17.0.255.g8bfb7c0704

Stefan Beller

author

2018-05-09 00:30 UTC #18 lore raw

PATCH v2 4/4 submodule: port submodule subcommand 'foreach' from shell to C

From: Prathamesh Chavan <[email protected]>

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

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

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

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

index c2403a915ff..4b0b773c06b 100644
@@ -439,6 +439,149 @@ static void for_each_listed_submodule(const struct module_list *list,
 		fn(list->entries[i], cb_data);
 }
 
+struct cb_foreach {
+	int argc;
+	const char **argv;
+	const char *prefix;
+	int quiet;
+	int recursive;
+};
+#define CB_FOREACH_INIT { 0 }
+
+static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
+				       void *cb_data)
+{
+	struct cb_foreach *info = cb_data;
+	const char *path = list_item->name;
+	const struct object_id *ce_oid = &list_item->oid;
+
+	const struct submodule *sub;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	char *displaypath;
+
+	displaypath = get_submodule_displaypath(path, info->prefix);
+
+	sub = submodule_from_path(the_repository, &null_oid, path);
+
+	if (!sub)
+		die(_("No url found for submodule path '%s' in .gitmodules"),
+			displaypath);
+
+	if (!is_submodule_populated_gently(path, NULL))
+		goto cleanup;
+
+	prepare_submodule_repo_env(&cp.env_array);
+
+	/*
+	* For the purpose of executing <command> in the submodule,
+	* separate shell is used for the purpose of running the
+	* child process.
+	*/
+	cp.use_shell = 1;
+	cp.dir = path;
+
+	/*
+	* NEEDSWORK: the command currently has access to the variables $name,
+	* $sm_path, $displaypath, $sha1 and $toplevel only when the command
+	* contains a single argument. This is done for maintaining a faithful
+	* translation from shell script.
+	*/
+	if (info->argc == 1) {
+		char *toplevel = xgetcwd();
+		struct strbuf sb = STRBUF_INIT;
+
+		argv_array_pushf(&cp.env_array, "name=%s", sub->name);
+		argv_array_pushf(&cp.env_array, "sm_path=%s", path);
+		argv_array_pushf(&cp.env_array, "displaypath=%s", displaypath);
+		argv_array_pushf(&cp.env_array, "sha1=%s",
+				oid_to_hex(ce_oid));
+		argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
+
+		/*
+		* Since the path variable was accessible from the script
+		* before porting, it is also made available after porting.
+		* The environment variable "PATH" has a very special purpose
+		* on windows. And since environment variables are
+		* case-insensitive in windows, it interferes with the
+		* existing PATH variable. Hence, to avoid that, we expose
+		* path via the args argv_array and not via env_array.
+		*/
+		sq_quote_buf(&sb, path);
+		argv_array_pushf(&cp.args, "path=%s; %s",
+				 sb.buf, info->argv[0]);
+		strbuf_release(&sb);
+		free(toplevel);
+	} else {
+		argv_array_pushv(&cp.args, info->argv);
+	}
+
+	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 = path;
+		prepare_submodule_repo_env(&cpr.env_array);
+
+		argv_array_pushl(&cpr.args, "--super-prefix", NULL);
+		argv_array_pushf(&cpr.args, "%s/", displaypath);
+		argv_array_pushl(&cpr.args, "submodule--helper", "foreach", "--recursive",
+				NULL);
+
+		if (info->quiet)
+			argv_array_push(&cpr.args, "--quiet");
+
+		argv_array_pushv(&cpr.args, info->argv);
+
+		if (run_command(&cpr))
+			die(_("run_command returned non-zero status while"
+				"recursing in the nested submodules of %s\n."),
+				displaypath);
+	}
+
+cleanup:
+	free(displaypath);
+}
+
+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;
+
+	struct option module_foreach_options[] = {
+		OPT__QUIET(&info.quiet, N_("Suppress output of entering each submodule command")),
+		OPT_BOOL(0, "recursive", &info.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)
+		return 1;
+
+	info.argc = argc;
+	info.argv = argv;
+	info.prefix = prefix;
+
+	for_each_listed_submodule(&list, runcommand_in_submodule_cb, &info);
+
+	return 0;
+}
+
 struct init_cb {
 	const char *prefix;
 	unsigned int flags;

@@ -1841,6 +1984,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},
 	{"status", module_status, SUPPORT_SUPER_PREFIX},
 	{"print-default-remote", print_default_remote, 0},

git-submodule.sh +1 ~~-38~~

index 331d71c908b..cba585f0754 100755
@@ -323,44 +323,7 @@ cmd_foreach()
 		shift
 	done
 
-	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 -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 &&
-				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
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
 }
 
 #

Signature · 1 line

2.17.0.255.g8bfb7c0704

Junio C Hamano

2018-05-10 06:37 UTC #19 lore raw

Re: [PATCH 4/4] submodule: port submodule subcommand 'foreach' from shell to C

Quoted 19 lines · Stefan Beller

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

+static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
+				       void *cb_data)
+{
+	struct cb_foreach *info = cb_data;
+	const char *path = list_item->name;
+	const struct object_id *ce_oid = &list_item->oid;
+
+	const struct submodule *sub;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	char *displaypath;
+
+	displaypath = get_submodule_displaypath(path, info->prefix);
+
+	sub = submodule_from_path(the_repository, &null_oid, path);
+
+	if (!sub)
+		die(_("No url found for submodule path '%s' in .gitmodules"),
+			displaypath);
+

builtin/submodule--helper.c static void for_each_listed_submodule(const struct module_list *list, · line 479

Quoted 10 lines

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

+	if (!is_submodule_populated_gently(path, NULL))
+		goto cleanup;
+
+	prepare_submodule_repo_env(&cp.env_array);
+
+	/*
+	* For the purpose of executing <command> in the submodule,
+	* separate shell is used for the purpose of running the
+	* child process.
+	*/

Micronit: this multi-line comment is indented in a funny way.

builtin/submodule--helper.c static void for_each_listed_submodule(const struct module_list *list, · line 488

Quoted 9 lines

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

+	cp.use_shell = 1;
+	cp.dir = path;
+
+	/*
+	* NEEDSWORK: the command currently has access to the variables $name,
+	* $sm_path, $displaypath, $sha1 and $toplevel only when the command
+	* contains a single argument. This is done for maintaining a faithful
+	* translation from shell script.
+	*/

Same micronit.

The scripted version does 'eval "$1"', so $1 could be something like

	for-each 'echo "$name:$sm_path:$displaypath:$sha1:$toplevel"'

and it can see any variable, not just these 5 (i.e. we could have fed e.g. $wt_prefix and $mode to the above 'echo' and with the scripted version the script would have learned their values; with this version it no longer does, but only these 5 are part of the documented API, so we choose not to consider it a regression).

Quoted 13 lines

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

+	if (info->argc == 1) {
+		char *toplevel = xgetcwd();
+		struct strbuf sb = STRBUF_INIT;
+
+		argv_array_pushf(&cp.env_array, "name=%s", sub->name);
+		argv_array_pushf(&cp.env_array, "sm_path=%s", path);
+		argv_array_pushf(&cp.env_array, "displaypath=%s", displaypath);
+		argv_array_pushf(&cp.env_array, "sha1=%s",
+				oid_to_hex(ce_oid));
+		argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
+
+		/*
+		* Since the path variable was accessible from the script

builtin/submodule--helper.c static void for_each_listed_submodule(const struct module_list *list, · line 511

Quoted 10 lines

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

+		* before porting, it is also made available after porting.
+		* The environment variable "PATH" has a very special purpose
+		* on windows. And since environment variables are
+		* case-insensitive in windows, it interferes with the
+		* existing PATH variable. Hence, to avoid that, we expose
+		* path via the args argv_array and not via env_array.
+		*/
+		sq_quote_buf(&sb, path);
+		argv_array_pushf(&cp.args, "path=%s; %s",
+				 sb.buf, info->argv[0]);

OK, so we do the equivalent of

	name=... sm_path=... displaypath=... sha1=... toplevel=... \
	sh -c 'path=...; echo "$name:$sm_path:..."'

when doing

	for-each 'echo "$name:$sm_path:..."'

with parts denoted with ... correctly quoted as necessary. I guess it would be the best we could do.

I myself do not know if it is true that bash ported to Windows won't get confused with the above "we use path (all lowercase) only as a pure shell variable without exporting it ourselves"; I'd trust those who are more familiar with the platform to raise objections and suggest a better alternative if it is not the case.

Thanks for the (malformatted;-) leading comment to highlight why the 'path' variable alone is treated differently from the others.

Quoted 39 lines

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

+		strbuf_release(&sb);
+		free(toplevel);
+	} else {
+		argv_array_pushv(&cp.args, info->argv);
+	}
+
+	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 = path;
+		prepare_submodule_repo_env(&cpr.env_array);
+
+		argv_array_pushl(&cpr.args, "--super-prefix", NULL);
+		argv_array_pushf(&cpr.args, "%s/", displaypath);
+		argv_array_pushl(&cpr.args, "submodule--helper", "foreach", "--recursive",
+				NULL);
+
+		if (info->quiet)
+			argv_array_push(&cpr.args, "--quiet");
+
+		argv_array_pushv(&cpr.args, info->argv);
+
+		if (run_command(&cpr))
+			die(_("run_command returned non-zero status while"
+				"recursing in the nested submodules of %s\n."),
+				displaypath);
+	}
+
+cleanup:
+	free(displaypath);
+}

Stefan Beller

author

2018-05-10 21:25 UTC #20 lore raw

PATCH 1/1 submodule: port submodule subcommand 'foreach' from shell to C

From: Prathamesh Chavan <[email protected]>

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

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 is a resend of the last commit in origin/pc/submodule-helper-foreach It addresses the micro nits of funny comment indentation.

Thanks,
Stefan

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

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

index c2403a915ff..4002026d1ac 100644
@@ -439,6 +439,149 @@ static void for_each_listed_submodule(const struct module_list *list,
 		fn(list->entries[i], cb_data);
 }
 
+struct cb_foreach {
+	int argc;
+	const char **argv;
+	const char *prefix;
+	int quiet;
+	int recursive;
+};
+#define CB_FOREACH_INIT { 0 }
+
+static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
+				       void *cb_data)
+{
+	struct cb_foreach *info = cb_data;
+	const char *path = list_item->name;
+	const struct object_id *ce_oid = &list_item->oid;
+
+	const struct submodule *sub;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	char *displaypath;
+
+	displaypath = get_submodule_displaypath(path, info->prefix);
+
+	sub = submodule_from_path(the_repository, &null_oid, path);
+
+	if (!sub)
+		die(_("No url found for submodule path '%s' in .gitmodules"),
+			displaypath);
+
+	if (!is_submodule_populated_gently(path, NULL))
+		goto cleanup;
+
+	prepare_submodule_repo_env(&cp.env_array);
+
+	/*
+	 * For the purpose of executing <command> in the submodule,
+	 * separate shell is used for the purpose of running the
+	 * child process.
+	 */
+	cp.use_shell = 1;
+	cp.dir = path;
+
+	/*
+	 * NEEDSWORK: the command currently has access to the variables $name,
+	 * $sm_path, $displaypath, $sha1 and $toplevel only when the command
+	 * contains a single argument. This is done for maintaining a faithful
+	 * translation from shell script.
+	 */
+	if (info->argc == 1) {
+		char *toplevel = xgetcwd();
+		struct strbuf sb = STRBUF_INIT;
+
+		argv_array_pushf(&cp.env_array, "name=%s", sub->name);
+		argv_array_pushf(&cp.env_array, "sm_path=%s", path);
+		argv_array_pushf(&cp.env_array, "displaypath=%s", displaypath);
+		argv_array_pushf(&cp.env_array, "sha1=%s",
+				oid_to_hex(ce_oid));
+		argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
+
+		/*
+		 * Since the path variable was accessible from the script
+		 * before porting, it is also made available after porting.
+		 * The environment variable "PATH" has a very special purpose
+		 * on windows. And since environment variables are
+		 * case-insensitive in windows, it interferes with the
+		 * existing PATH variable. Hence, to avoid that, we expose
+		 * path via the args argv_array and not via env_array.
+		 */
+		sq_quote_buf(&sb, path);
+		argv_array_pushf(&cp.args, "path=%s; %s",
+				 sb.buf, info->argv[0]);
+		strbuf_release(&sb);
+		free(toplevel);
+	} else {
+		argv_array_pushv(&cp.args, info->argv);
+	}
+
+	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 = path;
+		prepare_submodule_repo_env(&cpr.env_array);
+
+		argv_array_pushl(&cpr.args, "--super-prefix", NULL);
+		argv_array_pushf(&cpr.args, "%s/", displaypath);
+		argv_array_pushl(&cpr.args, "submodule--helper", "foreach", "--recursive",
+				NULL);
+
+		if (info->quiet)
+			argv_array_push(&cpr.args, "--quiet");
+
+		argv_array_pushv(&cpr.args, info->argv);
+
+		if (run_command(&cpr))
+			die(_("run_command returned non-zero status while"
+				"recursing in the nested submodules of %s\n."),
+				displaypath);
+	}
+
+cleanup:
+	free(displaypath);
+}
+
+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;
+
+	struct option module_foreach_options[] = {
+		OPT__QUIET(&info.quiet, N_("Suppress output of entering each submodule command")),
+		OPT_BOOL(0, "recursive", &info.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)
+		return 1;
+
+	info.argc = argc;
+	info.argv = argv;
+	info.prefix = prefix;
+
+	for_each_listed_submodule(&list, runcommand_in_submodule_cb, &info);
+
+	return 0;
+}
+
 struct init_cb {
 	const char *prefix;
 	unsigned int flags;

@@ -1841,6 +1984,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},
 	{"status", module_status, SUPPORT_SUPER_PREFIX},
 	{"print-default-remote", print_default_remote, 0},

git-submodule.sh +1 ~~-38~~

index 331d71c908b..cba585f0754 100755
@@ -323,44 +323,7 @@ cmd_foreach()
 		shift
 	done
 
-	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 -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 &&
-				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
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
 }
 
 #

Signature · 1 line

2.17.0.255.g8bfb7c0704

Jonathan Tan

2018-05-09 17:13 UTC #21 lore raw