Re: [BUG] completion.commands does not remove multiple commands
- Date: Fri, 1 Mar 2019 18:08:21 -0500
- From: Jeff King <peff@xxxxxxxx>
- Subject: Re: [BUG] completion.commands does not remove multiple commands
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
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
> 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@xxxxxxxx>
> 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.
I'd say "mechanism _which_ rightly does not use read_early_config..." to
make it clear we're talking about configset, not list_cmds().