[PATCH 2/6] submodule update --init: correct path handling in recursive submodules

[PATCH 2/6] submodule update --init: correct path handling in recursive submodules

[Stefan Beller]

The new test demonstrates a failure in the code prior to this patch.
Instead of getting the expected
    Submodule 'submodule' (${pwd}/submodule) registered for path '../super/submodule'
the `super` directory is omitted and you get
    Submodule 'submodule' (${pwd}/submodule) registered for path '../submodule'
instead.

That happens because the prefix is ignored in `git submodule add`, probably
because that function itself cannot recurse; it may however called by
recursive instances of `git submodule update`, so we need to respect the
`prefix`.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh            |  2 +-
 t/t7406-submodule-update.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 2838069..fdb5fbd 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -474,7 +474,7 @@ cmd_init()
 		die_if_unmatched "$mode"
 		name=$(git submodule--helper name "$sm_path") || exit
 
-		displaypath=$(relative_path "$sm_path")
+		displaypath=$(relative_path "$prefix$sm_path")
 
 		# Copy url setting when it is not set yet
 		if test -z "$(git config "submodule.$name.url")"
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 68ea31d..9a4ba41 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -63,6 +63,10 @@ test_expect_success 'setup a submodule tree' '
 	 git submodule add ../none none &&
 	 test_tick &&
 	 git commit -m "none"
+	) &&
+	git clone . recursivesuper &&
+	( cd recursivesuper
+	 git submodule add ../super super
 	)
 '
 
@@ -95,6 +99,35 @@ test_expect_success 'submodule update from subdirectory' '
 	)
 '
 
