Re: [PATCH 03/15] lib-submodule-update.sh: define tests for recursing into submodules

From: Stefan Beller <sbeller@google.com>
Date: Feb 22, 2017, 18:43 UTC
Message-ID: <CAGZ79kYhT4ikj2pfDJM_PQsh5XzGTVT2kByiE1moK8RUcusLcA@mail.gmail.com>
In-Reply-To: <xmqqlgt5x430.fsf@gitster.mtv.corp.google.com>

On Thu, Feb 16, 2017 at 12:39 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> Currently lib-submodule-update.sh provides 2 functions
>> test_submodule_switch and test_submodule_forced_switch that are used by a
>> variety of tests to ensure that submodules behave as expected. The current
>> expected behavior is that submodules are not touched at all (see
>> 42639d2317a for the exact setup).
>>
>> In the future we want to teach all these commands to properly recurse
>> into submodules. To do that, we'll add two testing functions to
>> submodule-update-lib.sh test_submodule_switch_recursing and
>> test_submodule_forced_switch_recursing.
>
> I'd remove "properly" and insert a colon after "add two ... to
> submodule-update-lib.sh" before the names of two functions.

ok

Show 11 quoted lines

>
>> +reset_work_tree_to_interested () {
>> +     reset_work_tree_to $1 &&
>> +     # indicate we are interested in the submodule:
>> +     git -C submodule_update config submodule.sub1.url "bogus" &&
>> +     # also have it available:
>> +     if ! test -d submodule_update/.git/modules/sub1
>> +     then
>> +             mkdir submodule_update/.git/modules &&
>
> Would we want "mkdir -p" here to be safe?

Yes I cannot think of a downside of being overly cautious here.

Show 6 quoted lines

>
>> +             cp -r submodule_update_repo/.git/modules/sub1 submodule_update/.git/modules/sub1
>
> ... ahh, wouldn't matter that much, we checked that modules/sub1
> does not exist, and as long as nobody creates modules/ or modules/somethingelse
> we are OK.

Well, I'll add the -p

Show 17 quoted lines

>> +# Test that submodule contents are correctly updated when switching
>> +# between commits that change a submodule.
>> +# Test that the following transitions are correctly handled:
>> +# (These tests are also above in the case where we expect no change
>> +#  in the submodule)
>> +# - Updated submodule
>> +# - New submodule
>> +# - Removed submodule
>> +# - Directory containing tracked files replaced by submodule
>> +# - Submodule replaced by tracked files in directory
>> +# - Submodule replaced by tracked file with the same name
>> +# - tracked file replaced by submodule
>
> These should work without trouble only when the paths involved in
> the operation in the working tree are clean, right?  Just double
> checking.  If they are dirty we should expect a failure, instead of
> silent loss of information.

yes, I'll go over the tests again and add those cases if missing.

Show 13 quoted lines

>> +     command="$1"
>
> The dq-pair is not strictly needed on the RHS of the assignment, but
> it is a good way to signal that we considered that we might receive
> an argument with $IFS in it...
>
>> +                     $command add_sub1 &&
>
> ... and after doing so, not quoting $command here signals that we
> expect command line splitting to happen.  Am I reading it correctly?
> Without an actual caller appearing in this step, it is rather hard
> to judge.
>

I followed the existing code without thinking about these points, but they are valid and exactly how we'd expect the code to behave. $1 / $command will be e.g. "git checkout --recurse-submodules" in this patch series; but later on we could also have functions. C.f. t4137 which defines a function

    apply_3way () {
        git diff --ignore-submodules=dirty "..$1" | git apply --3way -
    }
    test_submodule_switch "apply_3way"

We'd want to have a similar thing for the recursing part, e.g.

    apply_3way_recursing () {
        git diff --submodule=diff "..$1" | git apply
--recurse-submodules --3way -
    }
    test_submodule_switch_recursing "apply_3way_recursing"

>> +                     echo sub1 > .git/info/exclude
>
>     ">.git/info/exclude"

ok