[BUG] completion.commands does not remove multiple commands

[BUG] completion.commands does not remove multiple commands

[Todd Zullinger]

Hi,

I was playing with the completion.commands config added in
6532f3740b ("completion: allow to customize the completable
command list", 2018-05-20) and noticed an issue removing
multiple commands.

I wanted to remove completion for cherry and mergetool, so I
added them both to the config:

    git config completion.commands "-cherry -mergetool"

But git still completes cherry in this case, only removing
mergetool.  Swapping the items in the config yields the
opposite result (cherry is removed and mergetool is not).

With luck this will be a clear and easily resolved issue in
list_cmds_by_config() (in help.c).

Here's an example in test form:

-- 8< --
diff --git c/t/t9902-completion.sh w/t/t9902-completion.sh
index 3a2c6326d8..06cee36ae5 100755
--- c/t/t9902-completion.sh
+++ w/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' '
+	test_config completion.commands "-cherry -mergetool" &&
+	git --list-cmds=list-mainporcelain,list-complete,config |
+		grep ^cherry >actual &&
+	echo cherry-pick >expected &&
+	test_cmp expected actual
+'
+
 test_expect_success 'setup for integration tests' '
 	echo content >file1 &&
 	echo more >file2 &&
-- 8< --

That's not quite normal form for t9902, but I was undable to
create a working test using the test_completion functions.
The tests set GIT_TESTING_PORCELAIN_COMMAND_LIST and
GIT_TESTING_ALL_COMMAND_LIST which override the settings in
the completion script.  I played a bit with adjusting those
to add cherry{,-pick} to the command lists, unsetting those
vars for the test, and such, to no avail.

In the end, I'm not really sure that calling --list-cmds
directly is such a bad way to go for testing this issue.

-- 
Todd

Re: [BUG] completion.commands does not remove multiple commands

[Jeff King]

On Thu, Feb 28, 2019 at 05:31:23PM -0500, Todd Zullinger wrote:

> Hi,
> 
> I was playing with the completion.commands config added in
> 6532f3740b ("completion: allow to customize the completable
> command list", 2018-05-20) and noticed an issue removing
> multiple commands.
> 
> I wanted to remove completion for cherry and mergetool, so I
> added them both to the config:
> 
>     git config completion.commands "-cherry -mergetool"
> 
> But git still completes cherry in this case, only removing
> mergetool.  Swapping the items in the config yields the
> opposite result (cherry is removed and mergetool is not).

I can reproduce your problem, though the test you included passes for me
even with the current tip of master. I suspect this is the problem:

diff --git a/help.c b/help.c
index 520c9080e8..026f881715 100644
--- a/help.c
+++ b/help.c
@@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list)
 		const char *p = strchrnul(cmd_list, ' ');
 
 		strbuf_add(&sb, cmd_list, p - cmd_list);
-		if (*cmd_list == '-')
-			string_list_remove(list, cmd_list + 1, 0);
+		if (sb.buf[0] == '-')
+			string_list_remove(list, sb.buf + 1, 0);
 		else
 			string_list_insert(list, sb.buf);
 		strbuf_release(&sb);

though I can't help but wonder if the whole thing would be simpler using
string_list_split().

-Peff

Re: [BUG] completion.commands does not remove multiple commands

[SZEDER Gábor]

On Thu, Feb 28, 2019 at 05:31:23PM -0500, Todd Zullinger wrote:
> I was playing with the completion.commands config added in
> 6532f3740b ("completion: allow to customize the completable
> command list", 2018-05-20) and noticed an issue removing
> multiple commands.
> 
> I wanted to remove completion for cherry and mergetool, so I
> added them both to the config:
> 
>     git config completion.commands "-cherry -mergetool"
> 
> But git still completes cherry in this case, only removing
> mergetool.  Swapping the items in the config yields the
> opposite result (cherry is removed and mergetool is not).
> 
> With luck this will be a clear and easily resolved issue in
> list_cmds_by_config() (in help.c).

Indeed, this seems to fix it for me:

diff --git a/help.c b/help.c
index 520c9080e8..f2c6f0c9f7 100644
--- a/help.c
+++ b/help.c
@@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list)
 		const char *p = strchrnul(cmd_list, ' ');
 
 		strbuf_add(&sb, cmd_list, p - cmd_list);
-		if (*cmd_list == '-')
-			string_list_remove(list, cmd_list + 1, 0);
+		if (*sb.buf == '-')
+			string_list_remove(list, sb.buf + 1, 0);
 		else
 			string_list_insert(list, sb.buf);
 		strbuf_release(&sb);

Re: [BUG] completion.commands does not remove multiple commands

[Todd Zullinger]

Jeff King wrote:
> I can reproduce your problem, though the test you included passes for me
> even with the current tip of master.

Oh, hrm.  I think the issue is that completion.commands needs to be
set in the global (or system-wide) config, via test_config_global
rather than the local repo config which test_config sets.

In hindsight, that seems obvious.  But it's probably worth noting
that where completion.commands is documented, for anyone who might
spend a few cycles trying to configure it on a per-repo basis before
realizing it doesn't work.

> I suspect this is the problem:
> 
> diff --git a/help.c b/help.c
> index 520c9080e8..026f881715 100644
> --- a/help.c
> +++ b/help.c
> @@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list)
>  		const char *p = strchrnul(cmd_list, ' ');
>  
>  		strbuf_add(&sb, cmd_list, p - cmd_list);
> -		if (*cmd_list == '-')
> -			string_list_remove(list, cmd_list + 1, 0);
> +		if (sb.buf[0] == '-')
> +			string_list_remove(list, sb.buf + 1, 0);
>  		else
>  			string_list_insert(list, sb.buf);
>  		strbuf_release(&sb);
> 
> though I can't help but wonder if the whole thing would be simpler using
> string_list_split().

