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

[Jonathan Tan <jonathantanmy@google.com>]

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.