| Message ID | 20190301173443.16429-3-tmz@pobox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] doc: note config file restrictions for completion.commands | expand |
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. 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 '
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. > 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'. Furthermore, I think it would be prudent to check that 'mergetool' is not listed, either.
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!
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 3a2c6326d8..dd11bb660d 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -1483,6 +1483,14 @@ test_expect_success 'git --help completion' ' test_completion "git --help core" "core-tutorial " ' +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 +' + test_expect_success 'setup for integration tests' ' echo content >file1 && echo more >file2 &&
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> --- t/t9902-completion.sh | 8 ++++++++ 1 file changed, 8 insertions(+)