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

[Todd Zullinger <tmz@pobox.com>]

SZEDER Gábor wrote:
> 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.

>> 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