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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Mar 2, 2019, 04:07 UTC
Message-ID: <20190302040704.GN19739@szeder.dev>
In-Reply-To: <20190302024011.GF31362@zaya.teonanacatl.net>

On Fri, Mar 01, 2019 at 09:40:12PM -0500, Todd Zullinger wrote:

Show 28 quoted lines

> Hi,
> 
> Junio C Hamano wrote:
> > Todd Zullinger <tmz@pobox.com> writes:
> > 
> >> 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.
> >>          */
> > 
> > Doesn't the output from list-cmds-by-config get cached at the
> > completion script layer in $__git_blah variables, and the cached
> > values are not cleared when you chdir around?
> 
> In testing, I didn't find any evidence of caching.  Setting
> commands to be added and removed in the global and local
> configs worked reasonably.
> 
> Duy's reply suggests that was considered but not
> implemented.  I there were caching (and if it were tedious
> for the completion code to keep fresh between repos), then
> it would a bad plan to allow per-repo config.

The completion script used to cache the list of porcelain commands, but then Duy came along and removed it in 3301d36b29 (completion: add and use --list-cmds=alias, 2018-05-20).

We cached the commands, because it was a bit involved to get them: first we run 'git help -a' to list all commands, then extracted the command names from the help output with 'grep', and finally an enormous case statement removed all plumbing commands. Caching spared us the overhead of these external processes and maybe 2-3 subshells. Note that because of this caching if you dropped a third-party 'git-foo' command in your $PATH, it didn't show up in completion until you re-sourced the completion script, which was the standard way to invalidate/refresh the caches.

However, even when we cached porcelain commands, we didn't cache aliases, even though getting them is a bit involved as well, requiring running 'git config', two subshells and a shell loop. I presume that back then the reason for not caching aliases was that they could be repo-specific.

Now, ever since Duy revamped commands completion, we only run a simple $(git --list-cmds=...), i.e. a git command and a subshell takes care of all that. IMO this is the best we ever had, because it uses one less subshell than before to list both commands and aliases, and it lists everything afresh, including third-party 'git-foo' commands.

I don't see the benefit of bringing back caching. Yes, on one hand we could complete commands with a single variable expansion, which is clearly faster than that $(git --list-cmds=...). OTOH, that's only commands, but what about aliases? If we cache aliases, too, then that could be considered a regression by those who do have repo-specific aliases; if we don't, then we are back at running 'git config' and two subshells, i.e. one subshell more than we currently have.

And if we won't cache commands, then we might as well modify list_cmds_by_config() to read 'completion.commands' from the repo-specific config, too. I'm fairly neutral on this, because I can only imagine rather convoluted scenarios where a repo-specific command list would be useful, but at least it would make it consistent with aliases, which we read from the current repo's config and we always have.

Note, however, that for completeness sake, if we choose to update list_cmds_by_config() to read the repo's config as well, then we should also update the completion script to run $(__git --list-cmds=...), note the two leading underscores, so in case of 'git -C over/there <TAB>' it would read 'completion.commands' from the right repository.