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

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

[Robert Estelle via GitGitGadget]

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

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

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

[Junio C Hamano]

"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.

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1027 bytes --]

On 2021-10-08 at 20:50:33, Junio C Hamano wrote:
> "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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Robert Estelle]

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.

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

[Junio C Hamano]

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 ].

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

[Robert Estelle]

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

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

[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.

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

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

[Junio C Hamano]

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.

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

[Robert Estelle]

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…

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

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

[Robert Estelle via GitGitGadget]

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

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

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

[Junio C Hamano]

"Robert Estelle via GitGitGadget" <gitgitgadget@gmail.com> writes:

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