[PATCH] git-prompt: use builtin test

[PATCH] git-prompt: use builtin test

[Brad Forschinger via GitGitGadget]

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.

Signed-off-by: Brad Forschinger <bnjf@bnjf.id.au>
---
    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.
    
    Signed-off-by: Brad Forschinger bnjf@bnjf.id.au

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(-)

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"

base-commit: 5699ec1b0aec51b9e9ba5a2785f65970c5a95d84
-- 
gitgitgadget

Re: [PATCH] git-prompt: use builtin test

[Jeff King]

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

Re: [PATCH] git-prompt: use builtin test

[Brad Forschinger]

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

Re: [PATCH] git-prompt: use builtin test

[Jeff King]

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

Re: [PATCH] git-prompt: use builtin test

[Junio C Hamano]

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.