{"thread":{"id":"50619","subject":"[BUG] completion.commands does not remove multiple commands","startedAt":"2019-02-28T22:31:33Z","lastAt":"2019-03-21T17:18:46Z","messageCount":35,"participants":["Todd Zullinger","Jeff King","SZEDER Gábor","Eric Sunshine","Duy Nguyen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"370393","messageId":"20190228223123.GZ16125@zaya.teonanacatl.net","threadId":"50619","inReplyTo":null,"subject":"[BUG] completion.commands does not remove multiple commands","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-02-28T22:31:23Z","receivedAt":"2019-02-28T22:31:33Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi,\n\nI was playing with the completion.commands config added in\n6532f3740b (\"completion: allow to customize the completable\ncommand list\", 2018-05-20) and noticed an issue removing\nmultiple commands.\n\nI wanted to remove completion for cherry and mergetool, so I\nadded them both to the config:\n\n git config completion.commands \"-cherry -mergetool\"\n\nBut git still completes cherry in this case, only removing\nmergetool. Swapping the items in the config yields the\nopposite result (cherry is removed and mergetool is not).\n\nWith luck this will be a clear and easily resolved issue in\nlist_cmds_by_config() (in help.c).\n\nHere's an example in test form:\n\n-- 8< --\ndiff --git c/t/t9902-completion.sh w/t/t9902-completion.sh\nindex 3a2c6326d8..06cee36ae5 100755\n--- c/t/t9902-completion.sh\n+++ w/t/t9902-completion.sh\n@@ -1483,6 +1483,14 @@ test_expect_success 'git --help completion' '\n \ttest_completion \"git --help core\" \"core-tutorial \"\n '\n \n+test_expect_failure 'completion.commands removes multiple commands' '\n+\ttest_config completion.commands \"-cherry -mergetool\" &&\n+\tgit --list-cmds=list-mainporcelain,list-complete,config |\n+\t\tgrep ^cherry >actual &&\n+\techo cherry-pick >expected &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'setup for integration tests' '\n \techo content >file1 &&\n \techo more >file2 &&\n-- 8< --\n\nThat's not quite normal form for t9902, but I was undable to\ncreate a working test using the test_completion functions.\nThe tests set GIT_TESTING_PORCELAIN_COMMAND_LIST and\nGIT_TESTING_ALL_COMMAND_LIST which override the settings in\nthe completion script. I played a bit with adjusting those\nto add cherry{,-pick} to the command lists, unsetting those\nvars for the test, and such, to no avail.\n\nIn the end, I'm not really sure that calling --list-cmds\ndirectly is such a bad way to go for testing this issue.\n\n-- \nTodd\n"},{"id":"370394","messageId":"20190228230506.GA20625@sigill.intra.peff.net","threadId":"50619","inReplyTo":"20190228223123.GZ16125@zaya.teonanacatl.net","subject":"Re: [BUG] completion.commands does not remove multiple commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-28T23:05:06Z","receivedAt":"2019-02-28T23:05:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 28, 2019 at 05:31:23PM -0500, Todd Zullinger wrote:\n\n> Hi,\n> \n> I was playing with the completion.commands config added in\n> 6532f3740b (\"completion: allow to customize the completable\n> command list\", 2018-05-20) and noticed an issue removing\n> multiple commands.\n> \n> I wanted to remove completion for cherry and mergetool, so I\n> added them both to the config:\n> \n> git config completion.commands \"-cherry -mergetool\"\n> \n> But git still completes cherry in this case, only removing\n> mergetool. Swapping the items in the config yields the\n> opposite result (cherry is removed and mergetool is not).\n\nI can reproduce your problem, though the test you included passes for me\neven with the current tip of master. I suspect this is the problem:\n\ndiff --git a/help.c b/help.c\nindex 520c9080e8..026f881715 100644\n--- a/help.c\n+++ b/help.c\n@@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list)\n \t\tconst char *p = strchrnul(cmd_list, ' ');\n \n \t\tstrbuf_add(&sb, cmd_list, p - cmd_list);\n-\t\tif (*cmd_list == '-')\n-\t\t\tstring_list_remove(list, cmd_list + 1, 0);\n+\t\tif (sb.buf[0] == '-')\n+\t\t\tstring_list_remove(list, sb.buf + 1, 0);\n \t\telse\n \t\t\tstring_list_insert(list, sb.buf);\n \t\tstrbuf_release(&sb);\n\nthough I can't help but wonder if the whole thing would be simpler using\nstring_list_split().\n\n-Peff\n"},{"id":"370395","messageId":"20190228230517.GL19739@szeder.dev","threadId":"50619","inReplyTo":"20190228223123.GZ16125@zaya.teonanacatl.net","subject":"Re: [BUG] completion.commands does not remove multiple commands","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-02-28T23:05:17Z","receivedAt":"2019-02-28T23:05:24Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Feb 28, 2019 at 05:31:23PM -0500, Todd Zullinger wrote:\n> I was playing with the completion.commands config added in\n> 6532f3740b (\"completion: allow to customize the completable\n> command list\", 2018-05-20) and noticed an issue removing\n> multiple commands.\n> \n> I wanted to remove completion for cherry and mergetool, so I\n> added them both to the config:\n> \n> git config completion.commands \"-cherry -mergetool\"\n> \n> But git still completes cherry in this case, only removing\n> mergetool. Swapping the items in the config yields the\n> opposite result (cherry is removed and mergetool is not).\n> \n> With luck this will be a clear and easily resolved issue in\n> list_cmds_by_config() (in help.c).\n\nIndeed, this seems to fix it for me:\n\ndiff --git a/help.c b/help.c\nindex 520c9080e8..f2c6f0c9f7 100644\n--- a/help.c\n+++ b/help.c\n@@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list)\n \t\tconst char *p = strchrnul(cmd_list, ' ');\n \n \t\tstrbuf_add(&sb, cmd_list, p - cmd_list);\n-\t\tif (*cmd_list == '-')\n-\t\t\tstring_list_remove(list, cmd_list + 1, 0);\n+\t\tif (*sb.buf == '-')\n+\t\t\tstring_list_remove(list, sb.buf + 1, 0);\n \t\telse\n \t\t\tstring_list_insert(list, sb.buf);\n \t\tstrbuf_release(&sb);\n\n\n"},{"id":"370423","messageId":"20190301173443.16429-1-tmz@pobox.com","threadId":"50619","inReplyTo":"20190228230506.GA20625@sigill.intra.peff.net","subject":"Re: [BUG] completion.commands does not remove multiple commands","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-01T17:34:40Z","receivedAt":"2019-03-01T17:34:56Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Jeff King wrote:\n> I can reproduce your problem, though the test you included passes for me\n> even with the current tip of master.\n\nOh, hrm. I think the issue is that completion.commands needs to be\nset in the global (or system-wide) config, via test_config_global\nrather than the local repo config which test_config sets.\n\nIn hindsight, that seems obvious. But it's probably worth noting\nthat where completion.commands is documented, for anyone who might\nspend a few cycles trying to configure it on a per-repo basis before\nrealizing it doesn't work.\n\n> I suspect this is the problem:\n> \n> diff --git a/help.c b/help.c\n> index 520c9080e8..026f881715 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list)\n> \t\tconst char *p = strchrnul(cmd_list, ' ');\n> \n> \t\tstrbuf_add(&sb, cmd_list, p - cmd_list);\n> -\t\tif (*cmd_list == '-')\n> -\t\t\tstring_list_remove(list, cmd_list + 1, 0);\n> +\t\tif (sb.buf[0] == '-')\n> +\t\t\tstring_list_remove(list, sb.buf + 1, 0);\n> \t\telse\n> \t\t\tstring_list_insert(list, sb.buf);\n> \t\tstrbuf_release(&sb);\n> \n> though I can't help but wonder if the whole thing would be simpler using\n> string_list_split().\n\nThat does indeed fix things (as does SZEDER's similar version, which\narrived only a few seconds later). Thanks to both of you for the\nvery quick replies!\n\nI'll leave it to those who know better to follow up with a change to\nuse string_list_split().\n\nHere's a first stab at improving the docs and fixing the bug.\n\nJeff King (1):\n completion: fix multiple command removals\n\nTodd Zullinger (2):\n doc: note config file restrictions for completion.commands\n t9902: test multiple removals via completion.commands\n\n Documentation/config/completion.txt | 3 ++-\n Documentation/git.txt | 3 ++-\n help.c | 4 ++--\n t/t9902-completion.sh | 8 ++++++++\n 4 files changed, 14 insertions(+), 4 deletions(-)\n\nPublished-as: https://github.com/tmzullinger/git/releases/tag/completion.commands-v1\n\n-- \nTodd\n"},{"id":"370424","messageId":"20190301173443.16429-2-tmz@pobox.com","threadId":"50619","inReplyTo":"20190228230506.GA20625@sigill.intra.peff.net","subject":"[PATCH 1/3] doc: note config file restrictions for completion.commands","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-01T17:34:41Z","receivedAt":"2019-03-01T17:34:56Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"The completion.commands config var must be set in either the system-wide\nor global config file. The completion script does not read a local\nrepository config.\n\nSigned-off-by: Todd Zullinger \n---\n Documentation/config/completion.txt | 3 ++-\n Documentation/git.txt | 3 ++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/completion.txt b/Documentation/config/completion.txt\nindex 4d99bf33c9..4859d18e86 100644\n--- a/Documentation/config/completion.txt\n+++ b/Documentation/config/completion.txt\n@@ -4,4 +4,5 @@ completion.commands::\n \tporcelain commands and a few select others are completed. You\n \tcan add more commands, separated by space, in this\n \tvariable. Prefixing the command with '-' will remove it from\n-\tthe existing list.\n+\tthe existing list. The variable must be set in either the\n+\tsystem-wide or global config.\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 00156d64aa..638f4d6cc9 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -172,7 +172,8 @@ foo.bar= ...`) sets `foo.bar` to the empty string which `git config\n \tothers (all other commands in `$PATH` that have git- prefix),\n \tlist- (see categories in command-list.txt),\n \tnohelpers (exclude helper commands), alias and config\n-\t(retrieve command list from config variable completion.commands)\n+\t(retrieve command list from config variable completion.commands,\n+\tset in the global or system-wide config file)\n \n GIT COMMANDS\n ------------\n"},{"id":"370425","messageId":"20190301173443.16429-3-tmz@pobox.com","threadId":"50619","inReplyTo":"20190228230506.GA20625@sigill.intra.peff.net","subject":"[PATCH 2/3] t9902: test multiple removals via completion.commands","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-01T17:34:42Z","receivedAt":"2019-03-01T17:35:00Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"6532f3740b (\"completion: allow to customize the completable command\nlist\", 2018-05-20) added the completion.commands config variable.\nMultiple commands may be added or removed, separated by a space.\n\nDemonstrate the failure of multiple removals.\n\nSigned-off-by: Todd Zullinger \n---\n t/t9902-completion.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 3a2c6326d8..dd11bb660d 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1483,6 +1483,14 @@ test_expect_success 'git --help completion' '\n \ttest_completion \"git --help core\" \"core-tutorial \"\n '\n \n+test_expect_failure 'completion.commands removes multiple commands' '\n+\techo cherry-pick >expected &&\n+\ttest_config_global completion.commands \"-cherry -mergetool\" &&\n+\tgit --list-cmds=list-mainporcelain,list-complete,config |\n+\t\tgrep ^cherry >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'setup for integration tests' '\n \techo content >file1 &&\n \techo more >file2 &&\n"},{"id":"370426","messageId":"20190301173443.16429-4-tmz@pobox.com","threadId":"50619","inReplyTo":"20190228230506.GA20625@sigill.intra.peff.net","subject":"[PATCH 3/3] completion: fix multiple command removals","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-01T17:34:43Z","receivedAt":"2019-03-01T17:35:03Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"From: Jeff King \n\n6532f3740b (\"completion: allow to customize the completable\ncommand list\", 2018-05-20) added the completion.commands config\nvariable.\n\nThe documentation states multiple commands may be added,\nseparated by spaces. Adding multiple commands to remove fails,\nonly removing the last command in the config.\n\nFix multiple command removals.\n\nSigned-off-by: Jeff King \n---\nJeff,\n\nThe commit message could probably be worded better, particularly since it's\nforged in your name.\n\n help.c | 4 ++--\n t/t9902-completion.sh | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex 520c9080e8..026f881715 100644\n--- a/help.c\n+++ b/help.c\n@@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list)\n \t\tconst char *p = strchrnul(cmd_list, ' ');\n \n \t\tstrbuf_add(&sb, cmd_list, p - cmd_list);\n-\t\tif (*cmd_list == '-')\n-\t\t\tstring_list_remove(list, cmd_list + 1, 0);\n+\t\tif (sb.buf[0] == '-')\n+\t\t\tstring_list_remove(list, sb.buf + 1, 0);\n \t\telse\n \t\t\tstring_list_insert(list, sb.buf);\n \t\tstrbuf_release(&sb);\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex dd11bb660d..d7daa1ca92 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1483,7 +1483,7 @@ test_expect_success 'git --help completion' '\n \ttest_completion \"git --help core\" \"core-tutorial \"\n '\n \n-test_expect_failure 'completion.commands removes multiple commands' '\n+test_expect_success 'completion.commands removes multiple commands' '\n \techo cherry-pick >expected &&\n \ttest_config_global completion.commands \"-cherry -mergetool\" &&\n \tgit --list-cmds=list-mainporcelain,list-complete,config |\n"},{"id":"370435","messageId":"20190301181647.GA30847@sigill.intra.peff.net","threadId":"50619","inReplyTo":"20190301173443.16429-4-tmz@pobox.com","subject":"Re: [PATCH 3/3] completion: fix multiple command removals","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-01T18:16:47Z","receivedAt":"2019-03-01T18:16:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 01, 2019 at 12:34:43PM -0500, Todd Zullinger wrote:\n\n> The commit message could probably be worded better, particularly since it's\n> forged in your name.\n\nWhat you have is OK, but I'd have probably written it like this:\n\n-- >8 --\nSubject: [PATCH] completion: fix multiple command removals\n\nCommit 6532f3740b (\"completion: allow to customize the completable\ncommand list\", 2018-05-20) tried to allow multiple space-separated\nentries in completion.commands. To do this, it copies each parsed token\ninto a strbuf so that the result is NUL-terminated.\n\nHowever, for tokens starting with \"-\", it accidentally passes the\noriginal non-terminated string, meaning that only the final one worked.\nSwitch to using the strbuf.\n\nReported-by: Todd Zullinger \nSigned-off-by: Jeff King \n---\n help.c | 4 ++--\n t/t9902-completion.sh | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex 520c9080e8..026f881715 100644\n--- a/help.c\n+++ b/help.c\n@@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list)\n \t\tconst char *p = strchrnul(cmd_list, ' ');\n \n \t\tstrbuf_add(&sb, cmd_list, p - cmd_list);\n-\t\tif (*cmd_list == '-')\n-\t\t\tstring_list_remove(list, cmd_list + 1, 0);\n+\t\tif (sb.buf[0] == '-')\n+\t\t\tstring_list_remove(list, sb.buf + 1, 0);\n \t\telse\n \t\t\tstring_list_insert(list, sb.buf);\n \t\tstrbuf_release(&sb);\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex dd11bb660d..d7daa1ca92 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1483,7 +1483,7 @@ test_expect_success 'git --help completion' '\n \ttest_completion \"git --help core\" \"core-tutorial \"\n '\n \n-test_expect_failure 'completion.commands removes multiple commands' '\n+test_expect_success 'completion.commands removes multiple commands' '\n \techo cherry-pick >expected &&\n \ttest_config_global completion.commands \"-cherry -mergetool\" &&\n \tgit --list-cmds=list-mainporcelain,list-complete,config |\n-- \n2.21.0.675.g01c085a870\n\n"},{"id":"370437","messageId":"CAPig+cTQG+yLGs1Ur2awhLyrpSWj0QyrDpuxCnMNOqc2bBz3pQ@mail.gmail.com","threadId":"50619","inReplyTo":"20190301173443.16429-3-tmz@pobox.com","subject":"Re: [PATCH 2/3] t9902: test multiple removals via completion.commands","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-01T18:22:52Z","receivedAt":"2019-03-01T18:23:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 1, 2019 at 12:35 PM Todd Zullinger wrote:\n> 6532f3740b (\"completion: allow to customize the completable command\n> list\", 2018-05-20) added the completion.commands config variable.\n> Multiple commands may be added or removed, separated by a space.\n>\n> Demonstrate the failure of multiple removals.\n>\n> Signed-off-by: Todd Zullinger \n> ---\n> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> @@ -1483,6 +1483,14 @@ test_expect_success 'git --help completion' '\n> +test_expect_failure 'completion.commands removes multiple commands' '\n> + echo cherry-pick >expected &&\n> + test_config_global completion.commands \"-cherry -mergetool\" &&\n> + git --list-cmds=list-mainporcelain,list-complete,config |\n> + grep ^cherry >actual &&\n> + test_cmp expected actual\n> +'\n\nWe normally avoid placing a Git command upstream of a pipe. Instead,\nthe Git command output would be redirected to a file and then the file\ngrep'd. However, in this case, you might consider simplifying the test\nlike this:\n\n test_expect_failure 'completion.commands removes multiple commands' '\n test_config_global completion.commands \"-cherry -mergetool\" &&\n git --list-cmds=list-mainporcelain,list-complete,config >actual &&\n grep cherry-pick actual\n '\n"},{"id":"370438","messageId":"20190301183017.GB30847@sigill.intra.peff.net","threadId":"50619","inReplyTo":"20190301173443.16429-1-tmz@pobox.com","subject":"Re: [BUG] completion.commands does not remove multiple commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-01T18:30:17Z","receivedAt":"2019-03-01T18:30:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 01, 2019 at 12:34:40PM -0500, Todd Zullinger wrote:\n\n> Jeff King wrote:\n> > I can reproduce your problem, though the test you included passes for me\n> > even with the current tip of master.\n> \n> Oh, hrm. I think the issue is that completion.commands needs to be\n> set in the global (or system-wide) config, via test_config_global\n> rather than the local repo config which test_config sets.\n> \n> In hindsight, that seems obvious. But it's probably worth noting\n> that where completion.commands is documented, for anyone who might\n> spend a few cycles trying to configure it on a per-repo basis before\n> realizing it doesn't work.\n\nI think this is actually a bug. Normally code that is checking config\nbefore we've decide to do setup_git_directory() would use\nread_early_config(), which uses discover_git_directory() to tentatively\nsee if we're in a repo, and if so to add it to the config sequence.\n\nBut this code uses the caching configset mechanism. And that code\n(rightly) does not use read_early_config(), because it has no idea if\nit's being called early or what.\n\nI think we probably ought to be doing something like this:\n\ndiff --git a/git.c b/git.c\nindex 2dd588674f..ba3690245e 100644\n--- a/git.c\n+++ b/git.c\n@@ -62,6 +62,13 @@ static int list_cmds(const char *spec)\n {\n \tstruct string_list list = STRING_LIST_INIT_DUP;\n \tint i;\n+\tint nongit;\n+\n+\t/*\n+\t * Set up the repository so we can pick up any repo-level config (like\n+\t * completion.commands).\n+\t */\n+\tsetup_git_directory_gently(&nongit);\n \n \twhile (*spec) {\n \t\tconst char *sep = strchrnul(spec, ',');\n\n> I'll leave it to those who know better to follow up with a change to\n> use string_list_split().\n\nIt's less of an improvement than I'd hoped, since most of the code is\nactually handling the \"-\" thing. So I don't think it's really worth the\nchurn. But here's what it looks like for reference:\n\n---\n help.c | 24 +++++++++++-------------\n 1 file changed, 11 insertions(+), 13 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex 026f881715..9e4d258cb8 100644\n--- a/help.c\n+++ b/help.c\n@@ -373,7 +373,9 @@ void list_cmds_by_category(struct string_list *list,\n \n void list_cmds_by_config(struct string_list *list)\n {\n-\tconst char *cmd_list;\n+\tconst char *cmd_str;\n+\tstruct string_list cmd_list = STRING_LIST_INIT_DUP;\n+\tint i;\n \n \t/*\n \t * There's no actual repository setup at this point (and even\n@@ -382,26 +384,22 @@ void list_cmds_by_config(struct string_list *list)\n \t * too since the caller (git --list-cmds=) should exit shortly\n \t * anyway.\n \t */\n-\tif (git_config_get_string_const(\"completion.commands\", &cmd_list))\n+\tif (git_config_get_string_const(\"completion.commands\", &cmd_str))\n \t\treturn;\n \n \tstring_list_sort(list);\n \tstring_list_remove_duplicates(list, 0);\n \n-\twhile (*cmd_list) {\n-\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\tconst char *p = strchrnul(cmd_list, ' ');\n+\tstring_list_split(&cmd_list, cmd_str, ' ', -1);\n+\tfor (i = 0; i < cmd_list.nr; i++) {\n+\t\tconst char *cmd = cmd_list.items[0].string;\n \n-\t\tstrbuf_add(&sb, cmd_list, p - cmd_list);\n-\t\tif (sb.buf[0] == '-')\n-\t\t\tstring_list_remove(list, sb.buf + 1, 0);\n+\t\tif (*cmd == '-')\n+\t\t\tstring_list_remove(list, cmd + 1, 0);\n \t\telse\n-\t\t\tstring_list_insert(list, sb.buf);\n-\t\tstrbuf_release(&sb);\n-\t\twhile (*p == ' ')\n-\t\t\tp++;\n-\t\tcmd_list = p;\n+\t\t\tstring_list_insert(list, cmd);\n \t}\n+\tstring_list_clear(&cmd_list, 0);\n }\n \n void list_common_guides_help(void)\n"},{"id":"370448","messageId":"20190301205041.GM19739@szeder.dev","threadId":"50619","inReplyTo":"CAPig+cTQG+yLGs1Ur2awhLyrpSWj0QyrDpuxCnMNOqc2bBz3pQ@mail.gmail.com","subject":"Re: [PATCH 2/3] t9902: test multiple removals via completion.commands","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-01T20:50:41Z","receivedAt":"2019-03-01T20:50:48Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Mar 01, 2019 at 01:22:52PM -0500, Eric Sunshine wrote:\n> On Fri, Mar 1, 2019 at 12:35 PM Todd Zullinger wrote:\n> > 6532f3740b (\"completion: allow to customize the completable command\n> > list\", 2018-05-20) added the completion.commands config variable.\n> > Multiple commands may be added or removed, separated by a space.\n> >\n> > Demonstrate the failure of multiple removals.\n> >\n> > Signed-off-by: Todd Zullinger \n> > ---\n> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> > @@ -1483,6 +1483,14 @@ test_expect_success 'git --help completion' '\n> > +test_expect_failure 'completion.commands removes multiple commands' '\n> > + echo cherry-pick >expected &&\n> > + test_config_global completion.commands \"-cherry -mergetool\" &&\n> > + git --list-cmds=list-mainporcelain,list-complete,config |\n> > + grep ^cherry >actual &&\n> > + test_cmp expected actual\n> > +'\n> \n> We normally avoid placing a Git command upstream of a pipe. Instead,\n> the Git command output would be redirected to a file and then the file\n> grep'd.\n\nIndeed.\n\n> However, in this case, you might consider simplifying the test\n> like this:\n> \n> test_expect_failure 'completion.commands removes multiple commands' '\n> test_config_global completion.commands \"-cherry -mergetool\" &&\n> git --list-cmds=list-mainporcelain,list-complete,config >actual &&\n> grep cherry-pick actual\n\nThis wouldn't check what we want. The point is that the command\n'cherry' is not listed, so it should be '! grep cherry$ actual'.\n\nFurthermore, I think it would be prudent to check that 'mergetool' is\nnot listed, either.\n\n"},{"id":"370452","messageId":"20190301215624.GC31362@zaya.teonanacatl.net","threadId":"50619","inReplyTo":"20190301205041.GM19739@szeder.dev","subject":"Re: [PATCH 2/3] t9902: test multiple removals via completion.commands","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-01T21:56:24Z","receivedAt":"2019-03-01T21:56:37Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"SZEDER Gábor wrote:\n> On Fri, Mar 01, 2019 at 01:22:52PM -0500, Eric Sunshine wrote:\n>> On Fri, Mar 1, 2019 at 12:35 PM Todd Zullinger wrote:\n>>> 6532f3740b (\"completion: allow to customize the completable command\n>>> list\", 2018-05-20) added the completion.commands config variable.\n>>> Multiple commands may be added or removed, separated by a space.\n>>>\n>>> Demonstrate the failure of multiple removals.\n>>>\n>>> Signed-off-by: Todd Zullinger \n>>> ---\n>>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n>>> @@ -1483,6 +1483,14 @@ test_expect_success 'git --help completion' '\n>>> +test_expect_failure 'completion.commands removes multiple commands' '\n>>> + echo cherry-pick >expected &&\n>>> + test_config_global completion.commands \"-cherry -mergetool\" &&\n>>> + git --list-cmds=list-mainporcelain,list-complete,config |\n>>> + grep ^cherry >actual &&\n>>> + test_cmp expected actual\n>>> +'\n>> \n>> We normally avoid placing a Git command upstream of a pipe. Instead,\n>> the Git command output would be redirected to a file and then the file\n>> grep'd.\n> \n> Indeed.\n\nYes, that's a good point. And one I should have known from\nreading it here more than once. Thanks to both of you.\n\n>> However, in this case, you might consider simplifying the test\n>> like this:\n>> \n>> test_expect_failure 'completion.commands removes multiple commands' '\n>> test_config_global completion.commands \"-cherry -mergetool\" &&\n>> git --list-cmds=list-mainporcelain,list-complete,config >actual &&\n>> grep cherry-pick actual\n> \n> This wouldn't check what we want. The point is that the command\n> 'cherry' is not listed, so it should be '! grep cherry$ actual'.\n\nHeh, I had written a similar reply already, but then I got\nside-tracked for a bit...\n\n I think the grep needs to be negated, e.g.:\n\n ! grep ^cherry$ actual\n\n Otherwise it succeeds without the fix, as cherry-pick is\n expected to be in the --list-cmds output. (If we remove\n the 'expected' file, it might also make sense to rename\n 'actual' to 'out' perhaps.)\n\n> Furthermore, I think it would be prudent to check that 'mergetool' is\n> not listed, either.\n\nTrue. With the simplified test, that's easy enough to add\nand makes the test description more accurate and the test\nmore thorough. I'll adjust it like so when I re-send:\n\n test_expect_failure 'completion.commands removes multiple commands' '\n \ttest_config completion.commands \"-cherry -mergetool\" &&\n \tgit --list-cmds=list-mainporcelain,list-complete,config >out &&\n \t! grep -E \"^(cherry|mergetool)$\" out\n\n(Using test_config depends on setup_git_directory_gently() in\nlist_cmds(), which Jeff suggested elsewhere in the thread.)\n\nThanks!\n\n-- \nTodd\n"},{"id":"370454","messageId":"20190301221551.GD31362@zaya.teonanacatl.net","threadId":"50619","inReplyTo":"20190301183017.GB30847@sigill.intra.peff.net","subject":"Re: [BUG] completion.commands does not remove multiple commands","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-01T22:15:51Z","receivedAt":"2019-03-01T22:15:57Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Jeff King wrote:\n> On Fri, Mar 01, 2019 at 12:34:40PM -0500, Todd Zullinger wrote:\n> \n>> Jeff King wrote:\n>>> I can reproduce your problem, though the test you included passes for me\n>>> even with the current tip of master.\n>> \n>> Oh, hrm. I think the issue is that completion.commands needs to be\n>> set in the global (or system-wide) config, via test_config_global\n>> rather than the local repo config which test_config sets.\n>> \n>> In hindsight, that seems obvious. But it's probably worth noting\n>> that where completion.commands is documented, for anyone who might\n>> spend a few cycles trying to configure it on a per-repo basis before\n>> realizing it doesn't work.\n> \n> I think this is actually a bug. Normally code that is checking config\n> before we've decide to do setup_git_directory() would use\n> read_early_config(), which uses discover_git_directory() to tentatively\n> see if we're in a repo, and if so to add it to the config sequence.\n> \n> But this code uses the caching configset mechanism. And that code\n> (rightly) does not use read_early_config(), because it has no idea if\n> it's being called early or what.\n> \n> I think we probably ought to be doing something like this:\n> \n> diff --git a/git.c b/git.c\n> index 2dd588674f..ba3690245e 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -62,6 +62,13 @@ static int list_cmds(const char *spec)\n> {\n> \tstruct string_list list = STRING_LIST_INIT_DUP;\n> \tint i;\n> +\tint nongit;\n> +\n> +\t/*\n> +\t * Set up the repository so we can pick up any repo-level config (like\n> +\t * completion.commands).\n> +\t */\n> +\tsetup_git_directory_gently(&nongit);\n> \n> \twhile (*spec) {\n> \t\tconst char *sep = strchrnul(spec, ',');\n\nHmm. The comments in list_cmds_by_config() made me wonder\nif not using a local repo config was intentional:\n\n /*\n * There's no actual repository setup at this point (and even\n * if there is, we don't really care; only global config\n * matters). If we accidentally set up a repository, it's ok\n * too since the caller (git --list-cmds=) should exit shortly\n * anyway.\n */\n\nIs the cost of setting up a repository something which might\nnoticeably slow down interactive completion? In my testing\ntoday I haven't felt it, but I have loads of memory on this\nsystem.\n\nI did apply your change and that allows the test to use\ntest_config() rather than test_config_global(). The full\ntest suite passes, so the change doesn't trigger any new\nissues we have covered by a test, at least.\n\nIf we wanted to respect local configs, how does this look?\n\n-- 8< --\nFrom: Jeff King \nSubject: [PATCH] git: read local config in --list-cmds\n\nNormally code that is checking config before we've decide to do\nsetup_git_directory() would use read_early_config(), which uses\ndiscover_git_directory() to tentatively see if we're in a repo,\nand if so to add it to the config sequence.\n\nBut list_cmds() uses the caching configset mechanism and\n(rightly) does not use read_early_config(), because it has no\nidea if it's being called early.\n\nCall setup_git_directory_gently() so we can pick up repo-level\nconfig (like completion.commands).\n\nSigned-off-by: Jeff King \n---\n git.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/git.c b/git.c\nindex 2dd588674f..10e49d79f6 100644\n--- a/git.c\n+++ b/git.c\n@@ -62,6 +62,13 @@ static int list_cmds(const char *spec)\n {\n \tstruct string_list list = STRING_LIST_INIT_DUP;\n \tint i;\n+\tint nongit;\n+\n+\t/*\n+\t* Set up the repository so we can pick up any repo-level config (like\n+\t* completion.commands).\n+\t*/\n+\tsetup_git_directory_gently(&nongit);\n \n \twhile (*spec) {\n \t\tconst char *sep = strchrnul(spec, ',');\n-- 8< --\n\n-- \nTodd\n"},{"id":"370455","messageId":"20190301230821.GA16294@sigill.intra.peff.net","threadId":"50619","inReplyTo":"20190301221551.GD31362@zaya.teonanacatl.net","subject":"Re: [BUG] completion.commands does not remove multiple commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-01T23:08:21Z","receivedAt":"2019-03-01T23:08:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 01, 2019 at 05:15:51PM -0500, Todd Zullinger wrote:\n\n> Hmm. The comments in list_cmds_by_config() made me wonder\n> if not using a local repo config was intentional:\n> \n> /*\n> * There's no actual repository setup at this point (and even\n> * if there is, we don't really care; only global config\n> * matters). If we accidentally set up a repository, it's ok\n> * too since the caller (git --list-cmds=) should exit shortly\n> * anyway.\n> */\n\nWell, let's see what Duy says. :)\n\nI've never used completion.commands myself, but it seems reasonable that\nsomebody might want different completion in different repos (e.g., if\nthey never use \"mergetool\" in one repo, but do in another).\n\n> Is the cost of setting up a repository something which might\n> noticeably slow down interactive completion? In my testing\n> today I haven't felt it, but I have loads of memory on this\n> system.\n\nI'd doubt it. It is a few syscalls (and might have to walk up the\nfilesystem if you're not actually in a repository), but it's something\nthat basically every Git process does, and I don't think it's ever been\nnoticeable.\n\n> I did apply your change and that allows the test to use\n> test_config() rather than test_config_global(). The full\n> test suite passes, so the change doesn't trigger any new\n> issues we have covered by a test, at least.\n> \n> If we wanted to respect local configs, how does this look?\n\nLooks good, with two minor commit message nits:\n\n> -- 8< --\n> From: Jeff King \n> Subject: [PATCH] git: read local config in --list-cmds\n> \n> Normally code that is checking config before we've decide to do\n\ns/decide/&d/\n\n> setup_git_directory() would use read_early_config(), which uses\n> discover_git_directory() to tentatively see if we're in a repo,\n> and if so to add it to the config sequence.\n> \n> But list_cmds() uses the caching configset mechanism and\n> (rightly) does not use read_early_config(), because it has no\n> idea if it's being called early.\n\nI'd say \"mechanism _which_ rightly does not use read_early_config...\" to\nmake it clear we're talking about configset, not list_cmds().\n\n-Peff\n"},{"id":"370457","messageId":"CACsJy8ATwLyVi5gxO_apbC5CbH3yasW676-1s3VVuQLDK30AbA@mail.gmail.com","threadId":"50619","inReplyTo":"20190301230821.GA16294@sigill.intra.peff.net","subject":"Re: [BUG] completion.commands does not remove multiple commands","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-02T01:08:59Z","receivedAt":"2019-03-02T01:09:27Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 2, 2019 at 6:08 AM Jeff King wrote:\n>\n> On Fri, Mar 01, 2019 at 05:15:51PM -0500, Todd Zullinger wrote:\n>\n> > Hmm. The comments in list_cmds_by_config() made me wonder\n> > if not using a local repo config was intentional:\n> >\n> > /*\n> > * There's no actual repository setup at this point (and even\n> > * if there is, we don't really care; only global config\n> > * matters). If we accidentally set up a repository, it's ok\n> > * too since the caller (git --list-cmds=) should exit shortly\n> > * anyway.\n> > */\n>\n> Well, let's see what Duy says. :)\n\nI vaguely recall that I wanted to cache the results in\ngit-completion.bash at some point. But looking at that script I don't\nthink there's any caching. So yes it should be ok to read per-repo\nconfig as well (and adjust or drop this comment block)\n\n> I've never used completion.commands myself, but it seems reasonable that\n> somebody might want different completion in different repos (e.g., if\n> they never use \"mergetool\" in one repo, but do in another).\n\nIt sounds to me confusing that you want different command sets in\ndifferent repos. Which is why I made it global config only. But I\nsuspect that people who have per-repo aliases may find this natural.\nSo yeah, no objection.\n\nPS. and thanks for the bug fix.\n-- \nDuy\n"},{"id":"370458","messageId":"xmqqef7q6pm1.fsf@gitster-ct.c.googlers.com","threadId":"50619","inReplyTo":"20190301221551.GD31362@zaya.teonanacatl.net","subject":"Re: [BUG] completion.commands does not remove multiple commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-02T01:18:14Z","receivedAt":"2019-03-02T01:18:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Todd Zullinger writes:\n\n> Hmm. The comments in list_cmds_by_config() made me wonder\n> if not using a local repo config was intentional:\n>\n> /*\n> * There's no actual repository setup at this point (and even\n> * if there is, we don't really care; only global config\n> * matters). If we accidentally set up a repository, it's ok\n> * too since the caller (git --list-cmds=) should exit shortly\n> * anyway.\n> */\n\nDoesn't the output from list-cmds-by-config get cached at the\ncompletion script layer in $__git_blah variables, and the cached\nvalues are not cleared when you chdir around? If you allowed the\nrepo-specific configuration to affect its output, the cached values\nneed to be reset when you cross repository boundaries. Otherwise\nyou'd see complaints like \"I have this in repo A but not in repo B;\nwhen I start from repo A, it gets completed even after I go to repo\nB. If I start from repo B, I do not get completion in either of\nthem\" (the former is because repo-A specific result gets cached, the\nlatter is because the cache is populated with the result taken in\nrepo-B that doesn't have customization and stays around even when\nyou visit repo-B).\n\nI think it is a sensible design decision to forbid per-repo config\nto relieve us from having to worry about when to invalidate the\ncache and all associated complexities.\n\n"},{"id":"370459","messageId":"20190302024011.GF31362@zaya.teonanacatl.net","threadId":"50619","inReplyTo":"xmqqef7q6pm1.fsf@gitster-ct.c.googlers.com","subject":"Re: [BUG] completion.commands does not remove multiple commands","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-02T02:40:12Z","receivedAt":"2019-03-02T02:40:24Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n> Todd Zullinger writes:\n> \n>> Hmm. The comments in list_cmds_by_config() made me wonder\n>> if not using a local repo config was intentional:\n>>\n>> /*\n>> * There's no actual repository setup at this point (and even\n>> * if there is, we don't really care; only global config\n>> * matters). If we accidentally set up a repository, it's ok\n>> * too since the caller (git --list-cmds=) should exit shortly\n>> * anyway.\n>> */\n> \n> Doesn't the output from list-cmds-by-config get cached at the\n> completion script layer in $__git_blah variables, and the cached\n> values are not cleared when you chdir around?\n\nIn testing, I didn't find any evidence of caching. Setting\ncommands to be added and removed in the global and local\nconfigs worked reasonably.\n\nDuy's reply suggests that was considered but not\nimplemented. I there were caching (and if it were tedious\nfor the completion code to keep fresh between repos), then\nit would a bad plan to allow per-repo config.\n\nIf there was a goal of adding such caching it might also\nmake sense to avoid \"fixing\" the code here to allow per-repo\nconfig before it's known how that might affect such caching.\n\nIt sounds like that's not something Duy is planning on for\nthe near term though, so perhaps we're fine to allow local\nrepo config here? As Duy mentioned, maybe some users with\nlocal aliases want to add them to the completion locally as\nwell.\n\nIf we choose to avoid local repo config then we can add a\ncomment to the documetation like I had in 2/3. Maybe also\nupdate the comment in list_cmds_by_config() to note that we\nintentionally don't setup a repo -- or a similar comment in\nlist_cmds(), where Jeff's 1/3 was adding\nsetup_git_directory_gently().\n\nI don't have a strong opinion either way. I more or less\nhave the minor patches for either direction at this point.\n\nThanks,\n\n-- \nTodd\n"},{"id":"370462","messageId":"20190302040704.GN19739@szeder.dev","threadId":"50619","inReplyTo":"20190302024011.GF31362@zaya.teonanacatl.net","subject":"Re: [BUG] completion.commands does not remove multiple commands","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-02T04:07:04Z","receivedAt":"2019-03-02T04:07:32Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Mar 01, 2019 at 09:40:12PM -0500, Todd Zullinger wrote:\n> Hi,\n> \n> Junio C Hamano wrote:\n> > Todd Zullinger writes:\n> > \n> >> Hmm. The comments in list_cmds_by_config() made me wonder\n> >> if not using a local repo config was intentional:\n> >>\n> >> /*\n> >> * There's no actual repository setup at this point (and even\n> >> * if there is, we don't really care; only global config\n> >> * matters). If we accidentally set up a repository, it's ok\n> >> * too since the caller (git --list-cmds=) should exit shortly\n> >> * anyway.\n> >> */\n> > \n> > Doesn't the output from list-cmds-by-config get cached at the\n> > completion script layer in $__git_blah variables, and the cached\n> > values are not cleared when you chdir around?\n> \n> In testing, I didn't find any evidence of caching. Setting\n> commands to be added and removed in the global and local\n> configs worked reasonably.\n> \n> Duy's reply suggests that was considered but not\n> implemented. I there were caching (and if it were tedious\n> for the completion code to keep fresh between repos), then\n> it would a bad plan to allow per-repo config.\n\nThe completion script used to cache the list of porcelain commands,\nbut then Duy came along and removed it in 3301d36b29 (completion: add\nand use --list-cmds=alias, 2018-05-20).\n\nWe cached the commands, because it was a bit involved to get them:\nfirst we run 'git help -a' to list all commands, then extracted the\ncommand names from the help output with 'grep', and finally an\nenormous case statement removed all plumbing commands. Caching spared\nus the overhead of these external processes and maybe 2-3 subshells.\nNote that because of this caching if you dropped a third-party\n'git-foo' command in your $PATH, it didn't show up in completion until\nyou re-sourced the completion script, which was the standard way to\ninvalidate/refresh the caches.\n\nHowever, even when we cached porcelain commands, we didn't cache\naliases, even though getting them is a bit involved as well, requiring\nrunning 'git config', two subshells and a shell loop. I presume that\nback then the reason for not caching aliases was that they could be\nrepo-specific.\n\nNow, ever since Duy revamped commands completion, we only run a simple\n$(git --list-cmds=...), i.e. a git command and a subshell takes care\nof all that. IMO this is the best we ever had, because it uses one\nless subshell than before to list both commands and aliases, and it\nlists everything afresh, including third-party 'git-foo' commands.\n\nI don't see the benefit of bringing back caching. Yes, on one hand we\ncould complete commands with a single variable expansion, which is\nclearly faster than that $(git --list-cmds=...). OTOH, that's only\ncommands, but what about aliases? If we cache aliases, too, then that\ncould be considered a regression by those who do have repo-specific\naliases; if we don't, then we are back at running 'git config' and two\nsubshells, i.e. one subshell more than we currently have.\n\nAnd if we won't cache commands, then we might as well modify\nlist_cmds_by_config() to read 'completion.commands' from the\nrepo-specific config, too. I'm fairly neutral on this, because I\ncan only imagine rather convoluted scenarios where a repo-specific\ncommand list would be useful, but at least it would make it consistent\nwith aliases, which we read from the current repo's config and we\nalways have.\n\n\nNote, however, that for completeness sake, if we choose to update\nlist_cmds_by_config() to read the repo's config as well, then we\nshould also update the completion script to run $(__git\n--list-cmds=...), note the two leading underscores, so in case of 'git\n-C over/there ' it would read 'completion.commands' from the right\nrepository.\n\n\n"},{"id":"370496","messageId":"20190303013400.GA19806@zaya.teonanacatl.net","threadId":"50619","inReplyTo":"20190302040704.GN19739@szeder.dev","subject":"Re: [BUG] completion.commands does not remove multiple commands","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-03T01:34:00Z","receivedAt":"2019-03-03T01:34:08Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"SZEDER Gábor wrote:\n[... lots of good history ...]\n\nThanks for an excellent summary!\n\n> Note, however, that for completeness sake, if we choose to update\n> list_cmds_by_config() to read the repo's config as well, then we\n> should also update the completion script to run $(__git\n> --list-cmds=...), note the two leading underscores, so in case of 'git\n> -C over/there ' it would read 'completion.commands' from the right\n> repository.\n\nThanks for pointing this out. I'll add that to my local\nbranch for the \"respect local config\" case.\n\nAt the moment, I think it only matters for calls where\nconfig is in the --list-cmds list. But since the fix Jeff\nsuggested affects all --list-cmds calls, it seems prudent to\nadjust the 3-4 other uses of --list-cmds in the completion\nscript. Let me know if you see a reason not to do that.\n\nThanks again for such a nice summary,\n\n-- \nTodd\n"},{"id":"370538","messageId":"20190303170658.GA23811@sigill.intra.peff.net","threadId":"50619","inReplyTo":"20190302040704.GN19739@szeder.dev","subject":"Re: [BUG] completion.commands does not remove multiple commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-03T17:06:58Z","receivedAt":"2019-03-03T17:07:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 02, 2019 at 05:07:04AM +0100, SZEDER Gábor wrote:\n\n> The completion script used to cache the list of porcelain commands,\n> but then Duy came along and removed it in 3301d36b29 (completion: add\n> and use --list-cmds=alias, 2018-05-20).\n> [...]\n\nThanks for this summary.\n\nJust for the record, as the person who suggested that we should respect\nrepo config here, I don't personally care all that much either way. I do\nnot plan to use the feature myself, but it was just what I would have\nexpected to happen. So I can live with it either way.\n\nThat said, it seems like if we were to go back to caching, we'd need to\nhandle directory changes in order for aliases (which already do respect\nrepo config) to be correct anyway. So it doesn't seem like this is\nintroducing any burden that was not there already.\n\n-Peff\n"},{"id":"371723","messageId":"CACsJy8BuR=syjT1gjTxXXKKaevzpbdRGp+je+rsX6jV96F3sbA@mail.gmail.com","threadId":"50619","inReplyTo":"20190301173443.16429-2-tmz@pobox.com","subject":"Re: [PATCH 1/3] doc: note config file restrictions for completion.commands","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-17T13:12:51Z","receivedAt":"2019-03-17T13:13:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Poking this thread before it goes completely dead...\n\nOn Sat, Mar 2, 2019 at 12:34 AM Todd Zullinger wrote:\n>\n> The completion.commands config var must be set in either the system-wide\n> or global config file. The completion script does not read a local\n> repository config.\n\nAfter the last discussion, I think the consensus was it's ok to allow\nper-repo config so we can just drop this and update the code to read\nfrom any config file, right?\n\n>\n> Signed-off-by: Todd Zullinger \n> ---\n> Documentation/config/completion.txt | 3 ++-\n> Documentation/git.txt | 3 ++-\n> 2 files changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/config/completion.txt b/Documentation/config/completion.txt\n> index 4d99bf33c9..4859d18e86 100644\n> --- a/Documentation/config/completion.txt\n> +++ b/Documentation/config/completion.txt\n> @@ -4,4 +4,5 @@ completion.commands::\n> porcelain commands and a few select others are completed. You\n> can add more commands, separated by space, in this\n> variable. Prefixing the command with '-' will remove it from\n> - the existing list.\n> + the existing list. The variable must be set in either the\n> + system-wide or global config.\n> diff --git a/Documentation/git.txt b/Documentation/git.txt\n> index 00156d64aa..638f4d6cc9 100644\n> --- a/Documentation/git.txt\n> +++ b/Documentation/git.txt\n> @@ -172,7 +172,8 @@ foo.bar= ...`) sets `foo.bar` to the empty string which `git config\n> others (all other commands in `$PATH` that have git- prefix),\n> list- (see categories in command-list.txt),\n> nohelpers (exclude helper commands), alias and config\n> - (retrieve command list from config variable completion.commands)\n> + (retrieve command list from config variable completion.commands,\n> + set in the global or system-wide config file)\n>\n> GIT COMMANDS\n> ------------\n\n\n\n-- \nDuy\n"},{"id":"371736","messageId":"20190317181620.26727-2-tmz@pobox.com","threadId":"50619","inReplyTo":"CACsJy8BuR=syjT1gjTxXXKKaevzpbdRGp+je+rsX6jV96F3sbA@mail.gmail.com","subject":"[PATCH v2 1/4] git: read local config in --list-cmds","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-17T18:16:17Z","receivedAt":"2019-03-17T18:16:33Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"From: Jeff King \n\nNormally code that is checking config before we've decided to do\nsetup_git_directory() would use read_early_config(), which uses\ndiscover_git_directory() to tentatively see if we're in a repo,\nand if so to add it to the config sequence.\n\nBut list_cmds() uses the caching configset mechanism which\nrightly does not use read_early_config(), because it has no\nidea if it's being called early.\n\nCall setup_git_directory_gently() so we can pick up repo-level\nconfig (like completion.commands).\n\nSigned-off-by: Jeff King \n---\n git.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/git.c b/git.c\nindex 2dd588674f..10e49d79f6 100644\n--- a/git.c\n+++ b/git.c\n@@ -62,6 +62,13 @@ static int list_cmds(const char *spec)\n {\n \tstruct string_list list = STRING_LIST_INIT_DUP;\n \tint i;\n+\tint nongit;\n+\n+\t/*\n+\t* Set up the repository so we can pick up any repo-level config (like\n+\t* completion.commands).\n+\t*/\n+\tsetup_git_directory_gently(&nongit);\n \n \twhile (*spec) {\n \t\tconst char *sep = strchrnul(spec, ',');\n"},{"id":"371737","messageId":"20190317181620.26727-1-tmz@pobox.com","threadId":"50619","inReplyTo":"CACsJy8BuR=syjT1gjTxXXKKaevzpbdRGp+je+rsX6jV96F3sbA@mail.gmail.com","subject":"[PATCH v2 0/4] completion.commands: fix multiple command removals","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-17T18:16:16Z","receivedAt":"2019-03-17T18:16:35Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi,\n\nDuy Nguyen wrote:\n> Poking this thread before it goes completely dead...\n\nI've been meaning to send out a re-rolled version. I didn't\nrealize 2 weeks had slipped by already. :/\n\n> On Sat, Mar 2, 2019 at 12:34 AM Todd Zullinger wrote:\n>>\n>> The completion.commands config var must be set in either the system-wide\n>> or global config file. The completion script does not read a local\n>> repository config.\n> \n> After the last discussion, I think the consensus was it's ok to allow\n> per-repo config so we can just drop this and update the code to read\n> from any config file, right?\n\nYeah. Here's an updated series which I hope sums up the changes\nfrom the discussion.\n\nChanges since v1:\n\n - Change --list-commands to respect local config and use\n test_config rather than test_config_global\n - Avoid git upstream of pipes\n - Check that both cherry and mergetool are removed\n - Use __git in completion calls which use --list-cmds, to\n ensure the proper git repo config is checked with git -C\n /some/other/repo\n\nJeff King (2):\n git: read local config in --list-cmds\n completion: fix multiple command removals\n\nTodd Zullinger (2):\n t9902: test multiple removals via completion.commands\n completion: use __git when calling --list-cmds\n\n contrib/completion/git-completion.bash | 8 ++++----\n git.c | 7 +++++++\n help.c | 4 ++--\n t/t9902-completion.sh | 6 ++++++\n 4 files changed, 19 insertions(+), 6 deletions(-)\n\nRange-diff against v1:\n1: 385c596510 < -: ---------- doc: note config file restrictions for completion.commands\n-: ---------- > 1: e51bdea6d3 git: read local config in --list-cmds\n2: ffa5ed9780 ! 2: 2f5e9da9de t9902: test multiple removals via completion.commands\n @@ -18,11 +18,9 @@\n '\n \n +test_expect_failure 'completion.commands removes multiple commands' '\n -+\techo cherry-pick >expected &&\n -+\ttest_config_global completion.commands \"-cherry -mergetool\" &&\n -+\tgit --list-cmds=list-mainporcelain,list-complete,config |\n -+\t\tgrep ^cherry >actual &&\n -+\ttest_cmp expected actual\n ++\ttest_config completion.commands \"-cherry -mergetool\" &&\n ++\tgit --list-cmds=list-mainporcelain,list-complete,config >out &&\n ++\t! grep -E \"^(cherry|mergetool)$\" out\n +'\n +\n test_expect_success 'setup for integration tests' '\n3: af029e908d ! 3: 7548dcc23f completion: fix multiple command removals\n @@ -2,16 +2,16 @@\n \n completion: fix multiple command removals\n \n - 6532f3740b (\"completion: allow to customize the completable\n - command list\", 2018-05-20) added the completion.commands config\n - variable.\n + Commit 6532f3740b (\"completion: allow to customize the completable\n + command list\", 2018-05-20) tried to allow multiple space-separated\n + entries in completion.commands. To do this, it copies each parsed token\n + into a strbuf so that the result is NUL-terminated.\n \n - The documentation states multiple commands may be added,\n - separated by spaces. Adding multiple commands to remove fails,\n - only removing the last command in the config.\n -\n - Fix multiple command removals.\n + However, for tokens starting with \"-\", it accidentally passes the\n + original non-terminated string, meaning that only the final one worked.\n + Switch to using the strbuf.\n \n + Reported-by: Todd Zullinger \n Signed-off-by: Jeff King \n \n diff --git a/help.c b/help.c\n @@ -38,6 +38,6 @@\n \n -test_expect_failure 'completion.commands removes multiple commands' '\n +test_expect_success 'completion.commands removes multiple commands' '\n - \techo cherry-pick >expected &&\n - \ttest_config_global completion.commands \"-cherry -mergetool\" &&\n - \tgit --list-cmds=list-mainporcelain,list-complete,config |\n + \ttest_config completion.commands \"-cherry -mergetool\" &&\n + \tgit --list-cmds=list-mainporcelain,list-complete,config >out &&\n + \t! grep -E \"^(cherry|mergetool)$\" out\n-: ---------- > 4: 26bef0b2af completion: use __git when calling --list-cmds\n"},{"id":"371738","messageId":"20190317181620.26727-3-tmz@pobox.com","threadId":"50619","inReplyTo":"CACsJy8BuR=syjT1gjTxXXKKaevzpbdRGp+je+rsX6jV96F3sbA@mail.gmail.com","subject":"[PATCH v2 2/4] t9902: test multiple removals via completion.commands","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-17T18:16:18Z","receivedAt":"2019-03-17T18:16:37Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"6532f3740b (\"completion: allow to customize the completable command\nlist\", 2018-05-20) added the completion.commands config variable.\nMultiple commands may be added or removed, separated by a space.\n\nDemonstrate the failure of multiple removals.\n\nSigned-off-by: Todd Zullinger \n---\n t/t9902-completion.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 3a2c6326d8..3f5b420bf8 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1483,6 +1483,12 @@ test_expect_success 'git --help completion' '\n \ttest_completion \"git --help core\" \"core-tutorial \"\n '\n \n+test_expect_failure 'completion.commands removes multiple commands' '\n+\ttest_config completion.commands \"-cherry -mergetool\" &&\n+\tgit --list-cmds=list-mainporcelain,list-complete,config >out &&\n+\t! grep -E \"^(cherry|mergetool)$\" out\n+'\n+\n test_expect_success 'setup for integration tests' '\n \techo content >file1 &&\n \techo more >file2 &&\n"},{"id":"371739","messageId":"20190317181620.26727-4-tmz@pobox.com","threadId":"50619","inReplyTo":"CACsJy8BuR=syjT1gjTxXXKKaevzpbdRGp+je+rsX6jV96F3sbA@mail.gmail.com","subject":"[PATCH v2 3/4] completion: fix multiple command removals","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-17T18:16:19Z","receivedAt":"2019-03-17T18:16:41Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"From: Jeff King \n\nCommit 6532f3740b (\"completion: allow to customize the completable\ncommand list\", 2018-05-20) tried to allow multiple space-separated\nentries in completion.commands. To do this, it copies each parsed token\ninto a strbuf so that the result is NUL-terminated.\n\nHowever, for tokens starting with \"-\", it accidentally passes the\noriginal non-terminated string, meaning that only the final one worked.\nSwitch to using the strbuf.\n\nReported-by: Todd Zullinger \nSigned-off-by: Jeff King \n---\n help.c | 4 ++--\n t/t9902-completion.sh | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex 520c9080e8..026f881715 100644\n--- a/help.c\n+++ b/help.c\n@@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list)\n \t\tconst char *p = strchrnul(cmd_list, ' ');\n \n \t\tstrbuf_add(&sb, cmd_list, p - cmd_list);\n-\t\tif (*cmd_list == '-')\n-\t\t\tstring_list_remove(list, cmd_list + 1, 0);\n+\t\tif (sb.buf[0] == '-')\n+\t\t\tstring_list_remove(list, sb.buf + 1, 0);\n \t\telse\n \t\t\tstring_list_insert(list, sb.buf);\n \t\tstrbuf_release(&sb);\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 3f5b420bf8..050fac4fff 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1483,7 +1483,7 @@ test_expect_success 'git --help completion' '\n \ttest_completion \"git --help core\" \"core-tutorial \"\n '\n \n-test_expect_failure 'completion.commands removes multiple commands' '\n+test_expect_success 'completion.commands removes multiple commands' '\n \ttest_config completion.commands \"-cherry -mergetool\" &&\n \tgit --list-cmds=list-mainporcelain,list-complete,config >out &&\n \t! grep -E \"^(cherry|mergetool)$\" out\n"},{"id":"371740","messageId":"20190317181620.26727-5-tmz@pobox.com","threadId":"50619","inReplyTo":"CACsJy8BuR=syjT1gjTxXXKKaevzpbdRGp+je+rsX6jV96F3sbA@mail.gmail.com","subject":"[PATCH v2 4/4] completion: use __git when calling --list-cmds","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-17T18:16:20Z","receivedAt":"2019-03-17T18:16:44Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Since e51bdea6d3 (\"git: read local config in --list-cmds\", 2019-03-01),\nthe completion.commands variable respects repo-level configuration. Use\n__git which ensures that the proper repo config is consulted if the\ncommand line contains 'git -C /some/other/repo'.\n\nSuggested-by: SZEDER Gábor \nSigned-off-by: Todd Zullinger \n---\n\nJunio, I referenc the commit id for \"git: read local config in\n--list-cmds\" which is earlier in this series, so the id will, of\ncourse, differ when you apply it. Let me know if you'd prefer\nthis commit to be dropped and resent once the others in the\nseries are applied or if it's easy for you to adjust when it's\nqueued.\n\nAlso, as I wrote in an earlier reply, at the moment, I think\nusing __git only matters for calls where config is in the\n--list-cmds list. But since Jeff's fix affects all --list-cmds\ncalls, it seems prudent to adjust the 3 other uses of --list-cmds\nin the completion script.\n\n contrib/completion/git-completion.bash | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 499e56f83d..e505f04ff7 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1010,7 +1010,7 @@ __git_all_commands=\n __git_compute_all_commands ()\n {\n \ttest -n \"$__git_all_commands\" ||\n-\t__git_all_commands=$(git --list-cmds=main,others,alias,nohelpers)\n+\t__git_all_commands=$(__git --list-cmds=main,others,alias,nohelpers)\n }\n \n # Lists all set config variables starting with the given section prefix,\n@@ -1620,9 +1620,9 @@ _git_help ()\n \tesac\n \tif test -n \"$GIT_TESTING_ALL_COMMAND_LIST\"\n \tthen\n-\t\t__gitcomp \"$GIT_TESTING_ALL_COMMAND_LIST $(git --list-cmds=alias,list-guide) gitk\"\n+\t\t__gitcomp \"$GIT_TESTING_ALL_COMMAND_LIST $(__git --list-cmds=alias,list-guide) gitk\"\n \telse\n-\t\t__gitcomp \"$(git --list-cmds=main,nohelpers,alias,list-guide) gitk\"\n+\t\t__gitcomp \"$(__git --list-cmds=main,nohelpers,alias,list-guide) gitk\"\n \tfi\n }\n \n@@ -2888,7 +2888,7 @@ __git_main ()\n \t\t\tthen\n \t\t\t\t__gitcomp \"$GIT_TESTING_PORCELAIN_COMMAND_LIST\"\n \t\t\telse\n-\t\t\t\t__gitcomp \"$(git --list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)\"\n+\t\t\t\t__gitcomp \"$(__git --list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)\"\n \t\t\tfi\n \t\t\t;;\n \t\tesac\n"},{"id":"371799","messageId":"CACsJy8DiQwmAKMruOAO4roPbiRTvt5TESBTd682hTwkZjcoj2Q@mail.gmail.com","threadId":"50619","inReplyTo":"20190317181620.26727-2-tmz@pobox.com","subject":"Re: [PATCH v2 1/4] git: read local config in --list-cmds","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-18T09:41:13Z","receivedAt":"2019-03-18T09:41:47Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Mar 18, 2019 at 1:16 AM Todd Zullinger wrote:\n>\n> From: Jeff King \n>\n> Normally code that is checking config before we've decided to do\n> setup_git_directory() would use read_early_config(), which uses\n> discover_git_directory() to tentatively see if we're in a repo,\n> and if so to add it to the config sequence.\n>\n> But list_cmds() uses the caching configset mechanism which\n> rightly does not use read_early_config(), because it has no\n> idea if it's being called early.\n>\n> Call setup_git_directory_gently() so we can pick up repo-level\n> config (like completion.commands).\n>\n> Signed-off-by: Jeff King \n> ---\n> git.c | 7 +++++++\n> 1 file changed, 7 insertions(+)\n>\n> diff --git a/git.c b/git.c\n> index 2dd588674f..10e49d79f6 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -62,6 +62,13 @@ static int list_cmds(const char *spec)\n> {\n> struct string_list list = STRING_LIST_INIT_DUP;\n> int i;\n> + int nongit;\n> +\n> + /*\n> + * Set up the repository so we can pick up any repo-level config (like\n> + * completion.commands).\n> + */\n> + setup_git_directory_gently(&nongit);\n\nThis gave me a pause because we could try to find .git more than\nnecessary (e.g. when --list-cmds is requested without \"config\"). But I\ndon't think that happens often enough to be worried about.\n\nIt also subtly changes list_aliases() code flow, because the\nread_early_config() call inside will use the already-discovered gitdir\n here instead of trying to rediscover. So, everything is still fine.\n\nYou probably want to drop the comment block about repository setup\ninside list_cmds_by_config() too.\n\n>\n> while (*spec) {\n> const char *sep = strchrnul(spec, ',');\n-- \nDuy\n"},{"id":"372012","messageId":"20190320180329.22280-1-tmz@pobox.com","threadId":"50619","inReplyTo":"CACsJy8DiQwmAKMruOAO4roPbiRTvt5TESBTd682hTwkZjcoj2Q@mail.gmail.com","subject":"[PATCH v3 0/4] completion.commands: fix multiple command removals","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-20T18:03:25Z","receivedAt":"2019-03-20T18:03:42Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi,\n\nDuy Nguyen wrote:\n> You probably want to drop the comment block about repository setup\n> inside list_cmds_by_config() too.\n\nYou're right, of course. Thanks Duy. :)\n\nThat's the only change since v2.\n\nOther than a follow-up to update the commit reference in 4/4\nafter 1/4 is in the final form on pu, I think this might be good.\nIf it's easier, we can skip 4/4 and I'll resend it after the\nothers are on pu.\n\nJeff King (2):\n git: read local config in --list-cmds\n completion: fix multiple command removals\n\nTodd Zullinger (2):\n t9902: test multiple removals via completion.commands\n completion: use __git when calling --list-cmds\n\n contrib/completion/git-completion.bash | 8 ++++----\n git.c | 7 +++++++\n help.c | 11 ++---------\n t/t9902-completion.sh | 6 ++++++\n 4 files changed, 19 insertions(+), 13 deletions(-)\n\nRange-diff against v2:\n1: e51bdea6d3 ! 1: 6e9872b0e3 git: read local config in --list-cmds\n @@ -33,3 +33,21 @@\n \n \twhile (*spec) {\n \t\tconst char *sep = strchrnul(spec, ',');\n +\n + diff --git a/help.c b/help.c\n + --- a/help.c\n + +++ b/help.c\n +@@\n + {\n + \tconst char *cmd_list;\n + \n +-\t/*\n +-\t * There's no actual repository setup at this point (and even\n +-\t * if there is, we don't really care; only global config\n +-\t * matters). If we accidentally set up a repository, it's ok\n +-\t * too since the caller (git --list-cmds=) should exit shortly\n +-\t * anyway.\n +-\t */\n + \tif (git_config_get_string_const(\"completion.commands\", &cmd_list))\n + \t\treturn;\n + \n2: 2f5e9da9de = 2: 6873ae3868 t9902: test multiple removals via completion.commands\n3: 7548dcc23f = 3: f66bbc0b55 completion: fix multiple command removals\n4: 26bef0b2af = 4: 197b176483 completion: use __git when calling --list-cmds\n"},{"id":"372013","messageId":"20190320180329.22280-2-tmz@pobox.com","threadId":"50619","inReplyTo":"CACsJy8DiQwmAKMruOAO4roPbiRTvt5TESBTd682hTwkZjcoj2Q@mail.gmail.com","subject":"[PATCH v3 1/4] git: read local config in --list-cmds","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-20T18:03:26Z","receivedAt":"2019-03-20T18:03:43Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"From: Jeff King \n\nNormally code that is checking config before we've decided to do\nsetup_git_directory() would use read_early_config(), which uses\ndiscover_git_directory() to tentatively see if we're in a repo,\nand if so to add it to the config sequence.\n\nBut list_cmds() uses the caching configset mechanism which\nrightly does not use read_early_config(), because it has no\nidea if it's being called early.\n\nCall setup_git_directory_gently() so we can pick up repo-level\nconfig (like completion.commands).\n\nSigned-off-by: Jeff King \n---\n git.c | 7 +++++++\n help.c | 7 -------\n 2 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 2dd588674f..10e49d79f6 100644\n--- a/git.c\n+++ b/git.c\n@@ -62,6 +62,13 @@ static int list_cmds(const char *spec)\n {\n \tstruct string_list list = STRING_LIST_INIT_DUP;\n \tint i;\n+\tint nongit;\n+\n+\t/*\n+\t* Set up the repository so we can pick up any repo-level config (like\n+\t* completion.commands).\n+\t*/\n+\tsetup_git_directory_gently(&nongit);\n \n \twhile (*spec) {\n \t\tconst char *sep = strchrnul(spec, ',');\ndiff --git a/help.c b/help.c\nindex 520c9080e8..fac7e421d0 100644\n--- a/help.c\n+++ b/help.c\n@@ -375,13 +375,6 @@ void list_cmds_by_config(struct string_list *list)\n {\n \tconst char *cmd_list;\n \n-\t/*\n-\t * There's no actual repository setup at this point (and even\n-\t * if there is, we don't really care; only global config\n-\t * matters). If we accidentally set up a repository, it's ok\n-\t * too since the caller (git --list-cmds=) should exit shortly\n-\t * anyway.\n-\t */\n \tif (git_config_get_string_const(\"completion.commands\", &cmd_list))\n \t\treturn;\n \n"},{"id":"372014","messageId":"20190320180329.22280-3-tmz@pobox.com","threadId":"50619","inReplyTo":"CACsJy8DiQwmAKMruOAO4roPbiRTvt5TESBTd682hTwkZjcoj2Q@mail.gmail.com","subject":"[PATCH v3 2/4] t9902: test multiple removals via completion.commands","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-20T18:03:27Z","receivedAt":"2019-03-20T18:03:46Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"6532f3740b (\"completion: allow to customize the completable command\nlist\", 2018-05-20) added the completion.commands config variable.\nMultiple commands may be added or removed, separated by a space.\n\nDemonstrate the failure of multiple removals.\n\nSigned-off-by: Todd Zullinger \n---\n t/t9902-completion.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 3a2c6326d8..3f5b420bf8 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1483,6 +1483,12 @@ test_expect_success 'git --help completion' '\n \ttest_completion \"git --help core\" \"core-tutorial \"\n '\n \n+test_expect_failure 'completion.commands removes multiple commands' '\n+\ttest_config completion.commands \"-cherry -mergetool\" &&\n+\tgit --list-cmds=list-mainporcelain,list-complete,config >out &&\n+\t! grep -E \"^(cherry|mergetool)$\" out\n+'\n+\n test_expect_success 'setup for integration tests' '\n \techo content >file1 &&\n \techo more >file2 &&\n"},{"id":"372015","messageId":"20190320180329.22280-4-tmz@pobox.com","threadId":"50619","inReplyTo":"CACsJy8DiQwmAKMruOAO4roPbiRTvt5TESBTd682hTwkZjcoj2Q@mail.gmail.com","subject":"[PATCH v3 3/4] completion: fix multiple command removals","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-20T18:03:28Z","receivedAt":"2019-03-20T18:03:50Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"From: Jeff King \n\nCommit 6532f3740b (\"completion: allow to customize the completable\ncommand list\", 2018-05-20) tried to allow multiple space-separated\nentries in completion.commands. To do this, it copies each parsed token\ninto a strbuf so that the result is NUL-terminated.\n\nHowever, for tokens starting with \"-\", it accidentally passes the\noriginal non-terminated string, meaning that only the final one worked.\nSwitch to using the strbuf.\n\nReported-by: Todd Zullinger \nSigned-off-by: Jeff King \n---\n help.c | 4 ++--\n t/t9902-completion.sh | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex fac7e421d0..a9e451f2ee 100644\n--- a/help.c\n+++ b/help.c\n@@ -386,8 +386,8 @@ void list_cmds_by_config(struct string_list *list)\n \t\tconst char *p = strchrnul(cmd_list, ' ');\n \n \t\tstrbuf_add(&sb, cmd_list, p - cmd_list);\n-\t\tif (*cmd_list == '-')\n-\t\t\tstring_list_remove(list, cmd_list + 1, 0);\n+\t\tif (sb.buf[0] == '-')\n+\t\t\tstring_list_remove(list, sb.buf + 1, 0);\n \t\telse\n \t\t\tstring_list_insert(list, sb.buf);\n \t\tstrbuf_release(&sb);\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 3f5b420bf8..050fac4fff 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1483,7 +1483,7 @@ test_expect_success 'git --help completion' '\n \ttest_completion \"git --help core\" \"core-tutorial \"\n '\n \n-test_expect_failure 'completion.commands removes multiple commands' '\n+test_expect_success 'completion.commands removes multiple commands' '\n \ttest_config completion.commands \"-cherry -mergetool\" &&\n \tgit --list-cmds=list-mainporcelain,list-complete,config >out &&\n \t! grep -E \"^(cherry|mergetool)$\" out\n"},{"id":"372016","messageId":"20190320180329.22280-5-tmz@pobox.com","threadId":"50619","inReplyTo":"CACsJy8DiQwmAKMruOAO4roPbiRTvt5TESBTd682hTwkZjcoj2Q@mail.gmail.com","subject":"[PATCH v3 4/4] completion: use __git when calling --list-cmds","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-20T18:03:29Z","receivedAt":"2019-03-20T18:03:53Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Since e51bdea6d3 (\"git: read local config in --list-cmds\", 2019-03-01),\nthe completion.commands variable respects repo-level configuration. Use\n__git which ensures that the proper repo config is consulted if the\ncommand line contains 'git -C /some/other/repo'.\n\nSuggested-by: SZEDER Gábor \nSigned-off-by: Todd Zullinger \n---\n contrib/completion/git-completion.bash | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 499e56f83d..e505f04ff7 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1010,7 +1010,7 @@ __git_all_commands=\n __git_compute_all_commands ()\n {\n \ttest -n \"$__git_all_commands\" ||\n-\t__git_all_commands=$(git --list-cmds=main,others,alias,nohelpers)\n+\t__git_all_commands=$(__git --list-cmds=main,others,alias,nohelpers)\n }\n \n # Lists all set config variables starting with the given section prefix,\n@@ -1620,9 +1620,9 @@ _git_help ()\n \tesac\n \tif test -n \"$GIT_TESTING_ALL_COMMAND_LIST\"\n \tthen\n-\t\t__gitcomp \"$GIT_TESTING_ALL_COMMAND_LIST $(git --list-cmds=alias,list-guide) gitk\"\n+\t\t__gitcomp \"$GIT_TESTING_ALL_COMMAND_LIST $(__git --list-cmds=alias,list-guide) gitk\"\n \telse\n-\t\t__gitcomp \"$(git --list-cmds=main,nohelpers,alias,list-guide) gitk\"\n+\t\t__gitcomp \"$(__git --list-cmds=main,nohelpers,alias,list-guide) gitk\"\n \tfi\n }\n \n@@ -2888,7 +2888,7 @@ __git_main ()\n \t\t\tthen\n \t\t\t\t__gitcomp \"$GIT_TESTING_PORCELAIN_COMMAND_LIST\"\n \t\t\telse\n-\t\t\t\t__gitcomp \"$(git --list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)\"\n+\t\t\t\t__gitcomp \"$(__git --list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)\"\n \t\t\tfi\n \t\t\t;;\n \t\tesac\n"},{"id":"372050","messageId":"xmqq8sx9szlx.fsf@gitster-ct.c.googlers.com","threadId":"50619","inReplyTo":"20190320180329.22280-1-tmz@pobox.com","subject":"Re: [PATCH v3 0/4] completion.commands: fix multiple command removals","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-21T02:58:02Z","receivedAt":"2019-03-21T02:58:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Todd Zullinger writes:\n\n> Other than a follow-up to update the commit reference in 4/4\n> after 1/4 is in the final form on pu, I think this might be good.\n> If it's easier, we can skip 4/4 and I'll resend it after the\n> others are on pu.\n\nA series that makes a single topic should expect to be read by a\nreader who understand the context of individual pieces in the topic,\nso it is more common to refer to an earlier step of a series from a\nlater step without the object name. I would have written the log\nmessage like so instead:\n\n completion: use __git when calling --list-cmds\n\n As we made --list-cmds read the local configuration file in an\n earlier step, the completion.commands variable respects repo-level\n configuration. Use __git which ensures that the proper repo config\n is consulted if the command line contains 'git -C /some/other/repo'.\n\nThe whole series looks good to me. Thanks for working on it.\n"},{"id":"372084","messageId":"CACsJy8BM_3_XYhY9J3bp97N-Ux6AZvuTOzSMTn4fAxizk+2fYA@mail.gmail.com","threadId":"50619","inReplyTo":"20190320180329.22280-1-tmz@pobox.com","subject":"Re: [PATCH v3 0/4] completion.commands: fix multiple command removals","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-21T09:45:08Z","receivedAt":"2019-03-21T09:45:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 21, 2019 at 1:03 AM Todd Zullinger wrote:\n>\n> Hi,\n>\n> Duy Nguyen wrote:\n> > You probably want to drop the comment block about repository setup\n> > inside list_cmds_by_config() too.\n>\n> You're right, of course. Thanks Duy. :)\n>\n> That's the only change since v2.\n\nThe range-diff looks obviously correct. Thanks!\n-- \nDuy\n"},{"id":"372145","messageId":"20190321171836.GC4047@pobox.com","threadId":"50619","inReplyTo":"xmqq8sx9szlx.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 0/4] completion.commands: fix multiple command removals","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-21T17:18:36Z","receivedAt":"2019-03-21T17:18:46Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Junio C Hamano wrote:\n> Todd Zullinger writes:\n> \n>> Other than a follow-up to update the commit reference in 4/4\n>> after 1/4 is in the final form on pu, I think this might be good.\n>> If it's easier, we can skip 4/4 and I'll resend it after the\n>> others are on pu.\n> \n> A series that makes a single topic should expect to be read by a\n> reader who understand the context of individual pieces in the topic,\n> so it is more common to refer to an earlier step of a series from a\n> later step without the object name. I would have written the log\n> message like so instead:\n> \n> completion: use __git when calling --list-cmds\n> \n> As we made --list-cmds read the local configuration file in an\n> earlier step, the completion.commands variable respects repo-level\n> configuration. Use __git which ensures that the proper repo config\n> is consulted if the command line contains 'git -C /some/other/repo'.\n> \n> The whole series looks good to me. Thanks for working on it.\n\nThank you for amending the commit message, and to Jeff, Duy,\nEric, and Gábor for all the help.\n\n-- \nTodd\n"}]}