{"thread":{"id":"57997","subject":"[PATCH] git-prompt: use builtin test","startedAt":"2022-06-14T09:09:18Z","lastAt":"2022-06-21T17:35:18Z","messageCount":5,"participants":["Brad Forschinger via GitGitGadget","Jeff King","Brad Forschinger","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"457176","messageId":"pull.1260.git.1655197751403.gitgitgadget@gmail.com","threadId":"57997","inReplyTo":null,"subject":"[PATCH] git-prompt: use builtin test","fromName":"Brad Forschinger via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-14T09:09:11Z","receivedAt":"2022-06-14T09:09:18Z","isPatch":true,"sender":{"key":"bnjf@bnjf.id.au","avatar":null},"body":"From: Brad Forschinger <bnjf@bnjf.id.au>\n\nThe test and [ commands are used throughout the prompt generation.  They\nalso happen to be valid function names that can be defined, leading to\nunintentional results.  Prevent the somewhat unusual case of this\nhappening by simply using [[, which is reserved.\n\nSigned-off-by: Brad Forschinger <bnjf@bnjf.id.au>\n---\n    git-prompt: use builtin test\n    \n    The test and [ commands are used throughout the prompt generation. They\n    also happen to be valid function names that can be defined, leading to\n    unintentional results. Prevent the somewhat unusual case of this\n    happening by simply using [[, which is reserved.\n    \n    Signed-off-by: Brad Forschinger bnjf@bnjf.id.au\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1260%2Fbnjf%2Fprompt-use-builtins-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1260/bnjf/prompt-use-builtins-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1260\n\n contrib/completion/git-prompt.sh | 92 ++++++++++++++++----------------\n 1 file changed, 46 insertions(+), 46 deletions(-)\n\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex 87b2b916c03..e5a887a7c21 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -230,7 +230,7 @@ __git_ps1_show_upstream ()\n \t\tif [[ -n \"$count\" && -n \"$name\" ]]; then\n \t\t\t__git_ps1_upstream_name=$(git rev-parse \\\n \t\t\t\t--abbrev-ref \"$upstream_type\" 2>/dev/null)\n-\t\t\tif [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then\n+\t\t\tif [[ $pcmode = yes ]] && [[ $ps1_expanded = yes ]]; then\n \t\t\t\tupstream=\"$upstream \\${__git_ps1_upstream_name}\"\n \t\t\telse\n \t\t\t\tupstream=\"$upstream ${__git_ps1_upstream_name}\"\n@@ -266,7 +266,7 @@ __git_ps1_colorize_gitstring ()\n \tlocal flags_color=\"$c_lblue\"\n \n \tlocal branch_color=\"\"\n-\tif [ $detached = no ]; then\n+\tif [[ $detached = no ]]; then\n \t\tbranch_color=\"$ok_color\"\n \telse\n \t\tbranch_color=\"$bad_color\"\n@@ -274,16 +274,16 @@ __git_ps1_colorize_gitstring ()\n \tc=\"$branch_color$c\"\n \n \tz=\"$c_clear$z\"\n-\tif [ \"$w\" = \"*\" ]; then\n+\tif [[ \"$w\" = \"*\" ]]; then\n \t\tw=\"$bad_color$w\"\n \tfi\n-\tif [ -n \"$i\" ]; then\n+\tif [[ -n \"$i\" ]]; then\n \t\ti=\"$ok_color$i\"\n \tfi\n-\tif [ -n \"$s\" ]; then\n+\tif [[ -n \"$s\" ]]; then\n \t\ts=\"$flags_color$s\"\n \tfi\n-\tif [ -n \"$u\" ]; then\n+\tif [[ -n \"$u\" ]]; then\n \t\tu=\"$bad_color$u\"\n \tfi\n \tr=\"$c_clear$r\"\n@@ -294,7 +294,7 @@ __git_ps1_colorize_gitstring ()\n # variable, in that order.\n __git_eread ()\n {\n-\ttest -r \"$1\" && IFS=$'\\r\\n' read \"$2\" <\"$1\"\n+\t[[ -r \"$1\" ]] && IFS=$'\\r\\n' read \"$2\" <\"$1\"\n }\n \n # see if a cherry-pick or revert is in progress, if the user has committed a\n@@ -304,11 +304,11 @@ __git_eread ()\n __git_sequencer_status ()\n {\n \tlocal todo\n-\tif test -f \"$g/CHERRY_PICK_HEAD\"\n+\tif [[ -f \"$g/CHERRY_PICK_HEAD\" ]]\n \tthen\n \t\tr=\"|CHERRY-PICKING\"\n \t\treturn 0;\n-\telif test -f \"$g/REVERT_HEAD\"\n+\telif [[ -f \"$g/REVERT_HEAD\" ]]\n \tthen\n \t\tr=\"|REVERTING\"\n \t\treturn 0;\n@@ -399,8 +399,8 @@ __git_ps1 ()\n \t# incorrect.)\n \t#\n \tlocal ps1_expanded=yes\n-\t[ -z \"${ZSH_VERSION-}\" ] || [[ -o PROMPT_SUBST ]] || ps1_expanded=no\n-\t[ -z \"${BASH_VERSION-}\" ] || shopt -q promptvars || ps1_expanded=no\n+\t[[ -z \"${ZSH_VERSION-}\" ]] || [[ -o PROMPT_SUBST ]] || ps1_expanded=no\n+\t[[ -z \"${BASH_VERSION-}\" ]] || shopt -q promptvars || ps1_expanded=no\n \n \tlocal repo_info rev_parse_exit_code\n \trepo_info=\"$(git rev-parse --git-dir --is-inside-git-dir \\\n@@ -408,12 +408,12 @@ __git_ps1 ()\n \t\t--short HEAD 2>/dev/null)\"\n \trev_parse_exit_code=\"$?\"\n \n-\tif [ -z \"$repo_info\" ]; then\n+\tif [[ -z \"$repo_info\" ]]; then\n \t\treturn $exit\n \tfi\n \n \tlocal short_sha=\"\"\n-\tif [ \"$rev_parse_exit_code\" = \"0\" ]; then\n+\tif [[ \"$rev_parse_exit_code\" = \"0\" ]]; then\n \t\tshort_sha=\"${repo_info##*$'\\n'}\"\n \t\trepo_info=\"${repo_info%$'\\n'*}\"\n \tfi\n@@ -424,18 +424,18 @@ __git_ps1 ()\n \tlocal inside_gitdir=\"${repo_info##*$'\\n'}\"\n \tlocal g=\"${repo_info%$'\\n'*}\"\n \n-\tif [ \"true\" = \"$inside_worktree\" ] &&\n-\t   [ -n \"${GIT_PS1_HIDE_IF_PWD_IGNORED-}\" ] &&\n-\t   [ \"$(git config --bool bash.hideIfPwdIgnored)\" != \"false\" ] &&\n+\tif [[ \"true\" = \"$inside_worktree\" ]] &&\n+\t   [[ -n \"${GIT_PS1_HIDE_IF_PWD_IGNORED-}\" ]] &&\n+\t   [[ \"$(git config --bool bash.hideIfPwdIgnored)\" != \"false\" ]] &&\n \t   git check-ignore -q .\n \tthen\n \t\treturn $exit\n \tfi\n \n \tlocal sparse=\"\"\n-\tif [ -z \"${GIT_PS1_COMPRESSSPARSESTATE-}\" ] &&\n-\t   [ -z \"${GIT_PS1_OMITSPARSESTATE-}\" ] &&\n-\t   [ \"$(git config --bool core.sparseCheckout)\" = \"true\" ]; then\n+\tif [[ -z \"${GIT_PS1_COMPRESSSPARSESTATE-}\" ]] &&\n+\t   [[ -z \"${GIT_PS1_OMITSPARSESTATE-}\" ]] &&\n+\t   [[ \"$(git config --bool core.sparseCheckout)\" = \"true\" ]]; then\n \t\tsparse=\"|SPARSE\"\n \tfi\n \n@@ -443,34 +443,34 @@ __git_ps1 ()\n \tlocal b=\"\"\n \tlocal step=\"\"\n \tlocal total=\"\"\n-\tif [ -d \"$g/rebase-merge\" ]; then\n+\tif [[ -d \"$g/rebase-merge\" ]]; then\n \t\t__git_eread \"$g/rebase-merge/head-name\" b\n \t\t__git_eread \"$g/rebase-merge/msgnum\" step\n \t\t__git_eread \"$g/rebase-merge/end\" total\n \t\tr=\"|REBASE\"\n \telse\n-\t\tif [ -d \"$g/rebase-apply\" ]; then\n+\t\tif [[ -d \"$g/rebase-apply\" ]]; then\n \t\t\t__git_eread \"$g/rebase-apply/next\" step\n \t\t\t__git_eread \"$g/rebase-apply/last\" total\n-\t\t\tif [ -f \"$g/rebase-apply/rebasing\" ]; then\n+\t\t\tif [[ -f \"$g/rebase-apply/rebasing\" ]]; then\n \t\t\t\t__git_eread \"$g/rebase-apply/head-name\" b\n \t\t\t\tr=\"|REBASE\"\n-\t\t\telif [ -f \"$g/rebase-apply/applying\" ]; then\n+\t\t\telif [[ -f \"$g/rebase-apply/applying\" ]]; then\n \t\t\t\tr=\"|AM\"\n \t\t\telse\n \t\t\t\tr=\"|AM/REBASE\"\n \t\t\tfi\n-\t\telif [ -f \"$g/MERGE_HEAD\" ]; then\n+\t\telif [[ -f \"$g/MERGE_HEAD\" ]]; then\n \t\t\tr=\"|MERGING\"\n \t\telif __git_sequencer_status; then\n \t\t\t:\n-\t\telif [ -f \"$g/BISECT_LOG\" ]; then\n+\t\telif [[ -f \"$g/BISECT_LOG\" ]]; then\n \t\t\tr=\"|BISECTING\"\n \t\tfi\n \n-\t\tif [ -n \"$b\" ]; then\n+\t\tif [[ -n \"$b\" ]]; then\n \t\t\t:\n-\t\telif [ -h \"$g/HEAD\" ]; then\n+\t\telif [[ -h \"$g/HEAD\" ]]; then\n \t\t\t# symlink symbolic ref\n \t\t\tb=\"$(git symbolic-ref HEAD 2>/dev/null)\"\n \t\telse\n@@ -480,7 +480,7 @@ __git_ps1 ()\n \t\t\tfi\n \t\t\t# is it a symbolic ref?\n \t\t\tb=\"${head#ref: }\"\n-\t\t\tif [ \"$head\" = \"$b\" ]; then\n+\t\t\tif [[ \"$head\" = \"$b\" ]]; then\n \t\t\t\tdetached=yes\n \t\t\t\tb=\"$(\n \t\t\t\tcase \"${GIT_PS1_DESCRIBE_STYLE-}\" in\n@@ -502,7 +502,7 @@ __git_ps1 ()\n \t\tfi\n \tfi\n \n-\tif [ -n \"$step\" ] && [ -n \"$total\" ]; then\n+\tif [[ -n \"$step\" ]] && [[ -n \"$total\" ]]; then\n \t\tr=\"$r $step/$total\"\n \tfi\n \n@@ -515,41 +515,41 @@ __git_ps1 ()\n \tlocal p=\"\" # short version of upstream state indicator\n \tlocal upstream=\"\" # verbose version of upstream state indicator\n \n-\tif [ \"true\" = \"$inside_gitdir\" ]; then\n-\t\tif [ \"true\" = \"$bare_repo\" ]; then\n+\tif [[ \"true\" = \"$inside_gitdir\" ]]; then\n+\t\tif [[ \"true\" = \"$bare_repo\" ]]; then\n \t\t\tc=\"BARE:\"\n \t\telse\n \t\t\tb=\"GIT_DIR!\"\n \t\tfi\n-\telif [ \"true\" = \"$inside_worktree\" ]; then\n-\t\tif [ -n \"${GIT_PS1_SHOWDIRTYSTATE-}\" ] &&\n-\t\t   [ \"$(git config --bool bash.showDirtyState)\" != \"false\" ]\n+\telif [[ \"true\" = \"$inside_worktree\" ]]; then\n+\t\tif [[ -n \"${GIT_PS1_SHOWDIRTYSTATE-}\" ]] &&\n+\t\t   [[ \"$(git config --bool bash.showDirtyState)\" != \"false\" ]]\n \t\tthen\n \t\t\tgit diff --no-ext-diff --quiet || w=\"*\"\n \t\t\tgit diff --no-ext-diff --cached --quiet || i=\"+\"\n-\t\t\tif [ -z \"$short_sha\" ] && [ -z \"$i\" ]; then\n+\t\t\tif [[ -z \"$short_sha\" ]] && [[ -z \"$i\" ]]; then\n \t\t\t\ti=\"#\"\n \t\t\tfi\n \t\tfi\n-\t\tif [ -n \"${GIT_PS1_SHOWSTASHSTATE-}\" ] &&\n+\t\tif [[ -n \"${GIT_PS1_SHOWSTASHSTATE-}\" ]] &&\n \t\t   git rev-parse --verify --quiet refs/stash >/dev/null\n \t\tthen\n \t\t\ts=\"$\"\n \t\tfi\n \n-\t\tif [ -n \"${GIT_PS1_SHOWUNTRACKEDFILES-}\" ] &&\n-\t\t   [ \"$(git config --bool bash.showUntrackedFiles)\" != \"false\" ] &&\n+\t\tif [[ -n \"${GIT_PS1_SHOWUNTRACKEDFILES-}\" ]] &&\n+\t\t   [[ \"$(git config --bool bash.showUntrackedFiles)\" != \"false\" ]] &&\n \t\t   git ls-files --others --exclude-standard --directory --no-empty-directory --error-unmatch -- ':/*' >/dev/null 2>/dev/null\n \t\tthen\n \t\t\tu=\"%${ZSH_VERSION+%}\"\n \t\tfi\n \n-\t\tif [ -n \"${GIT_PS1_COMPRESSSPARSESTATE-}\" ] &&\n-\t\t   [ \"$(git config --bool core.sparseCheckout)\" = \"true\" ]; then\n+\t\tif [[ -n \"${GIT_PS1_COMPRESSSPARSESTATE-}\" ]] &&\n+\t\t   [[ \"$(git config --bool core.sparseCheckout)\" = \"true\" ]]; then\n \t\t\th=\"?\"\n \t\tfi\n \n-\t\tif [ -n \"${GIT_PS1_SHOWUPSTREAM-}\" ]; then\n+\t\tif [[ -n \"${GIT_PS1_SHOWUPSTREAM-}\" ]]; then\n \t\t\t__git_ps1_show_upstream\n \t\tfi\n \tfi\n@@ -557,14 +557,14 @@ __git_ps1 ()\n \tlocal z=\"${GIT_PS1_STATESEPARATOR-\" \"}\"\n \n \t# NO color option unless in PROMPT_COMMAND mode or it's Zsh\n-\tif [ -n \"${GIT_PS1_SHOWCOLORHINTS-}\" ]; then\n-\t\tif [ $pcmode = yes ] || [ -n \"${ZSH_VERSION-}\" ]; then\n+\tif [[ -n \"${GIT_PS1_SHOWCOLORHINTS-}\" ]]; then\n+\t\tif [[ $pcmode = yes ]] || [[ -n \"${ZSH_VERSION-}\" ]]; then\n \t\t\t__git_ps1_colorize_gitstring\n \t\tfi\n \tfi\n \n \tb=${b##refs/heads/}\n-\tif [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then\n+\tif [[ $pcmode = yes ]] && [[ $ps1_expanded = yes ]]; then\n \t\t__git_ps1_branch_name=$b\n \t\tb=\"\\${__git_ps1_branch_name}\"\n \tfi\n@@ -572,8 +572,8 @@ __git_ps1 ()\n \tlocal f=\"$h$w$i$s$u$p\"\n \tlocal gitstring=\"$c$b${f:+$z$f}${sparse}$r${upstream}\"\n \n-\tif [ $pcmode = yes ]; then\n-\t\tif [ \"${__git_printf_supports_v-}\" != yes ]; then\n+\tif [[ $pcmode = yes ]]; then\n+\t\tif [[ \"${__git_printf_supports_v-}\" != yes ]]; then\n \t\t\tgitstring=$(printf -- \"$printf_format\" \"$gitstring\")\n \t\telse\n \t\t\tprintf -v gitstring -- \"$printf_format\" \"$gitstring\"\n\nbase-commit: 5699ec1b0aec51b9e9ba5a2785f65970c5a95d84\n-- \ngitgitgadget\n"},{"id":"457431","messageId":"YquwpxEoAVWen8tZ@coredump.intra.peff.net","threadId":"57997","inReplyTo":"pull.1260.git.1655197751403.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-prompt: use builtin test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-16T22:37:27Z","receivedAt":"2022-06-16T22:37:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 14, 2022 at 09:09:11AM +0000, Brad Forschinger via GitGitGadget wrote:\n\n> From: Brad Forschinger <bnjf@bnjf.id.au>\n> \n> The test and [ commands are used throughout the prompt generation.  They\n> also happen to be valid function names that can be defined, leading to\n> unintentional results.  Prevent the somewhat unusual case of this\n> happening by simply using [[, which is reserved.\n\nHmm. I do think we need to be a bit more paranoid about style in the\nprompt and completion code, because they are sourced into the user's\nshell alongside whatever other weird customizations they'd have. So we\nalready have adjustments to work under \"set -u\", and so forth.\n\nBut at some point we may say \"you have made the environment too hostile\nfor us to function\". Is redefining \"test\" to something that doesn't\nbehave the same way such a case? Part of me wants to say yes. :)\n\nThat said, if it's not _hard_ to support, maybe it is worth doing to be\non the cautious side? A few thoughts:\n\n  - my biggest concern on cost is that this is an unusual style for our\n    project (which usually writes in POSIX shell, though of course this\n    file is meant to be bash/zsh specific). Will it be a maintenance\n    burden going forward?\n\n  - this only changes git-prompt.sh; doesn't the completion code have\n    the same issue?\n\n  - I don't write much bash-specific code, but I seem to recall that\n    \"[[\" has some subtle differences to \"[\". Is it sufficiently a\n    superset that these conversions are all equivalent?\n\n    I think some like:\n\n> -\tif [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then\n> +\tif [[ $pcmode = yes ]] && [[ $ps1_expanded = yes ]]; then\n\n    are not equivalent, but it's an actual improvement (bash's builtin\n    \"[[\" isn't confused by unquoted empty variables), but I don't know\n    if there may be other gotchas.\n\n    (I doubt this is an actual bug in the current code, as $pcmode\n    always seems to be set, but just a more defensive style).\n\n-Peff\n"},{"id":"457509","messageId":"Yq57MP47M5fAzkFC@bnjf.id.au","threadId":"57997","inReplyTo":"YquwpxEoAVWen8tZ@coredump.intra.peff.net","subject":"Re: [PATCH] git-prompt: use builtin test","fromName":"Brad Forschinger","fromEmail":"bnjf@bnjf.id.au","sentAt":"2022-06-19T01:26:08Z","receivedAt":"2022-06-19T01:35:19Z","isPatch":true,"sender":{"key":"bnjf@bnjf.id.au","avatar":null},"body":"On Thu, Jun 16, 2022 at 06:37:27PM -0400, Jeff King wrote:\n> > The test and [ commands are used throughout the prompt generation.  They\n> > also happen to be valid function names that can be defined, leading to\n> > unintentional results.  Prevent the somewhat unusual case of this\n> > happening by simply using [[, which is reserved.\n> \n> Hmm. I do think we need to be a bit more paranoid about style in the\n> prompt and completion code, because they are sourced into the user's\n> shell alongside whatever other weird customizations they'd have. So we\n> already have adjustments to work under \"set -u\", and so forth.\n> \n> But at some point we may say \"you have made the environment too hostile\n> for us to function\". Is redefining \"test\" to something that doesn't\n> behave the same way such a case? Part of me wants to say yes. :)\n\nI'd be inclined to agree!  But disregarding a user with malicious\nintent, these environment changes can also be unintentional: I came\nacross it when I stubbed out a quick test() function while prototyping\nsomething unrelated.\n\n> That said, if it's not _hard_ to support, maybe it is worth doing to be\n> on the cautious side? A few thoughts:\n> \n>   - my biggest concern on cost is that this is an unusual style for our\n>     project (which usually writes in POSIX shell, though of course this\n>     file is meant to be bash/zsh specific). Will it be a maintenance\n>     burden going forward?\n\nThat's possible, but I suspect the burden is minimal.  As you said, this\nis bash and zsh specific, and for those shell coders who only write\nBourne dialect it's to be read as a \"strong\" left square bracket.  For\nexample, to minimize any shock to the eyeballs I've intentionally not\nre-written string operations `[ a = b ] && [ c = d ]` to `[[ a == b && c\n== d ]]`.  I promise it wasn't mere laziness!\n\n>   - this only changes git-prompt.sh; doesn't the completion code have\n>     the same issue?\n\nIt does.  Although there has been some movement towards the\nbash-specific builtin:\n\n    $ git show v2.36.1:./git-completion.bash | awk '\n      /(^\\[|[^[]\\[) |\\<test\\>/ && !++slb || /\\[\\[ / && !++dlb || 0;\n      END { print slb, dlb; }'\n    119 15\n    $\n\nThis can be addressed in a future patch.\n\n>   - I don't write much bash-specific code, but I seem to recall that\n>     \"[[\" has some subtle differences to \"[\". Is it sufficiently a\n>     superset that these conversions are all equivalent?\n>\n>     I think some like:\n> \n> > -\tif [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then\n> > +\tif [[ $pcmode = yes ]] && [[ $ps1_expanded = yes ]]; then\n> \n>     are not equivalent, but it's an actual improvement (bash's builtin\n>     \"[[\" isn't confused by unquoted empty variables), but I don't know\n>     if there may be other gotchas.\n> \n>     (I doubt this is an actual bug in the current code, as $pcmode\n>     always seems to be set, but just a more defensive style).\n\nYeah, there's no word splitting or pathname expansion.  Also, bash4\nonwards changed the < and > operators within [[ to locale order rather\nthan ASCII.\n\nRegards,\nBrad\n"},{"id":"457591","messageId":"YrFrsZj8w0i6PPiz@coredump.intra.peff.net","threadId":"57997","inReplyTo":"Yq57MP47M5fAzkFC@bnjf.id.au","subject":"Re: [PATCH] git-prompt: use builtin test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-21T06:56:49Z","receivedAt":"2022-06-21T06:56:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 19, 2022 at 01:26:08AM +0000, Brad Forschinger wrote:\n\n> > But at some point we may say \"you have made the environment too hostile\n> > for us to function\". Is redefining \"test\" to something that doesn't\n> > behave the same way such a case? Part of me wants to say yes. :)\n> \n> I'd be inclined to agree!  But disregarding a user with malicious\n> intent, these environment changes can also be unintentional: I came\n> across it when I stubbed out a quick test() function while prototyping\n> something unrelated.\n\nI kind of wonder what else would have trouble with your accidental\nbreakage. I poked at the bash-completion package as the only other\nprominent \"source this into your bash script\" package I could think of.\nThey do indeed seem to mostly avoid \"test\", though there are a few cases\nin program-specific completions.\n\nIn the general case, though, I think it's an infinite rabbit hole. I can\nsimilarly redefine \"declare\" or \"local\" and cause all sorts of trouble.\nAnd there's no real defense for scripts there.\n\n> >   - my biggest concern on cost is that this is an unusual style for our\n> >     project (which usually writes in POSIX shell, though of course this\n> >     file is meant to be bash/zsh specific). Will it be a maintenance\n> >     burden going forward?\n> \n> That's possible, but I suspect the burden is minimal.  As you said, this\n> is bash and zsh specific, and for those shell coders who only write\n> Bourne dialect it's to be read as a \"strong\" left square bracket.  For\n> example, to minimize any shock to the eyeballs I've intentionally not\n> re-written string operations `[ a = b ] && [ c = d ]` to `[[ a == b && c\n> == d ]]`.  I promise it wasn't mere laziness!\n\nI guess my concern was less about doing it once, and more about: is this\nsomething we want to continue enforcing as time goes on? That is, would\nwe want to catch it in review and complain about people using \"test\"?\nThat's a subtle thing to remember to look for, though I guess we could\nautomate it via the tests. Or would we rely on people who cared to\nnotice new instances and submit patches? That's how we deal with some\nother portability issues (if nobody is screaming, how broken could it\nbe?). But it sounds from your description like this was a one-off even\nfor you.\n\nSo I dunno. I'm not really opposed, but I'm not convinced it's really\naccomplishing much here.\n\n-Peff\n"},{"id":"457632","messageId":"xmqqmte5hqcx.fsf@gitster.g","threadId":"57997","inReplyTo":"YrFrsZj8w0i6PPiz@coredump.intra.peff.net","subject":"Re: [PATCH] git-prompt: use builtin test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-21T17:35:10Z","receivedAt":"2022-06-21T17:35:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> That's possible, but I suspect the burden is minimal.  As you said, this\n>> is bash and zsh specific, and for those shell coders who only write\n>> Bourne dialect it's to be read as a \"strong\" left square bracket.  For\n>> example, to minimize any shock to the eyeballs I've intentionally not\n>> re-written string operations `[ a = b ] && [ c = d ]` to `[[ a == b && c\n>> == d ]]`.  I promise it wasn't mere laziness!\n>\n> I guess my concern was less about doing it once, and more about: is this\n> something we want to continue enforcing as time goes on? That is, would\n> we want to catch it in review and complain about people using \"test\"?\n\nYes, if this is just a mental exercise burning off excess calories\nand too much spare time and nothing else, I'm hesitant to burden our\nreviewers and casual contributors, who do not have excess calories\nor too much spare time to burn off just to play nice with the result\nof this patch, with an issue that is apparently only theoretical\nlike this one, with which nobody actually is hurt in practice.\n\nIt's not like local variables used in prompt and completion leaking\nout, which can hurt real users (as it is quite normal to use throw\naway variables in your interactive sessions).  It's not even like\nthese scripts being \"set -u\" clean, that may hurt those who use \"set\n-u\" in their interactive sessions (what for?) but nobody else.  \n\nIf you write your own shell function \"test\" that is grossly\nincompatible with everybody else's \"test\", where do you do that?\nNot in your .bashrc, or you would break more than just prompt and\ncompletion.\n\nIf this came with additions to t9903 that redefines \"test\" and tries\nto make sure we consistently use bashism [[...]] and such a change\nto \"test\" would not break it, I might have been more sympathetic,\nbut I dunno.  I sort-of like the purity of mind when we say \"this is\na #!/bin/bash script and we maximally write in a way a bash-script\nexpert would\", but many of our shell scripts outside these two\ncontrib/ scripts HAVE to be portable and free of bashisms, so...\n\n> That's a subtle thing to remember to look for, though I guess we could\n> automate it via the tests. Or would we rely on people who cared to\n> notice new instances and submit patches? That's how we deal with some\n> other portability issues (if nobody is screaming, how broken could it\n> be?). But it sounds from your description like this was a one-off even\n> for you.\n>\n> So I dunno. I'm not really opposed, but I'm not convinced it's really\n> accomplishing much here.\n\nI guess I am mostly on the same page here.\n\nThanks.\n"}]}