completion: use native ZSH array pattern matching
PATCH 1/1 completion: use native ZSH array pattern matching
From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= <[email protected]>
When clearing the builtin operations on re-sourcing in the ZSH case we can use the native ${parameters} associative array keys values to get the currently __gitcomp_builtin_* operations using pattern matching instead of using sed.
As also stated in commit 94408dc7, introducing this change the usage of sed has some overhead implications, while ZSH can do this check just using its native syntax.
---
completion: Use native ZSH array pattern matching
When clearing the builtin operations on re-sourcing in the ZSH case we can use the native ${parameters} associative array keys values to get the currently __gitcomp_builtin_* operations using pattern matching instead of using sed.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-645%2F3v1n0%2Fzsh-native-operation-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-645/3v1n0/zsh-native-operation-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/645
contrib/completion/git-completion.bash | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
contrib/completion/git-completion.bash +1 -1
@@ -373,7 +373,7 @@ __gitcomp () # Clear the variables caching builtins' options when (re-)sourcing # the completion script. if [[ -n ${ZSH_VERSION-} ]]; then - unset $(set |sed -ne 's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p') 2>/dev/null + unset ${(M)${(k)parameters[@]}:#__gitcomp_builtin_*} 2>/dev/null else unset $(compgen -v __gitcomp_builtin_) fi
base-commit: d2ecc46c0981fb829fdfb204604ed0a2798cbe07
Signature · 1 line
gitgitgadget
Quoted 32 lines · Marco Trevisan via GitGitGadget
From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= <[email protected]>
When clearing the builtin operations on re-sourcing in the ZSH case we can use the native ${parameters} associative array keys values to get the currently
__gitcomp_builtin_*operations using pattern matching instead of using sed.As also stated in commit
94408dc7, introducing this change the usage of sed has some overhead implications, while ZSH can do this check just using its native syntax.Signed-off-by Marco Trevisan (Treviño) <[email protected]>---
completion: Use native ZSH array pattern matchingWhen clearing the builtin operations on re-sourcing in the ZSH case we can use the native ${parameters} associative array keys values to get the currently __gitcomp_builtin_* operations using pattern matching instead of using sed.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-645%2F3v1n0%2Fzsh-native-operation-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-645/3v1n0/zsh-native-operation-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/645contrib/completion/git-completion.bash | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)contrib/completion/git-completion.bash +0
-0
contrib/completion/git-completion.bash __gitcomp () · line 379
Quoted 8 lines
contrib/completion/git-completion.bash +1
-1@@ -373,7 +373,7 @@ __gitcomp () # Clear the variables caching builtins' options when (re-)sourcing # the completion script. if [[ -n ${ZSH_VERSION-} ]]; then - unset $(set |sed -ne 's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p') 2>/dev/null + unset ${(M)${(k)parameters[@]}:#__gitcomp_builtin_*} 2>/dev/null else unset $(compgen -v __gitcomp_builtin_) fi
This file is necessarily used by both bash and zsh. Does bash (including the bash 3 used on macOS) happen to continue to work with this syntax?
We've had cases in the past where despite some code running under shell A, shell B, which also parsed file, choked on the data because it had to parse it even though it didn't execute it.
If so, and this works there, can you mention that in your commit message for future readers?
Signature · 2 lines
brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204
Carlo Marcelo Arenas Belón
Tested-byQuoted 4 lines · brian m. carlson
This file is necessarily used by both bash and zsh. Does bash (including the bash 3 used on macOS) happen to continue to work with this syntax?
if by that you meant t9902.[150-152] to succeed with macOS bash?:
$ bash --version GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin17) Copyright (C) 2007 Free Software Foundation, Inc.
then I think we can add (on top of master):
Carlo
Quoted 8 lines · Carlo Marcelo Arenas Belón
Quoted 4 lines · brian m. carlson
This file is necessarily used by both bash and zsh. Does bash (including the bash 3 used on macOS) happen to continue to work with this syntax?
if by that you meant t9902.[150-152] to succeed with macOS bash?:
Quoted 7 lines
$ bash --version GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin17) Copyright (C) 2007 Free Software Foundation, Inc.then I think we can add (on top of master):
Yes, that is what I meant. I'm glad to know my question has been answered and things work. I'm okay with the patch as it is in that case, although I'd give bonus points for mentioning that this syntax doesn't regress bash.
Signature · 2 lines
brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204
Quoted 13 lines · brian m. carlson
Quoted 12 lines · Carlo Marcelo Arenas Belón
Quoted 4 lines · brian m. carlson
This file is necessarily used by both bash and zsh. Does bash (including the bash 3 used on macOS) happen to continue to work with this syntax?
if by that you meant t9902.[150-152] to succeed with macOS bash?:
$ bash --version GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin17) Copyright (C) 2007 Free Software Foundation, Inc.
Quoted 8 lines
Quoted 3 lines
then I think we can add (on top of master):
Yes, that is what I meant. I'm glad to know my question has been answered and things work. I'm okay with the patch as it is in that case, although I'd give bonus points for mentioning that this syntax doesn't regress bash.
True. And we would want to also have tested-by on more recent versions of bash, no?
Quoted 8 lines · Junio C Hamano
Quoted 4 lines · brian m. carlson
Yes, that is what I meant. I'm glad to know my question has been answered and things work. I'm okay with the patch as it is in that case, although I'd give bonus points for mentioning that this syntax doesn't regress bash.
True. And we would want to also have tested-by on more recent versions of bash, no?
Sure, such testing would be welcome, but I believe those are tested with our tests on most platforms. macOS is special because it uses the last GPLv2 version of bash, which is less capable in some ways. I assumed that bash would not be more likely to break here in newer versions, but perhaps I shouldn't make that assumption.
Signature · 2 lines
brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204
Quoted 7 lines · brian m. carlson
Quoted 6 lines · Junio C Hamano
Quoted 4 lines · brian m. carlson
Yes, that is what I meant. I'm glad to know my question has been answered and things work. I'm okay with the patch as it is in that case, although I'd give bonus points for mentioning that this syntax doesn't regress bash.
Quoted 8 lines
Quoted 2 lines
True. And we would want to also have tested-by on more recent versions of bash, no?
Sure, such testing would be welcome, but I believe those are tested with our tests on most platforms. macOS is special because it uses the last GPLv2 version of bash, which is less capable in some ways. I assumed that bash would not be more likely to break here in newer versions, but perhaps I shouldn't make that assumption.
In any case, a few integration into 'pu' with the patch have been already made e.g. https://travis-ci.org/github/git/git/builds/692290894
so we should be good ;-)
Quoted 6 lines · Marco Trevisan via GitGitGadget
From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= <[email protected]>
When clearing the builtin operations on re-sourcing in the ZSH case we can use the native ${parameters} associative array keys values to get the currently
__gitcomp_builtin_*operations using pattern matching instead of using sed.
"We can" may be a statement of fact, but by itself it does not make a convincing argument why we should have this change in our codebase.
Is this change about correctness? Is it about performance? Or something else?
If it is about correctness, "the current code fails in this way given this input, but with the new code the breakage is gone" would be a good justification to give in the proposed log message. If it is about performance, of course a measured performance numbers would make an objective justification that is hard to argue with, especially if you can convince people that the patch does not change the behaviour in any negative way. If it is about something else, well, there would be an appropriate way to justify the change, depending on what it is ;-)
Thanks.