[PATCH 0/5] Rebooting pc/submodule-helper-foreach

[PATCH 0/5] Rebooting pc/submodule-helper-foreach

[Stefan Beller]

The "What's cooking" email carried this series for some time now:
> * 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. <20180206150044.1bffbb573c088d38c8e44bf5@google.com>

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.

With this series, we get
* keep the invariant of $toplevel + $path to be an absolute path that is
  correctly pointing at the submodule. "git -C $toplevel config" + $name
  allowing to ask configuration of the submodule.  
* $displaypath will have the relative path form $pwd to the submodule root.
* better documentation.

In later patches we could add $topmost, that points at the superproject
in which the command was started from, and $path_from_topmost, that would
be the relative path from $topmost to the submodule, potentially skipping
intermediate superprojects.

Thanks,
Stefan

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

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

-- 
2.17.0.441.gb46fe60e1d-goog

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

[Stefan Beller]

From: Prathamesh Chavan <pc44800@gmail.com>

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

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.

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.

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.

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 <ramsay@ramsayjones.plus.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh             |  1 -
 t/t7407-submodule-foreach.sh | 36 ++++++++++++++++++++++++++++++++++--
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 24914963ca2..331d71c908b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6ba5daf42ee..5144cc6926b 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -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
-- 
2.17.0.441.gb46fe60e1d-goog

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

[Ramsay Jones]

On 03/05/18 01:53, Stefan Beller wrote:
> From: Prathamesh Chavan <pc44800@gmail.com>
> 
> 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.]
> 
> 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

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

[Jonathan Tan]

On Wed,  2 May 2018 17:53:54 -0700
Stefan Beller <sbeller@google.com> wrote:

> From: Prathamesh Chavan <pc44800@gmail.com>
> 
> 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/20180206145406.b759164cead02cd3bb3fdce0@google.com/

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

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

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

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

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

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

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

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

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

[Stefan Beller]

On Thu, May 3, 2018 at 10:47 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
> On Wed,  2 May 2018 17:53:54 -0700
> Stefan Beller <sbeller@google.com> wrote:
>
>> From: Prathamesh Chavan <pc44800@gmail.com>
>>
>> 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/20180206145406.b759164cead02cd3bb3fdce0@google.com/
>
>> 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.

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

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

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

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

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

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


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


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

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

[Jonathan Tan]

On Thu, 3 May 2018 11:12:27 -0700
Stefan Beller <sbeller@google.com> wrote:

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

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

> >> (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'/

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

  <your first paragraph starting with "When running">

  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:

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

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

[Stefan Beller]

From: Prathamesh Chavan <pc44800@gmail.com>

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 <ramsay@ramsayjones.plus.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-submodule.txt | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 71c5618e82a..755ed695f08 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -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.
-- 
2.17.0.441.gb46fe60e1d-goog

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

[Jonathan Tan]

On Wed,  2 May 2018 17:53:55 -0700
Stefan Beller <sbeller@google.com> wrote:

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

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.

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

[Stefan Beller]

From: Prathamesh Chavan <pc44800@gmail.com>

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 <ramsay@ramsayjones.plus.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-submodule.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 755ed695f08..408d5a0387f 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -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
-- 
2.17.0.441.gb46fe60e1d-goog

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

[Jonathan Tan]

On Wed,  2 May 2018 17:53:56 -0700
Stefan Beller <sbeller@google.com> wrote:

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

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

[Stefan Beller]

From: Prathamesh Chavan <pc44800@gmail.com>

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

Discussed-with: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-submodule.txt |  6 ++++--
 t/t7407-submodule-foreach.sh    | 22 +++++++++++-----------
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 408d5a0387f..4372d00c42e 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -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.
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 5144cc6926b..77729ac4aa1 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -82,16 +82,16 @@ test_expect_success 'test basic "submodule foreach" usage' '
 
 cat >expect <<EOF
 Entering '../sub1'
-$pwd/clone-foo1-sub1-$sub1sha1
+$pwd/clone-foo1-sub1-../sub1-$sub1sha1
 Entering '../sub3'
-$pwd/clone-foo3-sub3-$sub3sha1
+$pwd/clone-foo3-sub3-../sub3-$sub3sha1
 EOF
 
 test_expect_success 'test "submodule foreach" from subdirectory' '
 	mkdir clone/sub &&
 	(
 		cd clone/sub &&
-		git submodule foreach "echo \$toplevel-\$name-\$sm_path-\$sha1" >../../actual
+		git submodule foreach "echo \$toplevel-\$name-\$sm_path-\$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
 '
-- 
2.17.0.441.gb46fe60e1d-goog

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

[Stefan Beller]

From: Prathamesh Chavan <pc44800@gmail.com>

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 <bmwill@google.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 148 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  39 +---------
 2 files changed, 149 insertions(+), 38 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a404df3ea49..bbbea5868de 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -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},
diff --git a/git-submodule.sh b/git-submodule.sh
index 331d71c908b..cba585f0754 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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} "$@"
 }
 
 #
