threads / patch / 41852

patch, 7 partssubmodule foreach: test path handling in recursive submodules

Subject: [PATCH 1/7] submodule foreach: test path handling in recursive submodules

## tl;dr

17 messages between Mar 28, 2016 and Mar 29, 2016. Diffs are folded; open one to read it.

replies: 16people: 2as markdown or json

Stefan Beller· Mar 28, 2016, 23:28 UTC · lore

[PATCH 0/7] Fix path bugs in submodule commands executed from sub dir [WAS: submodule--helper clone: lose the extra prefix option]

Junio wrote:
> I suspect that the fix in your 1&2 may be demonstratable without
> forcing an early failure by switching to "git -C".

So for now I present test coverage and their minimal fixes. This series follows a "tick-tock" pattern except for patch5, which I wrote quickly as I was annoying by the bells and whistles. I expect test code to be dumb, not tricking ourselves by "smart" code there.

The "tick" patches introduce failing tests. They need to fail to demonstrate the bugs exist, which are fixed in the "tock" patches, which are doing nothing fancy but just a one or two line correction of the path handling code.

This applies to 2.8.

As this is taking a completely different turn than I expected in "[PATCHv3 0/5] submodule helper: cleanup prefix passing", I made this a new series. (It also doesn't do cleanup any more, but just fixes bugs.)

Thanks, Stefan

Stefan Beller (7):
  submodule foreach: test path handling in recursive submodules
  submodule foreach: correct path computation in recursive submodules
  submodule update --init: test path handling in recursive submodules
  submodule update --init: correct path handling in recursive submodules
  t7407: make expectation as clear as possible
  submodule status: test path handling in recursive submodules
  submodule status: fix path handling in recursive submodules
 git-submodule.sh             |  9 ++++++---
 t/t7406-submodule-update.sh  | 33 +++++++++++++++++++++++++++++++
 t/t7407-submodule-foreach.sh | 47 ++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 84 insertions(+), 5 deletions(-)
-- 
2.8.0.rc4.23.gd22361a.dirty
Stefan Beller· Mar 28, 2016, 23:28 UTC · re: Stefan Beller · lore

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

Document this as failure in this patch, to be fixed in a later patch.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7407-submodule-foreach.sh | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
Show changes to t/t7407-submodule-foreach.sh +20 −0
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 7ca10b8..f868636 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_failure '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.rc4.23.gd22361a.dirty
Stefan Beller· Mar 28, 2016, 23:28 UTC · re: Stefan Beller · lore

[PATCH 2/7] submodule foreach: correct path computation in recursive submodules

The test which is fixed by this patch 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 | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)
Show changes to 2 files +3 −2

git-submodule.sh, t/t7407-submodule-foreach.sh

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 f868636..776b349 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -187,7 +187,7 @@ Entering '../sub2'
 Entering '../sub3'
 EOF
 
-test_expect_failure 'test messages from "foreach --recursive" from subdirectory' '
+test_expect_success 'test messages from "foreach --recursive" from subdirectory' '
 	(
 		cd clone2 &&
 		mkdir untracked &&

-- 
2.8.0.rc4.23.gd22361a.dirty
Junio C Hamano· Mar 29, 2016, 05:44 UTC · re: Stefan Beller · lore

Re: [PATCH 2/7] submodule foreach: correct path computation in recursive submodules

Stefan Beller <sbeller@google.com> writes:
Show 13 quoted lines
> The test which is fixed by this patch 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>
> ---
Nicely explained and executed.

FWIW, it is fine to have a fix and new tests to demonstrate the fix to pre-existing breakages in a single step. It is easier to review when we can see the body of the test (as opposed to just the change s/expect_failure/expect_success/) in the same patch as the change to the code for a focused and small fix like these patches 1 & 2; it is easy to partially revert the patch for such a focused and small fix when a reviewer wants to verify that the new tests fail without the code change.

Show 14 quoted lines
>  git-submodule.sh             | 3 ++-
>  t/t7407-submodule-foreach.sh | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> 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)/"

Make sure that "$prefix$sm_path" is given as a single string to relative_path. I'd probably write this like so:

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

Thanks.
Junio C Hamano· Mar 29, 2016, 19:00 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/7] submodule foreach: correct path computation in recursive submodules

Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> The test which is fixed by this patch 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>
>> ---
>
> Nicely explained and executed.

Interestingly, this breakage, as 1/7 shows, only affects the "Entering $there" message--I somehow expected from reading the description above that the command given to "foreach" is run in a wrong submodule directory, but there is no such bug that is fixed by this change, as far as "foreach" is concerned.

