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

From: Todd Zullinger <tmz@pobox.com>
Date: Mar 1, 2019, 22:15 UTC
Message-ID: <20190301221551.GD31362@zaya.teonanacatl.net>
In-Reply-To: <20190301183017.GB30847@sigill.intra.peff.net>

Jeff King wrote:

Show 44 quoted lines

> 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