{"thread":{"id":"53556","subject":"[PATCH] completion: use native ZSH array pattern matching","startedAt":"2020-05-26T19:13:23Z","lastAt":"2020-05-28T23:01:29Z","messageCount":8,"participants":["Marco Trevisan via GitGitGadget","brian m. carlson","Carlo Marcelo Arenas Belón","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"398597","messageId":"pull.645.git.1590520398132.gitgitgadget@gmail.com","threadId":"53556","inReplyTo":null,"subject":"[PATCH] completion: use native ZSH array pattern matching","fromName":"Marco Trevisan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-26T19:13:17Z","receivedAt":"2020-05-26T19:13:23Z","isPatch":true,"sender":{"key":"name:Marco Trevisan","avatar":null},"body":"From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= \n\nWhen clearing the builtin operations on re-sourcing in the ZSH case we\ncan use the native ${parameters} associative array keys values to get\nthe currently `__gitcomp_builtin_*` operations using pattern matching\ninstead of using sed.\n\nAs also stated in commit 94408dc7, introducing this change the usage of\nsed has some overhead implications, while ZSH can do this check just\nusing its native syntax.\n\nSigned-off-by: Marco Trevisan (Treviño) \n---\n completion: Use native ZSH array pattern matching\n \n When clearing the builtin operations on re-sourcing in the ZSH case we\n can use the native ${parameters} associative array keys values to get\n the currently __gitcomp_builtin_* operations using pattern matching\n instead of using sed.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-645%2F3v1n0%2Fzsh-native-operation-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-645/3v1n0/zsh-native-operation-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/645\n\n contrib/completion/git-completion.bash | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 70ad04e1b2a..ad6934a3864 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -373,7 +373,7 @@ __gitcomp ()\n # Clear the variables caching builtins' options when (re-)sourcing\n # the completion script.\n if [[ -n ${ZSH_VERSION-} ]]; then\n-\tunset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n+\tunset ${(M)${(k)parameters[@]}:#__gitcomp_builtin_*} 2>/dev/null\n else\n \tunset $(compgen -v __gitcomp_builtin_)\n fi\n\nbase-commit: d2ecc46c0981fb829fdfb204604ed0a2798cbe07\n-- \ngitgitgadget\n"},{"id":"398607","messageId":"20200526235834.GG1915090@camp.crustytoothpaste.net","threadId":"53556","inReplyTo":"pull.645.git.1590520398132.gitgitgadget@gmail.com","subject":"Re: [PATCH] completion: use native ZSH array pattern matching","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-05-26T23:58:34Z","receivedAt":"2020-05-26T23:59:12Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-05-26 at 19:13:17, Marco Trevisan via GitGitGadget wrote:\n> From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= \n> \n> When clearing the builtin operations on re-sourcing in the ZSH case we\n> can use the native ${parameters} associative array keys values to get\n> the currently `__gitcomp_builtin_*` operations using pattern matching\n> instead of using sed.\n> \n> As also stated in commit 94408dc7, introducing this change the usage of\n> sed has some overhead implications, while ZSH can do this check just\n> using its native syntax.\n> \n> Signed-off-by: Marco Trevisan (Treviño) \n> ---\n> completion: Use native ZSH array pattern matching\n> \n> When clearing the builtin operations on re-sourcing in the ZSH case we\n> can use the native ${parameters} associative array keys values to get\n> the currently __gitcomp_builtin_* operations using pattern matching\n> instead of using sed.\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-645%2F3v1n0%2Fzsh-native-operation-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-645/3v1n0/zsh-native-operation-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/645\n> \n> contrib/completion/git-completion.bash | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 70ad04e1b2a..ad6934a3864 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -373,7 +373,7 @@ __gitcomp ()\n> # Clear the variables caching builtins' options when (re-)sourcing\n> # the completion script.\n> if [[ -n ${ZSH_VERSION-} ]]; then\n> -\tunset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n> +\tunset ${(M)${(k)parameters[@]}:#__gitcomp_builtin_*} 2>/dev/null\n> else\n> \tunset $(compgen -v __gitcomp_builtin_)\n> fi\n\nThis file is necessarily used by both bash and zsh. Does bash\n(including the bash 3 used on macOS) happen to continue to work with\nthis syntax?\n\nWe've had cases in the past where despite some code running under shell\nA, shell B, which also parsed file, choked on the data because it\nhad to parse it even though it didn't execute it.\n\nIf so, and this works there, can you mention that in your commit message\nfor future readers?\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"398610","messageId":"20200527061359.GB40564@Carlos-MBP","threadId":"53556","inReplyTo":"20200526235834.GG1915090@camp.crustytoothpaste.net","subject":"Re: [PATCH] completion: use native ZSH array pattern matching","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-27T06:13:59Z","receivedAt":"2020-05-27T06:14:04Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, May 26, 2020 at 11:58:34PM +0000, brian m. carlson wrote:\n> \n> This file is necessarily used by both bash and zsh. Does bash\n> (including the bash 3 used on macOS) happen to continue to work with\n> this syntax?\n\nif by that you meant t9902.[150-152] to succeed with macOS bash?:\n\n$ bash --version\nGNU bash, version 3.2.57(1)-release (x86_64-apple-darwin17)\nCopyright (C) 2007 Free Software Foundation, Inc.\n\nthen I think we can add (on top of master):\n\nTested-by: Carlo Marcelo Arenas Belón \n\nCarlo\n"},{"id":"398720","messageId":"20200528021727.GH1915090@camp.crustytoothpaste.net","threadId":"53556","inReplyTo":"20200527061359.GB40564@Carlos-MBP","subject":"Re: [PATCH] completion: use native ZSH array pattern matching","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-05-28T02:17:27Z","receivedAt":"2020-05-28T02:18:07Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-05-27 at 06:13:59, Carlo Marcelo Arenas Belón wrote:\n> On Tue, May 26, 2020 at 11:58:34PM +0000, brian m. carlson wrote:\n> > \n> > This file is necessarily used by both bash and zsh. Does bash\n> > (including the bash 3 used on macOS) happen to continue to work with\n> > this syntax?\n> \n> if by that you meant t9902.[150-152] to succeed with macOS bash?:\n> \n> $ bash --version\n> GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin17)\n> Copyright (C) 2007 Free Software Foundation, Inc.\n> \n> then I think we can add (on top of master):\n> \n> Tested-by: Carlo Marcelo Arenas Belón \n\nYes, that is what I meant. I'm glad to know my question has been\nanswered and things work. I'm okay with the patch as it is in that\ncase, although I'd give bonus points for mentioning that this syntax\ndoesn't regress bash.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"398748","messageId":"xmqqd06o3vhy.fsf@gitster.c.googlers.com","threadId":"53556","inReplyTo":"20200528021727.GH1915090@camp.crustytoothpaste.net","subject":"Re: [PATCH] completion: use native ZSH array pattern matching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-28T15:54:49Z","receivedAt":"2020-05-28T15:54:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> On 2020-05-27 at 06:13:59, Carlo Marcelo Arenas Belón wrote:\n>> On Tue, May 26, 2020 at 11:58:34PM +0000, brian m. carlson wrote:\n>> > \n>> > This file is necessarily used by both bash and zsh. Does bash\n>> > (including the bash 3 used on macOS) happen to continue to work with\n>> > this syntax?\n>> \n>> if by that you meant t9902.[150-152] to succeed with macOS bash?:\n>> \n>> $ bash --version\n>> GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin17)\n>> Copyright (C) 2007 Free Software Foundation, Inc.\n>> \n>> then I think we can add (on top of master):\n>> \n>> Tested-by: Carlo Marcelo Arenas Belón \n>\n> Yes, that is what I meant. I'm glad to know my question has been\n> answered and things work. I'm okay with the patch as it is in that\n> case, although I'd give bonus points for mentioning that this syntax\n> doesn't regress bash.\n\nTrue. And we would want to also have tested-by on more recent\nversions of bash, no?\n"},{"id":"398750","messageId":"xmqq8shc3ukc.fsf@gitster.c.googlers.com","threadId":"53556","inReplyTo":"pull.645.git.1590520398132.gitgitgadget@gmail.com","subject":"Re: [PATCH] completion: use native ZSH array pattern matching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-28T16:14:59Z","receivedAt":"2020-05-28T16:15:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Marco Trevisan via GitGitGadget\" writes:\n\n> From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= \n>\n> When clearing the builtin operations on re-sourcing in the ZSH case we\n> can use the native ${parameters} associative array keys values to get\n> the currently `__gitcomp_builtin_*` operations using pattern matching\n> instead of using sed.\n\n\"We can\" may be a statement of fact, but by itself it does not make\na convincing argument why we should have this change in our\ncodebase. \n\nIs this change about correctness? Is it about performance?\nOr something else?\n\nIf it is about correctness, \"the current code fails in this way\ngiven this input, but with the new code the breakage is gone\" would\nbe a good justification to give in the proposed log message. If it\nis about performance, of course a measured performance numbers would\nmake an objective justification that is hard to argue with,\nespecially if you can convince people that the patch does not change\nthe behaviour in any negative way. If it is about something else,\nwell, there would be an appropriate way to justify the change,\ndepending on what it is ;-)\n\nThanks.\n"},{"id":"398809","messageId":"20200528225733.GI1915090@camp.crustytoothpaste.net","threadId":"53556","inReplyTo":"xmqqd06o3vhy.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] completion: use native ZSH array pattern matching","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-05-28T22:57:33Z","receivedAt":"2020-05-28T22:57:43Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-05-28 at 15:54:49, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n> > Yes, that is what I meant. I'm glad to know my question has been\n> > answered and things work. I'm okay with the patch as it is in that\n> > case, although I'd give bonus points for mentioning that this syntax\n> > doesn't regress bash.\n> \n> True. And we would want to also have tested-by on more recent\n> versions of bash, no?\n\nSure, such testing would be welcome, but I believe those are tested with\nour tests on most platforms. macOS is special because it uses the last\nGPLv2 version of bash, which is less capable in some ways. I assumed\nthat bash would not be more likely to break here in newer versions, but\nperhaps I shouldn't make that assumption.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"398810","messageId":"xmqqtuzz1x72.fsf@gitster.c.googlers.com","threadId":"53556","inReplyTo":"20200528225733.GI1915090@camp.crustytoothpaste.net","subject":"Re: [PATCH] completion: use native ZSH array pattern matching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-28T23:01:05Z","receivedAt":"2020-05-28T23:01:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> On 2020-05-28 at 15:54:49, Junio C Hamano wrote:\n>> \"brian m. carlson\" writes:\n>> > Yes, that is what I meant. I'm glad to know my question has been\n>> > answered and things work. I'm okay with the patch as it is in that\n>> > case, although I'd give bonus points for mentioning that this syntax\n>> > doesn't regress bash.\n>> \n>> True. And we would want to also have tested-by on more recent\n>> versions of bash, no?\n>\n> Sure, such testing would be welcome, but I believe those are tested with\n> our tests on most platforms. macOS is special because it uses the last\n> GPLv2 version of bash, which is less capable in some ways. I assumed\n> that bash would not be more likely to break here in newer versions, but\n> perhaps I shouldn't make that assumption.\n\nIn any case, a few integration into 'pu' with the patch have been\nalready made e.g. https://travis-ci.org/github/git/git/builds/692290894\nso we should be good ;-)\n\n"}]}