| Message ID | pull.1260.git.1655197751403.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-prompt: use builtin test | expand |
On Tue, Jun 14, 2022 at 09:09:11AM +0000, Brad Forschinger via GitGitGadget wrote: > From: Brad Forschinger <bnjf@bnjf.id.au> > > The test and [ commands are used throughout the prompt generation. They > also happen to be valid function names that can be defined, leading to > unintentional results. Prevent the somewhat unusual case of this > happening by simply using [[, which is reserved. Hmm. I do think we need to be a bit more paranoid about style in the prompt and completion code, because they are sourced into the user's shell alongside whatever other weird customizations they'd have. So we already have adjustments to work under "set -u", and so forth. But at some point we may say "you have made the environment too hostile for us to function". Is redefining "test" to something that doesn't behave the same way such a case? Part of me wants to say yes. :) That said, if it's not _hard_ to support, maybe it is worth doing to be on the cautious side? A few thoughts: - my biggest concern on cost is that this is an unusual style for our project (which usually writes in POSIX shell, though of course this file is meant to be bash/zsh specific). Will it be a maintenance burden going forward? - this only changes git-prompt.sh; doesn't the completion code have the same issue? - I don't write much bash-specific code, but I seem to recall that "[[" has some subtle differences to "[". Is it sufficiently a superset that these conversions are all equivalent? I think some like: > - if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then > + if [[ $pcmode = yes ]] && [[ $ps1_expanded = yes ]]; then are not equivalent, but it's an actual improvement (bash's builtin "[[" isn't confused by unquoted empty variables), but I don't know if there may be other gotchas. (I doubt this is an actual bug in the current code, as $pcmode always seems to be set, but just a more defensive style). -Peff
On Thu, Jun 16, 2022 at 06:37:27PM -0400, Jeff King wrote: > > The test and [ commands are used throughout the prompt generation. They > > also happen to be valid function names that can be defined, leading to > > unintentional results. Prevent the somewhat unusual case of this > > happening by simply using [[, which is reserved. > > Hmm. I do think we need to be a bit more paranoid about style in the > prompt and completion code, because they are sourced into the user's > shell alongside whatever other weird customizations they'd have. So we > already have adjustments to work under "set -u", and so forth. > > But at some point we may say "you have made the environment too hostile > for us to function". Is redefining "test" to something that doesn't > behave the same way such a case? Part of me wants to say yes. :) I'd be inclined to agree! But disregarding a user with malicious intent, these environment changes can also be unintentional: I came across it when I stubbed out a quick test() function while prototyping something unrelated. > That said, if it's not _hard_ to support, maybe it is worth doing to be > on the cautious side? A few thoughts: > > - my biggest concern on cost is that this is an unusual style for our > project (which usually writes in POSIX shell, though of course this > file is meant to be bash/zsh specific). Will it be a maintenance > burden going forward? That's possible, but I suspect the burden is minimal. As you said, this is bash and zsh specific, and for those shell coders who only write Bourne dialect it's to be read as a "strong" left square bracket. For example, to minimize any shock to the eyeballs I've intentionally not re-written string operations `[ a = b ] && [ c = d ]` to `[[ a == b && c == d ]]`. I promise it wasn't mere laziness! > - this only changes git-prompt.sh; doesn't the completion code have > the same issue? It does. Although there has been some movement towards the bash-specific builtin: $ git show v2.36.1:./git-completion.bash | awk ' /(^\[|[^[]\[) |\<test\>/ && !++slb || /\[\[ / && !++dlb || 0; END { print slb, dlb; }' 119 15 $ This can be addressed in a future patch. > - I don't write much bash-specific code, but I seem to recall that > "[[" has some subtle differences to "[". Is it sufficiently a > superset that these conversions are all equivalent? > > I think some like: > > > - if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then > > + if [[ $pcmode = yes ]] && [[ $ps1_expanded = yes ]]; then > > are not equivalent, but it's an actual improvement (bash's builtin > "[[" isn't confused by unquoted empty variables), but I don't know > if there may be other gotchas. > > (I doubt this is an actual bug in the current code, as $pcmode > always seems to be set, but just a more defensive style). Yeah, there's no word splitting or pathname expansion. Also, bash4 onwards changed the < and > operators within [[ to locale order rather than ASCII. Regards, Brad
On Sun, Jun 19, 2022 at 01:26:08AM +0000, Brad Forschinger wrote: > > But at some point we may say "you have made the environment too hostile > > for us to function". Is redefining "test" to something that doesn't > > behave the same way such a case? Part of me wants to say yes. :) > > I'd be inclined to agree! But disregarding a user with malicious > intent, these environment changes can also be unintentional: I came > across it when I stubbed out a quick test() function while prototyping > something unrelated. I kind of wonder what else would have trouble with your accidental breakage. I poked at the bash-completion package as the only other prominent "source this into your bash script" package I could think of. They do indeed seem to mostly avoid "test", though there are a few cases in program-specific completions. In the general case, though, I think it's an infinite rabbit hole. I can similarly redefine "declare" or "local" and cause all sorts of trouble. And there's no real defense for scripts there. > > - my biggest concern on cost is that this is an unusual style for our > > project (which usually writes in POSIX shell, though of course this > > file is meant to be bash/zsh specific). Will it be a maintenance > > burden going forward? > > That's possible, but I suspect the burden is minimal. As you said, this > is bash and zsh specific, and for those shell coders who only write > Bourne dialect it's to be read as a "strong" left square bracket. For > example, to minimize any shock to the eyeballs I've intentionally not > re-written string operations `[ a = b ] && [ c = d ]` to `[[ a == b && c > == d ]]`. I promise it wasn't mere laziness! I guess my concern was less about doing it once, and more about: is this something we want to continue enforcing as time goes on? That is, would we want to catch it in review and complain about people using "test"? That's a subtle thing to remember to look for, though I guess we could automate it via the tests. Or would we rely on people who cared to notice new instances and submit patches? That's how we deal with some other portability issues (if nobody is screaming, how broken could it be?). But it sounds from your description like this was a one-off even for you. So I dunno. I'm not really opposed, but I'm not convinced it's really accomplishing much here. -Peff
Jeff King <peff@peff.net> writes: >> That's possible, but I suspect the burden is minimal. As you said, this >> is bash and zsh specific, and for those shell coders who only write >> Bourne dialect it's to be read as a "strong" left square bracket. For >> example, to minimize any shock to the eyeballs I've intentionally not >> re-written string operations `[ a = b ] && [ c = d ]` to `[[ a == b && c >> == d ]]`. I promise it wasn't mere laziness! > > I guess my concern was less about doing it once, and more about: is this > something we want to continue enforcing as time goes on? That is, would > we want to catch it in review and complain about people using "test"? Yes, if this is just a mental exercise burning off excess calories and too much spare time and nothing else, I'm hesitant to burden our reviewers and casual contributors, who do not have excess calories or too much spare time to burn off just to play nice with the result of this patch, with an issue that is apparently only theoretical like this one, with which nobody actually is hurt in practice. It's not like local variables used in prompt and completion leaking out, which can hurt real users (as it is quite normal to use throw away variables in your interactive sessions). It's not even like these scripts being "set -u" clean, that may hurt those who use "set -u" in their interactive sessions (what for?) but nobody else. If you write your own shell function "test" that is grossly incompatible with everybody else's "test", where do you do that? Not in your .bashrc, or you would break more than just prompt and completion. If this came with additions to t9903 that redefines "test" and tries to make sure we consistently use bashism [[...]] and such a change to "test" would not break it, I might have been more sympathetic, but I dunno. I sort-of like the purity of mind when we say "this is a #!/bin/bash script and we maximally write in a way a bash-script expert would", but many of our shell scripts outside these two contrib/ scripts HAVE to be portable and free of bashisms, so... > That's a subtle thing to remember to look for, though I guess we could > automate it via the tests. Or would we rely on people who cared to > notice new instances and submit patches? That's how we deal with some > other portability issues (if nobody is screaming, how broken could it > be?). But it sounds from your description like this was a one-off even > for you. > > So I dunno. I'm not really opposed, but I'm not convinced it's really > accomplishing much here. I guess I am mostly on the same page here. Thanks.
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh index 87b2b916c03..e5a887a7c21 100644 --- a/contrib/completion/git-prompt.sh +++ b/contrib/completion/git-prompt.sh @@ -230,7 +230,7 @@ __git_ps1_show_upstream () if [[ -n "$count" && -n "$name" ]]; then __git_ps1_upstream_name=$(git rev-parse \ --abbrev-ref "$upstream_type" 2>/dev/null) - if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then + if [[ $pcmode = yes ]] && [[ $ps1_expanded = yes ]]; then upstream="$upstream \${__git_ps1_upstream_name}" else upstream="$upstream ${__git_ps1_upstream_name}" @@ -266,7 +266,7 @@ __git_ps1_colorize_gitstring () local flags_color="$c_lblue" local branch_color="" - if [ $detached = no ]; then + if [[ $detached = no ]]; then branch_color="$ok_color" else branch_color="$bad_color" @@ -274,16 +274,16 @@ __git_ps1_colorize_gitstring () c="$branch_color$c" z="$c_clear$z" - if [ "$w" = "*" ]; then + if [[ "$w" = "*" ]]; then w="$bad_color$w" fi - if [ -n "$i" ]; then + if [[ -n "$i" ]]; then i="$ok_color$i" fi - if [ -n "$s" ]; then + if [[ -n "$s" ]]; then s="$flags_color$s" fi - if [ -n "$u" ]; then + if [[ -n "$u" ]]; then u="$bad_color$u" fi r="$c_clear$r" @@ -294,7 +294,7 @@ __git_ps1_colorize_gitstring () # variable, in that order. __git_eread () { - test -r "$1" && IFS=$'\r\n' read "$2" <"$1" + [[ -r "$1" ]] && IFS=$'\r\n' read "$2" <"$1" } # see if a cherry-pick or revert is in progress, if the user has committed a @@ -304,11 +304,11 @@ __git_eread () __git_sequencer_status () { local todo - if test -f "$g/CHERRY_PICK_HEAD" + if [[ -f "$g/CHERRY_PICK_HEAD" ]] then r="|CHERRY-PICKING" return 0; - elif test -f "$g/REVERT_HEAD" + elif [[ -f "$g/REVERT_HEAD" ]] then r="|REVERTING" return 0; @@ -399,8 +399,8 @@ __git_ps1 () # incorrect.) # local ps1_expanded=yes - [ -z "${ZSH_VERSION-}" ] || [[ -o PROMPT_SUBST ]] || ps1_expanded=no - [ -z "${BASH_VERSION-}" ] || shopt -q promptvars || ps1_expanded=no + [[ -z "${ZSH_VERSION-}" ]] || [[ -o PROMPT_SUBST ]] || ps1_expanded=no + [[ -z "${BASH_VERSION-}" ]] || shopt -q promptvars || ps1_expanded=no local repo_info rev_parse_exit_code repo_info="$(git rev-parse --git-dir --is-inside-git-dir \ @@ -408,12 +408,12 @@ __git_ps1 () --short HEAD 2>/dev/null)" rev_parse_exit_code="$?" - if [ -z "$repo_info" ]; then + if [[ -z "$repo_info" ]]; then return $exit fi local short_sha="" - if [ "$rev_parse_exit_code" = "0" ]; then + if [[ "$rev_parse_exit_code" = "0" ]]; then short_sha="${repo_info##*$'\n'}" repo_info="${repo_info%$'\n'*}" fi @@ -424,18 +424,18 @@ __git_ps1 () local inside_gitdir="${repo_info##*$'\n'}" local g="${repo_info%$'\n'*}" - if [ "true" = "$inside_worktree" ] && - [ -n "${GIT_PS1_HIDE_IF_PWD_IGNORED-}" ] && - [ "$(git config --bool bash.hideIfPwdIgnored)" != "false" ] && + if [[ "true" = "$inside_worktree" ]] && + [[ -n "${GIT_PS1_HIDE_IF_PWD_IGNORED-}" ]] && + [[ "$(git config --bool bash.hideIfPwdIgnored)" != "false" ]] && git check-ignore -q . then return $exit fi local sparse="" - if [ -z "${GIT_PS1_COMPRESSSPARSESTATE-}" ] && - [ -z "${GIT_PS1_OMITSPARSESTATE-}" ] && - [ "$(git config --bool core.sparseCheckout)" = "true" ]; then + if [[ -z "${GIT_PS1_COMPRESSSPARSESTATE-}" ]] && + [[ -z "${GIT_PS1_OMITSPARSESTATE-}" ]] && + [[ "$(git config --bool core.sparseCheckout)" = "true" ]]; then sparse="|SPARSE" fi @@ -443,34 +443,34 @@ __git_ps1 () local b="" local step="" local total="" - if [ -d "$g/rebase-merge" ]; then + if [[ -d "$g/rebase-merge" ]]; then __git_eread "$g/rebase-merge/head-name" b __git_eread "$g/rebase-merge/msgnum" step __git_eread "$g/rebase-merge/end" total r="|REBASE" else - if [ -d "$g/rebase-apply" ]; then + if [[ -d "$g/rebase-apply" ]]; then __git_eread "$g/rebase-apply/next" step __git_eread "$g/rebase-apply/last" total - if [ -f "$g/rebase-apply/rebasing" ]; then + if [[ -f "$g/rebase-apply/rebasing" ]]; then __git_eread "$g/rebase-apply/head-name" b r="|REBASE" - elif [ -f "$g/rebase-apply/applying" ]; then + elif [[ -f "$g/rebase-apply/applying" ]]; then r="|AM" else r="|AM/REBASE" fi - elif [ -f "$g/MERGE_HEAD" ]; then + elif [[ -f "$g/MERGE_HEAD" ]]; then r="|MERGING" elif __git_sequencer_status; then : - elif [ -f "$g/BISECT_LOG" ]; then + elif [[ -f "$g/BISECT_LOG" ]]; then r="|BISECTING" fi - if [ -n "$b" ]; then + if [[ -n "$b" ]]; then : - elif [ -h "$g/HEAD" ]; then + elif [[ -h "$g/HEAD" ]]; then # symlink symbolic ref b="$(git symbolic-ref HEAD 2>/dev/null)" else @@ -480,7 +480,7 @@ __git_ps1 () fi # is it a symbolic ref? b="${head#ref: }" - if [ "$head" = "$b" ]; then + if [[ "$head" = "$b" ]]; then detached=yes b="$( case "${GIT_PS1_DESCRIBE_STYLE-}" in @@ -502,7 +502,7 @@ __git_ps1 () fi fi - if [ -n "$step" ] && [ -n "$total" ]; then + if [[ -n "$step" ]] && [[ -n "$total" ]]; then r="$r $step/$total" fi @@ -515,41 +515,41 @@ __git_ps1 () local p="" # short version of upstream state indicator local upstream="" # verbose version of upstream state indicator - if [ "true" = "$inside_gitdir" ]; then - if [ "true" = "$bare_repo" ]; then + if [[ "true" = "$inside_gitdir" ]]; then + if [[ "true" = "$bare_repo" ]]; then c="BARE:" else b="GIT_DIR!" fi - elif [ "true" = "$inside_worktree" ]; then - if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ] && - [ "$(git config --bool bash.showDirtyState)" != "false" ] + elif [[ "true" = "$inside_worktree" ]]; then + if [[ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]] && + [[ "$(git config --bool bash.showDirtyState)" != "false" ]] then git diff --no-ext-diff --quiet || w="*" git diff --no-ext-diff --cached --quiet || i="+" - if [ -z "$short_sha" ] && [ -z "$i" ]; then + if [[ -z "$short_sha" ]] && [[ -z "$i" ]]; then i="#" fi fi - if [ -n "${GIT_PS1_SHOWSTASHSTATE-}" ] && + if [[ -n "${GIT_PS1_SHOWSTASHSTATE-}" ]] && git rev-parse --verify --quiet refs/stash >/dev/null then s="$" fi - if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ] && - [ "$(git config --bool bash.showUntrackedFiles)" != "false" ] && + if [[ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]] && + [[ "$(git config --bool bash.showUntrackedFiles)" != "false" ]] && git ls-files --others --exclude-standard --directory --no-empty-directory --error-unmatch -- ':/*' >/dev/null 2>/dev/null then u="%${ZSH_VERSION+%}" fi - if [ -n "${GIT_PS1_COMPRESSSPARSESTATE-}" ] && - [ "$(git config --bool core.sparseCheckout)" = "true" ]; then + if [[ -n "${GIT_PS1_COMPRESSSPARSESTATE-}" ]] && + [[ "$(git config --bool core.sparseCheckout)" = "true" ]]; then h="?" fi - if [ -n "${GIT_PS1_SHOWUPSTREAM-}" ]; then + if [[ -n "${GIT_PS1_SHOWUPSTREAM-}" ]]; then __git_ps1_show_upstream fi fi @@ -557,14 +557,14 @@ __git_ps1 () local z="${GIT_PS1_STATESEPARATOR-" "}" # NO color option unless in PROMPT_COMMAND mode or it's Zsh - if [ -n "${GIT_PS1_SHOWCOLORHINTS-}" ]; then - if [ $pcmode = yes ] || [ -n "${ZSH_VERSION-}" ]; then + if [[ -n "${GIT_PS1_SHOWCOLORHINTS-}" ]]; then + if [[ $pcmode = yes ]] || [[ -n "${ZSH_VERSION-}" ]]; then __git_ps1_colorize_gitstring fi fi b=${b##refs/heads/} - if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then + if [[ $pcmode = yes ]] && [[ $ps1_expanded = yes ]]; then __git_ps1_branch_name=$b b="\${__git_ps1_branch_name}" fi @@ -572,8 +572,8 @@ __git_ps1 () local f="$h$w$i$s$u$p" local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}" - if [ $pcmode = yes ]; then - if [ "${__git_printf_supports_v-}" != yes ]; then + if [[ $pcmode = yes ]]; then + if [[ "${__git_printf_supports_v-}" != yes ]]; then gitstring=$(printf -- "$printf_format" "$gitstring") else printf -v gitstring -- "$printf_format" "$gitstring"