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

[Phil Hord <phil.hord@gmail.com>]

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