Which might be an indication that it wasn't so "Nicely explained" after all X-<. Are there codepaths that use the same incorrect information (which was fixed by this patch) for things other than the message and chdir to an incorrect place? Then this change is fixing more than "just a bug in foreach message".

The explanation does not make it clear what the extent of the fix is, in other words.

Nevertheless, it is a good fix for "foreach message". Thanks. It just needs to clarify if that is the only change, or if it fixes other things.

Stefan Beller· Mar 29, 2016, 19:21 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/7] submodule foreach: correct path computation in recursive submodules

On Tue, Mar 29, 2016 at 12:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> The test which is fixed by this patch 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>
>>> ---
>>
>> Nicely explained and executed.
>
> Interestingly, this breakage, as 1/7 shows, only affects the
> "Entering $there" message--I somehow expected from reading the
> description above that the command given to "foreach" is run
> in a wrong submodule directory, but there is no such bug that
> is fixed by this change, as far as "foreach" is concerned.
foreach is a special beast as it is the only submodule command that
ignores the current directory, i.e.
    cd repo/plugins && git submodule foreach ...
also affects submodules in repo/other-submodules.

So yeah this is only about the displaypath being relative to the users cwd, internally we operate from the toplevel directory. :/

Show 6 quoted lines
>
> Which might be an indication that it wasn't so "Nicely explained"
> after all X-<.  Are there codepaths that use the same incorrect
> information (which was fixed by this patch) for things other than
> the message and chdir to an incorrect place?  Then this change is
> fixing more than "just a bug in foreach message".

Each submodule command does its own chdir and computation of the prefix and displaypath. The common thing is the name and the meaning of the variables. Currently I am redoing the other patches to follow this patch. Apparently I still need to hone the commit message to actually transport the message that this only affects the display path.

Show 8 quoted lines
>
> The explanation does not make it clear what the extent of the fix
> is, in other words.
>
> Nevertheless, it is a good fix for "foreach message".  Thanks.  It
> just needs to clarify if that is the only change, or if it fixes
> other things.
>

Thanks, Stefan

Stefan Beller· Mar 29, 2016, 19:26 UTC · re: Stefan Beller · lore

Re: [PATCH 2/7] submodule foreach: correct path computation in recursive submodules

On Tue, Mar 29, 2016 at 12:21 PM, Stefan Beller <sbeller@google.com> wrote:
Show 31 quoted lines
> On Tue, Mar 29, 2016 at 12:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Stefan Beller <sbeller@google.com> writes:
>>>
>>>> The test which is fixed by this patch 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>
>>>> ---
>>>
>>> Nicely explained and executed.
>>
>> Interestingly, this breakage, as 1/7 shows, only affects the
>> "Entering $there" message--I somehow expected from reading the
>> description above that the command given to "foreach" is run
>> in a wrong submodule directory, but there is no such bug that
>> is fixed by this change, as far as "foreach" is concerned.
>
> foreach is a special beast as it is the only submodule command that
> ignores the current directory, i.e.
>     cd repo/plugins && git submodule foreach ...
> also affects submodules in repo/other-submodules.
I missspoke.
It actually respects the sub directory, but no further path spec. :(
Stefan Beller· Mar 28, 2016, 23:28 UTC · re: Stefan Beller · lore

[PATCH 3/7] submodule update --init: test path handling in recursive submodules

This demonstrates a failure to handle paths correctly.
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.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7406-submodule-update.sh | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)
Show changes to t/t7406-submodule-update.sh +33 −0
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 68ea31d..c1b9ffa 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_failure '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.rc4.23.gd22361a.dirty
Junio C Hamano· Mar 29, 2016, 05:48 UTC · re: Stefan Beller · lore

Re: [PATCH 3/7] submodule update --init: test path handling in recursive submodules

Stefan Beller <sbeller@google.com> writes:
Show 5 quoted lines
> @@ -95,6 +99,35 @@ test_expect_success 'submodule update from subdirectory' '
>  	)
>  '
>  
> +supersha1=$(cd super && git rev-parse HEAD)
"supersha1=$(git -C super rev-parse HEAD)" perhaps?
Show 11 quoted lines
> +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
Why all these {curly braces}?
Stefan Beller· Mar 28, 2016, 23:28 UTC · re: Stefan Beller · lore

[PATCH 4/7] submodule update --init: correct path handling in recursive submodules

This fixes the test introduced by the parent commit.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh            | 5 +++--
 t/t7406-submodule-update.sh | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)
Show changes to 2 files +4 −3

