threads / patch / 56661

patchcompletion: fix incorrect bash/zsh string equality check

Subject: [PATCH] completion: fix incorrect bash/zsh string equality check

## tl;dr

11 messages between Oct 7, 2021 and Oct 28, 2021. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Robert Estelle via GitGitGadget· Oct 7, 2021, 21:39 UTC · lore
From: Robert Estelle <robertestelle@gmail.com>

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:
> __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.

Signed-off-by: Robert Estelle <robertestelle@gmail.com>
---
    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(-)
Show changes to contrib/completion/git-completion.bash +1 −1
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 4bdd27ddc87..14de5efa734 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -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

-- 
gitgitgadget
Junio C Hamano· Oct 8, 2021, 20:50 UTC · re: Robert Estelle via GitGitGadget · lore

Re: [PATCH] completion: fix incorrect bash/zsh string equality check

"Robert Estelle via GitGitGadget" <gitgitgadget@gmail.com> writes:
>     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.
brian m. carlson· Oct 8, 2021, 20:57 UTC · re: Junio C Hamano · lore

Re: [PATCH] completion: fix incorrect bash/zsh string equality check

On 2021-10-08 at 20:50:33, Junio C Hamano wrote:
Show 12 quoted lines
> "Robert Estelle via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> >     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.

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.

-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
Robert Estelle· Oct 8, 2021, 22:17 UTC · re: brian m. carlson · lore

Re: [PATCH] completion: fix incorrect bash/zsh string equality check

On Fri, Oct 8, 2021 at 1:58 PM brian m. carlson <sandals@crustytoothpaste.net> wrote:

> 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 :)

Robert Estelle· Oct 8, 2021, 22:05 UTC · re: Junio C Hamano · lore

Re: [PATCH] completion: fix incorrect bash/zsh string equality check

On Fri, Oct 8, 2021 at 1:50 PM Junio C Hamano <gitster@pobox.com> wrote:
> 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.

Junio C Hamano· Oct 8, 2021, 22:16 UTC · re: Robert Estelle · lore

Re: [PATCH] completion: fix incorrect bash/zsh string equality check

Robert Estelle <robertestelle@gmail.com> writes:
Show 11 quoted lines
> On Fri, Oct 8, 2021 at 1:50 PM Junio C Hamano <gitster@pobox.com> wrote:
>> 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 ].

Robert Estelle· Oct 8, 2021, 22:26 UTC · re: Junio C Hamano · lore

Re: [PATCH] completion: fix incorrect bash/zsh string equality check

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.

On Fri, Oct 8, 2021 at 3:16 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 26 quoted lines
>
> Robert Estelle <robertestelle@gmail.com> writes:
>
> > On Fri, Oct 8, 2021 at 1:50 PM Junio C Hamano <gitster@pobox.com> wrote:
> >> 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 ].
>
Junio C Hamano· Oct 8, 2021, 23:09 UTC · re: Robert Estelle · lore

Re: [PATCH] completion: fix incorrect bash/zsh string equality check

Robert Estelle <robertestelle@gmail.com> writes:
Show 8 quoted lines
> 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.

Robert Estelle· Oct 25, 2021, 22:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] completion: fix incorrect bash/zsh string equality check

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…

Show 6 quoted 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

On Fri, Oct 8, 2021 at 4:09 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 21 quoted lines
>
> Robert Estelle <robertestelle@gmail.com> writes:
>
> > 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.
Robert Estelle via GitGitGadget· Oct 25, 2021, 22:29 UTC · re: Robert Estelle via GitGitGadget · lore

[PATCH v2] completion: fix incorrect bash/zsh string equality check

From: Robert Estelle <robertestelle@gmail.com>

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:
> __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 `[`.

Signed-off-by: Robert Estelle <robertestelle@gmail.com>
---
    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 `[`.
      
          Signed-off-by: Robert Estelle <robertestelle@gmail.com>
      
     @@ 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(-)
Show changes to contrib/completion/git-completion.bash +1 −1
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 4bdd27ddc87..8ca9b15f21d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -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

-- 
gitgitgadget
Junio C Hamano· Oct 28, 2021, 16:36 UTC · re: Robert Estelle via GitGitGadget · lore

Re: [PATCH v2] completion: fix incorrect bash/zsh string equality check

"Robert Estelle via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 15 quoted lines
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 4bdd27ddc87..8ca9b15f21d 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -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

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.

← back to recent threads