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

[Jeff King <peff@peff.net>]

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)