threads / patch / 31946

patchsubmodule status: properly pass options with --recursive

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

## tl;dr

12 messages between Oct 25, 2012 and Oct 29, 2012. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Jens Lehmann· Oct 25, 2012, 22:20 UTC · lore

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(-)
Show changes to 2 files +9 −1

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

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
Jeff King· Oct 26, 2012, 13:15 UTC · re: Jens Lehmann · lore

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

On Fri, Oct 26, 2012 at 12:20:29AM +0200, Jens Lehmann wrote:
Show 8 quoted lines
> 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
Phil Hord· Oct 26, 2012, 19:07 UTC · re: Jeff King · lore

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

On Fri, Oct 26, 2012 at 9:15 AM, Jeff King <peff@peff.net> wrote:
Show 14 quoted lines
> 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 --
Show changes to diff +2 −3
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
Phil Hord· Oct 26, 2012, 19:13 UTC · re: Phil Hord · lore

[PATCH] t7407: Fix recursive submodule test

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(-)
Show changes to t/t7407-submodule-foreach.sh +2 −2
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
Jens Lehmann· Oct 26, 2012, 19:29 UTC · re: Phil Hord · lore

Re: [PATCH] t7407: Fix recursive submodule test

Am 26.10.2012 21:13, schrieb Phil Hord:
Show 13 quoted lines
> 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.
Show 27 quoted lines
> 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
>
Jens Lehmann· Oct 28, 2012, 21:37 UTC · re: Jens Lehmann · lore

[PATCH] submodule status: remove unused orig_* variables

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:
Show 16 quoted lines
> 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(-)
Show changes to git-submodule.sh +1 −3
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
Jeff King· Oct 29, 2012, 07:27 UTC · re: Jens Lehmann · lore

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

On Sun, Oct 28, 2012 at 10:37:16PM +0100, Jens Lehmann wrote:
Show 14 quoted lines
> 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
Jens Lehmann· Oct 26, 2012, 19:26 UTC · re: Phil Hord · lore

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

Am 26.10.2012 21:07, schrieb Phil Hord:
Show 21 quoted lines
> 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 :-( )

Show 14 quoted lines
> 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.

Phil Hord· Oct 26, 2012, 19:44 UTC · re: Jens Lehmann · lore

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

[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.
Phil Hord· Oct 26, 2012, 19:44 UTC · re: Phil Hord · lore

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

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(-)
Show changes to git-submodule.sh +12 −2
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
Phil Hord· Oct 26, 2012, 19:44 UTC · re: Phil Hord · lore

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

Signed-off-by: Phil Hord <hordp@cisco.com>
---
 t/t7403-submodule-sync.sh | 55 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)
Show changes to t/t7403-submodule-sync.sh +53 −2
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
Jens Lehmann· Oct 28, 2012, 21:02 UTC · re: Phil Hord · lore

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

Am 26.10.2012 21:44, schrieb Phil Hord:
Show 8 quoted lines
> [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>

← back to recent threads