Re: [PATCH 4/4] submodule: add more tests for recursive submodule behavior

From: Stefan Beller <sbeller@google.com>
Date: Mar 25, 2016, 00:33 UTC
Message-ID: <CAGZ79ka9eRbqCEhE3FXpg+i6XJ03azaHYzUeJnOZGHZf_dJH=Q@mail.gmail.com>
In-Reply-To: <CAPig+cS8NSDb=apvtLHQL-Ph=RB0CTashhZMnQ3DkcLfnzB9ww@mail.gmail.com>

On Thu, Mar 24, 2016 at 5:25 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 25 quoted lines

> On Thu, Mar 24, 2016 at 7:34 PM, Stefan Beller <sbeller@google.com> wrote:
>> This adds a test for "submodule update", wich calls "submodule update"
>
> s/wich/which/
>
>> from an untracked repository in the superproject. When doing creating
>
> Grammo: "doing creating"
>
>> the parent patch a similar test failed for "submodule sync", but
>> all tests passed for "submodule update". It took me a long time
>> to figure out this was a difference in test coverage instead of
>> commands behaving differently. Let's improve the test coverage such
>> to make it a better place.
>>
>> When trying to fix the issue in the parent patch I could get
>> the test suite passing when removing the $@ argument from module_list
>> in the sync command. This also indicates a low test coverage, so
>> fix that.
>
> These two paragraphs are almost entirely commentary, thus probably
> belong below the "---" line. I'm having a difficult time trying to
> decipher from this commit message what this patch is actually about.
> Perhaps the commit message could do a better job explaining exactly
> what shortcoming(s) it's addressing.

The tests on submodule commands executed not from the top level are very sparse. I had a hard time to developing patches 1 and 2. And I felt

    "This patch adds more test coverage."

is not a sufficient commit message.

The current tests have found issues, which lead to fixing them in patch 1,2, but I think only by accident, as one command (sync) was testing from the non root. By having more test coverage it is easier to have a guess what is wrong with the code.

Any hint on how to write that into a commit message without being commentatory would be great!

Show 14 quoted lines

>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
>> @@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops module name before recur
>> +test_expect_success 'submodule update --recursive works from subdirectory' '
>> +       (cd super2 &&
>> +        (cd deeper/submodule/subsubmodule &&
>> +         git checkout HEAD^
>> +        ) &&
>
> Maybe use -C and drop the sub-subshell:
>
>     git -C deeper/submodule/subsubmodule checkout HEAD^

ok

Show 7 quoted lines

>
>> +        mkdir untracked &&
>> +        cd untracked &&
>> +        git submodule update --recursive >actual &&
>> +        test_i18ngrep "Submodule path .../deeper/submodule/subsubmodule.: checked out" actual
>> +       )
>> +'