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

From: Jeff King <peff@peff.net>
Date: Mar 1, 2019, 23:08 UTC
Message-ID: <20190301230821.GA16294@sigill.intra.peff.net>
In-Reply-To: <20190301221551.GD31362@zaya.teonanacatl.net>

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

Show 10 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 7 quoted lines

> 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