* [BUG] completion.commands does not remove multiple commands @ 2019-02-28 22:31 Todd Zullinger 2019-02-28 23:05 ` Jeff King 2019-02-28 23:05 ` [BUG] completion.commands does not remove multiple commands SZEDER Gábor 0 siblings, 2 replies; 35+ messages in thread From: Todd Zullinger @ 2019-02-28 22:31 UTC (permalink / raw) To: git; +Cc: Nguyễn Thái Ngọc Duy 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< -- 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 && -- 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. -- Todd ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [BUG] completion.commands does not remove multiple commands 2019-02-28 22:31 [BUG] completion.commands does not remove multiple commands Todd Zullinger @ 2019-02-28 23:05 ` Jeff King 2019-03-01 17:34 ` Todd Zullinger ` (3 more replies) 2019-02-28 23:05 ` [BUG] completion.commands does not remove multiple commands SZEDER Gábor 1 sibling, 4 replies; 35+ messages in thread From: Jeff King @ 2019-02-28 23:05 UTC (permalink / raw) To: Todd Zullinger; +Cc: git, Nguyễn Thái Ngọc Duy 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 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [BUG] completion.commands does not remove multiple commands 2019-02-28 23:05 ` Jeff King @ 2019-03-01 17:34 ` Todd Zullinger 2019-03-01 18:30 ` Jeff King 2019-03-01 17:34 ` [PATCH 1/3] doc: note config file restrictions for completion.commands Todd Zullinger ` (2 subsequent siblings) 3 siblings, 1 reply; 35+ messages in thread From: Todd Zullinger @ 2019-03-01 17:34 UTC (permalink / raw) To: Jeff King; +Cc: git, Nguyễn Thái Ngọc Duy, SZEDER Gábor 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 -- Todd ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] completion.commands does not remove multiple commands 2019-03-01 17:34 ` Todd Zullinger @ 2019-03-01 18:30 ` Jeff King 2019-03-01 22:15 ` Todd Zullinger 0 siblings, 1 reply; 35+ messages in thread From: Jeff King @ 2019-03-01 18:30 UTC (permalink / raw) To: Todd Zullinger Cc: git, Nguyễn Thái Ngọc Duy, SZEDER Gábor 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) ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [BUG] completion.commands does not remove multiple commands 2019-03-01 18:30 ` Jeff King @ 2019-03-01 22:15 ` Todd Zullinger 2019-03-01 23:08 ` Jeff King 2019-03-02 1:18 ` Junio C Hamano 0 siblings, 2 replies; 35+ messages in thread From: Todd Zullinger @ 2019-03-01 22:15 UTC (permalink / raw) To: Jeff King Cc: git, Nguyễn Thái Ngọc Duy, SZEDER Gábor, Eric Sunshine 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< -- -- Todd ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [BUG] completion.commands does not remove multiple commands 2019-03-01 22:15 ` Todd Zullinger @ 2019-03-01 23:08 ` Jeff King 2019-03-02 1:08 ` Duy Nguyen 2019-03-02 1:18 ` Junio C Hamano 1 sibling, 1 reply; 35+ messages in thread From: Jeff King @ 2019-03-01 23:08 UTC (permalink / raw) To: Todd Zullinger Cc: git, Nguyễn Thái Ngọc Duy, SZEDER Gábor, Eric Sunshine 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] completion.commands does not remove multiple commands 2019-03-01 23:08 ` Jeff King @ 2019-03-02 1:08 ` Duy Nguyen 0 siblings, 0 replies; 35+ messages in thread From: Duy Nguyen @ 2019-03-02 1:08 UTC (permalink / raw) To: Jeff King Cc: Todd Zullinger, Git Mailing List, SZEDER Gábor, Eric Sunshine 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. -- Duy ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] completion.commands does not remove multiple commands 2019-03-01 22:15 ` Todd Zullinger 2019-03-01 23:08 ` Jeff King @ 2019-03-02 1:18 ` Junio C Hamano 2019-03-02 2:40 ` Todd Zullinger 1 sibling, 1 reply; 35+ messages in thread From: Junio C Hamano @ 2019-03-02 1:18 UTC (permalink / raw) To: Todd Zullinger Cc: Jeff King, git, Nguyễn Thái Ngọc Duy, SZEDER Gábor, Eric Sunshine 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. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] completion.commands does not remove multiple commands 2019-03-02 1:18 ` Junio C Hamano @ 2019-03-02 2:40 ` Todd Zullinger 2019-03-02 4:07 ` SZEDER Gábor 0 siblings, 1 reply; 35+ messages in thread From: Todd Zullinger @ 2019-03-02 2:40 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, git, Nguyễn Thái Ngọc Duy, SZEDER Gábor, Eric Sunshine 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, -- Todd ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] completion.commands does not remove multiple commands 2019-03-02 2:40 ` Todd Zullinger @ 2019-03-02 4:07 ` SZEDER Gábor 2019-03-03 1:34 ` Todd Zullinger 2019-03-03 17:06 ` Jeff King 0 siblings, 2 replies; 35+ messages in thread From: SZEDER Gábor @ 2019-03-02 4:07 UTC (permalink / raw) To: Todd Zullinger Cc: Junio C Hamano, Jeff King, git, Nguyễn Thái Ngọc Duy, Eric Sunshine 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. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] completion.commands does not remove multiple commands 2019-03-02 4:07 ` SZEDER Gábor @ 2019-03-03 1:34 ` Todd Zullinger 2019-03-03 17:06 ` Jeff King 1 sibling, 0 replies; 35+ messages in thread From: Todd Zullinger @ 2019-03-03 1:34 UTC (permalink / raw) To: SZEDER Gábor Cc: Junio C Hamano, Jeff King, git, Nguyễn Thái Ngọc Duy, Eric Sunshine 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, -- Todd ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] completion.commands does not remove multiple commands 2019-03-02 4:07 ` SZEDER Gábor 2019-03-03 1:34 ` Todd Zullinger @ 2019-03-03 17:06 ` Jeff King 1 sibling, 0 replies; 35+ messages in thread From: Jeff King @ 2019-03-03 17:06 UTC (permalink / raw) To: SZEDER Gábor Cc: Todd Zullinger, Junio C Hamano, git, Nguyễn Thái Ngọc Duy, Eric Sunshine 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/3] doc: note config file restrictions for completion.commands 2019-02-28 23:05 ` Jeff King 2019-03-01 17:34 ` Todd Zullinger @ 2019-03-01 17:34 ` Todd Zullinger 2019-03-17 13:12 ` Duy Nguyen 2019-03-01 17:34 ` [PATCH 2/3] t9902: test multiple removals via completion.commands Todd Zullinger 2019-03-01 17:34 ` [PATCH 3/3] completion: fix multiple command removals Todd Zullinger 3 siblings, 1 reply; 35+ messages in thread From: Todd Zullinger @ 2019-03-01 17:34 UTC (permalink / raw) To: Jeff King; +Cc: git, Nguyễn Thái Ngọc Duy, SZEDER Gábor The completion.commands config var must be set in either the system-wide or global config file. The completion script does not read a local repository config. Signed-off-by: Todd Zullinger <tmz@pobox.com> --- Documentation/config/completion.txt | 3 ++- Documentation/git.txt | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Documentation/config/completion.txt b/Documentation/config/completion.txt index 4d99bf33c9..4859d18e86 100644 --- a/Documentation/config/completion.txt +++ b/Documentation/config/completion.txt @@ -4,4 +4,5 @@ completion.commands:: porcelain commands and a few select others are completed. You can add more commands, separated by space, in this variable. Prefixing the command with '-' will remove it from - the existing list. + the existing list. The variable must be set in either the + system-wide or global config. diff --git a/Documentation/git.txt b/Documentation/git.txt index 00156d64aa..638f4d6cc9 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -172,7 +172,8 @@ foo.bar= ...`) sets `foo.bar` to the empty string which `git config others (all other commands in `$PATH` that have git- prefix), list-<category> (see categories in command-list.txt), nohelpers (exclude helper commands), alias and config - (retrieve command list from config variable completion.commands) + (retrieve command list from config variable completion.commands, + set in the global or system-wide config file) GIT COMMANDS ------------ ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 1/3] doc: note config file restrictions for completion.commands 2019-03-01 17:34 ` [PATCH 1/3] doc: note config file restrictions for completion.commands Todd Zullinger @ 2019-03-17 13:12 ` Duy Nguyen 2019-03-17 18:16 ` [PATCH v2 0/4] completion.commands: fix multiple command removals Todd Zullinger ` (4 more replies) 0 siblings, 5 replies; 35+ messages in thread From: Duy Nguyen @ 2019-03-17 13:12 UTC (permalink / raw) To: Todd Zullinger; +Cc: Jeff King, Git Mailing List, SZEDER Gábor Poking this thread before it goes completely dead... On Sat, Mar 2, 2019 at 12:34 AM Todd Zullinger <tmz@pobox.com> wrote: > > The completion.commands config var must be set in either the system-wide > or global config file. The completion script does not read a local > repository config. After the last discussion, I think the consensus was it's ok to allow per-repo config so we can just drop this and update the code to read from any config file, right? > > Signed-off-by: Todd Zullinger <tmz@pobox.com> > --- > Documentation/config/completion.txt | 3 ++- > Documentation/git.txt | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/Documentation/config/completion.txt b/Documentation/config/completion.txt > index 4d99bf33c9..4859d18e86 100644 > --- a/Documentation/config/completion.txt > +++ b/Documentation/config/completion.txt > @@ -4,4 +4,5 @@ completion.commands:: > porcelain commands and a few select others are completed. You > can add more commands, separated by space, in this > variable. Prefixing the command with '-' will remove it from > - the existing list. > + the existing list. The variable must be set in either the > + system-wide or global config. > diff --git a/Documentation/git.txt b/Documentation/git.txt > index 00156d64aa..638f4d6cc9 100644 > --- a/Documentation/git.txt > +++ b/Documentation/git.txt > @@ -172,7 +172,8 @@ foo.bar= ...`) sets `foo.bar` to the empty string which `git config > others (all other commands in `$PATH` that have git- prefix), > list-<category> (see categories in command-list.txt), > nohelpers (exclude helper commands), alias and config > - (retrieve command list from config variable completion.commands) > + (retrieve command list from config variable completion.commands, > + set in the global or system-wide config file) > > GIT COMMANDS > ------------ -- Duy ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 0/4] completion.commands: fix multiple command removals 2019-03-17 13:12 ` Duy Nguyen @ 2019-03-17 18:16 ` Todd Zullinger 2019-03-17 18:16 ` [PATCH v2 1/4] git: read local config in --list-cmds Todd Zullinger ` (3 subsequent siblings) 4 siblings, 0 replies; 35+ messages in thread From: Todd Zullinger @ 2019-03-17 18:16 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy Cc: git, Junio C Hamano, Jeff King, SZEDER Gábor, Eric Sunshine Hi, Duy Nguyen wrote: > Poking this thread before it goes completely dead... I've been meaning to send out a re-rolled version. I didn't realize 2 weeks had slipped by already. :/ > On Sat, Mar 2, 2019 at 12:34 AM Todd Zullinger <tmz@pobox.com> wrote: >> >> The completion.commands config var must be set in either the system-wide >> or global config file. The completion script does not read a local >> repository config. > > After the last discussion, I think the consensus was it's ok to allow > per-repo config so we can just drop this and update the code to read > from any config file, right? Yeah. Here's an updated series which I hope sums up the changes from the discussion. Changes since v1: - Change --list-commands to respect local config and use test_config rather than test_config_global - Avoid git upstream of pipes - Check that both cherry and mergetool are removed - Use __git in completion calls which use --list-cmds, to ensure the proper git repo config is checked with git -C /some/other/repo Jeff King (2): git: read local config in --list-cmds completion: fix multiple command removals Todd Zullinger (2): t9902: test multiple removals via completion.commands completion: use __git when calling --list-cmds contrib/completion/git-completion.bash | 8 ++++---- git.c | 7 +++++++ help.c | 4 ++-- t/t9902-completion.sh | 6 ++++++ 4 files changed, 19 insertions(+), 6 deletions(-) Range-diff against v1: 1: 385c596510 < -: ---------- doc: note config file restrictions for completion.commands -: ---------- > 1: e51bdea6d3 git: read local config in --list-cmds 2: ffa5ed9780 ! 2: 2f5e9da9de t9902: test multiple removals via completion.commands @@ -18,11 +18,9 @@ ' +test_expect_failure 'completion.commands removes multiple commands' ' -+ echo cherry-pick >expected && -+ test_config_global completion.commands "-cherry -mergetool" && -+ git --list-cmds=list-mainporcelain,list-complete,config | -+ grep ^cherry >actual && -+ test_cmp expected actual ++ test_config completion.commands "-cherry -mergetool" && ++ git --list-cmds=list-mainporcelain,list-complete,config >out && ++ ! grep -E "^(cherry|mergetool)$" out +' + test_expect_success 'setup for integration tests' ' 3: af029e908d ! 3: 7548dcc23f completion: fix multiple command removals @@ -2,16 +2,16 @@ completion: fix multiple command removals - 6532f3740b ("completion: allow to customize the completable - command list", 2018-05-20) added the completion.commands config - variable. + Commit 6532f3740b ("completion: allow to customize the completable + command list", 2018-05-20) tried to allow multiple space-separated + entries in completion.commands. To do this, it copies each parsed token + into a strbuf so that the result is NUL-terminated. - The documentation states multiple commands may be added, - separated by spaces. Adding multiple commands to remove fails, - only removing the last command in the config. - - Fix multiple command removals. + However, for tokens starting with "-", it accidentally passes the + original non-terminated string, meaning that only the final one worked. + Switch to using the strbuf. + Reported-by: Todd Zullinger <tmz@pobox.com> Signed-off-by: Jeff King <peff@peff.net> diff --git a/help.c b/help.c @@ -38,6 +38,6 @@ -test_expect_failure 'completion.commands removes multiple commands' ' +test_expect_success 'completion.commands removes multiple commands' ' - echo cherry-pick >expected && - test_config_global completion.commands "-cherry -mergetool" && - git --list-cmds=list-mainporcelain,list-complete,config | + test_config completion.commands "-cherry -mergetool" && + git --list-cmds=list-mainporcelain,list-complete,config >out && + ! grep -E "^(cherry|mergetool)$" out -: ---------- > 4: 26bef0b2af completion: use __git when calling --list-cmds ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 1/4] git: read local config in --list-cmds 2019-03-17 13:12 ` Duy Nguyen 2019-03-17 18:16 ` [PATCH v2 0/4] completion.commands: fix multiple command removals Todd Zullinger @ 2019-03-17 18:16 ` Todd Zullinger 2019-03-18 9:41 ` Duy Nguyen 2019-03-17 18:16 ` [PATCH v2 2/4] t9902: test multiple removals via completion.commands Todd Zullinger ` (2 subsequent siblings) 4 siblings, 1 reply; 35+ messages in thread From: Todd Zullinger @ 2019-03-17 18:16 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy Cc: Jeff King, git, Junio C Hamano, SZEDER Gábor, Eric Sunshine From: Jeff King <peff@peff.net> Normally code that is checking config before we've decided 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 which 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, ','); ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v2 1/4] git: read local config in --list-cmds 2019-03-17 18:16 ` [PATCH v2 1/4] git: read local config in --list-cmds Todd Zullinger @ 2019-03-18 9:41 ` Duy Nguyen 2019-03-20 18:03 ` [PATCH v3 0/4] completion.commands: fix multiple command removals Todd Zullinger ` (4 more replies) 0 siblings, 5 replies; 35+ messages in thread From: Duy Nguyen @ 2019-03-18 9:41 UTC (permalink / raw) To: Todd Zullinger Cc: Jeff King, Git Mailing List, Junio C Hamano, SZEDER Gábor, Eric Sunshine On Mon, Mar 18, 2019 at 1:16 AM Todd Zullinger <tmz@pobox.com> wrote: > > From: Jeff King <peff@peff.net> > > Normally code that is checking config before we've decided 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 which > 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); This gave me a pause because we could try to find .git more than necessary (e.g. when --list-cmds is requested without "config"). But I don't think that happens often enough to be worried about. It also subtly changes list_aliases() code flow, because the read_early_config() call inside will use the already-discovered gitdir here instead of trying to rediscover. So, everything is still fine. You probably want to drop the comment block about repository setup inside list_cmds_by_config() too. > > while (*spec) { > const char *sep = strchrnul(spec, ','); -- Duy ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 0/4] completion.commands: fix multiple command removals 2019-03-18 9:41 ` Duy Nguyen @ 2019-03-20 18:03 ` Todd Zullinger 2019-03-21 2:58 ` Junio C Hamano 2019-03-21 9:45 ` Duy Nguyen 2019-03-20 18:03 ` [PATCH v3 1/4] git: read local config in --list-cmds Todd Zullinger ` (3 subsequent siblings) 4 siblings, 2 replies; 35+ messages in thread From: Todd Zullinger @ 2019-03-20 18:03 UTC (permalink / raw) To: Duy Nguyen Cc: Jeff King, git, Junio C Hamano, SZEDER Gábor, Eric Sunshine Hi, Duy Nguyen wrote: > You probably want to drop the comment block about repository setup > inside list_cmds_by_config() too. You're right, of course. Thanks Duy. :) That's the only change since v2. Other than a follow-up to update the commit reference in 4/4 after 1/4 is in the final form on pu, I think this might be good. If it's easier, we can skip 4/4 and I'll resend it after the others are on pu. Jeff King (2): git: read local config in --list-cmds completion: fix multiple command removals Todd Zullinger (2): t9902: test multiple removals via completion.commands completion: use __git when calling --list-cmds contrib/completion/git-completion.bash | 8 ++++---- git.c | 7 +++++++ help.c | 11 ++--------- t/t9902-completion.sh | 6 ++++++ 4 files changed, 19 insertions(+), 13 deletions(-) Range-diff against v2: 1: e51bdea6d3 ! 1: 6e9872b0e3 git: read local config in --list-cmds @@ -33,3 +33,21 @@ while (*spec) { const char *sep = strchrnul(spec, ','); + + diff --git a/help.c b/help.c + --- a/help.c + +++ b/help.c +@@ + { + const char *cmd_list; + +- /* +- * 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. +- */ + if (git_config_get_string_const("completion.commands", &cmd_list)) + return; + 2: 2f5e9da9de = 2: 6873ae3868 t9902: test multiple removals via completion.commands 3: 7548dcc23f = 3: f66bbc0b55 completion: fix multiple command removals 4: 26bef0b2af = 4: 197b176483 completion: use __git when calling --list-cmds ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 0/4] completion.commands: fix multiple command removals 2019-03-20 18:03 ` [PATCH v3 0/4] completion.commands: fix multiple command removals Todd Zullinger @ 2019-03-21 2:58 ` Junio C Hamano 2019-03-21 17:18 ` Todd Zullinger 2019-03-21 9:45 ` Duy Nguyen 1 sibling, 1 reply; 35+ messages in thread From: Junio C Hamano @ 2019-03-21 2:58 UTC (permalink / raw) To: Todd Zullinger Cc: Duy Nguyen, Jeff King, git, SZEDER Gábor, Eric Sunshine Todd Zullinger <tmz@pobox.com> writes: > Other than a follow-up to update the commit reference in 4/4 > after 1/4 is in the final form on pu, I think this might be good. > If it's easier, we can skip 4/4 and I'll resend it after the > others are on pu. A series that makes a single topic should expect to be read by a reader who understand the context of individual pieces in the topic, so it is more common to refer to an earlier step of a series from a later step without the object name. I would have written the log message like so instead: completion: use __git when calling --list-cmds As we made --list-cmds read the local configuration file in an earlier step, the completion.commands variable respects repo-level configuration. Use __git which ensures that the proper repo config is consulted if the command line contains 'git -C /some/other/repo'. The whole series looks good to me. Thanks for working on it. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 0/4] completion.commands: fix multiple command removals 2019-03-21 2:58 ` Junio C Hamano @ 2019-03-21 17:18 ` Todd Zullinger 0 siblings, 0 replies; 35+ messages in thread From: Todd Zullinger @ 2019-03-21 17:18 UTC (permalink / raw) To: Junio C Hamano Cc: Duy Nguyen, Jeff King, git, SZEDER Gábor, Eric Sunshine Junio C Hamano wrote: > Todd Zullinger <tmz@pobox.com> writes: > >> Other than a follow-up to update the commit reference in 4/4 >> after 1/4 is in the final form on pu, I think this might be good. >> If it's easier, we can skip 4/4 and I'll resend it after the >> others are on pu. > > A series that makes a single topic should expect to be read by a > reader who understand the context of individual pieces in the topic, > so it is more common to refer to an earlier step of a series from a > later step without the object name. I would have written the log > message like so instead: > > completion: use __git when calling --list-cmds > > As we made --list-cmds read the local configuration file in an > earlier step, the completion.commands variable respects repo-level > configuration. Use __git which ensures that the proper repo config > is consulted if the command line contains 'git -C /some/other/repo'. > > The whole series looks good to me. Thanks for working on it. Thank you for amending the commit message, and to Jeff, Duy, Eric, and Gábor for all the help. -- Todd ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 0/4] completion.commands: fix multiple command removals 2019-03-20 18:03 ` [PATCH v3 0/4] completion.commands: fix multiple command removals Todd Zullinger 2019-03-21 2:58 ` Junio C Hamano @ 2019-03-21 9:45 ` Duy Nguyen 1 sibling, 0 replies; 35+ messages in thread From: Duy Nguyen @ 2019-03-21 9:45 UTC (permalink / raw) To: Todd Zullinger Cc: Jeff King, Git Mailing List, Junio C Hamano, SZEDER Gábor, Eric Sunshine On Thu, Mar 21, 2019 at 1:03 AM Todd Zullinger <tmz@pobox.com> wrote: > > Hi, > > Duy Nguyen wrote: > > You probably want to drop the comment block about repository setup > > inside list_cmds_by_config() too. > > You're right, of course. Thanks Duy. :) > > That's the only change since v2. The range-diff looks obviously correct. Thanks! -- Duy ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 1/4] git: read local config in --list-cmds 2019-03-18 9:41 ` Duy Nguyen 2019-03-20 18:03 ` [PATCH v3 0/4] completion.commands: fix multiple command removals Todd Zullinger @ 2019-03-20 18:03 ` Todd Zullinger 2019-03-20 18:03 ` [PATCH v3 2/4] t9902: test multiple removals via completion.commands Todd Zullinger ` (2 subsequent siblings) 4 siblings, 0 replies; 35+ messages in thread From: Todd Zullinger @ 2019-03-20 18:03 UTC (permalink / raw) To: Duy Nguyen Cc: Jeff King, git, Junio C Hamano, SZEDER Gábor, Eric Sunshine From: Jeff King <peff@peff.net> Normally code that is checking config before we've decided 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 which 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 +++++++ help.c | 7 ------- 2 files changed, 7 insertions(+), 7 deletions(-) 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, ','); diff --git a/help.c b/help.c index 520c9080e8..fac7e421d0 100644 --- a/help.c +++ b/help.c @@ -375,13 +375,6 @@ void list_cmds_by_config(struct string_list *list) { const char *cmd_list; - /* - * 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. - */ if (git_config_get_string_const("completion.commands", &cmd_list)) return; ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 2/4] t9902: test multiple removals via completion.commands 2019-03-18 9:41 ` Duy Nguyen 2019-03-20 18:03 ` [PATCH v3 0/4] completion.commands: fix multiple command removals Todd Zullinger 2019-03-20 18:03 ` [PATCH v3 1/4] git: read local config in --list-cmds Todd Zullinger @ 2019-03-20 18:03 ` Todd Zullinger 2019-03-20 18:03 ` [PATCH v3 3/4] completion: fix multiple command removals Todd Zullinger 2019-03-20 18:03 ` [PATCH v3 4/4] completion: use __git when calling --list-cmds Todd Zullinger 4 siblings, 0 replies; 35+ messages in thread From: Todd Zullinger @ 2019-03-20 18:03 UTC (permalink / raw) To: Duy Nguyen Cc: Jeff King, git, Junio C Hamano, SZEDER Gábor, Eric Sunshine 6532f3740b ("completion: allow to customize the completable command list", 2018-05-20) added the completion.commands config variable. Multiple commands may be added or removed, separated by a space. Demonstrate the failure of multiple removals. Signed-off-by: Todd Zullinger <tmz@pobox.com> --- t/t9902-completion.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 3a2c6326d8..3f5b420bf8 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -1483,6 +1483,12 @@ 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 >out && + ! grep -E "^(cherry|mergetool)$" out +' + test_expect_success 'setup for integration tests' ' echo content >file1 && echo more >file2 && ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 3/4] completion: fix multiple command removals 2019-03-18 9:41 ` Duy Nguyen ` (2 preceding siblings ...) 2019-03-20 18:03 ` [PATCH v3 2/4] t9902: test multiple removals via completion.commands Todd Zullinger @ 2019-03-20 18:03 ` Todd Zullinger 2019-03-20 18:03 ` [PATCH v3 4/4] completion: use __git when calling --list-cmds Todd Zullinger 4 siblings, 0 replies; 35+ messages in thread From: Todd Zullinger @ 2019-03-20 18:03 UTC (permalink / raw) To: Duy Nguyen Cc: Jeff King, git, Junio C Hamano, SZEDER Gábor, Eric Sunshine From: Jeff King <peff@peff.net> Commit 6532f3740b ("completion: allow to customize the completable command list", 2018-05-20) tried to allow multiple space-separated entries in completion.commands. To do this, it copies each parsed token into a strbuf so that the result is NUL-terminated. However, for tokens starting with "-", it accidentally passes the original non-terminated string, meaning that only the final one worked. Switch to using the strbuf. Reported-by: Todd Zullinger <tmz@pobox.com> Signed-off-by: Jeff King <peff@peff.net> --- help.c | 4 ++-- t/t9902-completion.sh | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/help.c b/help.c index fac7e421d0..a9e451f2ee 100644 --- a/help.c +++ b/help.c @@ -386,8 +386,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); diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 3f5b420bf8..050fac4fff 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -1483,7 +1483,7 @@ test_expect_success 'git --help completion' ' test_completion "git --help core" "core-tutorial " ' -test_expect_failure 'completion.commands removes multiple commands' ' +test_expect_success 'completion.commands removes multiple commands' ' test_config completion.commands "-cherry -mergetool" && git --list-cmds=list-mainporcelain,list-complete,config >out && ! grep -E "^(cherry|mergetool)$" out ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 4/4] completion: use __git when calling --list-cmds 2019-03-18 9:41 ` Duy Nguyen ` (3 preceding siblings ...) 2019-03-20 18:03 ` [PATCH v3 3/4] completion: fix multiple command removals Todd Zullinger @ 2019-03-20 18:03 ` Todd Zullinger 4 siblings, 0 replies; 35+ messages in thread From: Todd Zullinger @ 2019-03-20 18:03 UTC (permalink / raw) To: Duy Nguyen Cc: Jeff King, git, Junio C Hamano, SZEDER Gábor, Eric Sunshine Since e51bdea6d3 ("git: read local config in --list-cmds", 2019-03-01), the completion.commands variable respects repo-level configuration. Use __git which ensures that the proper repo config is consulted if the command line contains 'git -C /some/other/repo'. Suggested-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Todd Zullinger <tmz@pobox.com> --- contrib/completion/git-completion.bash | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 499e56f83d..e505f04ff7 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1010,7 +1010,7 @@ __git_all_commands= __git_compute_all_commands () { test -n "$__git_all_commands" || - __git_all_commands=$(git --list-cmds=main,others,alias,nohelpers) + __git_all_commands=$(__git --list-cmds=main,others,alias,nohelpers) } # Lists all set config variables starting with the given section prefix, @@ -1620,9 +1620,9 @@ _git_help () esac if test -n "$GIT_TESTING_ALL_COMMAND_LIST" then - __gitcomp "$GIT_TESTING_ALL_COMMAND_LIST $(git --list-cmds=alias,list-guide) gitk" + __gitcomp "$GIT_TESTING_ALL_COMMAND_LIST $(__git --list-cmds=alias,list-guide) gitk" else - __gitcomp "$(git --list-cmds=main,nohelpers,alias,list-guide) gitk" + __gitcomp "$(__git --list-cmds=main,nohelpers,alias,list-guide) gitk" fi } @@ -2888,7 +2888,7 @@ __git_main () then __gitcomp "$GIT_TESTING_PORCELAIN_COMMAND_LIST" else - __gitcomp "$(git --list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)" + __gitcomp "$(__git --list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)" fi ;; esac ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 2/4] t9902: test multiple removals via completion.commands 2019-03-17 13:12 ` Duy Nguyen 2019-03-17 18:16 ` [PATCH v2 0/4] completion.commands: fix multiple command removals Todd Zullinger 2019-03-17 18:16 ` [PATCH v2 1/4] git: read local config in --list-cmds Todd Zullinger @ 2019-03-17 18:16 ` Todd Zullinger 2019-03-17 18:16 ` [PATCH v2 3/4] completion: fix multiple command removals Todd Zullinger 2019-03-17 18:16 ` [PATCH v2 4/4] completion: use __git when calling --list-cmds Todd Zullinger 4 siblings, 0 replies; 35+ messages in thread From: Todd Zullinger @ 2019-03-17 18:16 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy Cc: git, Junio C Hamano, Jeff King, SZEDER Gábor, Eric Sunshine 6532f3740b ("completion: allow to customize the completable command list", 2018-05-20) added the completion.commands config variable. Multiple commands may be added or removed, separated by a space. Demonstrate the failure of multiple removals. Signed-off-by: Todd Zullinger <tmz@pobox.com> --- t/t9902-completion.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 3a2c6326d8..3f5b420bf8 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -1483,6 +1483,12 @@ 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 >out && + ! grep -E "^(cherry|mergetool)$" out +' + test_expect_success 'setup for integration tests' ' echo content >file1 && echo more >file2 && ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 3/4] completion: fix multiple command removals 2019-03-17 13:12 ` Duy Nguyen ` (2 preceding siblings ...) 2019-03-17 18:16 ` [PATCH v2 2/4] t9902: test multiple removals via completion.commands Todd Zullinger @ 2019-03-17 18:16 ` Todd Zullinger 2019-03-17 18:16 ` [PATCH v2 4/4] completion: use __git when calling --list-cmds Todd Zullinger 4 siblings, 0 replies; 35+ messages in thread From: Todd Zullinger @ 2019-03-17 18:16 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy Cc: Jeff King, git, Junio C Hamano, SZEDER Gábor, Eric Sunshine From: Jeff King <peff@peff.net> Commit 6532f3740b ("completion: allow to customize the completable command list", 2018-05-20) tried to allow multiple space-separated entries in completion.commands. To do this, it copies each parsed token into a strbuf so that the result is NUL-terminated. However, for tokens starting with "-", it accidentally passes the original non-terminated string, meaning that only the final one worked. Switch to using the strbuf. Reported-by: Todd Zullinger <tmz@pobox.com> Signed-off-by: Jeff King <peff@peff.net> --- help.c | 4 ++-- t/t9902-completion.sh | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) 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); diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 3f5b420bf8..050fac4fff 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -1483,7 +1483,7 @@ test_expect_success 'git --help completion' ' test_completion "git --help core" "core-tutorial " ' -test_expect_failure 'completion.commands removes multiple commands' ' +test_expect_success 'completion.commands removes multiple commands' ' test_config completion.commands "-cherry -mergetool" && git --list-cmds=list-mainporcelain,list-complete,config >out && ! grep -E "^(cherry|mergetool)$" out ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 4/4] completion: use __git when calling --list-cmds 2019-03-17 13:12 ` Duy Nguyen ` (3 preceding siblings ...) 2019-03-17 18:16 ` [PATCH v2 3/4] completion: fix multiple command removals Todd Zullinger @ 2019-03-17 18:16 ` Todd Zullinger 4 siblings, 0 replies; 35+ messages in thread From: Todd Zullinger @ 2019-03-17 18:16 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy Cc: git, Junio C Hamano, Jeff King, SZEDER Gábor, Eric Sunshine Since e51bdea6d3 ("git: read local config in --list-cmds", 2019-03-01), the completion.commands variable respects repo-level configuration. Use __git which ensures that the proper repo config is consulted if the command line contains 'git -C /some/other/repo'. Suggested-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Todd Zullinger <tmz@pobox.com> --- Junio, I referenc the commit id for "git: read local config in --list-cmds" which is earlier in this series, so the id will, of course, differ when you apply it. Let me know if you'd prefer this commit to be dropped and resent once the others in the series are applied or if it's easy for you to adjust when it's queued. Also, as I wrote in an earlier reply, at the moment, I think using __git only matters for calls where config is in the --list-cmds list. But since Jeff's fix affects all --list-cmds calls, it seems prudent to adjust the 3 other uses of --list-cmds in the completion script. contrib/completion/git-completion.bash | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 499e56f83d..e505f04ff7 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1010,7 +1010,7 @@ __git_all_commands= __git_compute_all_commands () { test -n "$__git_all_commands" || - __git_all_commands=$(git --list-cmds=main,others,alias,nohelpers) + __git_all_commands=$(__git --list-cmds=main,others,alias,nohelpers) } # Lists all set config variables starting with the given section prefix, @@ -1620,9 +1620,9 @@ _git_help () esac if test -n "$GIT_TESTING_ALL_COMMAND_LIST" then - __gitcomp "$GIT_TESTING_ALL_COMMAND_LIST $(git --list-cmds=alias,list-guide) gitk" + __gitcomp "$GIT_TESTING_ALL_COMMAND_LIST $(__git --list-cmds=alias,list-guide) gitk" else - __gitcomp "$(git --list-cmds=main,nohelpers,alias,list-guide) gitk" + __gitcomp "$(__git --list-cmds=main,nohelpers,alias,list-guide) gitk" fi } @@ -2888,7 +2888,7 @@ __git_main () then __gitcomp "$GIT_TESTING_PORCELAIN_COMMAND_LIST" else - __gitcomp "$(git --list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)" + __gitcomp "$(__git --list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)" fi ;; esac ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 2/3] t9902: test multiple removals via completion.commands 2019-02-28 23:05 ` Jeff King 2019-03-01 17:34 ` Todd Zullinger 2019-03-01 17:34 ` [PATCH 1/3] doc: note config file restrictions for completion.commands Todd Zullinger @ 2019-03-01 17:34 ` Todd Zullinger 2019-03-01 18:22 ` Eric Sunshine 2019-03-01 17:34 ` [PATCH 3/3] completion: fix multiple command removals Todd Zullinger 3 siblings, 1 reply; 35+ messages in thread From: Todd Zullinger @ 2019-03-01 17:34 UTC (permalink / raw) To: Jeff King; +Cc: git, Nguyễn Thái Ngọc Duy, SZEDER Gábor 6532f3740b ("completion: allow to customize the completable command list", 2018-05-20) added the completion.commands config variable. Multiple commands may be added or removed, separated by a space. Demonstrate the failure of multiple removals. Signed-off-by: Todd Zullinger <tmz@pobox.com> --- t/t9902-completion.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 3a2c6326d8..dd11bb660d 100755 --- a/t/t9902-completion.sh +++ b/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' ' + echo cherry-pick >expected && + test_config_global completion.commands "-cherry -mergetool" && + git --list-cmds=list-mainporcelain,list-complete,config | + grep ^cherry >actual && + test_cmp expected actual +' + test_expect_success 'setup for integration tests' ' echo content >file1 && echo more >file2 && ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] t9902: test multiple removals via completion.commands 2019-03-01 17:34 ` [PATCH 2/3] t9902: test multiple removals via completion.commands Todd Zullinger @ 2019-03-01 18:22 ` Eric Sunshine 2019-03-01 20:50 ` SZEDER Gábor 0 siblings, 1 reply; 35+ messages in thread From: Eric Sunshine @ 2019-03-01 18:22 UTC (permalink / raw) To: Todd Zullinger Cc: Jeff King, Git List, Nguyễn Thái Ngọc Duy, SZEDER Gábor On Fri, Mar 1, 2019 at 12:35 PM Todd Zullinger <tmz@pobox.com> wrote: > 6532f3740b ("completion: allow to customize the completable command > list", 2018-05-20) added the completion.commands config variable. > Multiple commands may be added or removed, separated by a space. > > Demonstrate the failure of multiple removals. > > Signed-off-by: Todd Zullinger <tmz@pobox.com> > --- > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > @@ -1483,6 +1483,14 @@ test_expect_success 'git --help completion' ' > +test_expect_failure 'completion.commands removes multiple commands' ' > + echo cherry-pick >expected && > + test_config_global completion.commands "-cherry -mergetool" && > + git --list-cmds=list-mainporcelain,list-complete,config | > + grep ^cherry >actual && > + test_cmp expected actual > +' We normally avoid placing a Git command upstream of a pipe. Instead, the Git command output would be redirected to a file and then the file grep'd. However, in this case, you might consider simplifying the test like this: test_expect_failure 'completion.commands removes multiple commands' ' test_config_global completion.commands "-cherry -mergetool" && git --list-cmds=list-mainporcelain,list-complete,config >actual && grep cherry-pick actual ' ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] t9902: test multiple removals via completion.commands 2019-03-01 18:22 ` Eric Sunshine @ 2019-03-01 20:50 ` SZEDER Gábor 2019-03-01 21:56 ` Todd Zullinger 0 siblings, 1 reply; 35+ messages in thread From: SZEDER Gábor @ 2019-03-01 20:50 UTC (permalink / raw) To: Eric Sunshine Cc: Todd Zullinger, Jeff King, Git List, Nguyễn Thái Ngọc Duy On Fri, Mar 01, 2019 at 01:22:52PM -0500, Eric Sunshine wrote: > On Fri, Mar 1, 2019 at 12:35 PM Todd Zullinger <tmz@pobox.com> wrote: > > 6532f3740b ("completion: allow to customize the completable command > > list", 2018-05-20) added the completion.commands config variable. > > Multiple commands may be added or removed, separated by a space. > > > > Demonstrate the failure of multiple removals. > > > > Signed-off-by: Todd Zullinger <tmz@pobox.com> > > --- > > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > > @@ -1483,6 +1483,14 @@ test_expect_success 'git --help completion' ' > > +test_expect_failure 'completion.commands removes multiple commands' ' > > + echo cherry-pick >expected && > > + test_config_global completion.commands "-cherry -mergetool" && > > + git --list-cmds=list-mainporcelain,list-complete,config | > > + grep ^cherry >actual && > > + test_cmp expected actual > > +' > > We normally avoid placing a Git command upstream of a pipe. Instead, > the Git command output would be redirected to a file and then the file > grep'd. Indeed. > However, in this case, you might consider simplifying the test > like this: > > test_expect_failure 'completion.commands removes multiple commands' ' > test_config_global completion.commands "-cherry -mergetool" && > git --list-cmds=list-mainporcelain,list-complete,config >actual && > grep cherry-pick actual This wouldn't check what we want. The point is that the command 'cherry' is not listed, so it should be '! grep cherry$ actual'. Furthermore, I think it would be prudent to check that 'mergetool' is not listed, either. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] t9902: test multiple removals via completion.commands 2019-03-01 20:50 ` SZEDER Gábor @ 2019-03-01 21:56 ` Todd Zullinger 0 siblings, 0 replies; 35+ messages in thread From: Todd Zullinger @ 2019-03-01 21:56 UTC (permalink / raw) To: SZEDER Gábor Cc: Eric Sunshine, Jeff King, Git List, Nguyễn Thái Ngọc Duy SZEDER Gábor wrote: > On Fri, Mar 01, 2019 at 01:22:52PM -0500, Eric Sunshine wrote: >> On Fri, Mar 1, 2019 at 12:35 PM Todd Zullinger <tmz@pobox.com> wrote: >>> 6532f3740b ("completion: allow to customize the completable command >>> list", 2018-05-20) added the completion.commands config variable. >>> Multiple commands may be added or removed, separated by a space. >>> >>> Demonstrate the failure of multiple removals. >>> >>> Signed-off-by: Todd Zullinger <tmz@pobox.com> >>> --- >>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh >>> @@ -1483,6 +1483,14 @@ test_expect_success 'git --help completion' ' >>> +test_expect_failure 'completion.commands removes multiple commands' ' >>> + echo cherry-pick >expected && >>> + test_config_global completion.commands "-cherry -mergetool" && >>> + git --list-cmds=list-mainporcelain,list-complete,config | >>> + grep ^cherry >actual && >>> + test_cmp expected actual >>> +' >> >> We normally avoid placing a Git command upstream of a pipe. Instead, >> the Git command output would be redirected to a file and then the file >> grep'd. > > Indeed. Yes, that's a good point. And one I should have known from reading it here more than once. Thanks to both of you. >> However, in this case, you might consider simplifying the test >> like this: >> >> test_expect_failure 'completion.commands removes multiple commands' ' >> test_config_global completion.commands "-cherry -mergetool" && >> git --list-cmds=list-mainporcelain,list-complete,config >actual && >> grep cherry-pick actual > > This wouldn't check what we want. The point is that the command > 'cherry' is not listed, so it should be '! grep cherry$ actual'. Heh, I had written a similar reply already, but then I got side-tracked for a bit... I think the grep needs to be negated, e.g.: ! grep ^cherry$ actual Otherwise it succeeds without the fix, as cherry-pick is expected to be in the --list-cmds output. (If we remove the 'expected' file, it might also make sense to rename 'actual' to 'out' perhaps.) > Furthermore, I think it would be prudent to check that 'mergetool' is > not listed, either. True. With the simplified test, that's easy enough to add and makes the test description more accurate and the test more thorough. I'll adjust it like so when I re-send: test_expect_failure 'completion.commands removes multiple commands' ' test_config completion.commands "-cherry -mergetool" && git --list-cmds=list-mainporcelain,list-complete,config >out && ! grep -E "^(cherry|mergetool)$" out (Using test_config depends on setup_git_directory_gently() in list_cmds(), which Jeff suggested elsewhere in the thread.) Thanks! -- Todd ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 3/3] completion: fix multiple command removals 2019-02-28 23:05 ` Jeff King ` (2 preceding siblings ...) 2019-03-01 17:34 ` [PATCH 2/3] t9902: test multiple removals via completion.commands Todd Zullinger @ 2019-03-01 17:34 ` Todd Zullinger 2019-03-01 18:16 ` Jeff King 3 siblings, 1 reply; 35+ messages in thread From: Todd Zullinger @ 2019-03-01 17:34 UTC (permalink / raw) To: Jeff King; +Cc: git, Nguyễn Thái Ngọc Duy, SZEDER Gábor From: Jeff King <peff@peff.net> 6532f3740b ("completion: allow to customize the completable command list", 2018-05-20) added the completion.commands config variable. The documentation states multiple commands may be added, separated by spaces. Adding multiple commands to remove fails, only removing the last command in the config. Fix multiple command removals. Signed-off-by: Jeff King <peff@peff.net> --- Jeff, The commit message could probably be worded better, particularly since it's forged in your name. help.c | 4 ++-- t/t9902-completion.sh | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) 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); diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index dd11bb660d..d7daa1ca92 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -1483,7 +1483,7 @@ test_expect_success 'git --help completion' ' test_completion "git --help core" "core-tutorial " ' -test_expect_failure 'completion.commands removes multiple commands' ' +test_expect_success 'completion.commands removes multiple commands' ' echo cherry-pick >expected && test_config_global completion.commands "-cherry -mergetool" && git --list-cmds=list-mainporcelain,list-complete,config | ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 3/3] completion: fix multiple command removals 2019-03-01 17:34 ` [PATCH 3/3] completion: fix multiple command removals Todd Zullinger @ 2019-03-01 18:16 ` Jeff King 0 siblings, 0 replies; 35+ messages in thread From: Jeff King @ 2019-03-01 18:16 UTC (permalink / raw) To: Todd Zullinger Cc: git, Nguyễn Thái Ngọc Duy, SZEDER Gábor On Fri, Mar 01, 2019 at 12:34:43PM -0500, Todd Zullinger wrote: > The commit message could probably be worded better, particularly since it's > forged in your name. What you have is OK, but I'd have probably written it like this: -- >8 -- Subject: [PATCH] completion: fix multiple command removals Commit 6532f3740b ("completion: allow to customize the completable command list", 2018-05-20) tried to allow multiple space-separated entries in completion.commands. To do this, it copies each parsed token into a strbuf so that the result is NUL-terminated. However, for tokens starting with "-", it accidentally passes the original non-terminated string, meaning that only the final one worked. Switch to using the strbuf. Reported-by: Todd Zullinger <tmz@pobox.com> Signed-off-by: Jeff King <peff@peff.net> --- help.c | 4 ++-- t/t9902-completion.sh | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) 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); diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index dd11bb660d..d7daa1ca92 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -1483,7 +1483,7 @@ test_expect_success 'git --help completion' ' test_completion "git --help core" "core-tutorial " ' -test_expect_failure 'completion.commands removes multiple commands' ' +test_expect_success 'completion.commands removes multiple commands' ' echo cherry-pick >expected && test_config_global completion.commands "-cherry -mergetool" && git --list-cmds=list-mainporcelain,list-complete,config | -- 2.21.0.675.g01c085a870 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [BUG] completion.commands does not remove multiple commands 2019-02-28 22:31 [BUG] completion.commands does not remove multiple commands Todd Zullinger 2019-02-28 23:05 ` Jeff King @ 2019-02-28 23:05 ` SZEDER Gábor 1 sibling, 0 replies; 35+ messages in thread From: SZEDER Gábor @ 2019-02-28 23:05 UTC (permalink / raw) To: Todd Zullinger; +Cc: git, Nguyễn Thái Ngọc Duy 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); ^ permalink raw reply related [flat|nested] 35+ messages in thread
end of thread, other threads:[~2019-03-21 17:18 UTC | newest] Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-28 22:31 [BUG] completion.commands does not remove multiple commands Todd Zullinger 2019-02-28 23:05 ` Jeff King 2019-03-01 17:34 ` Todd Zullinger 2019-03-01 18:30 ` Jeff King 2019-03-01 22:15 ` Todd Zullinger 2019-03-01 23:08 ` Jeff King 2019-03-02 1:08 ` Duy Nguyen 2019-03-02 1:18 ` Junio C Hamano 2019-03-02 2:40 ` Todd Zullinger 2019-03-02 4:07 ` SZEDER Gábor 2019-03-03 1:34 ` Todd Zullinger 2019-03-03 17:06 ` Jeff King 2019-03-01 17:34 ` [PATCH 1/3] doc: note config file restrictions for completion.commands Todd Zullinger 2019-03-17 13:12 ` Duy Nguyen 2019-03-17 18:16 ` [PATCH v2 0/4] completion.commands: fix multiple command removals Todd Zullinger 2019-03-17 18:16 ` [PATCH v2 1/4] git: read local config in --list-cmds Todd Zullinger 2019-03-18 9:41 ` Duy Nguyen 2019-03-20 18:03 ` [PATCH v3 0/4] completion.commands: fix multiple command removals Todd Zullinger 2019-03-21 2:58 ` Junio C Hamano 2019-03-21 17:18 ` Todd Zullinger 2019-03-21 9:45 ` Duy Nguyen 2019-03-20 18:03 ` [PATCH v3 1/4] git: read local config in --list-cmds Todd Zullinger 2019-03-20 18:03 ` [PATCH v3 2/4] t9902: test multiple removals via completion.commands Todd Zullinger 2019-03-20 18:03 ` [PATCH v3 3/4] completion: fix multiple command removals Todd Zullinger 2019-03-20 18:03 ` [PATCH v3 4/4] completion: use __git when calling --list-cmds Todd Zullinger 2019-03-17 18:16 ` [PATCH v2 2/4] t9902: test multiple removals via completion.commands Todd Zullinger 2019-03-17 18:16 ` [PATCH v2 3/4] completion: fix multiple command removals Todd Zullinger 2019-03-17 18:16 ` [PATCH v2 4/4] completion: use __git when calling --list-cmds Todd Zullinger 2019-03-01 17:34 ` [PATCH 2/3] t9902: test multiple removals via completion.commands Todd Zullinger 2019-03-01 18:22 ` Eric Sunshine 2019-03-01 20:50 ` SZEDER Gábor 2019-03-01 21:56 ` Todd Zullinger 2019-03-01 17:34 ` [PATCH 3/3] completion: fix multiple command removals Todd Zullinger 2019-03-01 18:16 ` Jeff King 2019-02-28 23:05 ` [BUG] completion.commands does not remove multiple commands SZEDER Gábor
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).