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@xxxxxxxx> > 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
