[PATCH v2 0/4] completion.commands: fix multiple command removals

From: Todd Zullinger <tmz@pobox.com>
Date: Mar 17, 2019, 18:16 UTC
Message-ID: <20190317181620.26727-1-tmz@pobox.com>
In-Reply-To: <CACsJy8BuR=syjT1gjTxXXKKaevzpbdRGp+je+rsX6jV96F3sbA@mail.gmail.com>

Hi,

Duy Nguyen wrote:

> Poking this thread before it goes completely dead...

I've been meaning to send out a re-rolled version. I didn't realize 2 weeks had slipped by already. :/

Show 9 quoted lines

> On Sat, Mar 2, 2019 at 12:34 AM Todd Zullinger <tmz@pobox.com> wrote:
>>
>> The completion.commands config var must be set in either the system-wide
>> or global config file.  The completion script does not read a local
>> repository config.
> 
> After the last discussion, I think the consensus was it's ok to allow
> per-repo config so we can just drop this and update the code to read
> from any config file, right?

Yeah. Here's an updated series which I hope sums up the changes from the discussion.

Changes since v1:

    - Change --list-commands to respect local config and use
      test_config rather than test_config_global
    - Avoid git upstream of pipes
    - Check that both cherry and mergetool are removed
    - Use __git in completion calls which use --list-cmds, to
      ensure the proper git repo config is checked with git -C
      /some/other/repo

Jeff King (2):
  git: read local config in --list-cmds
  completion: fix multiple command removals

Todd Zullinger (2):
  t9902: test multiple removals via completion.commands
  completion: use __git when calling --list-cmds

 contrib/completion/git-completion.bash | 8 ++++----
 git.c                                  | 7 +++++++
 help.c                                 | 4 ++--
 t/t9902-completion.sh                  | 6 ++++++
 4 files changed, 19 insertions(+), 6 deletions(-)

Range-diff against v1:
1:  385c596510 < -:  ---------- doc: note config file restrictions for completion.commands
-:  ---------- > 1:  e51bdea6d3 git: read local config in --list-cmds
2:  ffa5ed9780 ! 2:  2f5e9da9de t9902: test multiple removals via completion.commands
    @@ -18,11 +18,9 @@
      '
      
     +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_config completion.commands "-cherry -mergetool" &&
    ++	git --list-cmds=list-mainporcelain,list-complete,config >out &&
    ++	! grep -E "^(cherry|mergetool)$" out
     +'
     +
      test_expect_success 'setup for integration tests' '
3:  af029e908d ! 3:  7548dcc23f completion: fix multiple command removals
    @@ -2,16 +2,16 @@
     
         completion: fix multiple command removals
     
    -    6532f3740b ("completion: allow to customize the completable
    -    command list", 2018-05-20) added the completion.commands config
    -    variable.
    +    Commit 6532f3740b ("completion: allow to customize the completable
    +    command list", 2018-05-20) tried to allow multiple space-separated
    +    entries in completion.commands. To do this, it copies each parsed token
    +    into a strbuf so that the result is NUL-terminated.
     
    -    The documentation states multiple commands may be added,
    -    separated by spaces.  Adding multiple commands to remove fails,
    -    only removing the last command in the config.
    -
    -    Fix multiple command removals.
    +    However, for tokens starting with "-", it accidentally passes the
    +    original non-terminated string, meaning that only the final one worked.
    +    Switch to using the strbuf.
     
    +    Reported-by: Todd Zullinger <tmz@pobox.com>
         Signed-off-by: Jeff King <peff@peff.net>
     
      diff --git a/help.c b/help.c
    @@ -38,6 +38,6 @@
      
     -test_expect_failure 'completion.commands removes multiple commands' '
     +test_expect_success 'completion.commands removes multiple commands' '
    - 	echo cherry-pick >expected &&
    - 	test_config_global completion.commands "-cherry -mergetool" &&
    - 	git --list-cmds=list-mainporcelain,list-complete,config |
    + 	test_config completion.commands "-cherry -mergetool" &&
    + 	git --list-cmds=list-mainporcelain,list-complete,config >out &&
    + 	! grep -E "^(cherry|mergetool)$" out
-:  ---------- > 4:  26bef0b2af completion: use __git when calling --list-cmds