[v2,5/8] git-prompt: add some missing quotes

Commit Message

avih Aug. 15, 2024, 1:14 p.m. UTC

From: "Avi Halachmi (:avih)" <[e-mail]>

The issues which this commit fixes are unlikely to be broken
in real life, but the fixes improve correctness, and would prevent
bugs in some uncommon cases, such as weird IFS values.

Listing some portability guideline here for future reference.

I'm leaving it to someone else to decide whether to include
it in the file itself, place is as a new file, or not.

---------

The command "local" is non standard, but is allowed in this file:
- Quote initialization if it can expand (local x="$y"). See below.
- Don't assume initial value after "local x". Either initialize it
  (local x=..), or set before first use (local x;.. x=..; <use $x>).
  (between shells, "local x" can unset x, or inherit it, or do x= )

Other non-standard features beyond "local" are to be avoided.

Use the standard "test" - [...] instead of non-standard [[...]] .

--------

Quotes (some portability things, but mainly general correctness):

Quotes prevent tilde-expansion of some unquoted literal tildes (~).
If the expansion is undesirable, quotes would ensure that.
  Tilds expanded: a=~user:~/ ;  echo ~user ~/dir
  not expanded:   t="~"; a=${t}user  b=\~foo~;  echo "~user" $t/dir

But the main reason for quoting is to prevent IFS field splitting
(which also coalesces IFS chars) and glob expansion after parameter
expansion or command substitution.

In _command-arguments_, expanded/substituted values must be quoted:
  Good: [ "$mode" = yes ]; local s="*" x="$y" e="$?" z="$(cmd ...)"
  Bad:  [ $mode = yes ];   local s=*   x=$y   e=$?   z=$(cmd...)

Still in _agumemts_, no need to quote non-expandable values:
  Good:                 local x=   y=yes;   echo OK
  OK, but not required: local x="" y="yes"; echo "OK"
But completely empty (NULL) arguments must be quoted:
  foo ""   is not the same as:   foo

Assignments in simple commands - with or without an actual command,
don't need quoting becase there's no IFS split or glob expansion:
  Good:   s=* a=$b c=$(cmd...)${x# foo }${y-   } [cmd ...]
  It's also OK to use double quotes, but not required.

This behavior (no IFS/glob) is called "assignment context", and
"local" does not behave with assignment context in some shells,
hence we require quotes when using "local" - for compatibility.

First value in 'case...' doesn't IFS-split/glob, doesn't need quotes:
  Good:       case  * $foo $(cmd...)  in ... ; esac
  identical:  case "* $foo $(cmd...)" in ... ; esac

Nested quotes in command substitution are fine, often necessary:
  Good: echo "$(foo... "$x" "$(bar ...)")"

Nested quotes in substring ops are legal, and sometimes needed
to prevent interpretation as a pattern, but not the most readable:
  Legal:  foo "${x#*"$y" }"

Nested quotes in "maybe other value" subst are invalid, unnecessary:
  Good:  local x="${y- }";   foo "${z:+ $a }"
  Bad:   local x="${y-" "}"; foo "${z:+" $a "}"
Outer/inner quotes in "maybe other value" have different use cases:
  "${x-$y}"  always one quoted arg: "$x" if x is set, else "$y".
  ${x+"$x"}  one quoted arg "$x" if x is set, else no arg at all.
  Unquoted $x is similar to the second case, but it would get split
  into few arguments if it includes any of the IFS chars.

Assignments don't need the outer quotes, and the braces delimit the
value, so nested quotes can be avoided, for readability:
  a=$(foo "$x")  a=${x#*"$y" }  c=${y- };  bar "$a" "$b" "$c"

Signed-off-by: Avi Halachmi (:avih) <[e-mail]>
---
 contrib/completion/git-prompt.sh | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

Comments

Junio C Hamano Aug. 15, 2024, 4:36 p.m. UTC | #1

"Avi Halachmi (:avih) via GitGitGadget" <[e-mail]>
writes:

> In _command-arguments_, expanded/substituted values must be quoted:
>   Good: [ "$mode" = yes ]; local s="*" x="$y" e="$?" z="$(cmd ...)"
>   Bad:  [ $mode = yes ];   local s=*   x=$y   e=$?   z=$(cmd...)
>
> Still in _agumemts_, no need to quote non-expandable values:

arguments.

> -	local bad_color=$c_red
> -	local ok_color=$c_green
> +	local bad_color="$c_red"
> +	local ok_color="$c_green"
>  	local flags_color="$c_lblue"

Good.  I think we in the past was burned by some shells that want to
see these assignments with "local" always quoted.

>  	# preserve exit status
> -	local exit=$?
> +	local exit="$?"
>  	local pcmode=no

Well no matter what value $? has, it by definition has a few digits
without any $IFS funnies.  Does this really matter?  I'd imagine
that we would prefer to treat "$?" exactly the same way as "no".

> @@ -379,7 +379,7 @@ __git_ps1 ()
>  		;;
>  		0|1)	printf_format="${1:-$printf_format}"
>  		;;
> -		*)	return $exit
> +		*)	return "$exit"
>  		;;

