{"thread":{"id":"56661","subject":"[PATCH] completion: fix incorrect bash/zsh string equality check","startedAt":"2021-10-07T21:39:36Z","lastAt":"2021-10-28T16:36:39Z","messageCount":11,"participants":["Robert Estelle via GitGitGadget","Junio C Hamano","brian m. carlson","Robert Estelle"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"438240","messageId":"pull.1096.git.git.1633642772432.gitgitgadget@gmail.com","threadId":"56661","inReplyTo":null,"subject":"[PATCH] completion: fix incorrect bash/zsh string equality check","fromName":"Robert Estelle via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-07T21:39:32Z","receivedAt":"2021-10-07T21:39:36Z","isPatch":true,"sender":{"key":"robertestelle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/447615?v=4"},"body":"From: Robert Estelle \n\nIn the basic `[`/`test` command, the string equality operator is a\nsingle `=`. The `==` operator is only available in `[[`, which is a\nbash-ism also supported by zsh.\n\nThis mix-up was causing the following completion error in zsh:\n> __git_ls_files_helper:7: = not found\n\n(That message refers to the extraneous symbol in `==` ← `=`).\n\nThis updates that comparison to use the extended `[[ … ]]` conditional\nfor consistency with the other checks in this file.\n\nSigned-off-by: Robert Estelle \n---\n completion: Fix incorrect bash/zsh string equality check\n \n This fixes an error in contrib/completion/git-completion.bash caused by\n the incorrect use of == (vs. single =) inside a basic [/test command.\n Double-equals == should only be used with the extended [[ comparison.\n \n This was causing the following completion error in zsh:\n \n > __git_ls_files_helper:7: = not found\n \n \n That message refers to the extraneous = symbol in ==.\n \n This updates that comparison to use the extended [[ … ]] conditional for\n consistency with the other checks in this file.\n \n Note that there may be some contributing cause to this error related to\n emulation mode inheritance/stickiness, since it seems that the function\n is intended to run with emulate ksh and that does not appear to be\n happening properly. Nevertheless, fixing this comparison fixes this\n particular error in a compatible way, and I have not observed any other\n errors.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1096%2Frwe%2Ffix-completion-sh-eq-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1096/rwe/fix-completion-sh-eq-v1\nPull-Request: https://github.com/git/git/pull/1096\n\n contrib/completion/git-completion.bash | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 4bdd27ddc87..14de5efa734 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -515,7 +515,7 @@ __gitcomp_file ()\n # argument, and using the options specified in the second argument.\n __git_ls_files_helper ()\n {\n-\tif [ \"$2\" == \"--committable\" ]; then\n+\tif [[ \"$2\" == \"--committable\" ]]; then\n \t\t__git -C \"$1\" -c core.quotePath=false diff-index \\\n \t\t\t--name-only --relative HEAD -- \"${3//\\\\/\\\\\\\\}*\"\n \telse\n\nbase-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf\n-- \ngitgitgadget\n"},{"id":"438349","messageId":"xmqqbl3zjl5y.fsf@gitster.g","threadId":"56661","inReplyTo":"pull.1096.git.git.1633642772432.gitgitgadget@gmail.com","subject":"Re: [PATCH] completion: fix incorrect bash/zsh string equality check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-08T20:50:33Z","receivedAt":"2021-10-08T20:50:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Robert Estelle via GitGitGadget\" writes:\n\n> This fixes an error in contrib/completion/git-completion.bash caused by\n> the incorrect use of == (vs. single =) inside a basic [/test command.\n> Double-equals == should only be used with the extended [[ comparison.\n\nCurious. \n\nWould it be equally a valid fix to use \"=\" instead of \"==\", or would\nthat change the meaning? This is a bash-only piece of code, so use\nof [[ ... ]] is not technically incorrect, but if the basic [] works\nwell enough with \"=\", we should prefer that.\n\nThanks.\n\n"},{"id":"438350","messageId":"YWCwvOdW/L5aMpr7@camp.crustytoothpaste.net","threadId":"56661","inReplyTo":"xmqqbl3zjl5y.fsf@gitster.g","subject":"Re: [PATCH] completion: fix incorrect bash/zsh string equality check","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-10-08T20:57:32Z","receivedAt":"2021-10-08T20:58:13Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-10-08 at 20:50:33, Junio C Hamano wrote:\n> \"Robert Estelle via GitGitGadget\" writes:\n> \n> > This fixes an error in contrib/completion/git-completion.bash caused by\n> > the incorrect use of == (vs. single =) inside a basic [/test command.\n> > Double-equals == should only be used with the extended [[ comparison.\n> \n> Curious.\n> \n> Would it be equally a valid fix to use \"=\" instead of \"==\", or would\n> that change the meaning? This is a bash-only piece of code, so use\n> of [[ ... ]] is not technically incorrect, but if the basic [] works\n> well enough with \"=\", we should prefer that.\n\nIt's actually preferable in most cases to use [ and = rather than [[ and\n==, because the former looks for strict equality and the latter looks\nfor pattern matching. If one is placing a glob pattern on the right\nside, then [[ and == can be desirable, but otherwise it's better to\nstick to the POSIX syntax.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"438371","messageId":"CAMmwyQpP0m6dTdC-L9abZqk9nJkcYy=j4_s0+EdGDr__pykeXQ@mail.gmail.com","threadId":"56661","inReplyTo":"xmqqbl3zjl5y.fsf@gitster.g","subject":"Re: [PATCH] completion: fix incorrect bash/zsh string equality check","fromName":"Robert Estelle","fromEmail":"robertestelle@gmail.com","sentAt":"2021-10-08T22:05:15Z","receivedAt":"2021-10-08T22:05:45Z","isPatch":true,"sender":{"key":"robertestelle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/447615?v=4"},"body":"On Fri, Oct 8, 2021 at 1:50 PM Junio C Hamano wrote:\n> Would it be equally a valid fix to use \"=\" instead of \"==\", or would\n> that change the meaning? This is a bash-only piece of code, so use\n> of [[ ... ]] is not technically incorrect, but if the basic [] works\n> well enough with \"=\", we should prefer that.\n\nYes, `[` is preferable for portability and they'd behave the same in\nthis case. I consciously chose to use `[[` because that's what all the\nother comparisons in that script use. (I think I noted that in the\ncommit message, maybe). I think there's value in consistency, and not\nenough value of `[` over `[[` to justify changing all the other lines.\n"},{"id":"438373","messageId":"xmqqh7dri2m1.fsf@gitster.g","threadId":"56661","inReplyTo":"CAMmwyQpP0m6dTdC-L9abZqk9nJkcYy=j4_s0+EdGDr__pykeXQ@mail.gmail.com","subject":"Re: [PATCH] completion: fix incorrect bash/zsh string equality check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-08T22:16:38Z","receivedAt":"2021-10-08T22:16:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Estelle writes:\n\n> On Fri, Oct 8, 2021 at 1:50 PM Junio C Hamano wrote:\n>> Would it be equally a valid fix to use \"=\" instead of \"==\", or would\n>> that change the meaning? This is a bash-only piece of code, so use\n>> of [[ ... ]] is not technically incorrect, but if the basic [] works\n>> well enough with \"=\", we should prefer that.\n>\n> Yes, `[` is preferable for portability and they'd behave the same in\n> this case. I consciously chose to use `[[` because that's what all the\n> other comparisons in that script use. (I think I noted that in the\n> commit message, maybe). I think there's value in consistency, and not\n> enough value of `[` over `[[` to justify changing all the other lines.\n\nI do not know if we mind eventual consistency using [] not [[]], but\nthis topic is certainly not about aiming for consistency.\n\nIf the difference affects correctness, as brian mentioned, that is a\ndifferent matter. We should use [ x = y ] in this patch while\nleaving the existing uses of [[ x == y ]].\n\nLater in a separate patch series, we may need to examine other uses\nof [[ x == y ]] and correct the ones that do not need/want the\npattern matching semantics to use [ x = y ].\n\n"},{"id":"438374","messageId":"CAMmwyQqw3ebSxnUnYDgignpF5jyca7TgzzduCeKdbzEn97Tddg@mail.gmail.com","threadId":"56661","inReplyTo":"YWCwvOdW/L5aMpr7@camp.crustytoothpaste.net","subject":"Re: [PATCH] completion: fix incorrect bash/zsh string equality check","fromName":"Robert Estelle","fromEmail":"robertestelle@gmail.com","sentAt":"2021-10-08T22:17:01Z","receivedAt":"2021-10-08T22:17:32Z","isPatch":true,"sender":{"key":"robertestelle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/447615?v=4"},"body":"On Fri, Oct 8, 2021 at 1:58 PM brian m. carlson\n wrote:\n> It's actually preferable in most cases to use [ and = rather than [[ and\n> ==, because the former looks for strict equality and the latter looks\n> for pattern matching.\n\nYep, I agree with these style notes, but went with the prevalent style\nto avoid confusion. And note that the args here are both quoted, and\nso are treated as a literal comparison rather than globbed.\n\nThat said, whoever maintains that script, since this is a one-line\nchange, I'm not insistent about one way of addressing this or another\n:)\n"},{"id":"438375","messageId":"CAMmwyQp7ChC7RZAZX4Y3UyAghOAmvHA7e3BBJonGYJNoSc+nFA@mail.gmail.com","threadId":"56661","inReplyTo":"xmqqh7dri2m1.fsf@gitster.g","subject":"Re: [PATCH] completion: fix incorrect bash/zsh string equality check","fromName":"Robert Estelle","fromEmail":"robertestelle@gmail.com","sentAt":"2021-10-08T22:26:30Z","receivedAt":"2021-10-08T22:26:59Z","isPatch":true,"sender":{"key":"robertestelle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/447615?v=4"},"body":"The choice between the two does not affect correctness, it's purely\nstylistic here. It only would affect correctness for unquoted\narguments or extended comparison operators. Those *are* in use\nelsewhere in this script and force the use of `[[` in those places.\n\nKeep in mind also that this is an autocomplete script. Although it's\nsourced by both bash and zsh, it does not make sense to attempt to\nmake it work for bare POSIX sh.\n\nOn Fri, Oct 8, 2021 at 3:16 PM Junio C Hamano wrote:\n>\n> Robert Estelle writes:\n>\n> > On Fri, Oct 8, 2021 at 1:50 PM Junio C Hamano wrote:\n> >> Would it be equally a valid fix to use \"=\" instead of \"==\", or would\n> >> that change the meaning? This is a bash-only piece of code, so use\n> >> of [[ ... ]] is not technically incorrect, but if the basic [] works\n> >> well enough with \"=\", we should prefer that.\n> >\n> > Yes, `[` is preferable for portability and they'd behave the same in\n> > this case. I consciously chose to use `[[` because that's what all the\n> > other comparisons in that script use. (I think I noted that in the\n> > commit message, maybe). I think there's value in consistency, and not\n> > enough value of `[` over `[[` to justify changing all the other lines.\n>\n> I do not know if we mind eventual consistency using [] not [[]], but\n> this topic is certainly not about aiming for consistency.\n>\n> If the difference affects correctness, as brian mentioned, that is a\n> different matter. We should use [ x = y ] in this patch while\n> leaving the existing uses of [[ x == y ]].\n>\n> Later in a separate patch series, we may need to examine other uses\n> of [[ x == y ]] and correct the ones that do not need/want the\n> pattern matching semantics to use [ x = y ].\n>\n"},{"id":"438382","messageId":"xmqqzgrjglma.fsf@gitster.g","threadId":"56661","inReplyTo":"CAMmwyQp7ChC7RZAZX4Y3UyAghOAmvHA7e3BBJonGYJNoSc+nFA@mail.gmail.com","subject":"Re: [PATCH] completion: fix incorrect bash/zsh string equality check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-08T23:09:01Z","receivedAt":"2021-10-08T23:09:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Estelle writes:\n\n> The choice between the two does not affect correctness, it's purely\n> stylistic here. It only would affect correctness for unquoted\n> arguments or extended comparison operators. Those *are* in use\n> elsewhere in this script and force the use of `[[` in those places.\n>\n> Keep in mind also that this is an autocomplete script. Although it's\n> sourced by both bash and zsh, it does not make sense to attempt to\n> make it work for bare POSIX sh.\n\nNobody is trying to. It is more for reducing the risk of people\nshooting their foot by cutting and pasting without thinking.\n\nWhen you do not mean pattern matching and want exact matching, even\nif it is guaranteed that the data you pass through the codepath does\nnot have pattern to cause the former, hence the distinction between\n[[ x == y ]] and [ x = y ] does not make a difference, that is a\nmere happenstance, and use of [ x = y ] is the correct thing to do\nin such a case.\n"},{"id":"439603","messageId":"CAMmwyQqSJRgQ+-tFwYSSw2FU9TJ0TbjP-M_1rJSmo7bFpLW76w@mail.gmail.com","threadId":"56661","inReplyTo":"xmqqzgrjglma.fsf@gitster.g","subject":"Re: [PATCH] completion: fix incorrect bash/zsh string equality check","fromName":"Robert Estelle","fromEmail":"robertestelle@gmail.com","sentAt":"2021-10-25T22:20:04Z","receivedAt":"2021-10-25T22:20:34Z","isPatch":true,"sender":{"key":"robertestelle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/447615?v=4"},"body":"I considered dropping this patch entirely since this discussion was\ngoing so far off the rails for a single-char fix, but I hate to leave\nthings hanging…\n\n> When you do not mean pattern matching and want exact matching, even\n> if it is guaranteed that the data you pass through the codepath does\n> not have pattern to cause the former, hence the distinction between\n> [[ x == y ]] and [ x = y ] does not make a difference, that is a\n> mere happenstance, and use of [ x = y ] is the correct thing to do\n> in such a case.\n\nI think you've misread what's going on here, or might be\nmisremembering the shell quoting rules (which would surprise me!).\n\nMy assertion about their equivalence was syntactic: there's no\nhappenstance or reliance on codepaths whatsoever.\nIf you write:\n [[ \"$x\" == \"$y\" ]]\nthen it doesn't matter what x and y contain. They're quoted: it's just\na literal string comparison of their values.\nTheir values can contain quotes, newlines, spaces, asterisks, emoji,\netc. and none of that gets any special interpretation.\n\nRegardless, since they're functionally equivalent, I've pushed up a\nchange to do it the other way and will submit that shortly.\n\nBest,\nRobert\n\nOn Fri, Oct 8, 2021 at 4:09 PM Junio C Hamano wrote:\n>\n> Robert Estelle writes:\n>\n> > The choice between the two does not affect correctness, it's purely\n> > stylistic here. It only would affect correctness for unquoted\n> > arguments or extended comparison operators. Those *are* in use\n> > elsewhere in this script and force the use of `[[` in those places.\n> >\n> > Keep in mind also that this is an autocomplete script. Although it's\n> > sourced by both bash and zsh, it does not make sense to attempt to\n> > make it work for bare POSIX sh.\n>\n> Nobody is trying to. It is more for reducing the risk of people\n> shooting their foot by cutting and pasting without thinking.\n>\n> When you do not mean pattern matching and want exact matching, even\n> if it is guaranteed that the data you pass through the codepath does\n> not have pattern to cause the former, hence the distinction between\n> [[ x == y ]] and [ x = y ] does not make a difference, that is a\n> mere happenstance, and use of [ x = y ] is the correct thing to do\n> in such a case.\n"},{"id":"439605","messageId":"pull.1096.v2.git.git.1635200973354.gitgitgadget@gmail.com","threadId":"56661","inReplyTo":"pull.1096.git.git.1633642772432.gitgitgadget@gmail.com","subject":"[PATCH v2] completion: fix incorrect bash/zsh string equality check","fromName":"Robert Estelle via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-25T22:29:33Z","receivedAt":"2021-10-25T22:29:37Z","isPatch":true,"sender":{"key":"robertestelle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/447615?v=4"},"body":"From: Robert Estelle \n\nIn the basic `[`/`test` command, the string equality operator is a\nsingle `=`. The `==` operator is only available in `[[`, which is a\nbash-ism also supported by zsh.\n\nThis mix-up was causing the following completion error in zsh:\n> __git_ls_files_helper:7: = not found\n\n(That message refers to the extraneous symbol in `==` ← `=`).\n\nThis updates that comparison to use a single `=` inside the\nbasic `[ … ]` test conditional.\n\nAlthough this fix is inconsistent with the other comparisons in this\nfile, which use `[[ … == … ]]`, and the two expressions are functionally\nidentical in this context, that approach was rejected due to a\npreference for `[`.\n\nSigned-off-by: Robert Estelle \n---\n completion: Fix incorrect bash/zsh string equality check\n \n v2: This updates the comparison to remove the extraneous = symbol in ==,\n and use the [ … ] conditional instead.\n \n v1: (rejected) updated that comparison to use the extended [[ … ]]\n conditional for consistency with the other checks in this file.\n \n This fixes an error in contrib/completion/git-completion.bash caused by\n the incorrect use of == (vs. single =) inside a basic [/test command.\n Double-equals == should only be used with the extended [[ comparison.\n \n This was causing the following completion error in zsh:\n \n > __git_ls_files_helper:7: = not found\n \n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1096%2Frwe%2Ffix-completion-sh-eq-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1096/rwe/fix-completion-sh-eq-v2\nPull-Request: https://github.com/git/git/pull/1096\n\nRange-diff vs v1:\n\n 1: 6fd09347385 ! 1: eee166c8c99 completion: fix incorrect bash/zsh string equality check\n @@ Commit message\n \n (That message refers to the extraneous symbol in `==` ← `=`).\n \n - This updates that comparison to use the extended `[[ … ]]` conditional\n - for consistency with the other checks in this file.\n + This updates that comparison to use a single `=` inside the\n + basic `[ … ]` test conditional.\n +\n + Although this fix is inconsistent with the other comparisons in this\n + file, which use `[[ … == … ]]`, and the two expressions are functionally\n + identical in this context, that approach was rejected due to a\n + preference for `[`.\n \n Signed-off-by: Robert Estelle \n \n @@ contrib/completion/git-completion.bash: __gitcomp_file ()\n __git_ls_files_helper ()\n {\n -\tif [ \"$2\" == \"--committable\" ]; then\n -+\tif [[ \"$2\" == \"--committable\" ]]; then\n ++\tif [ \"$2\" = \"--committable\" ]; then\n \t\t__git -C \"$1\" -c core.quotePath=false diff-index \\\n \t\t\t--name-only --relative HEAD -- \"${3//\\\\/\\\\\\\\}*\"\n \telse\n\n\n contrib/completion/git-completion.bash | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 4bdd27ddc87..8ca9b15f21d 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -515,7 +515,7 @@ __gitcomp_file ()\n # argument, and using the options specified in the second argument.\n __git_ls_files_helper ()\n {\n-\tif [ \"$2\" == \"--committable\" ]; then\n+\tif [ \"$2\" = \"--committable\" ]; then\n \t\t__git -C \"$1\" -c core.quotePath=false diff-index \\\n \t\t\t--name-only --relative HEAD -- \"${3//\\\\/\\\\\\\\}*\"\n \telse\n\nbase-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf\n-- \ngitgitgadget\n"},{"id":"439893","messageId":"xmqq8rydt8by.fsf@gitster.g","threadId":"56661","inReplyTo":"pull.1096.v2.git.git.1635200973354.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] completion: fix incorrect bash/zsh string equality check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-28T16:36:33Z","receivedAt":"2021-10-28T16:36:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Robert Estelle via GitGitGadget\" writes:\n\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 4bdd27ddc87..8ca9b15f21d 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -515,7 +515,7 @@ __gitcomp_file ()\n> # argument, and using the options specified in the second argument.\n> __git_ls_files_helper ()\n> {\n> -\tif [ \"$2\" == \"--committable\" ]; then\n> +\tif [ \"$2\" = \"--committable\" ]; then\n> \t\t__git -C \"$1\" -c core.quotePath=false diff-index \\\n> \t\t\t--name-only --relative HEAD -- \"${3//\\\\/\\\\\\\\}*\"\n> \telse\n>\n> base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf\n\nThanks. We can trace this back to April 2013, if not earlier. It\nis sort of surprising that nobody else has noticed it since then\nX-<.\n\nWill queue. Thanks, again.\n"}]}