[PATCH] submodule status: properly pass options with --recursive

[PATCH] submodule status: properly pass options with --recursive

[Jens Lehmann]

When renaming orig_args to orig_flags in 98dbe63d (submodule: only
preserve flags across recursive status/update invocations) the call site
of the recursive cmd_status was forgotten. At that place orig_args is
still passed into the recursion, which is always empty now. This clears
all options when recursing, as that variable is never set.

Fix that by renaming orig_args to orig_flags there too and add a test to
catch that bug.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

I noticed that when reviewing Phil's "Teach --recursive to submodule
sync" patch.

 git-submodule.sh             | 2 +-
 t/t7407-submodule-foreach.sh | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index ab6b110..c089d48 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -990,7 +990,7 @@ cmd_status()
 				prefix="$displaypath/"
 				clear_local_git_env
 				cd "$sm_path" &&
-				eval cmd_status "$orig_args"
+				eval cmd_status "$orig_flags"
 			) ||
 			die "$(eval_gettext "Failed to recurse into submodule path '\$sm_path'")"
 		fi
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 9b69fe2..eca36b5 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -245,6 +245,14 @@ test_expect_success 'ensure "status --cached --recursive" preserves the --cached
 	test_cmp expect actual
 '

+test_expect_success 'ensure "status --quiet --recursive" preserves the --quiet flag' '
+	(
+		cd clone3 &&
+		git submodule status --quiet --recursive -- nested1 > ../actual
+	) &&
+	! test -s actual
+'
+
 test_expect_success 'use "git clone --recursive" to checkout all submodules' '
 	git clone --recursive super clone4 &&
 	(
-- 
1.8.0.dirty

Re: [PATCH] submodule status: properly pass options with --recursive

[Jeff King]

On Fri, Oct 26, 2012 at 12:20:29AM +0200, Jens Lehmann wrote:

> When renaming orig_args to orig_flags in 98dbe63d (submodule: only
> preserve flags across recursive status/update invocations) the call site
> of the recursive cmd_status was forgotten. At that place orig_args is
> still passed into the recursion, which is always empty now. This clears
> all options when recursing, as that variable is never set.
> 
> Fix that by renaming orig_args to orig_flags there too and add a test to
> catch that bug.

Thanks. I back-ported your patch on top of 98dbe63d so it can go to
'maint'. I'm curious, though: why didn't the test right before (which
checks recursion for --cached) catch this?

-Peff

Re: [PATCH] submodule status: properly pass options with --recursive

[Phil Hord]

On Fri, Oct 26, 2012 at 9:15 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Oct 26, 2012 at 12:20:29AM +0200, Jens Lehmann wrote:
>
>> When renaming orig_args to orig_flags in 98dbe63d (submodule: only
>> preserve flags across recursive status/update invocations) the call site
>> of the recursive cmd_status was forgotten. At that place orig_args is
>> still passed into the recursion, which is always empty now. This clears
>> all options when recursing, as that variable is never set.
>>
>> Fix that by renaming orig_args to orig_flags there too and add a test to
>> catch that bug.
>
> Thanks. I back-ported your patch on top of 98dbe63d so it can go to
> 'maint'. I'm curious, though: why didn't the test right before (which
> checks recursion for --cached) catch this?

I was wondering the same thing about why 'git submodule sync
--recursive --quiet' succeeded, so I checked on it.  The answer is
that "--quiet" sets GIT_QUIET=1, which is then inherited by the
recursive call. Indeed, Jens' new test passes even without the
accompanying fix.  :-\

The 'status --recursive --cached' test passes for two reasons.  The
first is that the test is checking for a cached change in a level-1
submodule, but it is the level-2+ submodules which do not get the
flags passed down correctly in "$@".  The 2nd reason is that the
$cached variable is _also_ inherited by the recursive call to
cmd_status, specifically because it is a call to cmd_status() and not
to '$SHELL git submodule status', where the latter would have cleared
the flags at startup, but the former does not.

I'm a bit wary about "fixing" the flags for the recursion machinery.
I'm starting to think $orig_flags is moot in almost all cases.  It
looks like clearing all the flags on each iteration would break 'git
submodule foreach --recursive --quiet' because it does not use
$orig_flags at all.  Who knows what other surprises lurk there.

Here's a fix for the test, though, even though it still does not fail
in the absence of the $orig_flags patch.

-- >8 --

diff --git i/t/t7407-submodule-foreach.sh w/t/t7407-submodule-foreach.sh
index 9b69fe2..8442b32 100755
--- i/t/t7407-submodule-foreach.sh
+++ w/t/t7407-submodule-foreach.sh
@@ -226,14 +226,14 @@ test_expect_success 'test "status --recursive"' '
        test_cmp expect actual
 '

-sed -e "/nested1 /s/.*/+$nested1sha1 nested1 (file2~1)/;/sub[1-3]/d"
< expect > expect2
+sed -e "/nested2 /s/.*/+$nested2sha1 nested1\/nested2
(file2~1)/;/sub[1-3]/d" < expect > expect2
 mv -f expect2 expect

 test_expect_success 'ensure "status --cached --recursive" preserves
the --cached flag' '
        (
                cd clone3 &&
                (
-                       cd nested1 &&
+                       cd nested1/nested2 &&
                        test_commit file2
                ) &&
                git submodule status --cached --recursive -- nested1 > ../actual
@@ -245,6 +245,14 @@ test_expect_success 'ensure "status --cached
--recursive" preserves the --cached
        test_cmp expect actual
 '
 test_expect_success 'use "git clone --recursive" to checkout all submodules' '
        git clone --recursive super clone4 &&
        (

Phil

[PATCH] t7407: Fix recursive submodule test

[Phil Hord]

A test in t7404-submodule-foreach purports to test that
the --cached flag is properly noticed by --recursive calls
to the foreach command as it descends into nested
submodules.  However, the test really does not perform this
test since the change it looks for is in a top-level
submodule handled by the first invocation of the command.
To properly test for the flag being passed to recursive
invocations, the change must be buried deeper in the
hierarchy.

Move the change one level deeper so it properly verifies
the recursive machinery of the 'git submodule status'
command.

Signed-off-by: Phil Hord <hordp@cisco.com>
---
 t/t7407-submodule-foreach.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 9b69fe2..107b4b7 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -226,14 +226,14 @@ test_expect_success 'test "status --recursive"' '
 	test_cmp expect actual
 '
 
-sed -e "/nested1 /s/.*/+$nested1sha1 nested1 (file2~1)/;/sub[1-3]/d" < expect > expect2
+sed -e "/nested2 /s/.*/+$nested2sha1 nested1\/nested2 (file2~1)/;/sub[1-3]/d" < expect > expect2
 mv -f expect2 expect
 
 test_expect_success 'ensure "status --cached --recursive" preserves the --cached flag' '
 	(
 		cd clone3 &&
 		(
-			cd nested1 &&
+			cd nested1/nested2 &&
 			test_commit file2
 		) &&
 		git submodule status --cached --recursive -- nested1 > ../actual
-- 
1.8.0.2.gd008466.dirty

Re: [PATCH] t7407: Fix recursive submodule test

[Jens Lehmann]

Am 26.10.2012 21:13, schrieb Phil Hord:
> A test in t7404-submodule-foreach purports to test that
> the --cached flag is properly noticed by --recursive calls
> to the foreach command as it descends into nested
> submodules.  However, the test really does not perform this
> test since the change it looks for is in a top-level
> submodule handled by the first invocation of the command.
> To properly test for the flag being passed to recursive
> invocations, the change must be buried deeper in the
> hierarchy.
> 
> Move the change one level deeper so it properly verifies
> the recursive machinery of the 'git submodule status'
> command.

Me thinks we should definitely do this.

> Signed-off-by: Phil Hord <hordp@cisco.com>
> ---
>  t/t7407-submodule-foreach.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
> index 9b69fe2..107b4b7 100755
> --- a/t/t7407-submodule-foreach.sh
> +++ b/t/t7407-submodule-foreach.sh
> @@ -226,14 +226,14 @@ test_expect_success 'test "status --recursive"' '
>  	test_cmp expect actual
>  '
>  
> -sed -e "/nested1 /s/.*/+$nested1sha1 nested1 (file2~1)/;/sub[1-3]/d" < expect > expect2
> +sed -e "/nested2 /s/.*/+$nested2sha1 nested1\/nested2 (file2~1)/;/sub[1-3]/d" < expect > expect2
>  mv -f expect2 expect
>  
>  test_expect_success 'ensure "status --cached --recursive" preserves the --cached flag' '
>  	(
>  		cd clone3 &&
>  		(
> -			cd nested1 &&
> +			cd nested1/nested2 &&
>  			test_commit file2
>  		) &&
>  		git submodule status --cached --recursive -- nested1 > ../actual
> 

[PATCH] submodule status: remove unused orig_* variables

[Jens Lehmann]

When renaming orig_args to orig_flags in 98dbe63d (submodule: only
preserve flags across recursive status/update invocations) the call site
of the recursive cmd_status was forgotten. At that place orig_args is
still passed into the recursion, which is always empty since then. This
did not break anything because the orig_flags logic is not needed at all
when a function from the submodule script is called with eval, as that
inherits all the variables set by the option parsing done in the first
level of the recursion.

Now that we know that orig_flags and orig_args aren't needed at all,
let's just remove them from cmd_status().

Thanks-to: Phil Hord <hordp@cisco.com>
Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

Am 26.10.2012 21:29, schrieb Jens Lehmann:
> Am 26.10.2012 21:13, schrieb Phil Hord:
>> A test in t7404-submodule-foreach purports to test that
>> the --cached flag is properly noticed by --recursive calls
>> to the foreach command as it descends into nested
>> submodules.  However, the test really does not perform this
>> test since the change it looks for is in a top-level
>> submodule handled by the first invocation of the command.
>> To properly test for the flag being passed to recursive
>> invocations, the change must be buried deeper in the
>> hierarchy.
>>
>> Move the change one level deeper so it properly verifies
>> the recursive machinery of the 'git submodule status'
>> command.
> 
> Me thinks we should definitely do this.

And I also think this patch should go on top of Phil's patch after what
we learned so far.

I'm currently checking if we can also safely remove orig_flags from
cmd_update(). At least the test suite runs fine without it ...


 git-submodule.sh | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index ab6b110..c287464 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -926,7 +926,6 @@ cmd_summary() {
 cmd_status()
 {
 	# parse $args after "submodule ... status".
-	orig_flags=
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -950,7 +949,6 @@ cmd_status()
 			break
 			;;
 		esac
-		orig_flags="$orig_flags $(git rev-parse --sq-quote "$1")"
 		shift
 	done

@@ -990,7 +988,7 @@ cmd_status()
 				prefix="$displaypath/"
 				clear_local_git_env
 				cd "$sm_path" &&
-				eval cmd_status "$orig_args"
+				eval cmd_status
 			) ||
 			die "$(eval_gettext "Failed to recurse into submodule path '\$sm_path'")"
 		fi
-- 
1.8.0.42.g2ea983b

Re: [PATCH] submodule status: remove unused orig_* variables

[Jeff King]

On Sun, Oct 28, 2012 at 10:37:16PM +0100, Jens Lehmann wrote:

> When renaming orig_args to orig_flags in 98dbe63d (submodule: only
> preserve flags across recursive status/update invocations) the call site
> of the recursive cmd_status was forgotten. At that place orig_args is
> still passed into the recursion, which is always empty since then. This
> did not break anything because the orig_flags logic is not needed at all
> when a function from the submodule script is called with eval, as that
> inherits all the variables set by the option parsing done in the first
> level of the recursion.
> 
> Now that we know that orig_flags and orig_args aren't needed at all,
> let's just remove them from cmd_status().
> 
> Thanks-to: Phil Hord <hordp@cisco.com>
> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>

I'll replace your earlier fix (which just fixes the s/args/flags/ and
adds a test) with Phil's patch, and this on top.

Thanks.

-Peff

Re: [PATCH] submodule status: properly pass options with --recursive

[Jens Lehmann]

Am 26.10.2012 21:07, schrieb Phil Hord:
> On Fri, Oct 26, 2012 at 9:15 AM, Jeff King <peff@peff.net> wrote:
>> On Fri, Oct 26, 2012 at 12:20:29AM +0200, Jens Lehmann wrote:
>>
>>> When renaming orig_args to orig_flags in 98dbe63d (submodule: only
>>> preserve flags across recursive status/update invocations) the call site
>>> of the recursive cmd_status was forgotten. At that place orig_args is
>>> still passed into the recursion, which is always empty now. This clears
>>> all options when recursing, as that variable is never set.
>>>
>>> Fix that by renaming orig_args to orig_flags there too and add a test to
>>> catch that bug.
>>
>> Thanks. I back-ported your patch on top of 98dbe63d so it can go to
>> 'maint'. I'm curious, though: why didn't the test right before (which
>> checks recursion for --cached) catch this?
> 
> I was wondering the same thing about why 'git submodule sync
> --recursive --quiet' succeeded, so I checked on it.  The answer is
> that "--quiet" sets GIT_QUIET=1, which is then inherited by the
> recursive call. Indeed, Jens' new test passes even without the
> accompanying fix.  :-\

Dang, you're right! At least that explains why nobody noticed that
so far ... (and that's what you get for skipping the "does the test
fail without your fix?" part because the fix is so obvious :-( )

> The 'status --recursive --cached' test passes for two reasons.  The
> first is that the test is checking for a cached change in a level-1
> submodule, but it is the level-2+ submodules which do not get the
> flags passed down correctly in "$@".  The 2nd reason is that the
> $cached variable is _also_ inherited by the recursive call to
> cmd_status, specifically because it is a call to cmd_status() and not
> to '$SHELL git submodule status', where the latter would have cleared
> the flags at startup, but the former does not.
> 
> I'm a bit wary about "fixing" the flags for the recursion machinery.
> I'm starting to think $orig_flags is moot in almost all cases.  It
> looks like clearing all the flags on each iteration would break 'git
> submodule foreach --recursive --quiet' because it does not use
> $orig_flags at all.  Who knows what other surprises lurk there.

I agree, it looks like ripping out the orig_flags would be the way
to go.

[PATCHv3 0/2] Teach --recursive to submodule sync

[Phil Hord]

[PATCHv3 1/2] Teach --recursive to submodule sync

Now with less noise and no redundant flags passed to the recursive call.


[PATCHv3 2/2] Add tests for submodule sync --recursive

The test remains unchanged.

[PATCHv3 1/2] Teach --recursive to submodule sync

[Phil Hord]

The submodule sync command was somehow left out when
--recursive was added to the other submodule commands.

Teach sync to handle the --recursive switch by recursing
when we're in a submodule we are sync'ing.

Change the report during sync to show submodule-path
instead of submodule-name to be consistent with the other
submodule commands and to help recursed paths make sense.

Signed-off-by: Phil Hord <hordp@cisco.com>
---
 git-submodule.sh | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index ab6b110..0ca3af2 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -11,7 +11,7 @@ USAGE="[--quiet] add [-b branch] [-f|--force] [--reference <repository>] [--] <r
    or: $dashless [--quiet] update [--init] [-N|--no-fetch] [-f|--force] [--rebase] [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
    or: $dashless [--quiet] foreach [--recursive] <command>
-   or: $dashless [--quiet] sync [--] [<path>...]"
+   or: $dashless [--quiet] sync [--recursive] [--] [<path>...]"
 OPTIONS_SPEC=
 . git-sh-setup
 . git-sh-i18n
@@ -1010,6 +1010,10 @@ cmd_sync()
 			GIT_QUIET=1
 			shift
 			;;
+		--recursive)
+			recursive=1
+			shift
+			;;
 		--)
 			shift
 			break
@@ -1051,7 +1055,7 @@ cmd_sync()
 
 		if git config "submodule.$name.url" >/dev/null 2>/dev/null
 		then
-			say "$(eval_gettext "Synchronizing submodule url for '\$name'")"
+			say "$(eval_gettext "Synchronizing submodule url for '\$prefix\$sm_path'")"
 			git config submodule."$name".url "$super_config_url"
 
 			if test -e "$sm_path"/.git
@@ -1061,6 +1065,12 @@ cmd_sync()
 				cd "$sm_path"
 				remote=$(get_default_remote)
 				git config remote."$remote".url "$sub_origin_url"
+
+				if test -n "$recursive"
+				then
+					prefix="$prefix$sm_path/"
+					eval cmd_sync
+				fi
 			)
 			fi
 		fi
-- 
1.8.0.3.g9dae067

[PATCHv3 2/2] Add tests for submodule sync --recursive

[Phil Hord]

Signed-off-by: Phil Hord <hordp@cisco.com>
---
 t/t7403-submodule-sync.sh | 55 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 524d5c1..94e26c4 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -17,18 +17,25 @@ test_expect_success setup '
 	git commit -m upstream &&
 	git clone . super &&
 	git clone super submodule &&
+	(cd submodule &&
+	 git submodule add ../submodule sub-submodule &&
+	 test_tick &&
+	 git commit -m "sub-submodule"
+	) &&
 	(cd super &&
 	 git submodule add ../submodule submodule &&
 	 test_tick &&
 	 git commit -m "submodule"
 	) &&
 	git clone super super-clone &&
-	(cd super-clone && git submodule update --init) &&
+	(cd super-clone && git submodule update --init --recursive) &&
 	git clone super empty-clone &&
 	(cd empty-clone && git submodule init) &&
 	git clone super top-only-clone &&
 	git clone super relative-clone &&
-	(cd relative-clone && git submodule update --init)
+	(cd relative-clone && git submodule update --init --recursive) &&
+	git clone super recursive-clone &&
+	(cd recursive-clone && git submodule update --init --recursive)
 '
 
 test_expect_success 'change submodule' '
@@ -46,6 +53,11 @@ test_expect_success 'change submodule url' '
 	 git pull
 	) &&
 	mv submodule moved-submodule &&
+	(cd moved-submodule &&
+	 git config -f .gitmodules submodule.sub-submodule.url ../moved-submodule &&
+	 test_tick &&
+	 git commit -a -m moved-sub-submodule
+	) &&
 	(cd super &&
 	 git config -f .gitmodules submodule.submodule.url ../moved-submodule &&
 	 test_tick &&
@@ -61,6 +73,9 @@ test_expect_success '"git submodule sync" should update submodule URLs' '
 	test -d "$(cd super-clone/submodule &&
 	 git config remote.origin.url
 	)" &&
+	test ! -d "$(cd super-clone/submodule/sub-submodule &&
+	 git config remote.origin.url
+	)" &&
 	(cd super-clone/submodule &&
 	 git checkout master &&
 	 git pull
@@ -70,6 +85,25 @@ test_expect_success '"git submodule sync" should update submodule URLs' '
 	)
 '
 
+test_expect_success '"git submodule sync --recursive" should update all submodule URLs' '
+	(cd super-clone &&
+	 (cd submodule &&
+	  git pull --no-recurse-submodules
+	 ) &&
+	 git submodule sync --recursive
+	) &&
+	test -d "$(cd super-clone/submodule &&
+	 git config remote.origin.url
+	)" &&
+	test -d "$(cd super-clone/submodule/sub-submodule &&
+	 git config remote.origin.url
+	)" &&
+	(cd super-clone/submodule/sub-submodule &&
+	 git checkout master &&
+	 git pull
+	)
+'
+
 test_expect_success '"git submodule sync" should update known submodule URLs' '
 	(cd empty-clone &&
 	 git pull &&
@@ -107,6 +141,23 @@ test_expect_success '"git submodule sync" handles origin URL of the form foo/bar
 	 #actual foo/submodule
 	 test "$(git config remote.origin.url)" = "../foo/submodule"
 	)
+	(cd submodule/sub-submodule &&
+	 test "$(git config remote.origin.url)" != "../../foo/submodule"
+	)
+	)
+'
+
+test_expect_success '"git submodule sync --recursive" propagates changes in origin' '
+	(cd recursive-clone &&
+	 git remote set-url origin foo/bar &&
+	 git submodule sync --recursive &&
+	(cd submodule &&
+	 #actual foo/submodule
+	 test "$(git config remote.origin.url)" = "../foo/submodule"
+	)
+	(cd submodule/sub-submodule &&
+	 test "$(git config remote.origin.url)" = "../../foo/submodule"
+	)
 	)
 '
 
-- 
1.8.0.3.g9dae067

Re: [PATCHv3 0/2] Teach --recursive to submodule sync

[Jens Lehmann]

Am 26.10.2012 21:44, schrieb Phil Hord:
> [PATCHv3 1/2] Teach --recursive to submodule sync
> 
> Now with less noise and no redundant flags passed to the recursive call.
> 
> 
> [PATCHv3 2/2] Add tests for submodule sync --recursive
> 
> The test remains unchanged.

Both are looking good.

Acked-By: Jens Lehmann <Jens.Lehmann@web.de>