Likewise.

avih Aug. 15, 2024, 5:35 p.m. UTC | #2

On Thursday, August 15, 2024 at 07:36:43 PM GMT+3, Junio C Hamano <[e-mail]> wrote:
>> > Still in _agumemts_, no need to quote non-expandable values:
>>
> arguments.

Thanks. Will fix in v3 (after more comments unless asked otherwise).

>> -    local bad_color=$c_red
>> +    local bad_color="$c_red"
>
> Good.  I think we in the past was burned by some shells that want to
> see these assignments with "local" always quoted.

Yes. After I reached the same conclusion I noticed it was also added
to CodingGuidelines not long ago at be34b510 (CodingGuidelines: quote
assigned value in 'local var=$val').

>>      # preserve exit status
>> -    local exit=$?
>> +    local exit="$?"
>
> Well no matter what value $? has, it by definition has a few digits
> without any $IFS funnies.  Does this really matter?  I'd imagine
> that we would prefer to treat "$?" exactly the same way as "no".
>
> -        *)    return $exit
> +        *)    return "$exit"
>
> Likewise.

Two things here:

1. It can matter, because we don't control IFS. __git_ps1 is
   a function which runs in the user's shell, so if the user did
   IFS=0123, then unquoted $? or $exit can get IFS-split.
   As the commit message notes, this is unlikely to fix things in
   practice, but it will fix things with weird IFS values.

2. In general, yes, $? is only needed as yes/no, and there's only
   one place which tests $? instead of using "&&" or "||" after
   a command in this file (rev_parse_exit_code="$?"). I didn't feel
   this needs any portability fix. It works.

   But with $exit, $? is not used as yes/no, but rather to preserve
   the exit status when __git_ps1 was entered. This is important if
   the user wants the shell's last command $? at the prompt, e.g.:

   PS1='\w$(__git_ps1)$(e=$?; [ "$e" = 0 ] || echo " E:$e") \$ '

   If __git_ps1 didn't exit with the same $? it saw on entry, then
   $e will be __git_ps1's exit code rather than the exit code of
   the last command which ran in the shell, so it should be the
   same value as before and not only yes/no.

Junio C Hamano Aug. 15, 2024, 7:15 p.m. UTC | #3

avih <[e-mail]> writes:

>> Well no matter what value $? has, it by definition has a few digits
>> without any $IFS funnies.  Does this really matter?  I'd imagine
>> that we would prefer to treat "$?" exactly the same way as "no".
> ...
> Two things here:
>
> 1. It can matter, because we don't control IFS. __git_ps1 is
>    a function which runs in the user's shell, so if the user did
>    IFS=0123, then unquoted $? or $exit can get IFS-split.

Fair enough.  My "we would prefer to treat $? exactly the same way
as no" still stands.  If the user did IFS=o, "no" would be broken.

>    As the commit message notes, this is unlikely to fix things in
>    practice, but it will fix things with weird IFS values.

Yes, so I'd prefer to see us being consistent.  If we quote "$?" to
protect ourselves from crazy folks who set insane values to $IFS, we
should quote "no" the same way, no?

avih Aug. 15, 2024, 7:53 p.m. UTC | #4

On Thursday, August 15, 2024 at 10:15:53 PM GMT+3, Junio C Hamano <[e-mail]> wrote:

> Fair enough.  My "we would prefer to treat $? exactly the same way
> as no" still stands.  If the user did IFS=o, "no" would be broken.
>
>>    As the commit message notes, this is unlikely to fix things in
>>    practice, but it will fix things with weird IFS values.
>
>
> Yes, so I'd prefer to see us being consistent.  If we quote "$?" to
> protect ourselves from crazy folks who set insane values to $IFS, we
> should quote "no" the same way, no?