-- 
2.17.0.441.gb46fe60e1d-goog

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

[Stefan Beller]

On Wed, May 2, 2018 at 5:53 PM, Stefan Beller <sbeller@google.com> wrote:

> +struct cb_foreach {
> +       char *toplevel;
...
> +               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.

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

[Jonathan Tan]

On Wed,  2 May 2018 17:53:58 -0700
Stefan Beller <sbeller@google.com> wrote:

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

[PATCHv2 0/4] Rebooting pc/submodule-helper-foreach

[Stefan Beller]

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:
> * 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. <20180206150044.1bffbb573c088d38c8e44bf5@google.com>

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.

With this series, we get
* keep the invariant of $toplevel + $path to be an absolute path that is
  correctly pointing at the submodule. "git -C $toplevel config" + $name
  allowing to ask configuration of the submodule.  
* $displaypath will have the relative path form $pwd to the submodule root.
* better documentation.

In later patches we could add $topmost, that points at the superproject
in which the command was started from, and $path_from_topmost, that would
be the relative path from $topmost to the submodule, potentially skipping
intermediate superprojects.

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

-- 
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 <ramsay@ramsayjones.plus.com>
         Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
         Signed-off-by: Stefan Beller <sbeller@google.com>
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()
     +	};
     +

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

[Stefan Beller]

From: Prathamesh Chavan <pc44800@gmail.com>

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.

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

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.

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.

Discussed-with: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh             |  1 -
 t/t7407-submodule-foreach.sh | 36 ++++++++++++++++++++++++++++++++++--
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 24914963ca2..331d71c908b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6ba5daf42ee..5144cc6926b 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -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
-- 
2.17.0.255.g8bfb7c0704

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

[Stefan Beller]

From: Prathamesh Chavan <pc44800@gmail.com>

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 <ramsay@ramsayjones.plus.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-submodule.txt | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 630999f41a9..066c7b6c18e 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -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.
-- 
2.17.0.255.g8bfb7c0704

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

[Stefan Beller]

From: Prathamesh Chavan <pc44800@gmail.com>

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

Discussed-with: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-submodule.txt |  8 +++++---
 t/t7407-submodule-foreach.sh    | 22 +++++++++++-----------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 066c7b6c18e..500dfdafd16 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -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'
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 5144cc6926b..77729ac4aa1 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -82,16 +82,16 @@ test_expect_success 'test basic "submodule foreach" usage' '
 
 cat >expect <<EOF
 Entering '../sub1'
-$pwd/clone-foo1-sub1-$sub1sha1
+$pwd/clone-foo1-sub1-../sub1-$sub1sha1
 Entering '../sub3'
-$pwd/clone-foo3-sub3-$sub3sha1
+$pwd/clone-foo3-sub3-../sub3-$sub3sha1
 EOF
 
 test_expect_success 'test "submodule foreach" from subdirectory' '
 	mkdir clone/sub &&
 	(
 		cd clone/sub &&
-		git submodule foreach "echo \$toplevel-\$name-\$sm_path-\$sha1" >../../actual
+		git submodule foreach "echo \$toplevel-\$name-\$sm_path-\$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
 '
-- 
2.17.0.255.g8bfb7c0704

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

[Stefan Beller]

From: Prathamesh Chavan <pc44800@gmail.com>

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 <bmwill@google.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 144 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  39 +---------
 2 files changed, 145 insertions(+), 38 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c2403a915ff..4b0b773c06b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -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},
diff --git a/git-submodule.sh b/git-submodule.sh
index 331d71c908b..cba585f0754 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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} "$@"
 }
 
 #
-- 
2.17.0.255.g8bfb7c0704

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

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

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

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

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.

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

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

[Stefan Beller]

From: Prathamesh Chavan <pc44800@gmail.com>

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 <bmwill@google.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c2403a915ff..4002026d1ac 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -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},
diff --git a/git-submodule.sh b/git-submodule.sh
index 331d71c908b..cba585f0754 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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} "$@"
 }
 
 #
-- 
2.17.0.255.g8bfb7c0704

Re: [PATCHv2 0/4] Rebooting pc/submodule-helper-foreach

[Jonathan Tan]

On Tue,  8 May 2018 17:29:48 -0700
Stefan Beller <sbeller@google.com> wrote:

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

Patches 1-3 look good to me.

I also can't see anything wrong with patch 4, but I am not an expert at
shell and how we call it from C, so a review from another reviewer would
be appreciated.