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

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

[Stefan Beller]

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

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

[Stefan Beller]

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(+)

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

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

[Stefan Beller]

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(-)

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

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

[Stefan Beller]

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(+)

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

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

[Stefan Beller]

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(-)

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

[PATCH 5/7] 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 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

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

[Stefan Beller]

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(+)

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

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

[Stefan Beller]

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(-)

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

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

[Junio C Hamano]

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.

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.

>  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.

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> @@ -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?

> +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}?

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

[Junio C Hamano]

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.

> ---
>  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).

>  				clear_local_git_env
> +				wt_prefix=
>  				cd "$sm_path" &&
>  				eval cmd_update
>  			)

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

[Junio C Hamano]

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.

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.

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

[Stefan Beller]

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.

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

>
> 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.

>
> 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

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

[Stefan Beller]

On Tue, Mar 29, 2016 at 12:21 PM, Stefan Beller <sbeller@google.com> wrote:
> 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. :(

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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.

>  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' '
>  	(

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

[Junio C Hamano]

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.

>  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 &&

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

[Stefan Beller]

On Tue, Mar 29, 2016 at 12:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

>
> 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 &&