+supersha1=$(cd super && git rev-parse HEAD)
+mergingsha1=$(cd super/merging && git rev-parse HEAD)
+nonesha1=$(cd super/none && git rev-parse HEAD)
+rebasingsha1=$(cd super/rebasing && git rev-parse HEAD)
+submodulesha1=$(cd super/submodule && git rev-parse HEAD)
+pwd=$(pwd)
+
+cat <<EOF >expect
+Submodule path '../super': checked out '$supersha1'
+Submodule 'merging' ($pwd/merging) registered for path '../super/merging'
+Submodule 'none' ($pwd/none) registered for path '../super/none'
+Submodule 'rebasing' ($pwd/rebasing) registered for path '../super/rebasing'
+Submodule 'submodule' ($pwd/submodule) registered for path '../super/submodule'
+Submodule path '../super/merging': checked out '$mergingsha1'
+Submodule path '../super/none': checked out '$nonesha1'
+Submodule path '../super/rebasing': checked out '$rebasingsha1'
+Submodule path '../super/submodule': checked out '$submodulesha1'
+EOF
+
+test_expect_success 'submodule update --init --recursive from subdirectory' '
+	git -C recursivesuper/super reset --hard HEAD^ &&
+	(cd recursivesuper &&
+	 mkdir tmp &&
+	 cd tmp &&
+	 git submodule update --init --recursive ../super >../../actual
+	) &&
+	test_cmp expect actual
+'
+
 apos="'";
 test_expect_success 'submodule update does not fetch already present commits' '
 	(cd submodule &&
-- 
2.8.0.4.g5639dee.dirty

[PATCHv3 0/6] Fix path bugs in submodule commands executed from sub dir

[Stefan Beller]

v3:
Resending without `test_pause` in the last test.

v2:
The first 3 commits are
* Test and bugfix in one commit each
* better explained than before

The expansion of an expected test result moved to the back of the series.

There are two new commits
* one being another bugfix of the display path for `submodule update`
* another test for `submodule update` as I suspect it may break further on
  refactoring and we currently have no tests for it.

Thanks,
Stefan

Stefan Beller (6):
  submodule foreach: correct path display in recursive submodules
  submodule update --init: correct path handling in recursive submodules
  submodule status: correct path handling in recursive submodules
  t7407: make expectation as clear as possible
  submodule update: align reporting path for custom command execution
  submodule update: test recursive path reporting from subdirectory

 git-submodule.sh             | 10 +++---
 t/t7406-submodule-update.sh  | 83 ++++++++++++++++++++++++++++++++++++++++++--
 t/t7407-submodule-foreach.sh | 49 ++++++++++++++++++++++++--
 3 files changed, 133 insertions(+), 9 deletions(-)

-- 
2.8.0.4.g5639dee.dirty

[PATCH 1/6] submodule foreach: correct path display in recursive submodules

[Stefan Beller]

The new test replicates the previous test with the difference of executing
from a sub directory. By executing from a sub directory all we would
expect all displayed paths to be prefixed by '../'.

Prior to this patch the test would report
    Entering 'nested1/nested2/../nested3'
instead of the expected
    Entering '../nested1/nested2/nested3'

because the prefix is put unconditionally in front and after that a
computed display path with is affected by `wt_prefix`. This is wrong as
any relative path computation would need to be at the front. By emptying
the `wt_prefix` in recursive submodules and adding the information of any
relative path into the `prefix` this is fixed.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh             |  3 ++-
 t/t7407-submodule-foreach.sh | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 43c68de..2838069 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -417,10 +417,11 @@ cmd_foreach()
 			say "$(eval_gettext "Entering '\$prefix\$displaypath'")"
 			name=$(git submodule--helper name "$sm_path")
 			(
-				prefix="$prefix$sm_path/"
+				prefix="$(relative_path $prefix$sm_path)/"
 				clear_local_git_env
 				cd "$sm_path" &&
 				sm_path=$(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 7ca10b8..776b349 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -178,6 +178,26 @@ test_expect_success 'test messages from "foreach --recursive"' '
 '
 
 cat > expect <<EOF
+Entering '../nested1'
+Entering '../nested1/nested2'
+Entering '../nested1/nested2/nested3'
+Entering '../nested1/nested2/nested3/submodule'
+Entering '../sub1'
+Entering '../sub2'
+Entering '../sub3'
+EOF
+
+test_expect_success 'test messages from "foreach --recursive" from subdirectory' '
+	(
+		cd clone2 &&
+		mkdir untracked &&
+		cd untracked &&
+		git submodule foreach --recursive >../../actual
+	) &&
+	test_i18ncmp expect actual
+'
+
+cat > expect <<EOF
 nested1-nested1
 nested2-nested2
 nested3-nested3
-- 
2.8.0.6.g3d0b0ba

[PATCH 2/6] submodule update --init: correct path handling in recursive submodules

[Stefan Beller]

The new test demonstrates a failure in the code prior to this patch.
Instead of getting the expected
    Submodule 'submodule' (${pwd}/submodule) registered for path '../super/submodule'
the `super` directory is omitted and you get
    Submodule 'submodule' (${pwd}/submodule) registered for path '../submodule'
instead.

That happens because the prefix is ignored in `git submodule add`, probably
because that function itself cannot recurse; it may however called by
recursive instances of `git submodule update`, so we need to respect the
`prefix`.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh            |  2 +-
 t/t7406-submodule-update.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 2838069..fdb5fbd 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -474,7 +474,7 @@ cmd_init()
 		die_if_unmatched "$mode"
 		name=$(git submodule--helper name "$sm_path") || exit
 
-		displaypath=$(relative_path "$sm_path")
+		displaypath=$(relative_path "$prefix$sm_path")
 
 		# Copy url setting when it is not set yet
 		if test -z "$(git config "submodule.$name.url")"
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 68ea31d..9a4ba41 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -63,6 +63,10 @@ test_expect_success 'setup a submodule tree' '
 	 git submodule add ../none none &&
 	 test_tick &&
 	 git commit -m "none"
+	) &&
+	git clone . recursivesuper &&
+	( cd recursivesuper
+	 git submodule add ../super super
 	)
 '
 
@@ -95,6 +99,35 @@ test_expect_success 'submodule update from subdirectory' '
 	)
 '
 
+supersha1=$(cd super && git rev-parse HEAD)
+mergingsha1=$(cd super/merging && git rev-parse HEAD)
+nonesha1=$(cd super/none && git rev-parse HEAD)
+rebasingsha1=$(cd super/rebasing && git rev-parse HEAD)
+submodulesha1=$(cd super/submodule && git rev-parse HEAD)
+pwd=$(pwd)
+
+cat <<EOF >expect
+Submodule path '../super': checked out '$supersha1'
+Submodule 'merging' ($pwd/merging) registered for path '../super/merging'
+Submodule 'none' ($pwd/none) registered for path '../super/none'
+Submodule 'rebasing' ($pwd/rebasing) registered for path '../super/rebasing'
+Submodule 'submodule' ($pwd/submodule) registered for path '../super/submodule'
+Submodule path '../super/merging': checked out '$mergingsha1'
+Submodule path '../super/none': checked out '$nonesha1'
+Submodule path '../super/rebasing': checked out '$rebasingsha1'
+Submodule path '../super/submodule': checked out '$submodulesha1'
+EOF
+
+test_expect_success 'submodule update --init --recursive from subdirectory' '
+	git -C recursivesuper/super reset --hard HEAD^ &&
+	(cd recursivesuper &&
+	 mkdir tmp &&
+	 cd tmp &&
+	 git submodule update --init --recursive ../super >../../actual
+	) &&
+	test_cmp expect actual
+'
+
 apos="'";
 test_expect_success 'submodule update does not fetch already present commits' '
 	(cd submodule &&
-- 
2.8.0.6.g3d0b0ba

[PATCH 3/6] submodule status: correct path handling in recursive submodules

[Stefan Beller]

The new test which is a replica of the previous test except
that it executes from a sub directory. Prior to this patch
the test failed by having too many '../' prefixed:

  --- expect	2016-03-29 19:02:33.087336115 +0000
  +++ actual	2016-03-29 19:02:33.359343311 +0000
  @@ -1,7 +1,7 @@
    b23f134787d96fae589a6b76da41f4db112fc8db ../nested1 (heads/master)
  -+25d56d1ddfb35c3e91ff7d8f12331c2e53147dcc ../nested1/nested2 (file2)
  - 5ec83512b76a0b8170b899f8e643913c3e9b72d9 ../nested1/nested2/nested3 (heads/master)
  - 509f622a4f36a3e472affcf28fa959174f3dd5b5 ../nested1/nested2/nested3/submodule (heads/master)
  ++25d56d1ddfb35c3e91ff7d8f12331c2e53147dcc ../../nested1/nested2 (file2)
  + 5ec83512b76a0b8170b899f8e643913c3e9b72d9 ../../../nested1/nested2/nested3 (heads/master)
  + 509f622a4f36a3e472affcf28fa959174f3dd5b5 ../../../../nested1/nested2/nested3/submodule (heads/master)
    0c90624ab7f1aaa301d3bb79f60dcfed1ec4897f ../sub1 (0c90624)
    0c90624ab7f1aaa301d3bb79f60dcfed1ec4897f ../sub2 (0c90624)
    509f622a4f36a3e472affcf28fa959174f3dd5b5 ../sub3 (heads/master)

The path code in question:
  displaypath=$(relative_path "$prefix$sm_path")
  prefix=$displaypath
  if recursive:
    eval cmd_status

That way we change `prefix` each iteration to contain another
'../', because of the the relative_path computation is done
on an already computed relative path.

We must call relative_path exactly once with `wt_prefix` non empty.
Further calls in recursive instances to to calculate the displaypath
already incorporate the correct prefix from before. Fix the issue by
clearing `wt_prefix` in recursive calls.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh             |  1 +
 t/t7407-submodule-foreach.sh | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/git-submodule.sh b/git-submodule.sh
index fdb5fbd..11ed32a 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1160,6 +1160,7 @@ cmd_status()
 			(
 				prefix="$displaypath/"
 				clear_local_git_env
+				wt_prefix=
 				cd "$sm_path" &&
 				eval cmd_status
 			) ||
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 776b349..4b35e12 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -277,6 +277,27 @@ test_expect_success 'ensure "status --cached --recursive" preserves the --cached
 	test_cmp expect actual
 '
 
+nested2sha1=$(git -C clone3/nested1/nested2 rev-parse HEAD)
+
+cat > expect <<EOF
+ $nested1sha1 ../nested1 (heads/master)
++$nested2sha1 ../nested1/nested2 (file2)
+ $nested3sha1 ../nested1/nested2/nested3 (heads/master)
+ $submodulesha1 ../nested1/nested2/nested3/submodule (heads/master)
+ $sub1sha1 ../sub1 ($sub1sha1_short)
+ $sub2sha1 ../sub2 ($sub2sha1_short)
+ $sub3sha1 ../sub3 (heads/master)
+EOF
+
+test_expect_success 'test "status --recursive" from sub directory' '
+	(
+		cd clone3 &&
+		mkdir tmp && cd tmp &&
+		git submodule status --recursive > ../../actual
+	) &&
+	test_cmp expect actual
+'
+
 test_expect_success 'use "git clone --recursive" to checkout all submodules' '
 	git clone --recursive super clone4 &&
 	(
-- 
2.8.0.6.g3d0b0ba

[PATCH 4/6] submodule update: align reporting path for custom command execution

[Stefan Beller]

In the predefined actions (merge, rebase, none, checkout), we use
the display path, which is relative to the current working directory.
Also use the display path when running a custom command.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh            |  4 ++--
 t/t7406-submodule-update.sh | 29 ++++++++++++++++++++++++++---
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 11ed32a..be2a2b5 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -803,8 +803,8 @@ Maybe you want to use 'update --init'?")"
 				;;
 			!*)
 				command="${update_module#!}"
-				die_msg="$(eval_gettext "Execution of '\$command \$sha1' failed in submodule path '\$prefix\$sm_path'")"
-				say_msg="$(eval_gettext "Submodule path '\$prefix\$sm_path': '\$command \$sha1'")"
+				die_msg="$(eval_gettext "Execution of '\$command \$sha1' failed in submodule path '\$displaypath'")"
+				say_msg="$(eval_gettext "Submodule path '\$displaypath': '\$command \$sha1'")"
 				must_die_on_failure=yes
 				;;
 			*)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 9a4ba41..f062065 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -344,16 +344,39 @@ test_expect_success 'submodule update - command in .git/config' '
 	)
 '
 
