Re: [PATCH 2/3] t9902: test multiple removals via completion.commands

From: Todd Zullinger <tmz@pobox.com>
Date: Mar 1, 2019, 21:56 UTC
Message-ID: <20190301215624.GC31362@zaya.teonanacatl.net>
In-Reply-To: <20190301205041.GM19739@szeder.dev>

SZEDER Gábor wrote:

Show 25 quoted lines

> On Fri, Mar 01, 2019 at 01:22:52PM -0500, Eric Sunshine wrote:
>> On Fri, Mar 1, 2019 at 12:35 PM Todd Zullinger <tmz@pobox.com> wrote:
>>> 6532f3740b ("completion: allow to customize the completable command
>>> list", 2018-05-20) added the completion.commands config variable.
>>> Multiple commands may be added or removed, separated by a space.
>>>
>>> Demonstrate the failure of multiple removals.
>>>
>>> Signed-off-by: Todd Zullinger <tmz@pobox.com>
>>> ---
>>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>>> @@ -1483,6 +1483,14 @@ test_expect_success 'git --help completion' '
>>> +test_expect_failure 'completion.commands removes multiple commands' '
>>> +       echo cherry-pick >expected &&
>>> +       test_config_global completion.commands "-cherry -mergetool" &&
>>> +       git --list-cmds=list-mainporcelain,list-complete,config |
>>> +               grep ^cherry >actual &&
>>> +       test_cmp expected actual
>>> +'
>> 
>> We normally avoid placing a Git command upstream of a pipe. Instead,
>> the Git command output would be redirected to a file and then the file
>> grep'd.
> 
> Indeed.

Yes, that's a good point. And one I should have known from reading it here more than once. Thanks to both of you.

Show 10 quoted lines

>> However, in this case, you might consider simplifying the test
>> like this:
>> 
>>     test_expect_failure 'completion.commands removes multiple commands' '
>>         test_config_global completion.commands "-cherry -mergetool" &&
>>         git --list-cmds=list-mainporcelain,list-complete,config >actual &&
>>         grep cherry-pick actual
> 
> This wouldn't check what we want.  The point is that the command
> 'cherry' is not listed, so it should be '! grep cherry$ actual'.

Heh, I had written a similar reply already, but then I got side-tracked for a bit...

    I think the grep needs to be negated, e.g.:

        ! grep ^cherry$ actual

    Otherwise it succeeds without the fix, as cherry-pick is
    expected to be in the --list-cmds output.  (If we remove
    the 'expected' file, it might also make sense to rename
    'actual' to 'out' perhaps.)

> Furthermore, I think it would be prudent to check that 'mergetool' is
> not listed, either.

True. With the simplified test, that's easy enough to add and makes the test description more accurate and the test more thorough. I'll adjust it like so when I re-send:

    test_expect_failure 'completion.commands removes multiple commands' '
    	test_config completion.commands "-cherry -mergetool" &&
    	git --list-cmds=list-mainporcelain,list-complete,config >out &&
    	! grep -E "^(cherry|mergetool)$" out

(Using test_config depends on setup_git_directory_gently() in list_cmds(), which Jeff suggested elsewhere in the thread.)

Thanks!

-- 
Todd