OK, I see what you mean. But IFS-split doesn't happen on literal
shell input (i.e. script source). It only happens on parts which
get expanded with parameter or arithmetic expansion or command
substitution. To quote from POSIX (2024):

  After parameter expansion (2.6.2), command substitution (2.6.3),
  and arithmetic expansion (2.6.4), if the shell variable IFS
  (see 2.5.3 Shell Variables ) is set and its value is not empty,
  or if IFS is unset, the shell shall scan each field containing
  results of expansions and substitutions that did not occur in
  double-quotes for field splitting; zero, one or multiple fields
  can result.

So 'IFS=n; reply=no; echo no; local x=no' work regardless of IFS,
because there's no expansion, and IFS is not involved.

But 'IFS=n; reply=no; echo $reply; local x=$reply' will be affected
when unquoted $reply expands as argument to "echo" (or "local"), and
then gets split by "n", so it would echo "o" (" o" because reasons).
Fixed with quotes: IFS=n; reply=no; echo "$reply"; local x="$reply"

Both can even be adjacent: 'IFS=n reply=no; echo no $reply'
would echo "no  o" in all shells, because the literal `no' is
unaffected, but the expanded $reply is affacted.

So $? is the same as $reply in this regards - it expands to some
value, so IFS gets involved, so it needs quotes. But a literal `no'
works the same regardless if quoted or not.

Worth noting in our context is that zsh doesn't do IFS word-split
by default on parameter expansion like $x, but does split the output
of command substitution $(cmd....), and code in git-prompt.sh is
expected to work in zsh as well (like it always did).

Patch

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 4781261f868..5d7f236fe48 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -246,7 +246,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}"
@@ -278,12 +278,12 @@  __git_ps1_colorize_gitstring ()
 		local c_lblue=$'\001\e[1;34m\002'
 		local c_clear=$'\001\e[0m\002'
 	fi
-	local bad_color=$c_red
-	local ok_color=$c_green
+	local bad_color="$c_red"
+	local ok_color="$c_green"
 	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"
@@ -360,7 +360,7 @@  __git_sequencer_status ()
 __git_ps1 ()
 {
 	# preserve exit status
-	local exit=$?
+	local exit="$?"
 	local pcmode=no
 	local detached=no
 	local ps1pc_start='\u@\h:\w '
@@ -379,7 +379,7 @@  __git_ps1 ()
 		;;
 		0|1)	printf_format="${1:-$printf_format}"
 		;;
-		*)	return $exit
+		*)	return "$exit"
 		;;
 	esac
 
@@ -427,7 +427,7 @@  __git_ps1 ()
 	rev_parse_exit_code="$?"
 
 	if [ -z "$repo_info" ]; then
-		return $exit
+		return "$exit"
 	fi
 
 	local short_sha=""
@@ -449,7 +449,7 @@  __git_ps1 ()
 	   [ "$(git config --bool bash.hideIfPwdIgnored)" != "false" ] &&
 	   git check-ignore -q .
 	then
-		return $exit
+		return "$exit"
 	fi
 
 	local sparse=""
@@ -499,7 +499,7 @@  __git_ps1 ()
 			case "$ref_format" in
 			files)
 				if ! __git_eread "$g/HEAD" head; then
-					return $exit
+					return "$exit"
 				fi
 
 				case $head in
@@ -597,10 +597,10 @@  __git_ps1 ()
 		fi
 	fi
 
-	local z="${GIT_PS1_STATESEPARATOR-" "}"
+	local z="${GIT_PS1_STATESEPARATOR- }"
 
 	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
@@ -612,7 +612,7 @@  __git_ps1 ()
 	local f="$h$w$i$s$u$p"
 	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}${conflict}"
 
-	if [ $pcmode = yes ]; then
+	if [ "$pcmode" = yes ]; then
 		if [ "${__git_printf_supports_v-}" != yes ]; then
 			gitstring=$(printf -- "$printf_format" "$gitstring")
 		else
@@ -623,5 +623,5 @@  __git_ps1 ()
 		printf -- "$printf_format" "$gitstring"
 	fi
 
-	return $exit
+	return "$exit"
 }