+cat << EOF >expect
+Execution of 'false $submodulesha1' failed in submodule path 'submodule'
+EOF
+
 test_expect_success 'submodule update - command in .git/config catches failure' '
 	(cd super &&
 	 git config submodule.submodule.update "!false"
 	) &&
 	(cd super/submodule &&
-	  git reset --hard HEAD^
+	  git reset --hard $submodulesha1^
 	) &&
 	(cd super &&
-	 test_must_fail git submodule update submodule
-	)
+	 test_must_fail git submodule update submodule 2>../actual
+	) &&
+	test_cmp actual expect
+'
+
+cat << EOF >expect
+Execution of 'false $submodulesha1' failed in submodule path '../submodule'
+EOF
+
+test_expect_success 'submodule update - command in .git/config catches failure -- subdirectory' '
+	(cd super &&
+	 git config submodule.submodule.update "!false"
+	) &&
+	(cd super/submodule &&
+	  git reset --hard $submodulesha1^
+	) &&
+	(cd super &&
+	 mkdir tmp && cd tmp &&
+	 test_must_fail git submodule update ../submodule 2>../../actual
+	) &&
+	test_cmp actual expect
 '
 
 test_expect_success 'submodule init does not copy command into .git/config' '
-- 
2.8.0.6.g3d0b0ba

