From: Jonathan Tan <email@example.com>
To: Stefan Beller <firstname.lastname@example.org>
Cc: email@example.com, firstname.lastname@example.org,
Subject: Re: [PATCH 1/5] submodule foreach: correct '$path' in nested submodules from a subdirectory
Date: Thu, 3 May 2018 10:47:54 -0700 [thread overview]
Message-ID: <email@example.com> (raw)
On Wed, 2 May 2018 17:53:54 -0700
Stefan Beller <firstname.lastname@example.org> wrote:
> From: Prathamesh Chavan <email@example.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  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
> 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
> (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
> 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
> 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.
next prev parent reply other threads:[~2018-05-03 17:48 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-03 0:53 [PATCH 0/5] Rebooting pc/submodule-helper-foreach Stefan Beller
2018-05-03 0:53 ` [PATCH 1/5] submodule foreach: correct '$path' in nested submodules from a subdirectory Stefan Beller
2018-05-03 13:29 ` Ramsay Jones
2018-05-03 17:47 ` Jonathan Tan [this message]
2018-05-03 18:12 ` Stefan Beller
2018-05-04 21:03 ` Jonathan Tan
2018-05-03 0:53 ` [PATCH 2/5] submodule foreach: document '$sm_path' instead of '$path' Stefan Beller
2018-05-03 17:50 ` Jonathan Tan
2018-05-03 0:53 ` [PATCH 3/5] submodule foreach: clarify the '$toplevel' variable documentation Stefan Beller
2018-05-03 17:51 ` Jonathan Tan
2018-05-03 0:53 ` [PATCH 4/5] submodule foreach: document variable '$displaypath' Stefan Beller
2018-05-03 0:53 ` [PATCH 5/5] submodule: port submodule subcommand 'foreach' from shell to C Stefan Beller
2018-05-03 1:06 ` Stefan Beller
2018-05-03 18:05 ` Jonathan Tan
2018-05-09 0:29 ` [PATCHv2 0/4] Rebooting pc/submodule-helper-foreach Stefan Beller
2018-05-09 0:29 ` [PATCH 1/4] submodule foreach: correct '$path' in nested submodules from a subdirectory Stefan Beller
2018-05-09 0:29 ` [PATCH 2/4] submodule foreach: document '$sm_path' instead of '$path' Stefan Beller
2018-05-09 0:29 ` [PATCH 3/4] submodule foreach: document variable '$displaypath' Stefan Beller
2018-05-09 0:29 ` [PATCH 4/4] submodule: port submodule subcommand 'foreach' from shell to C Stefan Beller
2018-05-10 6:37 ` Junio C Hamano
2018-05-10 21:25 ` [PATCH] " Stefan Beller
2018-05-09 17:13 ` [PATCHv2 0/4] Rebooting pc/submodule-helper-foreach Jonathan Tan
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).