Jeff King wrote:
> 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@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.
Call setup_git_directory_gently() so we can pick up repo-level
config (like completion.commands).
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
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
