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

From: Phil Hord <phil.hord@gmail.com>
Date: Oct 26, 2012, 19:07 UTC
Message-ID: <CABURp0op2+QUvusUmAFUxT8s8c02bB9V3=ag9gTTSiiN4t96OA@mail.gmail.com>
In-Reply-To: <20121026131507.GA2747@sigill.intra.peff.net>

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

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