Re: [PATCHv2 2/6] submodule test invocation: only pass additional arguments

From: Stefan Beller <sbeller@google.com>
Date: May 23, 2017, 18:29 UTC
Message-ID: <CAGZ79kb2iu1D1hRbGNx9aP_ebCyXzrCZQorbop7BG11vSUNzoA@mail.gmail.com>
In-Reply-To: <xmqqefvgks0g.fsf@gitster.mtv.corp.google.com>

On Mon, May 22, 2017 at 11:26 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 19 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
>> index e8f70b806f..2672f104cf 100755
>> --- a/t/t2013-checkout-submodule.sh
>> +++ b/t/t2013-checkout-submodule.sh
>> @@ -65,9 +65,9 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .git/
>>
>>  KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1
>>  KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1
>> -test_submodule_switch_recursing "git checkout --recurse-submodules"
>> +test_submodule_switch_recursing "checkout"
>>
>> -test_submodule_forced_switch_recursing "git checkout -f --recurse-submodules"
>> +test_submodule_forced_switch_recursing "checkout -f"
>>
>>  test_submodule_switch "git checkout"
>
> Doesn't the above look crazy to you?

Oh well. The commit message doesn't explain why the craziness is required here (really!).

    submodule test invocation: only pass additional arguments

    In a later patch we want to introduce a config option to trigger
    the submodule recursing by default. As this option should be
    available and uniform across all commands that deal with submodules
    we'd want to test for this option in the submodule update library.

    So instead of calling the whole test set again for
    "git -c submodule.recurse foo" instead of
    "git foo --recurse-submodules", we'd only want to introduce one
    basic test that tests if the option is recognized and respected
    to not overload the test suite.

Show 5 quoted lines

>
> It is hostile to other people (and those who need to make merges)
> who have to work with test_submodule_switch_recursing that older one
> used to take the full command but its definition suddenly changes so
> that the caller now must omit the leading "git".

I am not aware of other people (or other series in flight by myself) that use one of the switches currently.

>  Even worse,
> another helper with a similar-sounding name, test_submodule_switch,
> still must be called with the leading "git".

Oh, yeah that is a real issue. I will migrate all of them.

>
> The same comment applies to the one we can see below.

An alternative would be to come up with a slightly different name to ensure we do not have issues with other series in flight. The function name is already pretty long, so encoding even more information in it may be not a good idea. But the argument is shorter, so maybe:

- test_submodule_switch_recursing "git reset --hard --recurse-submodules"
+ test_submodule_switch_recursing_args_only  "reset --hard"

Thanks, Stefan