Re: [PATCH] Improve tests for detached worktree in git-submodule

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 30, 2012, 17:44 UTC
Message-ID: <7vsjc9qh33.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAHCkQtPmJdFBk7G-EP70FwtkRk11Utp+rppjNrEZFtZgOVrObA@mail.gmail.com>

Daniel Graña <dangra@gmail.com> writes:

Show 32 quoted lines

> On Mon, Jul 30, 2012 at 2:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Daniel Graña <dangra@gmail.com> writes:
>>
>>> Signed-off-by: Daniel Graña <dangra@gmail.com>
>>> ---
>>>  t/t7409-submodule-detached-worktree.sh |   31 ++++++++++++++++++++++++-------
>>>  1 files changed, 24 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/t/t7409-submodule-detached-worktree.sh b/t/t7409-submodule-detached-worktree.sh
>>> index db75642..d88f400 100755
>>> --- a/t/t7409-submodule-detached-worktree.sh
>>> +++ b/t/t7409-submodule-detached-worktree.sh
>>> @@ -15,7 +15,11 @@ TEST_NO_CREATE_REPO=1
>>>  test_expect_success 'submodule on detached working tree' '
>>>       git init --bare remote &&
>>>       test_create_repo bundle1 &&
>>> -     (cd bundle1 && test_commit "shoot") &&
>>> +     (
>>> +             cd bundle1 &&
>>> +             test_commit "shoot" &&
>>> +             git rev-list --max-count=1 HEAD > "$TRASH_DIRECTORY/expect"
>>
>> Better written as
>>
>>         git rev-parse --verify HEAD >../expect
>>
>> methinks.
>
> You rule here,
>
> is it still better than "git rev-parse --max-count=1 HEAD" seen in
> t7406-submodule.update.sh?

"git rev-parse --max-count=1 HEAD" will show "--max-count=1\n" followed by the value of HEAD, so if the expected result and the actual result were both prepared by that command, the comparison would succeed (as the irrelevant --max-count=1 line will appear in both output), but honestly, I do not think it makes any sense.

Who wrote that crap?

Show 24 quoted lines

>
>>
>>> +     ) &&
>>>       mkdir home &&
>>>       (
>>>               cd home &&
>>> @@ -23,14 +27,27 @@ test_expect_success 'submodule on detached working tree' '
>>>               git clone --bare ../remote .dotfiles &&
>>>               git submodule add ../bundle1 .vim/bundle/sogood &&
>>>               test_commit "sogood" &&
>>> +             (
>>> +                     unset GIT_WORK_TREE GIT_DIR &&
>>> +                     cd .vim/bundle/sogood &&
>>> +                     git rev-list --max-count=1 HEAD > actual &&
>>> +                     test_cmp actual "$TRASH_DIRECTORY/expect"
>>
>> Likewise.
>>
>>         git rev-parse --verify HEAD >actual &&
>>         test_cmp ../expect actual
>
> I tried to avoid the too many ".." usage, in that case it'd be:
>
>     test_cmp ../../../../expect actual

"$TRASH_DIRECTORY/expect" is fine as well. Just drop the extra SP between the redirection '>' and the filename, and make sure the filename is inside double quotes for some versions of bash that issue an unnecessary warning.

Show 7 quoted lines

>>> +             git checkout master &&
>>
>> So you populate the newly created home2 working tree out of the .otfiles
>> repository in it.
>
> right, before it wasn't creating ~/.gitmodules and "git subodule
> update --init" wasn't taking effect.

Good.

Show 7 quoted lines

>> Is the "existence" the only thing you care about?  That's not all
>> that different from the old test that only checked the existence of
>> the directory dupe, no?
>
> Except the submodule wasn't updating but the directory still existed
> so test passed, now it check for a file that exists only if the
> submodule update works.

OK.