On 07/25, Prathamesh Chavan wrote: > When running 'git submodule foreach' from a subdirectory of your > repository, nested submodules get a bogus value for $sm_path: > For a submodule 'sub' that contains a nested submodule 'nested', > running 'git -C dir submodule foreach echo $path' 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 two 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. > In this case we would want to have path='sub/nested'. > > (b) As Ramsay noticed the documented value is wrong. For the non-nested > case the path is equal to the relative path from $pwd to the > submodules working directory. When following this model, > the expected value would be path='../sub/nested'. > > The behavior for (b) 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) such that "path" > is "the path from the toplevel of the 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 would fix the meaning of the $path using (b), such that "path" > is "the relative path from $pwd to the submodule", then we would break > any user that uses nested submodules (even from the root directory) as > the 'nested' would become 'sub/nested'. > > Both 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.
Great explanation, and I agree with going with choice (a). > > Discussed-with: Ramsay Jones <[email protected]> > Signed-off-by: Prathamesh Chavan <[email protected]> > Signed-off-by: Stefan Beller <[email protected]> > --- > 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 a427ddafd..493a64372 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -320,7 +320,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 6ba5daf42..0663622a4 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' > +$pwd/clone2-nested1-nested1-$nested1sha1 > +Entering '../nested1/nested2' > +$pwd/clone2/nested1-nested2-nested2-$nested2sha1 > +Entering '../nested1/nested2/nested3' > +$pwd/clone2/nested1/nested2-nested3-nested3-$nested3sha1 > +Entering '../nested1/nested2/nested3/submodule' > +$pwd/clone2/nested1/nested2/nested3-submodule-submodule-$submodulesha1 > +Entering '../sub1' > +$pwd/clone2-foo1-sub1-$sub1sha1 > +Entering '../sub2' > +$pwd/clone2-foo2-sub2-$sub2sha1 > +Entering '../sub3' > +$pwd/clone2-foo3-sub3-$sub3sha1 > +EOF > + > +test_expect_success 'test "submodule foreach --recursive" from subdirectory' > ' > + ( > + cd clone2/untracked && > + git submodule foreach --recursive "echo > \$toplevel-\$name-\$sm_path-\$sha1" >../../actual > + ) && > + test_i18ncmp expect actual > +' > > cat > expect <<EOF > nested1-nested1 > -- > 2.13.0 > -- Brandon Williams