[PATCH 5/6] submodule update: test recursive path reporting from subdirectory

[Stefan Beller]

This patch is just a test and fixes no bug as there is currently no bug
in the path handling of `submodule update`.

In `submodule update` we make a call to `submodule--helper list --prefix
"$wt_prefix"` which looks a bit brittle and likely to introduce a bug
for the path handling. It is not a bug as the prefix is ignored inside
the submodule helper for now. If this test breaks eventually, we want
to make sure the `wt_prefix` is passed correctly into recursive submodules.
Hint: In recursive submodules we expect `wt_prefix` to be empty.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7406-submodule-update.sh | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f062065..6dcf2d4 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -379,6 +379,26 @@ test_expect_success 'submodule update - command in .git/config catches failure -
 	test_cmp actual expect
 '
 
+cat << EOF >expect
+Execution of 'false $submodulesha1' failed in submodule path '../super/submodule'
+Failed to recurse into submodule path '../super'
+EOF
+
+test_expect_success 'recursive submodule update - command in .git/config catches failure -- subdirectory' '
+	(cd recursivesuper &&
+	 git submodule update --remote super &&
+	 git add super &&
+	 git commit -m "update to latest to have more than one commit in submodules"
+	) &&
+	git -C recursivesuper/super config submodule.submodule.update "!false" &&
+	git -C recursivesuper/super/submodule reset --hard $submodulesha1^ &&
+	(cd recursivesuper &&
+	 mkdir -p tmp && cd tmp &&
+	 test_must_fail git submodule update --recursive ../super 2>../../actual
+	) &&
+	test_cmp actual expect
+'
+
 test_expect_success 'submodule init does not copy command into .git/config' '
 	(cd super &&
 	 H=$(git ls-files -s submodule | cut -d" " -f2) &&
-- 
2.8.0.6.g3d0b0ba

[PATCH 6/6] t7407: make expectation as clear as possible

[Stefan Beller]

Not everyone (including me) grasps the sed expression in a split second as
they would grasp the 4 lines printed as is.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7407-submodule-foreach.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 4b35e12..6ba5daf 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -262,8 +262,12 @@ test_expect_success 'test "status --recursive"' '
 	test_cmp expect actual
 '
 
-sed -e "/nested2 /s/.*/+$nested2sha1 nested1\/nested2 (file2~1)/;/sub[1-3]/d" < expect > expect2
-mv -f expect2 expect
+cat > expect <<EOF
+ $nested1sha1 nested1 (heads/master)
++$nested2sha1 nested1/nested2 (file2~1)
+ $nested3sha1 nested1/nested2/nested3 (heads/master)
+ $submodulesha1 nested1/nested2/nested3/submodule (heads/master)
+EOF
 
 test_expect_success 'ensure "status --cached --recursive" preserves the --cached flag' '
 	(
-- 
2.8.0.6.g3d0b0ba

Re: [PATCH 1/6] submodule foreach: correct path display in recursive submodules

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The new test replicates the previous test with the difference of executing
> from a sub directory. By executing from a sub directory all we would
> expect all displayed paths to be prefixed by '../'.

-ECANTPARSE on the last sentence.  Remove the first "all" perhaps?

As this patch is no longer about a new test but is about a fix of a
problem, for which a new test serves as a typical example, it sounds
somewhat funny to start the log message with description of the test.

> Prior to this patch the test would report
>     Entering 'nested1/nested2/../nested3'
> instead of the expected
>     Entering '../nested1/nested2/nested3'
>
> because the prefix is put unconditionally in front and after that a
> computed display path with is affected by `wt_prefix`.

-ECANTPARSE even though I can guess what you want to say.

> diff --git a/git-submodule.sh b/git-submodule.sh
> index 43c68de..2838069 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -417,10 +417,11 @@ cmd_foreach()
>  			say "$(eval_gettext "Entering '\$prefix\$displaypath'")"
>  			name=$(git submodule--helper name "$sm_path")
>  			(
> -				prefix="$prefix$sm_path/"
> +				prefix="$(relative_path $prefix$sm_path)/"

What happens when prefix or sm_path has $IFS whitespace?  Imitate the
correct quoting you do three lines below, i.e.

	prefix=$(relative_path "$prefix$sm_path")/

>  				clear_local_git_env
>  				cd "$sm_path" &&
>  				sm_path=$(relative_path "$sm_path") &&
> +				wt_prefix=
>  				# we make $path available to scripts ...
>  				path=$sm_path &&
>  				if test $# -eq 1

Re: [PATCH 2/6] submodule update --init: correct path handling in recursive submodules

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The new test demonstrates a failure in the code prior to this patch.
> Instead of getting the expected
>     Submodule 'submodule' (${pwd}/submodule) registered for path '../super/submodule'
> the `super` directory is omitted and you get
>     Submodule 'submodule' (${pwd}/submodule) registered for path '../submodule'
> instead.

Same "is this about test?" comment applies here.

> That happens because the prefix is ignored in `git submodule add`, probably
> because that function itself cannot recurse;

"probably"???

> it may however called by

Probably "be" needs to be somewhere on this line.

> recursive instances of `git submodule update`, so we need to respect the
> `prefix`.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  git-submodule.sh            |  2 +-
>  t/t7406-submodule-update.sh | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 2838069..fdb5fbd 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -474,7 +474,7 @@ cmd_init()
>  		die_if_unmatched "$mode"
>  		name=$(git submodule--helper name "$sm_path") || exit
>  
> -		displaypath=$(relative_path "$sm_path")
> +		displaypath=$(relative_path "$prefix$sm_path")
>  
>  		# Copy url setting when it is not set yet
>  		if test -z "$(git config "submodule.$name.url")"
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 68ea31d..9a4ba41 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -63,6 +63,10 @@ test_expect_success 'setup a submodule tree' '
>  	 git submodule add ../none none &&
>  	 test_tick &&
>  	 git commit -m "none"
> +	) &&
> +	git clone . recursivesuper &&
> +	( cd recursivesuper
> +	 git submodule add ../super super
>  	)
>  '
>  
> @@ -95,6 +99,35 @@ test_expect_success 'submodule update from subdirectory' '
>  	)
>  '
>  
> +supersha1=$(cd super && git rev-parse HEAD)

Perhaps "git -C super rev-parse HEAD"?

> +test_expect_success 'submodule update --init --recursive from subdirectory' '
> +	git -C recursivesuper/super reset --hard HEAD^ &&
> +	(cd recursivesuper &&
> +	 mkdir tmp &&
> +	 cd tmp &&
> +	 git submodule update --init --recursive ../super >../../actual
> +	) &&
> +	test_cmp expect actual
> +'
> +
>  apos="'";
>  test_expect_success 'submodule update does not fetch already present commits' '
>  	(cd submodule &&

Re: [PATCH 3/6] submodule status: correct path handling in recursive submodules

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The new test which is a replica of the previous test except
> that it executes from a sub directory. Prior to this patch
> the test failed by having too many '../' prefixed:
>
>   --- expect	2016-03-29 19:02:33.087336115 +0000
>   +++ actual	2016-03-29 19:02:33.359343311 +0000
>   @@ -1,7 +1,7 @@
>     b23f134787d96fae589a6b76da41f4db112fc8db ../nested1 (heads/master)
>   -+25d56d1ddfb35c3e91ff7d8f12331c2e53147dcc ../nested1/nested2 (file2)
>   - 5ec83512b76a0b8170b899f8e643913c3e9b72d9 ../nested1/nested2/nested3 (heads/master)
>   - 509f622a4f36a3e472affcf28fa959174f3dd5b5 ../nested1/nested2/nested3/submodule (heads/master)
>   ++25d56d1ddfb35c3e91ff7d8f12331c2e53147dcc ../../nested1/nested2 (file2)
>   + 5ec83512b76a0b8170b899f8e643913c3e9b72d9 ../../../nested1/nested2/nested3 (heads/master)
>   + 509f622a4f36a3e472affcf28fa959174f3dd5b5 ../../../../nested1/nested2/nested3/submodule (heads/master)
>     0c90624ab7f1aaa301d3bb79f60dcfed1ec4897f ../sub1 (0c90624)
>     0c90624ab7f1aaa301d3bb79f60dcfed1ec4897f ../sub2 (0c90624)
>     509f622a4f36a3e472affcf28fa959174f3dd5b5 ../sub3 (heads/master)
>
> The path code in question:
>   displaypath=$(relative_path "$prefix$sm_path")
>   prefix=$displaypath
>   if recursive:
>     eval cmd_status
>
> That way we change `prefix` each iteration to contain another
> '../', because of the the relative_path computation is done
> on an already computed relative path.
>
> We must call relative_path exactly once with `wt_prefix` non empty.
> Further calls in recursive instances to to calculate the displaypath
> already incorporate the correct prefix from before. Fix the issue by
> clearing `wt_prefix` in recursive calls.

OK, nicely analyzed and explained.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  git-submodule.sh             |  1 +
>  t/t7407-submodule-foreach.sh | 21 +++++++++++++++++++++
>  2 files changed, 22 insertions(+)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index fdb5fbd..11ed32a 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -1160,6 +1160,7 @@ cmd_status()
>  			(
>  				prefix="$displaypath/"
>  				clear_local_git_env
> +				wt_prefix=
>  				cd "$sm_path" &&
>  				eval cmd_status
>  			) ||

Makes sense.

Thanks.


> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
> index 776b349..4b35e12 100755
> --- a/t/t7407-submodule-foreach.sh
> +++ b/t/t7407-submodule-foreach.sh
> @@ -277,6 +277,27 @@ test_expect_success 'ensure "status --cached --recursive" preserves the --cached
>  	test_cmp expect actual
>  '
>  
> +nested2sha1=$(git -C clone3/nested1/nested2 rev-parse HEAD)
> +
> +cat > expect <<EOF
> + $nested1sha1 ../nested1 (heads/master)
> ++$nested2sha1 ../nested1/nested2 (file2)
> + $nested3sha1 ../nested1/nested2/nested3 (heads/master)
> + $submodulesha1 ../nested1/nested2/nested3/submodule (heads/master)
> + $sub1sha1 ../sub1 ($sub1sha1_short)
> + $sub2sha1 ../sub2 ($sub2sha1_short)
> + $sub3sha1 ../sub3 (heads/master)
> +EOF
> +
> +test_expect_success 'test "status --recursive" from sub directory' '
> +	(
> +		cd clone3 &&
> +		mkdir tmp && cd tmp &&
> +		git submodule status --recursive > ../../actual
> +	) &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'use "git clone --recursive" to checkout all submodules' '
>  	git clone --recursive super clone4 &&
>  	(

Re: [PATCH 4/6] submodule update: align reporting path for custom command execution

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> In the predefined actions (merge, rebase, none, checkout), we use
> the display path, which is relative to the current working directory.
> Also use the display path when running a custom command.

Very sensible.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  git-submodule.sh            |  4 ++--
>  t/t7406-submodule-update.sh | 29 ++++++++++++++++++++++++++---
>  2 files changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 11ed32a..be2a2b5 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -803,8 +803,8 @@ Maybe you want to use 'update --init'?")"
>  				;;
>  			!*)
>  				command="${update_module#!}"
> -				die_msg="$(eval_gettext "Execution of '\$command \$sha1' failed in submodule path '\$prefix\$sm_path'")"
> -				say_msg="$(eval_gettext "Submodule path '\$prefix\$sm_path': '\$command \$sha1'")"
> +				die_msg="$(eval_gettext "Execution of '\$command \$sha1' failed in submodule path '\$displaypath'")"
> +				say_msg="$(eval_gettext "Submodule path '\$displaypath': '\$command \$sha1'")"
>  				must_die_on_failure=yes
>  				;;
>  			*)
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 9a4ba41..f062065 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -344,16 +344,39 @@ test_expect_success 'submodule update - command in .git/config' '
>  	)
>  '
>  
> +cat << EOF >expect
> +Execution of 'false $submodulesha1' failed in submodule path 'submodule'
> +EOF
> +
>  test_expect_success 'submodule update - command in .git/config catches failure' '
>  	(cd super &&
>  	 git config submodule.submodule.update "!false"
>  	) &&
>  	(cd super/submodule &&
> -	  git reset --hard HEAD^
> +	  git reset --hard $submodulesha1^
>  	) &&
>  	(cd super &&
> -	 test_must_fail git submodule update submodule
> -	)
> +	 test_must_fail git submodule update submodule 2>../actual
> +	) &&
> +	test_cmp actual expect
> +'
> +
> +cat << EOF >expect
> +Execution of 'false $submodulesha1' failed in submodule path '../submodule'
> +EOF
> +
> +test_expect_success 'submodule update - command in .git/config catches failure -- subdirectory' '
> +	(cd super &&
> +	 git config submodule.submodule.update "!false"
> +	) &&
> +	(cd super/submodule &&
> +	  git reset --hard $submodulesha1^
> +	) &&
> +	(cd super &&
> +	 mkdir tmp && cd tmp &&
> +	 test_must_fail git submodule update ../submodule 2>../../actual
> +	) &&
> +	test_cmp actual expect
>  '
>  
>  test_expect_success 'submodule init does not copy command into .git/config' '

Re: [PATCH 2/6] submodule update --init: correct path handling in recursive submodules

[Stefan Beller]

On Tue, Mar 29, 2016 at 4:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The new test demonstrates a failure in the code prior to this patch.
>> Instead of getting the expected
>>     Submodule 'submodule' (${pwd}/submodule) registered for path '../super/submodule'
>> the `super` directory is omitted and you get
>>     Submodule 'submodule' (${pwd}/submodule) registered for path '../submodule'
>> instead.
>
> Same "is this about test?" comment applies here.
>
>> That happens because the prefix is ignored in `git submodule add`, probably
>> because that function itself cannot recurse;
>
> "probably"???

I am speculating there. I have no idea why it originally was written that way
but I would assume this is the most likeliest explanation.
I'll reword the whole commit message.

>
>>
>> +supersha1=$(cd super && git rev-parse HEAD)
>
> Perhaps "git -C super rev-parse HEAD"?

done