| Message ID | 20190228223123.GZ16125@zaya.teonanacatl.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [BUG] completion.commands does not remove multiple commands | expand |
On Thu, Feb 28, 2019 at 05:31:23PM -0500, Todd Zullinger wrote: > Hi, > > I was playing with the completion.commands config added in > 6532f3740b ("completion: allow to customize the completable > command list", 2018-05-20) and noticed an issue removing > multiple commands. > > I wanted to remove completion for cherry and mergetool, so I > added them both to the config: > > git config completion.commands "-cherry -mergetool" > > But git still completes cherry in this case, only removing > mergetool. Swapping the items in the config yields the > opposite result (cherry is removed and mergetool is not). I can reproduce your problem, though the test you included passes for me even with the current tip of master. I suspect this is the problem: diff --git a/help.c b/help.c index 520c9080e8..026f881715 100644 --- a/help.c +++ b/help.c @@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list) const char *p = strchrnul(cmd_list, ' '); strbuf_add(&sb, cmd_list, p - cmd_list); - if (*cmd_list == '-') - string_list_remove(list, cmd_list + 1, 0); + if (sb.buf[0] == '-') + string_list_remove(list, sb.buf + 1, 0); else string_list_insert(list, sb.buf); strbuf_release(&sb); though I can't help but wonder if the whole thing would be simpler using string_list_split(). -Peff
On Thu, Feb 28, 2019 at 05:31:23PM -0500, Todd Zullinger wrote: > I was playing with the completion.commands config added in > 6532f3740b ("completion: allow to customize the completable > command list", 2018-05-20) and noticed an issue removing > multiple commands. > > I wanted to remove completion for cherry and mergetool, so I > added them both to the config: > > git config completion.commands "-cherry -mergetool" > > But git still completes cherry in this case, only removing > mergetool. Swapping the items in the config yields the > opposite result (cherry is removed and mergetool is not). > > With luck this will be a clear and easily resolved issue in > list_cmds_by_config() (in help.c). Indeed, this seems to fix it for me: diff --git a/help.c b/help.c index 520c9080e8..f2c6f0c9f7 100644 --- a/help.c +++ b/help.c @@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list) const char *p = strchrnul(cmd_list, ' '); strbuf_add(&sb, cmd_list, p - cmd_list); - if (*cmd_list == '-') - string_list_remove(list, cmd_list + 1, 0); + if (*sb.buf == '-') + string_list_remove(list, sb.buf + 1, 0); else string_list_insert(list, sb.buf); strbuf_release(&sb);
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 suspect this is the problem: > > diff --git a/help.c b/help.c > index 520c9080e8..026f881715 100644 > --- a/help.c > +++ b/help.c > @@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list) > const char *p = strchrnul(cmd_list, ' '); > > strbuf_add(&sb, cmd_list, p - cmd_list); > - if (*cmd_list == '-') > - string_list_remove(list, cmd_list + 1, 0); > + if (sb.buf[0] == '-') > + string_list_remove(list, sb.buf + 1, 0); > else > string_list_insert(list, sb.buf); > strbuf_release(&sb); > > though I can't help but wonder if the whole thing would be simpler using > string_list_split(). That does indeed fix things (as does SZEDER's similar version, which arrived only a few seconds later). Thanks to both of you for the very quick replies! I'll leave it to those who know better to follow up with a change to use string_list_split(). Here's a first stab at improving the docs and fixing the bug. Jeff King (1): completion: fix multiple command removals Todd Zullinger (2): doc: note config file restrictions for completion.commands t9902: test multiple removals via completion.commands Documentation/config/completion.txt | 3 ++- Documentation/git.txt | 3 ++- help.c | 4 ++-- t/t9902-completion.sh | 8 ++++++++ 4 files changed, 14 insertions(+), 4 deletions(-) Published-as: https://github.com/tmzullinger/git/releases/tag/completion.commands-v1
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, ','); > I'll leave it to those who know better to follow up with a change to > use string_list_split(). It's less of an improvement than I'd hoped, since most of the code is actually handling the "-" thing. So I don't think it's really worth the churn. But here's what it looks like for reference: --- help.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/help.c b/help.c index 026f881715..9e4d258cb8 100644 --- a/help.c +++ b/help.c @@ -373,7 +373,9 @@ void list_cmds_by_category(struct string_list *list, void list_cmds_by_config(struct string_list *list) { - const char *cmd_list; + const char *cmd_str; + struct string_list cmd_list = STRING_LIST_INIT_DUP; + int i; /* * There's no actual repository setup at this point (and even @@ -382,26 +384,22 @@ void list_cmds_by_config(struct string_list *list) * too since the caller (git --list-cmds=) should exit shortly * anyway. */ - if (git_config_get_string_const("completion.commands", &cmd_list)) + if (git_config_get_string_const("completion.commands", &cmd_str)) return; string_list_sort(list); string_list_remove_duplicates(list, 0); - while (*cmd_list) { - struct strbuf sb = STRBUF_INIT; - const char *p = strchrnul(cmd_list, ' '); + string_list_split(&cmd_list, cmd_str, ' ', -1); + for (i = 0; i < cmd_list.nr; i++) { + const char *cmd = cmd_list.items[0].string; - strbuf_add(&sb, cmd_list, p - cmd_list); - if (sb.buf[0] == '-') - string_list_remove(list, sb.buf + 1, 0); + if (*cmd == '-') + string_list_remove(list, cmd + 1, 0); else - string_list_insert(list, sb.buf); - strbuf_release(&sb); - while (*p == ' ') - p++; - cmd_list = p; + string_list_insert(list, cmd); } + string_list_clear(&cmd_list, 0); } void list_common_guides_help(void)
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@peff.net> 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@peff.net> --- 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< --
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@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/ > 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
On Sat, Mar 2, 2019 at 6:08 AM Jeff King <peff@peff.net> wrote: > > 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 vaguely recall that I wanted to cache the results in git-completion.bash at some point. But looking at that script I don't think there's any caching. So yes it should be ok to read per-repo config as well (and adjust or drop this comment block) > 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). It sounds to me confusing that you want different command sets in different repos. Which is why I made it global config only. But I suspect that people who have per-repo aliases may find this natural. So yeah, no objection. PS. and thanks for the bug fix.
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? If you allowed the repo-specific configuration to affect its output, the cached values need to be reset when you cross repository boundaries. Otherwise you'd see complaints like "I have this in repo A but not in repo B; when I start from repo A, it gets completed even after I go to repo B. If I start from repo B, I do not get completion in either of them" (the former is because repo-A specific result gets cached, the latter is because the cache is populated with the result taken in repo-B that doesn't have customization and stays around even when you visit repo-B). I think it is a sensible design decision to forbid per-repo config to relieve us from having to worry about when to invalidate the cache and all associated complexities.
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. If there was a goal of adding such caching it might also make sense to avoid "fixing" the code here to allow per-repo config before it's known how that might affect such caching. It sounds like that's not something Duy is planning on for the near term though, so perhaps we're fine to allow local repo config here? As Duy mentioned, maybe some users with local aliases want to add them to the completion locally as well. If we choose to avoid local repo config then we can add a comment to the documetation like I had in 2/3. Maybe also update the comment in list_cmds_by_config() to note that we intentionally don't setup a repo -- or a similar comment in list_cmds(), where Jeff's 1/3 was adding setup_git_directory_gently(). I don't have a strong opinion either way. I more or less have the minor patches for either direction at this point. Thanks,
On Fri, Mar 01, 2019 at 09:40:12PM -0500, Todd Zullinger wrote: > 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.
SZEDER Gábor wrote: [... lots of good history ...] Thanks for an excellent summary! > 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. Thanks for pointing this out. I'll add that to my local branch for the "respect local config" case. At the moment, I think it only matters for calls where config is in the --list-cmds list. But since the fix Jeff suggested affects all --list-cmds calls, it seems prudent to adjust the 3-4 other uses of --list-cmds in the completion script. Let me know if you see a reason not to do that. Thanks again for such a nice summary,
On Sat, Mar 02, 2019 at 05:07:04AM +0100, SZEDER Gábor wrote: > 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). > [...] Thanks for this summary. Just for the record, as the person who suggested that we should respect repo config here, I don't personally care all that much either way. I do not plan to use the feature myself, but it was just what I would have expected to happen. So I can live with it either way. That said, it seems like if we were to go back to caching, we'd need to handle directory changes in order for aliases (which already do respect repo config) to be correct anyway. So it doesn't seem like this is introducing any burden that was not there already. -Peff
diff --git c/t/t9902-completion.sh w/t/t9902-completion.sh index 3a2c6326d8..06cee36ae5 100755 --- c/t/t9902-completion.sh +++ w/t/t9902-completion.sh @@ -1483,6 +1483,14 @@ test_expect_success 'git --help completion' ' test_completion "git --help core" "core-tutorial " ' +test_expect_failure 'completion.commands removes multiple commands' ' + test_config completion.commands "-cherry -mergetool" && + git --list-cmds=list-mainporcelain,list-complete,config | + grep ^cherry >actual && + echo cherry-pick >expected && + test_cmp expected actual +' + test_expect_success 'setup for integration tests' ' echo content >file1 && echo more >file2 &&
Hi, I was playing with the completion.commands config added in 6532f3740b ("completion: allow to customize the completable command list", 2018-05-20) and noticed an issue removing multiple commands. I wanted to remove completion for cherry and mergetool, so I added them both to the config: git config completion.commands "-cherry -mergetool" But git still completes cherry in this case, only removing mergetool. Swapping the items in the config yields the opposite result (cherry is removed and mergetool is not). With luck this will be a clear and easily resolved issue in list_cmds_by_config() (in help.c). Here's an example in test form: -- 8< -- -- 8< -- That's not quite normal form for t9902, but I was undable to create a working test using the test_completion functions. The tests set GIT_TESTING_PORCELAIN_COMMAND_LIST and GIT_TESTING_ALL_COMMAND_LIST which override the settings in the completion script. I played a bit with adjusting those to add cherry{,-pick} to the command lists, unsetting those vars for the test, and such, to no avail. In the end, I'm not really sure that calling --list-cmds directly is such a bad way to go for testing this issue.