| Message ID | pull.1096.git.git.1633642772432.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | completion: fix incorrect bash/zsh string equality check | expand |
"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.
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.
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.
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 ].
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 :)
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 ]. >
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.
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.
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