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

[Jonathan Tan]

On Tue,  8 May 2018 17:29:48 -0700
Stefan Beller  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.

[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.1bffbb573c088d38c8e44...@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