git-prompt: use builtin test
PATCH 1/1 git-prompt: use builtin test
From: Brad Forschinger <[email protected]>
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.
---
git-prompt: use builtin test
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.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1260%2Fbnjf%2Fprompt-use-builtins-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1260/bnjf/prompt-use-builtins-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1260
contrib/completion/git-prompt.sh | 92 ++++++++++++++++---------------- 1 file changed, 46 insertions(+), 46 deletions(-)
contrib/completion/git-prompt.sh +46 -46
@@ -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"
base-commit: 5699ec1b0aec51b9e9ba5a2785f65970c5a95d84
Signature · 1 line
gitgitgadget
Quoted 6 lines · Brad Forschinger via GitGitGadget
From: Brad Forschinger <[email protected]>
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:
Review
Quoted 2 lines
(quoted diff) +1
-1- 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
Quoted 5 lines · Jeff King
Quoted 4 lines
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.
Quoted 8 lines
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.
Quoted 7 lines
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!
Quoted 2 lines
- 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.
Quoted 13 lines
- 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:
Review
Quoted 2 lines
(quoted diff) +1
-1- if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then + if [[ $pcmode = yes ]] && [[ $ps1_expanded = yes ]]; thenare 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.
Quoted 2 lines
(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
Quoted 8 lines · Brad Forschinger
Quoted 3 lines
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.
Quoted 11 lines
Quoted 4 lines
- 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
Quoted 10 lines · Jeff King
Quoted 6 lines
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...
Quoted 9 lines
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.