That does indeed fix things (as does SZEDER's similar version, which
arrived only a few seconds later).  Thanks to both of you for the
very quick replies!

I'll leave it to those who know better to follow up with a change to
use string_list_split().

Here's a first stab at improving the docs and fixing the bug.

Jeff King (1):
  completion: fix multiple command removals

Todd Zullinger (2):
  doc: note config file restrictions for completion.commands
  t9902: test multiple removals via completion.commands

 Documentation/config/completion.txt | 3 ++-
 Documentation/git.txt               | 3 ++-
 help.c                              | 4 ++--
 t/t9902-completion.sh               | 8 ++++++++
 4 files changed, 14 insertions(+), 4 deletions(-)

Published-as: https://github.com/tmzullinger/git/releases/tag/completion.commands-v1

-- 
Todd

[PATCH 1/3] doc: note config file restrictions for completion.commands

[Todd Zullinger]

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.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
 Documentation/config/completion.txt | 3 ++-
 Documentation/git.txt               | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/completion.txt b/Documentation/config/completion.txt
index 4d99bf33c9..4859d18e86 100644
--- a/Documentation/config/completion.txt
+++ b/Documentation/config/completion.txt
@@ -4,4 +4,5 @@ completion.commands::
 	porcelain commands and a few select others are completed. You
 	can add more commands, separated by space, in this
 	variable. Prefixing the command with '-' will remove it from
-	the existing list.
+	the existing list.  The variable must be set in either the
+	system-wide or global config.
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 00156d64aa..638f4d6cc9 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -172,7 +172,8 @@ foo.bar= ...`) sets `foo.bar` to the empty string which `git config
 	others (all other commands in `$PATH` that have git- prefix),
 	list-<category> (see categories in command-list.txt),
 	nohelpers (exclude helper commands), alias and config
-	(retrieve command list from config variable completion.commands)
+	(retrieve command list from config variable completion.commands,
+	set in the global or system-wide config file)
 
 GIT COMMANDS
 ------------

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

[Todd Zullinger]

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(+)

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

[PATCH 3/3] completion: fix multiple command removals

[Todd Zullinger]

From: Jeff King <peff@peff.net>

6532f3740b ("completion: allow to customize the completable
command list", 2018-05-20) added the completion.commands config
variable.

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.

Signed-off-by: Jeff King <peff@peff.net>
---
Jeff,

The commit message could probably be worded better, particularly since it's
forged in your name.

 help.c                | 4 ++--
 t/t9902-completion.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/help.c b/help.c
index 520c9080e8..026f881715 100644
--- a/help.c
+++ b/help.c
@@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list)
 		const char *p = strchrnul(cmd_list, ' ');
 
 		strbuf_add(&sb, cmd_list, p - cmd_list);
-		if (*cmd_list == '-')
-			string_list_remove(list, cmd_list + 1, 0);
+		if (sb.buf[0] == '-')
+			string_list_remove(list, sb.buf + 1, 0);
 		else
 			string_list_insert(list, sb.buf);
 		strbuf_release(&sb);
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index dd11bb660d..d7daa1ca92 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1483,7 +1483,7 @@ test_expect_success 'git --help completion' '
 	test_completion "git --help core" "core-tutorial "
 '
 
-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 |

Re: [PATCH 3/3] completion: fix multiple command removals

[Jeff King]

On Fri, Mar 01, 2019 at 12:34:43PM -0500, Todd Zullinger wrote:

> The commit message could probably be worded better, particularly since it's
> forged in your name.

What you have is OK, but I'd have probably written it like this:

-- >8 --
Subject: [PATCH] completion: fix multiple command removals

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.

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>
---
 help.c                | 4 ++--
 t/t9902-completion.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/help.c b/help.c
index 520c9080e8..026f881715 100644
--- a/help.c
+++ b/help.c
@@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list)
 		const char *p = strchrnul(cmd_list, ' ');
 
 		strbuf_add(&sb, cmd_list, p - cmd_list);
-		if (*cmd_list == '-')
-			string_list_remove(list, cmd_list + 1, 0);
+		if (sb.buf[0] == '-')
+			string_list_remove(list, sb.buf + 1, 0);
 		else
 			string_list_insert(list, sb.buf);
 		strbuf_release(&sb);
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index dd11bb660d..d7daa1ca92 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1483,7 +1483,7 @@ test_expect_success 'git --help completion' '
 	test_completion "git --help core" "core-tutorial "
 '
 
-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 |
-- 
2.21.0.675.g01c085a870

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

[Eric Sunshine]

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
    '

Re: [BUG] completion.commands does not remove multiple commands

[Jeff King]

On Fri, Mar 01, 2019 at 12:34:40PM -0500, Todd Zullinger wrote:

> Jeff King wrote:
> > I can reproduce your problem, though the test you included passes for me
> > even with the current tip of master.
> 
> Oh, hrm.  I think the issue is that completion.commands needs to be
> set in the global (or system-wide) config, via test_config_global
> rather than the local repo config which test_config sets.
> 
> In hindsight, that seems obvious.  But it's probably worth noting
> that where completion.commands is documented, for anyone who might
> spend a few cycles trying to configure it on a per-repo basis before
> realizing it doesn't work.

I think this is actually a bug. Normally code that is checking config
before we've decide to do setup_git_directory() would use
read_early_config(), which uses discover_git_directory() to tentatively
see if we're in a repo, and if so to add it to the config sequence.

But this code uses the caching configset mechanism. And that code
(rightly) does not use read_early_config(), because it has no idea if
it's being called early or what.

I think we probably ought to be doing something like this:

diff --git a/git.c b/git.c
index 2dd588674f..ba3690245e 100644
--- a/git.c
+++ b/git.c
@@ -62,6 +62,13 @@ static int list_cmds(const char *spec)
 {
 	struct string_list list = STRING_LIST_INIT_DUP;
 	int i;
+	int nongit;
+
+	/*
+	 * Set up the repository so we can pick up any repo-level config (like
+	 * completion.commands).
+	 */
+	setup_git_directory_gently(&nongit);
 
 	while (*spec) {
 		const char *sep = strchrnul(spec, ',');

> I'll leave it to those who know better to follow up with a change to
> use string_list_split().

It's less of an improvement than I'd hoped, since most of the code is
actually handling the "-" thing. So I don't think it's really worth the
churn. But here's what it looks like for reference:

---
 help.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/help.c b/help.c
index 026f881715..9e4d258cb8 100644
--- a/help.c
+++ b/help.c
@@ -373,7 +373,9 @@ void list_cmds_by_category(struct string_list *list,
 
 void list_cmds_by_config(struct string_list *list)
 {
-	const char *cmd_list;
+	const char *cmd_str;
+	struct string_list cmd_list = STRING_LIST_INIT_DUP;
+	int i;
 
 	/*
 	 * There's no actual repository setup at this point (and even
@@ -382,26 +384,22 @@ void list_cmds_by_config(struct string_list *list)
 	 * too since the caller (git --list-cmds=) should exit shortly
 	 * anyway.
 	 */
-	if (git_config_get_string_const("completion.commands", &cmd_list))
+	if (git_config_get_string_const("completion.commands", &cmd_str))
 		return;
 
 	string_list_sort(list);
 	string_list_remove_duplicates(list, 0);
 
-	while (*cmd_list) {
-		struct strbuf sb = STRBUF_INIT;
-		const char *p = strchrnul(cmd_list, ' ');
+	string_list_split(&cmd_list, cmd_str, ' ', -1);
+	for (i = 0; i < cmd_list.nr; i++) {
+		const char *cmd = cmd_list.items[0].string;
 
-		strbuf_add(&sb, cmd_list, p - cmd_list);
-		if (sb.buf[0] == '-')
-			string_list_remove(list, sb.buf + 1, 0);
+		if (*cmd == '-')
+			string_list_remove(list, cmd + 1, 0);
 		else
-			string_list_insert(list, sb.buf);
-		strbuf_release(&sb);
-		while (*p == ' ')
-			p++;
-		cmd_list = p;
+			string_list_insert(list, cmd);
 	}
+	string_list_clear(&cmd_list, 0);
 }
 
 void list_common_guides_help(void)

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

[SZEDER Gábor]

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.

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

[Todd Zullinger]

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

Re: [BUG] completion.commands does not remove multiple commands

[Todd Zullinger]

Jeff King wrote:
> On Fri, Mar 01, 2019 at 12:34:40PM -0500, Todd Zullinger wrote:
> 
>> Jeff King wrote:
>>> I can reproduce your problem, though the test you included passes for me
>>> even with the current tip of master.
>> 
>> Oh, hrm.  I think the issue is that completion.commands needs to be
>> set in the global (or system-wide) config, via test_config_global
>> rather than the local repo config which test_config sets.
>> 
>> In hindsight, that seems obvious.  But it's probably worth noting
>> that where completion.commands is documented, for anyone who might
>> spend a few cycles trying to configure it on a per-repo basis before
>> realizing it doesn't work.
> 
> I think this is actually a bug. Normally code that is checking config
> before we've decide to do setup_git_directory() would use
> read_early_config(), which uses discover_git_directory() to tentatively
> see if we're in a repo, and if so to add it to the config sequence.
> 
> But this code uses the caching configset mechanism. And that code
> (rightly) does not use read_early_config(), because it has no idea if
> it's being called early or what.
> 
> I think we probably ought to be doing something like this:
> 
> diff --git a/git.c b/git.c
> index 2dd588674f..ba3690245e 100644
> --- a/git.c
> +++ b/git.c
> @@ -62,6 +62,13 @@ static int list_cmds(const char *spec)
>  {
>  	struct string_list list = STRING_LIST_INIT_DUP;
>  	int i;
> +	int nongit;
> +
> +	/*
> +	 * Set up the repository so we can pick up any repo-level config (like
> +	 * completion.commands).
> +	 */
> +	setup_git_directory_gently(&nongit);
>  
>  	while (*spec) {
>  		const char *sep = strchrnul(spec, ',');

Hmm.  The comments in list_cmds_by_config() made me wonder
if not using a local repo config was intentional:

        /*
         * There's no actual repository setup at this point (and even
         * if there is, we don't really care; only global config
         * matters). If we accidentally set up a repository, it's ok
         * too since the caller (git --list-cmds=) should exit shortly
         * anyway.
         */

Is the cost of setting up a repository something which might
noticeably slow down interactive completion?  In my testing
today I haven't felt it, but I have loads of memory on this
system.

I did apply your change and that allows the test to use
test_config() rather than test_config_global().  The full
test suite passes, so the change doesn't trigger any new
issues we have covered by a test, at least.

If we wanted to respect local configs, how does this look?

-- 8< --
From: Jeff King <peff@peff.net>
Subject: [PATCH] git: read local config in --list-cmds

Normally code that is checking config before we've decide to do
setup_git_directory() would use read_early_config(), which uses
discover_git_directory() to tentatively see if we're in a repo,
and if so to add it to the config sequence.

But list_cmds() uses the caching configset mechanism and
(rightly) does not use read_early_config(), because it has no
idea if it's being called early.

Call setup_git_directory_gently() so we can pick up repo-level
config (like completion.commands).

Signed-off-by: Jeff King <peff@peff.net>
---
 git.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/git.c b/git.c
index 2dd588674f..10e49d79f6 100644
--- a/git.c
+++ b/git.c
@@ -62,6 +62,13 @@ static int list_cmds(const char *spec)
 {
 	struct string_list list = STRING_LIST_INIT_DUP;
 	int i;
+	int nongit;
+
+	/*
+	* Set up the repository so we can pick up any repo-level config (like
+	* completion.commands).
+	*/
+	setup_git_directory_gently(&nongit);
 
 	while (*spec) {
 		const char *sep = strchrnul(spec, ',');
-- 8< --

-- 
Todd

Re: [BUG] completion.commands does not remove multiple commands

[Jeff King]

On Fri, Mar 01, 2019 at 05:15:51PM -0500, Todd Zullinger wrote:

> Hmm.  The comments in list_cmds_by_config() made me wonder
> if not using a local repo config was intentional:
> 
>         /*
>          * There's no actual repository setup at this point (and even
>          * if there is, we don't really care; only global config
>          * matters). If we accidentally set up a repository, it's ok
>          * too since the caller (git --list-cmds=) should exit shortly
>          * anyway.
>          */

Well, let's see what Duy says. :)

I've never used completion.commands myself, but it seems reasonable that
somebody might want different completion in different repos (e.g., if
they never use "mergetool" in one repo, but do in another).

> Is the cost of setting up a repository something which might
> noticeably slow down interactive completion?  In my testing
> today I haven't felt it, but I have loads of memory on this
> system.

I'd doubt it. It is a few syscalls (and might have to walk up the
filesystem if you're not actually in a repository), but it's something
that basically every Git process does, and I don't think it's ever been
noticeable.

> I did apply your change and that allows the test to use
> test_config() rather than test_config_global().  The full
> test suite passes, so the change doesn't trigger any new
> issues we have covered by a test, at least.
> 
> If we wanted to respect local configs, how does this look?

Looks good, with two minor commit message nits:

> -- 8< --
> From: Jeff King <peff@peff.net>
> Subject: [PATCH] git: read local config in --list-cmds
> 
> Normally code that is checking config before we've decide to do

s/decide/&d/

> setup_git_directory() would use read_early_config(), which uses
> discover_git_directory() to tentatively see if we're in a repo,
> and if so to add it to the config sequence.
> 
> But list_cmds() uses the caching configset mechanism and
> (rightly) does not use read_early_config(), because it has no
> idea if it's being called early.

I'd say "mechanism _which_ rightly does not use read_early_config..." to
make it clear we're talking about configset, not list_cmds().

-Peff

Re: [BUG] completion.commands does not remove multiple commands

[Duy Nguyen]

On Sat, Mar 2, 2019 at 6:08 AM Jeff King <peff@peff.net> wrote:
>
> On Fri, Mar 01, 2019 at 05:15:51PM -0500, Todd Zullinger wrote:
>
> > Hmm.  The comments in list_cmds_by_config() made me wonder
> > if not using a local repo config was intentional:
> >
> >         /*
> >          * There's no actual repository setup at this point (and even
> >          * if there is, we don't really care; only global config
> >          * matters). If we accidentally set up a repository, it's ok
> >          * too since the caller (git --list-cmds=) should exit shortly
> >          * anyway.
> >          */
>
> Well, let's see what Duy says. :)

I vaguely recall that I wanted to cache the results in
git-completion.bash at some point. But looking at that script I don't
think there's any caching. So yes it should be ok to read per-repo
config as well (and adjust or drop this comment block)

> I've never used completion.commands myself, but it seems reasonable that
> somebody might want different completion in different repos (e.g., if
> they never use "mergetool" in one repo, but do in another).

It sounds to me confusing that you want different command sets in
different repos. Which is why I made it global config only. But I
suspect that people who have per-repo aliases may find this natural.
So yeah, no objection.

PS. and thanks for the bug fix.
-- 
Duy

Re: [BUG] completion.commands does not remove multiple commands

[Junio C Hamano]

Todd Zullinger <tmz@pobox.com> writes:

> Hmm.  The comments in list_cmds_by_config() made me wonder
> if not using a local repo config was intentional:
>
>         /*
>          * There's no actual repository setup at this point (and even
>          * if there is, we don't really care; only global config
>          * matters). If we accidentally set up a repository, it's ok
>          * too since the caller (git --list-cmds=) should exit shortly
>          * anyway.
>          */

Doesn't the output from list-cmds-by-config get cached at the
completion script layer in $__git_blah variables, and the cached
values are not cleared when you chdir around?  If you allowed the
repo-specific configuration to affect its output, the cached values
need to be reset when you cross repository boundaries.  Otherwise
you'd see complaints like "I have this in repo A but not in repo B;
when I start from repo A, it gets completed even after I go to repo
B.  If I start from repo B, I do not get completion in either of
them" (the former is because repo-A specific result gets cached, the
latter is because the cache is populated with the result taken in
repo-B that doesn't have customization and stays around even when
you visit repo-B).

I think it is a sensible design decision to forbid per-repo config
to relieve us from having to worry about when to invalidate the
cache and all associated complexities.

Re: [BUG] completion.commands does not remove multiple commands

[Todd Zullinger]

Hi,

Junio C Hamano wrote:
> Todd Zullinger <tmz@pobox.com> writes:
> 
>> Hmm.  The comments in list_cmds_by_config() made me wonder
>> if not using a local repo config was intentional:
>>
>>         /*
>>          * There's no actual repository setup at this point (and even
>>          * if there is, we don't really care; only global config
>>          * matters). If we accidentally set up a repository, it's ok
>>          * too since the caller (git --list-cmds=) should exit shortly
>>          * anyway.
>>          */
> 
> Doesn't the output from list-cmds-by-config get cached at the
> completion script layer in $__git_blah variables, and the cached
> values are not cleared when you chdir around?

In testing, I didn't find any evidence of caching.  Setting
commands to be added and removed in the global and local
configs worked reasonably.

Duy's reply suggests that was considered but not
implemented.  I there were caching (and if it were tedious
for the completion code to keep fresh between repos), then
it would a bad plan to allow per-repo config.

If there was a goal of adding such caching it might also
make sense to avoid "fixing" the code here to allow per-repo
config before it's known how that might affect such caching.

It sounds like that's not something Duy is planning on for
the near term though, so perhaps we're fine to allow local
repo config here?  As Duy mentioned, maybe some users with
local aliases want to add them to the completion locally as
well.

If we choose to avoid local repo config then we can add a
comment to the documetation like I had in 2/3.  Maybe also
update the comment in list_cmds_by_config() to note that we
intentionally don't setup a repo -- or a similar comment in
list_cmds(), where Jeff's 1/3 was adding
setup_git_directory_gently().

I don't have a strong opinion either way.  I more or less
have the minor patches for either direction at this point.

Thanks,

-- 
Todd

Re: [BUG] completion.commands does not remove multiple commands

[SZEDER Gábor]

On Fri, Mar 01, 2019 at 09:40:12PM -0500, Todd Zullinger wrote:
> Hi,
> 
> Junio C Hamano wrote:
> > Todd Zullinger <tmz@pobox.com> writes:
> > 
> >> Hmm.  The comments in list_cmds_by_config() made me wonder
> >> if not using a local repo config was intentional:
> >>
> >>         /*
> >>          * There's no actual repository setup at this point (and even
> >>          * if there is, we don't really care; only global config
> >>          * matters). If we accidentally set up a repository, it's ok
> >>          * too since the caller (git --list-cmds=) should exit shortly
> >>          * anyway.
> >>          */
> > 
> > Doesn't the output from list-cmds-by-config get cached at the
> > completion script layer in $__git_blah variables, and the cached
> > values are not cleared when you chdir around?
> 
> In testing, I didn't find any evidence of caching.  Setting
> commands to be added and removed in the global and local
> configs worked reasonably.
> 
> Duy's reply suggests that was considered but not
> implemented.  I there were caching (and if it were tedious
> for the completion code to keep fresh between repos), then
> it would a bad plan to allow per-repo config.

The completion script used to cache the list of porcelain commands,
but then Duy came along and removed it in 3301d36b29 (completion: add
and use --list-cmds=alias, 2018-05-20).

We cached the commands, because it was a bit involved to get them:
first we run 'git help -a' to list all commands, then extracted the
command names from the help output with 'grep', and finally an
enormous case statement removed all plumbing commands.  Caching spared
us the overhead of these external processes and maybe 2-3 subshells.
Note that because of this caching if you dropped a third-party
'git-foo' command in your $PATH, it didn't show up in completion until
you re-sourced the completion script, which was the standard way to
invalidate/refresh the caches.

However, even when we cached porcelain commands, we didn't cache
aliases, even though getting them is a bit involved as well, requiring
running 'git config', two subshells and a shell loop.  I presume that
back then the reason for not caching aliases was that they could be
repo-specific.

Now, ever since Duy revamped commands completion, we only run a simple
$(git --list-cmds=...), i.e. a git command and a subshell takes care
of all that.  IMO this is the best we ever had, because it uses one
less subshell than before to list both commands and aliases, and it
lists everything afresh, including third-party 'git-foo' commands.

I don't see the benefit of bringing back caching.  Yes, on one hand we
could complete commands with a single variable expansion, which is
clearly faster than that $(git --list-cmds=...).  OTOH, that's only
commands, but what about aliases?  If we cache aliases, too, then that
could be considered a regression by those who do have repo-specific
aliases; if we don't, then we are back at running 'git config' and two
subshells, i.e. one subshell more than we currently have.

And if we won't cache commands, then we might as well modify
list_cmds_by_config() to read 'completion.commands' from the
repo-specific config, too.  I'm fairly neutral on this, because I
can only imagine rather convoluted scenarios where a repo-specific
command list would be useful, but at least it would make it consistent
with aliases, which we read from the current repo's config and we
always have.


Note, however, that for completeness sake, if we choose to update
list_cmds_by_config() to read the repo's config as well, then we
should also update the completion script to run $(__git
--list-cmds=...), note the two leading underscores, so in case of 'git
-C over/there <TAB>' it would read 'completion.commands' from the right
repository.

Re: [BUG] completion.commands does not remove multiple commands

[Todd Zullinger]

SZEDER Gábor wrote:
[... lots of good history ...]

Thanks for an excellent summary!

> Note, however, that for completeness sake, if we choose to update
> list_cmds_by_config() to read the repo's config as well, then we
> should also update the completion script to run $(__git
> --list-cmds=...), note the two leading underscores, so in case of 'git
> -C over/there <TAB>' it would read 'completion.commands' from the right
> repository.

Thanks for pointing this out. I'll add that to my local
branch for the "respect local config" case.

At the moment, I think it only matters for calls where
config is in the --list-cmds list. But since the fix Jeff
suggested affects all --list-cmds calls, it seems prudent to
adjust the 3-4 other uses of --list-cmds in the completion
script.  Let me know if you see a reason not to do that.

Thanks again for such a nice summary,

-- 
Todd

Re: [BUG] completion.commands does not remove multiple commands

[Jeff King]

On Sat, Mar 02, 2019 at 05:07:04AM +0100, SZEDER Gábor wrote:

> The completion script used to cache the list of porcelain commands,
> but then Duy came along and removed it in 3301d36b29 (completion: add
> and use --list-cmds=alias, 2018-05-20).
> [...]

Thanks for this summary.

Just for the record, as the person who suggested that we should respect
repo config here, I don't personally care all that much either way. I do
not plan to use the feature myself, but it was just what I would have
expected to happen. So I can live with it either way.

That said, it seems like if we were to go back to caching, we'd need to
handle directory changes in order for aliases (which already do respect
repo config) to be correct anyway. So it doesn't seem like this is
introducing any burden that was not there already.

-Peff

Re: [PATCH 1/3] doc: note config file restrictions for completion.commands

[Duy Nguyen]

Poking this thread before it goes completely dead...

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?

>
> Signed-off-by: Todd Zullinger <tmz@pobox.com>
> ---
>  Documentation/config/completion.txt | 3 ++-
>  Documentation/git.txt               | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config/completion.txt b/Documentation/config/completion.txt
> index 4d99bf33c9..4859d18e86 100644
> --- a/Documentation/config/completion.txt
> +++ b/Documentation/config/completion.txt
> @@ -4,4 +4,5 @@ completion.commands::
>         porcelain commands and a few select others are completed. You
>         can add more commands, separated by space, in this
>         variable. Prefixing the command with '-' will remove it from
> -       the existing list.
> +       the existing list.  The variable must be set in either the
> +       system-wide or global config.
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 00156d64aa..638f4d6cc9 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -172,7 +172,8 @@ foo.bar= ...`) sets `foo.bar` to the empty string which `git config
>         others (all other commands in `$PATH` that have git- prefix),
>         list-<category> (see categories in command-list.txt),
>         nohelpers (exclude helper commands), alias and config
> -       (retrieve command list from config variable completion.commands)
> +       (retrieve command list from config variable completion.commands,
> +       set in the global or system-wide config file)
>
>  GIT COMMANDS
>  ------------



-- 
Duy

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

[Todd Zullinger]

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. :/

> 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

[PATCH v2 1/4] git: read local config in --list-cmds

[Todd Zullinger]

From: Jeff King <peff@peff.net>

Normally code that is checking config before we've decided to do
setup_git_directory() would use read_early_config(), which uses
discover_git_directory() to tentatively see if we're in a repo,
and if so to add it to the config sequence.

But list_cmds() uses the caching configset mechanism which
rightly does not use read_early_config(), because it has no
idea if it's being called early.

Call setup_git_directory_gently() so we can pick up repo-level
config (like completion.commands).

Signed-off-by: Jeff King <peff@peff.net>
---
 git.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/git.c b/git.c
index 2dd588674f..10e49d79f6 100644
--- a/git.c
+++ b/git.c
@@ -62,6 +62,13 @@ static int list_cmds(const char *spec)
 {
 	struct string_list list = STRING_LIST_INIT_DUP;
 	int i;
+	int nongit;
+
+	/*
+	* Set up the repository so we can pick up any repo-level config (like
+	* completion.commands).
+	*/
+	setup_git_directory_gently(&nongit);
 
 	while (*spec) {
 		const char *sep = strchrnul(spec, ',');

[PATCH v2 2/4] t9902: test multiple removals via completion.commands

[Todd Zullinger]

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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3a2c6326d8..3f5b420bf8 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1483,6 +1483,12 @@ test_expect_success 'git --help completion' '
 	test_completion "git --help core" "core-tutorial "
 '
 
+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
+'
+
 test_expect_success 'setup for integration tests' '
 	echo content >file1 &&
 	echo more >file2 &&

[PATCH v2 3/4] completion: fix multiple command removals

[Todd Zullinger]

From: Jeff King <peff@peff.net>

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.

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>
---
 help.c                | 4 ++--
 t/t9902-completion.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/help.c b/help.c
index 520c9080e8..026f881715 100644
--- a/help.c
+++ b/help.c
@@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list)
 		const char *p = strchrnul(cmd_list, ' ');
 
 		strbuf_add(&sb, cmd_list, p - cmd_list);
-		if (*cmd_list == '-')
-			string_list_remove(list, cmd_list + 1, 0);
+		if (sb.buf[0] == '-')
+			string_list_remove(list, sb.buf + 1, 0);
 		else
 			string_list_insert(list, sb.buf);
 		strbuf_release(&sb);
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3f5b420bf8..050fac4fff 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1483,7 +1483,7 @@ test_expect_success 'git --help completion' '
 	test_completion "git --help core" "core-tutorial "
 '
 
-test_expect_failure 'completion.commands removes multiple commands' '
+test_expect_success '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

[PATCH v2 4/4] completion: use __git when calling --list-cmds

[Todd Zullinger]

Since e51bdea6d3 ("git: read local config in --list-cmds", 2019-03-01),
the completion.commands variable respects repo-level configuration.  Use
__git which ensures that the proper repo config is consulted if the
command line contains 'git -C /some/other/repo'.

Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Todd Zullinger <tmz@pobox.com>
---

Junio, I referenc the commit id for "git: read local config in
--list-cmds" which is earlier in this series, so the id will, of
course, differ when you apply it.  Let me know if you'd prefer
this commit to be dropped and resent once the others in the
series are applied or if it's easy for you to adjust when it's
queued.

Also, as I wrote in an earlier reply, at the moment, I think
using __git only matters for calls where config is in the
--list-cmds list.  But since Jeff's fix affects all --list-cmds
calls, it seems prudent to adjust the 3 other uses of --list-cmds
in the completion script.

 contrib/completion/git-completion.bash | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 499e56f83d..e505f04ff7 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1010,7 +1010,7 @@ __git_all_commands=
 __git_compute_all_commands ()
 {
 	test -n "$__git_all_commands" ||
-	__git_all_commands=$(git --list-cmds=main,others,alias,nohelpers)
+	__git_all_commands=$(__git --list-cmds=main,others,alias,nohelpers)
 }
 
 # Lists all set config variables starting with the given section prefix,
@@ -1620,9 +1620,9 @@ _git_help ()
 	esac
 	if test -n "$GIT_TESTING_ALL_COMMAND_LIST"
 	then
-		__gitcomp "$GIT_TESTING_ALL_COMMAND_LIST $(git --list-cmds=alias,list-guide) gitk"
+		__gitcomp "$GIT_TESTING_ALL_COMMAND_LIST $(__git --list-cmds=alias,list-guide) gitk"
 	else
-		__gitcomp "$(git --list-cmds=main,nohelpers,alias,list-guide) gitk"
+		__gitcomp "$(__git --list-cmds=main,nohelpers,alias,list-guide) gitk"
 	fi
 }
 
@@ -2888,7 +2888,7 @@ __git_main ()
 			then
 				__gitcomp "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
 			else
-				__gitcomp "$(git --list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)"
+				__gitcomp "$(__git --list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)"
 			fi
 			;;
 		esac

Re: [PATCH v2 1/4] git: read local config in --list-cmds

[Duy Nguyen]

On Mon, Mar 18, 2019 at 1:16 AM Todd Zullinger <tmz@pobox.com> wrote:
>
> From: Jeff King <peff@peff.net>
>
> Normally code that is checking config before we've decided to do
> setup_git_directory() would use read_early_config(), which uses
> discover_git_directory() to tentatively see if we're in a repo,
> and if so to add it to the config sequence.
>
> But list_cmds() uses the caching configset mechanism which
> rightly does not use read_early_config(), because it has no
> idea if it's being called early.
>
> Call setup_git_directory_gently() so we can pick up repo-level
> config (like completion.commands).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  git.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/git.c b/git.c
> index 2dd588674f..10e49d79f6 100644
> --- a/git.c
> +++ b/git.c
> @@ -62,6 +62,13 @@ static int list_cmds(const char *spec)
>  {
>         struct string_list list = STRING_LIST_INIT_DUP;
>         int i;
> +       int nongit;
> +
> +       /*
> +       * Set up the repository so we can pick up any repo-level config (like
> +       * completion.commands).
> +       */
> +       setup_git_directory_gently(&nongit);

This gave me a pause because we could try to find .git more than
necessary (e.g. when --list-cmds is requested without "config"). But I
don't think that happens often enough to be worried about.

It also subtly changes list_aliases() code flow, because the
read_early_config() call inside will use the already-discovered gitdir
 here instead of trying to rediscover. So, everything is still fine.

You probably want to drop the comment block about repository setup
inside list_cmds_by_config() too.

>
>         while (*spec) {
>                 const char *sep = strchrnul(spec, ',');
-- 
Duy

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

[Todd Zullinger]

Hi,

Duy Nguyen wrote:
> You probably want to drop the comment block about repository setup
> inside list_cmds_by_config() too.

You're right, of course.  Thanks Duy. :)

That's the only change since v2.

Other than a follow-up to update the commit reference in 4/4
after 1/4 is in the final form on pu, I think this might be good.
If it's easier, we can skip 4/4 and I'll resend it after the
others are on pu.

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                                 | 11 ++---------
 t/t9902-completion.sh                  |  6 ++++++
 4 files changed, 19 insertions(+), 13 deletions(-)

Range-diff against v2:
1:  e51bdea6d3 ! 1:  6e9872b0e3 git: read local config in --list-cmds
    @@ -33,3 +33,21 @@
      
      	while (*spec) {
      		const char *sep = strchrnul(spec, ',');
    +
    + diff --git a/help.c b/help.c
    + --- a/help.c
    + +++ b/help.c
    +@@
    + {
    + 	const char *cmd_list;
    + 
    +-	/*
    +-	 * There's no actual repository setup at this point (and even
    +-	 * if there is, we don't really care; only global config
    +-	 * matters). If we accidentally set up a repository, it's ok
    +-	 * too since the caller (git --list-cmds=) should exit shortly
    +-	 * anyway.
    +-	 */
    + 	if (git_config_get_string_const("completion.commands", &cmd_list))
    + 		return;
    + 
2:  2f5e9da9de = 2:  6873ae3868 t9902: test multiple removals via completion.commands
3:  7548dcc23f = 3:  f66bbc0b55 completion: fix multiple command removals
4:  26bef0b2af = 4:  197b176483 completion: use __git when calling --list-cmds

[PATCH v3 1/4] git: read local config in --list-cmds

[Todd Zullinger]

From: Jeff King <peff@peff.net>

Normally code that is checking config before we've decided to do
setup_git_directory() would use read_early_config(), which uses
discover_git_directory() to tentatively see if we're in a repo,
and if so to add it to the config sequence.

But list_cmds() uses the caching configset mechanism which
rightly does not use read_early_config(), because it has no
idea if it's being called early.

Call setup_git_directory_gently() so we can pick up repo-level
config (like completion.commands).

Signed-off-by: Jeff King <peff@peff.net>
---
 git.c  | 7 +++++++
 help.c | 7 -------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/git.c b/git.c
index 2dd588674f..10e49d79f6 100644
--- a/git.c
+++ b/git.c
@@ -62,6 +62,13 @@ static int list_cmds(const char *spec)
 {
 	struct string_list list = STRING_LIST_INIT_DUP;
 	int i;
+	int nongit;
+
+	/*
+	* Set up the repository so we can pick up any repo-level config (like
+	* completion.commands).
+	*/
+	setup_git_directory_gently(&nongit);
 
 	while (*spec) {
 		const char *sep = strchrnul(spec, ',');
diff --git a/help.c b/help.c
index 520c9080e8..fac7e421d0 100644
--- a/help.c
+++ b/help.c
@@ -375,13 +375,6 @@ void list_cmds_by_config(struct string_list *list)
 {
 	const char *cmd_list;
 
-	/*
-	 * There's no actual repository setup at this point (and even
-	 * if there is, we don't really care; only global config
-	 * matters). If we accidentally set up a repository, it's ok
-	 * too since the caller (git --list-cmds=) should exit shortly
-	 * anyway.
-	 */
 	if (git_config_get_string_const("completion.commands", &cmd_list))
 		return;
 

[PATCH v3 2/4] t9902: test multiple removals via completion.commands

[Todd Zullinger]

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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3a2c6326d8..3f5b420bf8 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1483,6 +1483,12 @@ test_expect_success 'git --help completion' '
 	test_completion "git --help core" "core-tutorial "
 '
 
+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
+'
+
 test_expect_success 'setup for integration tests' '
 	echo content >file1 &&
 	echo more >file2 &&

[PATCH v3 3/4] completion: fix multiple command removals

[Todd Zullinger]

From: Jeff King <peff@peff.net>

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.

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>
---
 help.c                | 4 ++--
 t/t9902-completion.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/help.c b/help.c
index fac7e421d0..a9e451f2ee 100644
--- a/help.c
+++ b/help.c
@@ -386,8 +386,8 @@ void list_cmds_by_config(struct string_list *list)
 		const char *p = strchrnul(cmd_list, ' ');
 
 		strbuf_add(&sb, cmd_list, p - cmd_list);
-		if (*cmd_list == '-')
-			string_list_remove(list, cmd_list + 1, 0);
+		if (sb.buf[0] == '-')
+			string_list_remove(list, sb.buf + 1, 0);
 		else
 			string_list_insert(list, sb.buf);
 		strbuf_release(&sb);
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3f5b420bf8..050fac4fff 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1483,7 +1483,7 @@ test_expect_success 'git --help completion' '
 	test_completion "git --help core" "core-tutorial "
 '
 
-test_expect_failure 'completion.commands removes multiple commands' '
+test_expect_success '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

[PATCH v3 4/4] completion: use __git when calling --list-cmds

[Todd Zullinger]

Since e51bdea6d3 ("git: read local config in --list-cmds", 2019-03-01),
the completion.commands variable respects repo-level configuration.  Use
__git which ensures that the proper repo config is consulted if the
command line contains 'git -C /some/other/repo'.

Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
 contrib/completion/git-completion.bash | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 499e56f83d..e505f04ff7 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1010,7 +1010,7 @@ __git_all_commands=
 __git_compute_all_commands ()
 {
 	test -n "$__git_all_commands" ||
-	__git_all_commands=$(git --list-cmds=main,others,alias,nohelpers)
+	__git_all_commands=$(__git --list-cmds=main,others,alias,nohelpers)
 }
 
 # Lists all set config variables starting with the given section prefix,
@@ -1620,9 +1620,9 @@ _git_help ()
 	esac
 	if test -n "$GIT_TESTING_ALL_COMMAND_LIST"
 	then
-		__gitcomp "$GIT_TESTING_ALL_COMMAND_LIST $(git --list-cmds=alias,list-guide) gitk"
+		__gitcomp "$GIT_TESTING_ALL_COMMAND_LIST $(__git --list-cmds=alias,list-guide) gitk"
 	else
-		__gitcomp "$(git --list-cmds=main,nohelpers,alias,list-guide) gitk"
+		__gitcomp "$(__git --list-cmds=main,nohelpers,alias,list-guide) gitk"
 	fi
 }
 
@@ -2888,7 +2888,7 @@ __git_main ()
 			then
 				__gitcomp "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
 			else
-				__gitcomp "$(git --list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)"
+				__gitcomp "$(__git --list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)"
 			fi
 			;;
 		esac

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

[Junio C Hamano]

Todd Zullinger <tmz@pobox.com> writes:

> Other than a follow-up to update the commit reference in 4/4
> after 1/4 is in the final form on pu, I think this might be good.
> If it's easier, we can skip 4/4 and I'll resend it after the
> others are on pu.

A series that makes a single topic should expect to be read by a
reader who understand the context of individual pieces in the topic,
so it is more common to refer to an earlier step of a series from a
later step without the object name.  I would have written the log
message like so instead:

    completion: use __git when calling --list-cmds

    As we made --list-cmds read the local configuration file in an
    earlier step, the completion.commands variable respects repo-level
    configuration.  Use __git which ensures that the proper repo config
    is consulted if the command line contains 'git -C /some/other/repo'.

The whole series looks good to me.  Thanks for working on it.

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

[Duy Nguyen]

On Thu, Mar 21, 2019 at 1:03 AM Todd Zullinger <tmz@pobox.com> wrote:
>
> Hi,
>
> Duy Nguyen wrote:
> > You probably want to drop the comment block about repository setup
> > inside list_cmds_by_config() too.
>
> You're right, of course.  Thanks Duy. :)
>
> That's the only change since v2.

The range-diff looks obviously correct. Thanks!
-- 
Duy

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

[Todd Zullinger]

Junio C Hamano wrote:
> Todd Zullinger <tmz@pobox.com> writes:
> 
>> Other than a follow-up to update the commit reference in 4/4
>> after 1/4 is in the final form on pu, I think this might be good.
>> If it's easier, we can skip 4/4 and I'll resend it after the
>> others are on pu.
> 
> A series that makes a single topic should expect to be read by a
> reader who understand the context of individual pieces in the topic,
> so it is more common to refer to an earlier step of a series from a
> later step without the object name.  I would have written the log
> message like so instead:
> 
>     completion: use __git when calling --list-cmds
> 
>     As we made --list-cmds read the local configuration file in an
>     earlier step, the completion.commands variable respects repo-level
>     configuration.  Use __git which ensures that the proper repo config
>     is consulted if the command line contains 'git -C /some/other/repo'.
> 
> The whole series looks good to me.  Thanks for working on it.

Thank you for amending the commit message, and to Jeff, Duy,
Eric, and Gábor for all the help.

-- 
Todd