git-submodule.sh, t/t7406-submodule-update.sh

diff --git a/git-submodule.sh b/git-submodule.sh
index 2838069..a7c8599 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")"
@@ -826,8 +826,9 @@ Maybe you want to use 'update --init'?")"
 		if test -n "$recursive"
 		then
 			(
-				prefix="$prefix$sm_path/"
+				prefix="$(relative_path $prefix$sm_path)/"
 				clear_local_git_env
+				wt_prefix=
 				cd "$sm_path" &&
 				eval cmd_update
 			)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index c1b9ffa..3bd1552 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -118,7 +118,7 @@ Submodule path '../super/rebasing': checked out '${rebasingsha1}'
 Submodule path '../super/submodule': checked out '${submodulesha1}'
 EOF
 
-test_expect_failure 'submodule update --init --recursive from subdirectory' '
+test_expect_success 'submodule update --init --recursive from subdirectory' '
 	git -C recursivesuper/super reset --hard HEAD^ &&
 	(cd recursivesuper &&
 	 mkdir tmp &&

-- 
2.8.0.rc4.23.gd22361a.dirty
Junio C Hamano· Mar 29, 2016, 05:50 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
> This fixes the test introduced by the parent commit.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>

Unlike 2/7, this is kinda on the thin side in the explanation department, it looks.

Show 24 quoted lines
> ---
>  git-submodule.sh            | 5 +++--
>  t/t7406-submodule-update.sh | 2 +-
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 2838069..a7c8599 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")"
> @@ -826,8 +826,9 @@ Maybe you want to use 'update --init'?")"
>  		if test -n "$recursive"
>  		then
>  			(
> -				prefix="$prefix$sm_path/"
> +				prefix="$(relative_path $prefix$sm_path)/"
Same here as 2/7 (see the above hunk which does this correctly).
Show 5 quoted lines
>  				clear_local_git_env
> +				wt_prefix=
>  				cd "$sm_path" &&
>  				eval cmd_update
>  			)
Junio C Hamano· Mar 29, 2016, 19:46 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
> This fixes the test introduced by the parent commit.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

The first hunk in this patch touches lines that goes away with d5bc3cd2 (submodule: port init from shell to C, 2016-03-14) on your sb/submodule-init topic and the whole block is replaced by a call to "submodule--helper init".

I'll drop the first hunk when merging this series to 'pu' for now; hopefully you did not inherit the bug when rewriting the part into "submodule--helper init".

Thanks.
Show 41 quoted lines
>  git-submodule.sh            | 5 +++--
>  t/t7406-submodule-update.sh | 2 +-
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 2838069..a7c8599 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")"
> @@ -826,8 +826,9 @@ Maybe you want to use 'update --init'?")"
>  		if test -n "$recursive"
>  		then
>  			(
> -				prefix="$prefix$sm_path/"
> +				prefix="$(relative_path $prefix$sm_path)/"
>  				clear_local_git_env
> +				wt_prefix=
>  				cd "$sm_path" &&
>  				eval cmd_update
>  			)
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index c1b9ffa..3bd1552 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -118,7 +118,7 @@ Submodule path '../super/rebasing': checked out '${rebasingsha1}'
>  Submodule path '../super/submodule': checked out '${submodulesha1}'
>  EOF
>  
> -test_expect_failure 'submodule update --init --recursive from subdirectory' '
> +test_expect_success 'submodule update --init --recursive from subdirectory' '
>  	git -C recursivesuper/super reset --hard HEAD^ &&
>  	(cd recursivesuper &&
>  	 mkdir tmp &&
Stefan Beller· Mar 29, 2016, 19:49 UTC · re: Junio C Hamano · lore

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

On Tue, Mar 29, 2016 at 12:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> This fixes the test introduced by the parent commit.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>
> The first hunk in this patch touches lines that goes away with
> d5bc3cd2 (submodule: port init from shell to C, 2016-03-14) on
> your sb/submodule-init topic and the whole block is replaced
> by a call to "submodule--helper init".
>
> I'll drop the first hunk when merging this series to 'pu' for now;
> hopefully you did not inherit the bug when rewriting the part into
> "submodule--helper init".

After examining this patch more closely for a better commit message, the second hunk also goes away, i.e. only the test will remain.

Maybe I can roll this series on top of origin/sb/submodule-init, to avoid confusion.

Show 44 quoted lines
>
> Thanks.
>
>>  git-submodule.sh            | 5 +++--
>>  t/t7406-submodule-update.sh | 2 +-
>>  2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 2838069..a7c8599 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")"
>> @@ -826,8 +826,9 @@ Maybe you want to use 'update --init'?")"
>>               if test -n "$recursive"
>>               then
>>                       (
>> -                             prefix="$prefix$sm_path/"
>> +                             prefix="$(relative_path $prefix$sm_path)/"
>>                               clear_local_git_env
>> +                             wt_prefix=
>>                               cd "$sm_path" &&
>>                               eval cmd_update
>>                       )
>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
>> index c1b9ffa..3bd1552 100755
>> --- a/t/t7406-submodule-update.sh
>> +++ b/t/t7406-submodule-update.sh
>> @@ -118,7 +118,7 @@ Submodule path '../super/rebasing': checked out '${rebasingsha1}'
>>  Submodule path '../super/submodule': checked out '${submodulesha1}'
>>  EOF
>>
>> -test_expect_failure 'submodule update --init --recursive from subdirectory' '
>> +test_expect_success 'submodule update --init --recursive from subdirectory' '
>>       git -C recursivesuper/super reset --hard HEAD^ &&
>>       (cd recursivesuper &&
>>        mkdir tmp &&
Stefan Beller· Mar 28, 2016, 23:28 UTC · re: Stefan Beller · lore

[PATCH 5/7] t7407: make expectation as clear as possible

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(-)
Show changes to t/t7407-submodule-foreach.sh +6 −2
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 776b349..808c6c6 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.rc4.23.gd22361a.dirty
Junio C Hamano· Mar 29, 2016, 19:30 UTC · re: Stefan Beller · lore

Re: [PATCH 5/7] t7407: make expectation as clear as possible

Stefan Beller <sbeller@google.com> writes:
Show 5 quoted lines
> 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>
> ---

I agree that the overlong sed expression is not very readable. Spelling the expectation out like this patch does would make sense. A slight possible downside is that future changes in the test setup may require updating two places now instead of one, but I would say that would not make a very strong objection--after all, such future changes may affect the line that is munged by the sed script, in which case you'd need to change two places anyway (i.e. the previous "expect" and also the script), and updating two explicit expectation would be far easier than updating one explicit expectation and the overlong sed expression.

Perhaps this wants to come much earlier in the series? It felt a bit distracting while reading the series in sequence, derailing my train of thought.

Thanks.
Show 22 quoted lines
>  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 776b349..808c6c6 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' '
>  	(
Stefan Beller· Mar 28, 2016, 23:28 UTC · re: Stefan Beller · lore

[PATCH 6/7] submodule status: test path handling in recursive submodules

This replicates the previous test except that it executes from a sub directory. Document the expected outcome, which currently fails by having too many '../' prefixed.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7407-submodule-foreach.sh | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
Show changes to t/t7407-submodule-foreach.sh +19 −0
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 808c6c6..5c57151 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -263,6 +263,25 @@ test_expect_success 'test "status --recursive"' '
 '
 
 cat > expect <<EOF
+ $nested1sha1 ../nested1 (heads/master)
+ $nested2sha1 ../nested1/nested2 (heads/master)
+ $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_failure 'test "status --recursive" from sub directory' '
+	(
+		cd clone3 &&
+		mkdir tmp && cd tmp &&
+		git submodule status --recursive > ../../actual
+	) &&
+	test_cmp expect actual
+'
+
+cat > expect <<EOF
  $nested1sha1 nested1 (heads/master)
 +$nested2sha1 nested1/nested2 (file2~1)
  $nested3sha1 nested1/nested2/nested3 (heads/master)

-- 
2.8.0.rc4.23.gd22361a.dirty
Stefan Beller· Mar 28, 2016, 23:28 UTC · re: Stefan Beller · lore

[PATCH 7/7] submodule status: fix path handling in recursive submodules

This fixes the test which was introduced in the parent commit.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh             | 1 +
 t/t7407-submodule-foreach.sh | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)
Show changes to 2 files +2 −1

git-submodule.sh, t/t7407-submodule-foreach.sh

diff --git a/git-submodule.sh b/git-submodule.sh
index a7c8599..7503b27 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1161,6 +1161,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 5c57151..91f9ca9 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -272,7 +272,7 @@ cat > expect <<EOF
  $sub3sha1 ../sub3 (heads/master)
 EOF
 
-test_expect_failure 'test "status --recursive" from sub directory' '
+test_expect_success 'test "status --recursive" from sub directory' '
 	(
 		cd clone3 &&
 		mkdir tmp && cd tmp &&

-- 
2.8.0.rc4.23.gd22361a.dirty

← back to recent threads