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@xxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Prathamesh Chavan <pc44800@xxxxxxxxx>
Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
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()
+ };
+
