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

Commit Message

avih July 23, 2024, 7:18 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 July 23, 2024, 7:40 p.m. UTC | #1

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

> 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.

Check Documentation/CodingGuidelines; I think we have something to
say about local var="val" construct to help dash.

We allowed liberal uses of bash-ism in this file, as it was
initially written for bash anyway.  If we were rewriting the prompt
scripts to be usable by other shells, great.  But then we'd want to
make sure it adheres to existing coding guidelines we have.

avih July 24, 2024, 12:47 a.m. UTC | #2

On Tuesday, July 23, 2024 at 10:40:30 PM GMT+3, Junio C Hamano <[e-mail]> wrote:

Thanks for the quick reply, and aplogies for my delayed reply.

I replied at the github PR https://github.com/git/git/pull/1750
and didn't realize GitGitGadget doesn't forward it to the list.
Then I accidentally sent email with HTML. 3rd time the charm...

>> 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.

> Check Documentation/CodingGuidelines; I think we have something to
> say about local var="val" construct to help dash.

I wasn't aware of this file, but I should have searched for it before
posting. Thanks for the pointer.

As far as I can tell CodingGuidelines and my guideline align perfectly
on every subject which both mention, down to nuances like that quoted
initial value in "local", though each also has few subjects which the
other doesn't.

> ... If we were rewriting the prompt
> scripts to be usable by other shells, great.  But then we'd want to
> make sure it adheres to existing coding guidelines we have.

Not sure how many prompt scripts there are, but if you're referring
to the scripts at contrib/completion then only git-prompt.sh is
applicable in many shells and would gain by being portable. The
others are shell-specific, so I wouldn't think they need be portable.

As for git-prompt.sh, as far as I can tell, after this patchset, this
file adheres to CodingGuidelines completely as far as correctness and
compatibility go.

However, regardless of not being aware of CodingGuidelines, the goal
of this patchset was to improve compatibility and correctness, and I
wouldn't have chosen or felt comfortable to included style changes
("'then' in new line" can have also portability implications, though
not in the many shells which I tested).

So no change in terms of style, it still diverges from the guidelines.

Shall I add a commit which fixes style issues?

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"
 }