completion: fix incorrect bash/zsh string equality check
PATCH 1/1 completion: fix incorrect bash/zsh string equality check
From: Robert Estelle <[email protected]>
In the basic [/test command, the string equality operator is a single =. The == operator is only available in [[, which is a bash-ism also supported by zsh.
This mix-up was causing the following completion error in zsh:
Quoted 1 line
__git_ls_files_helper:7: = not found
(That message refers to the extraneous symbol in == ← =).
This updates that comparison to use the extended [[ … ]] conditional for consistency with the other checks in this file.
---
completion: Fix incorrect bash/zsh string equality check
This fixes an error in contrib/completion/git-completion.bash caused by the incorrect use of == (vs. single =) inside a basic [/test command. Double-equals == should only be used with the extended [[ comparison.
This was causing the following completion error in zsh:
> __git_ls_files_helper:7: = not found
That message refers to the extraneous = symbol in ==.
This updates that comparison to use the extended [[ … ]] conditional for consistency with the other checks in this file.
Note that there may be some contributing cause to this error related to emulation mode inheritance/stickiness, since it seems that the function is intended to run with emulate ksh and that does not appear to be happening properly. Nevertheless, fixing this comparison fixes this particular error in a compatible way, and I have not observed any other errors.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1096%2Frwe%2Ffix-completion-sh-eq-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1096/rwe/fix-completion-sh-eq-v1
Pull-Request: https://github.com/git/git/pull/1096
contrib/completion/git-completion.bash | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
contrib/completion/git-completion.bash +1 -1
@@ -515,7 +515,7 @@ __gitcomp_file () # argument, and using the options specified in the second argument. __git_ls_files_helper () { - if [ "$2" == "--committable" ]; then + if [[ "$2" == "--committable" ]]; then __git -C "$1" -c core.quotePath=false diff-index \ --name-only --relative HEAD -- "${3//\\/\\\\}*" else
base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
Signature · 1 line
gitgitgadget
Quoted 3 lines · Robert Estelle via GitGitGadget
This fixes an error in contrib/completion/git-completion.bash caused by
the incorrect use of == (vs. single =) inside a basic [/test command.
Double-equals == should only be used with the extended [[ comparison.Curious.
Would it be equally a valid fix to use "=" instead of "==", or would that change the meaning? This is a bash-only piece of code, so use of [[ ... ]] is not technically incorrect, but if the basic [] works well enough with "=", we should prefer that.
Thanks.
Quoted 6 lines · Junio C Hamano
Quoted 3 lines · Robert Estelle via GitGitGadget
This fixes an error in contrib/completion/git-completion.bash caused by the incorrect use of == (vs. single =) inside a basic [/test command. Double-equals == should only be used with the extended [[ comparison.
Quoted 6 lines
Curious.
Would it be equally a valid fix to use "=" instead of "==", or would that change the meaning? This is a bash-only piece of code, so use of [[ ... ]] is not technically incorrect, but if the basic [] works well enough with "=", we should prefer that.
It's actually preferable in most cases to use [ and = rather than [[ and ==, because the former looks for strict equality and the latter looks for pattern matching. If one is placing a glob pattern on the right side, then [[ and == can be desirable, but otherwise it's better to stick to the POSIX syntax.
Signature · 2 lines
brian m. carlson (he/him or they/them) Toronto, Ontario, CA
Quoted 3 lines · brian m. carlson
It's actually preferable in most cases to use [ and = rather than [[ and ==, because the former looks for strict equality and the latter looks for pattern matching.
Yep, I agree with these style notes, but went with the prevalent style to avoid confusion. And note that the args here are both quoted, and so are treated as a literal comparison rather than globbed.
That said, whoever maintains that script, since this is a one-line change, I'm not insistent about one way of addressing this or another :)
Quoted 4 lines · Junio C Hamano
Would it be equally a valid fix to use "=" instead of "==", or would that change the meaning? This is a bash-only piece of code, so use of [[ ... ]] is not technically incorrect, but if the basic [] works well enough with "=", we should prefer that.
Yes, [ is preferable for portability and they'd behave the same in this case. I consciously chose to use [[ because that's what all the other comparisons in that script use. (I think I noted that in the commit message, maybe). I think there's value in consistency, and not enough value of [ over [[ to justify changing all the other lines.
Quoted 11 lines · Robert Estelle
Quoted 4 lines · Junio C Hamano
Would it be equally a valid fix to use "=" instead of "==", or would that change the meaning? This is a bash-only piece of code, so use of [[ ... ]] is not technically incorrect, but if the basic [] works well enough with "=", we should prefer that.
Yes,
[is preferable for portability and they'd behave the same in this case. I consciously chose to use[[because that's what all the other comparisons in that script use. (I think I noted that in the commit message, maybe). I think there's value in consistency, and not enough value of[over[[to justify changing all the other lines.
I do not know if we mind eventual consistency using [] not [[]], but this topic is certainly not about aiming for consistency.
If the difference affects correctness, as brian mentioned, that is a different matter. We should use [ x = y ] in this patch while leaving the existing uses of [[ x == y ]].
Later in a separate patch series, we may need to examine other uses of [[ x == y ]] and correct the ones that do not need/want the pattern matching semantics to use [ x = y ].
The choice between the two does not affect correctness, it's purely stylistic here. It only would affect correctness for unquoted arguments or extended comparison operators. Those *are* in use elsewhere in this script and force the use of [[ in those places.
Keep in mind also that this is an autocomplete script. Although it's sourced by both bash and zsh, it does not make sense to attempt to make it work for bare POSIX sh.
Quoted 26 lines · Junio C Hamano
Quoted 11 lines · Robert Estelle
Quoted 4 lines · Junio C Hamano
Would it be equally a valid fix to use "=" instead of "==", or would that change the meaning? This is a bash-only piece of code, so use of [[ ... ]] is not technically incorrect, but if the basic [] works well enough with "=", we should prefer that.
Yes,
[is preferable for portability and they'd behave the same in this case. I consciously chose to use[[because that's what all the other comparisons in that script use. (I think I noted that in the commit message, maybe). I think there's value in consistency, and not enough value of[over[[to justify changing all the other lines.I do not know if we mind eventual consistency using [] not [[]], but this topic is certainly not about aiming for consistency.
If the difference affects correctness, as brian mentioned, that is a different matter. We should use [ x = y ] in this patch while leaving the existing uses of [[ x == y ]].
Later in a separate patch series, we may need to examine other uses of [[ x == y ]] and correct the ones that do not need/want the pattern matching semantics to use [ x = y ].
Quoted 8 lines · Robert Estelle
The choice between the two does not affect correctness, it's purely stylistic here. It only would affect correctness for unquoted arguments or extended comparison operators. Those *are* in use elsewhere in this script and force the use of
[[in those places.Keep in mind also that this is an autocomplete script. Although it's sourced by both bash and zsh, it does not make sense to attempt to make it work for bare POSIX sh.
Nobody is trying to. It is more for reducing the risk of people shooting their foot by cutting and pasting without thinking.
When you do not mean pattern matching and want exact matching, even if it is guaranteed that the data you pass through the codepath does not have pattern to cause the former, hence the distinction between [[ x == y ]] and [ x = y ] does not make a difference, that is a mere happenstance, and use of [ x = y ] is the correct thing to do in such a case.
I considered dropping this patch entirely since this discussion was going so far off the rails for a single-char fix, but I hate to leave things hanging…
Quoted 6 lines
When you do not mean pattern matching and want exact matching, even if it is guaranteed that the data you pass through the codepath does not have pattern to cause the former, hence the distinction between [[ x == y ]] and [ x = y ] does not make a difference, that is a mere happenstance, and use of [ x = y ] is the correct thing to do in such a case.
I think you've misread what's going on here, or might be misremembering the shell quoting rules (which would surprise me!).
My assertion about their equivalence was syntactic: there's no happenstance or reliance on codepaths whatsoever.
If you write:
[[ "$x" == "$y" ]]
then it doesn't matter what x and y contain. They're quoted: it's just a literal string comparison of their values.
Their values can contain quotes, newlines, spaces, asterisks, emoji, etc. and none of that gets any special interpretation.
Regardless, since they're functionally equivalent, I've pushed up a change to do it the other way and will submit that shortly.
Best,
Robert
Quoted 21 lines · Junio C Hamano
Quoted 8 lines · Robert Estelle
The choice between the two does not affect correctness, it's purely stylistic here. It only would affect correctness for unquoted arguments or extended comparison operators. Those *are* in use elsewhere in this script and force the use of
[[in those places.Keep in mind also that this is an autocomplete script. Although it's sourced by both bash and zsh, it does not make sense to attempt to make it work for bare POSIX sh.
Nobody is trying to. It is more for reducing the risk of people shooting their foot by cutting and pasting without thinking.
When you do not mean pattern matching and want exact matching, even if it is guaranteed that the data you pass through the codepath does not have pattern to cause the former, hence the distinction between [[ x == y ]] and [ x = y ] does not make a difference, that is a mere happenstance, and use of [ x = y ] is the correct thing to do in such a case.
PATCH v2 1/1 completion: fix incorrect bash/zsh string equality check
In reply to Robert Estelle via GitGitGadgetFrom: Robert Estelle <[email protected]>
In the basic [/test command, the string equality operator is a single =. The == operator is only available in [[, which is a bash-ism also supported by zsh.
This mix-up was causing the following completion error in zsh:
Quoted 1 line
__git_ls_files_helper:7: = not found
(That message refers to the extraneous symbol in == ← =).
This updates that comparison to use a single = inside the basic [ … ] test conditional.
Although this fix is inconsistent with the other comparisons in this file, which use [[ … == … ]], and the two expressions are functionally identical in this context, that approach was rejected due to a preference for [.
---
completion: Fix incorrect bash/zsh string equality check
v2: This updates the comparison to remove the extraneous = symbol in ==, and use the [ … ] conditional instead.
v1: (rejected) updated that comparison to use the extended [[ … ]] conditional for consistency with the other checks in this file.
This fixes an error in contrib/completion/git-completion.bash caused by the incorrect use of == (vs. single =) inside a basic [/test command. Double-equals == should only be used with the extended [[ comparison.
This was causing the following completion error in zsh:
> __git_ls_files_helper:7: = not found
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1096%2Frwe%2Ffix-completion-sh-eq-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1096/rwe/fix-completion-sh-eq-v2
Pull-Request: https://github.com/git/git/pull/1096
Range-diff vs v1:
1: 6fd09347385 ! 1: eee166c8c99 completion: fix incorrect bash/zsh string equality check
@@ Commit message
(That message refers to the extraneous symbol in == ← =).
- This updates that comparison to use the extended [[ … ]] conditional
- for consistency with the other checks in this file.
+ This updates that comparison to use a single = inside the
+ basic [ … ] test conditional.
+
+ Although this fix is inconsistent with the other comparisons in this
+ file, which use [[ … == … ]], and the two expressions are functionally
+ identical in this context, that approach was rejected due to a
+ preference for [.
@@ contrib/completion/git-completion.bash: __gitcomp_file ()
__git_ls_files_helper ()
{
- if [ "$2" == "--committable" ]; then
-+ if [[ "$2" == "--committable" ]]; then
++ if [ "$2" = "--committable" ]; then
__git -C "$1" -c core.quotePath=false diff-index \
--name-only --relative HEAD -- "${3//\\/\\\\}*"
else
contrib/completion/git-completion.bash | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
contrib/completion/git-completion.bash +1 -1
@@ -515,7 +515,7 @@ __gitcomp_file () # argument, and using the options specified in the second argument. __git_ls_files_helper () { - if [ "$2" == "--committable" ]; then + if [ "$2" = "--committable" ]; then __git -C "$1" -c core.quotePath=false diff-index \ --name-only --relative HEAD -- "${3//\\/\\\\}*" else
base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
Signature · 1 line
gitgitgadget
Quoted 14 lines · Robert Estelle via GitGitGadget
contrib/completion/git-completion.bash +1
-1@@ -515,7 +515,7 @@ __gitcomp_file () # argument, and using the options specified in the second argument. __git_ls_files_helper () { - if [ "$2" == "--committable" ]; then + if [ "$2" = "--committable" ]; then __git -C "$1" -c core.quotePath=false diff-index \ --name-only --relative HEAD -- "${3//\\/\\\\}*" else
Quoted 1 line
base-commit:
225bc32a989d7a22fa6addafd4ce7dcd04675dbf
Thanks. We can trace this back to April 2013, if not earlier. It is sort of surprising that nobody else has noticed it since then X-<.
Will queue. Thanks, again.