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

Commit Message

avih Aug. 17, 2024, 9:25 a.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 in parts which
contain parameter/arithmetic expansion or command substitution.

"Simple command" (POSIX term) is assignment[s] and/or command [args].
Examples:
  foo=bar         # one assignment
  foo=$bar x=y    # two assignments
  foo bar         # command, no assignments
  x=123 foo bar   # one assignment and a command

The assignments part is not IFS-split or glob-expanded.

The command+args part does get IFS field split and glob expanded,
but only at unquoted expanded/substituted parts.

In the command+args part, expanded/substituted values must be quoted.
(the commands here are "[" and "local"):
  Good: [ "$mode" = yes ]; local s="*" x="$y" e="$?" z="$(cmd ...)"
  Bad:  [ $mode = yes ];   local s=*   x=$y   e=$?   z=$(cmd...)

The arguments to "local" do look like assignments, but they're not
the assignment part of a simple command. they're at the command part.

Still at the command part, 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.

The value between 'case' and 'in' doesn't IFS-split/glob-expand:
  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

Eric Sunshine Aug. 17, 2024, 9:38 a.m. UTC | #1

On Sat, Aug 17, 2024 at 5:26 AM Avi Halachmi (:avih) via GitGitGadget
<[e-mail]> wrote:
> 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.

s/guideline/guidelines/

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

perhaps: s/is as/it as/

> "Simple command" (POSIX term) is assignment[s] and/or command [args].
> Examples:
>   foo=bar         # one assignment
>   foo=$bar x=y    # two assignments
>   foo bar         # command, no assignments
>   x=123 foo bar   # one assignment and a command
>
> The assignments part is not IFS-split or glob-expanded.
>
> The command+args part does get IFS field split and glob expanded,
> but only at unquoted expanded/substituted parts.
>
> In the command+args part, expanded/substituted values must be quoted.
> (the commands here are "[" and "local"):
>   Good: [ "$mode" = yes ]; local s="*" x="$y" e="$?" z="$(cmd ...)"
>   Bad:  [ $mode = yes ];   local s=*   x=$y   e=$?   z=$(cmd...)

This new explanation in v3 is a helpful addition.

> The arguments to "local" do look like assignments, but they're not
> the assignment part of a simple command. they're at the command part.

either: s/they're/They're/
or: s/. they're/; they're/

I doubt that any of the above extremely minor commit message botches
is worth a reroll.

avih Aug. 17, 2024, 10:07 a.m. UTC | #2

On Saturday, August 17, 2024 at 12:38:23 PM GMT+3, Eric Sunshine <[e-mail]> wrote:
> On Sat, Aug 17, 2024 at 5:26 AM Avi Halachmi (:avih) via GitGitGadget
>> Listing some portability guideline here for future reference.
>
>> s/guideline/guidelines/

Right.

>> 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.
>
> perhaps: s/is as/it as/

Indeed.

>> "Simple command" (POSIX term) is assignment[s] and/or command [args].
>> Examples:
>> ...
>
> This new explanation in v3 is a helpful addition.

Thanks.

>> The arguments to "local" do look like assignments, but they're not
>> the assignment part of a simple command. they're at the command part.
>
> either: s/they're/They're/
> or: s/. they're/; they're/

Indeed. Thanks for the comments.

> I doubt that any of the above extremely minor commit message botches
> is worth a reroll.

I'm not the one to judge that, but I'm OK to push as many new versions
as deemed required/preferred. For now I'm waiting for someone to say
whether I should do that or not... and if yes - when.

I was trying to wait few days for more comments on v2 (perhaps
like yours), but I noticed that v2 was already was just integrated
into "seen", so I posted v3 to address the existing comments on v2.

This is my 1st patch to "git", so still finding my feet with the
procedures.

avih

Junio C Hamano Aug. 17, 2024, 4:28 p.m. UTC | #3

avih <[e-mail]> writes:

> I was trying to wait few days for more comments on v2 (perhaps
> like yours), but I noticed that v2 was already was just integrated
> into "seen", so I posted v3 to address the existing comments on v2.

Please consider that it is just like being on the list and nothing
else to be in "seen".  It merely is another place some patches I've
"seen" are published, to help those of you who find "git fetch &&
git log -pW origin/master..origin/topic" a more convenient way to
review the changes.  This is outlined in the note I send out
occasionally.

    https://lore.kernel.org/git/xmqqmslewwpo.fsf@gitster.g/

If you think that v2 needs a few more days' exposure to receive more
feedback from reviewers, and that v3 might be incomplete before
waiting for their feedback, just saying so as a response to the
"What's cooking" message is a very effective way to make sure I'll
wait for an updated iteration.  Such a comment on individual topics
is *not* limited to the author of the topic, e.g.

  https://lore.kernel.org/git/[e-mail]/

is an example (but the particular example was sent a bit too
late---such a "wait, don't merge, an update is coming" needs to come
before the topic is merged down to 'next'.

  https://lore.kernel.org/git/[e-mail]/

is an example of a reviewer sending a "stop, I have an improvement
to suggest on this one I'll send soon" on a topic by somebody else.

  https://lore.kernel.org/git/Zr2_4sNu56_frqlr@tanuki/

is an example of the author saying "this topic of mine should be
complete already", which is a bit less usual and takes some finesse
to avoid sounding too self-promoting (this particular example does
so well).

avih Aug. 17, 2024, 6:02 p.m. UTC | #4

On Saturday, August 17, 2024 at 07:28:44 PM GMT+3, Junio C Hamano <[e-mail]> wrote:
> avih <[e-mail]> writes:
>> I was trying to wait few days for more comments on v2 (perhaps
>> like yours), but I noticed that v2 was already was just integrated
>> into "seen", so I posted v3 to address the existing comments on v2.
>
> Please consider that it is just like being on the list and nothing
> else to be in "seen".  It merely is another place some patches I've
> "seen" are published, to help those of you who find "git fetch &&
> git log -pW origin/master..origin/topic" a more convenient way to
> review the changes.  This is outlined in the note I send out
> occasionally.
>
>     https://lore.kernel.org/git/xmqqmslewwpo.fsf@gitster.g/

Thanks for the time and info. That's a useful intro, and I now do
see it says this:

  until a topic is merged to "next", updates to it is expected
  by replacing the patch(es) in the topic with an improved version

> If you think that v2 needs a few more days' exposure to receive more
> feedback from reviewers, and that v3 might be incomplete before
> waiting for their feedback, just saying so as a response to the
> "What's cooking" message is a very effective way to make sure I'll
> wait for an updated iteration.  Such a comment on individual topics
> is *not* limited to the author of the topic, e.g.
>
>   https://lore.kernel.org/git/[e-mail]/
>
> is an example ...

Thanks. I replied few times that requested changes will be included
in v3, so I thought it's apparent that v3 is sill to come, but in
retrospect, when you replied to [PATCH v2 0/8]:

> I've read the series and they looked all sensible.  Will queue but
> I'd appreciate a second set of eyes before marking it for 'next'.

Then I should have replied with something along those lines:

  Please wait for v3 with requested non-critical changes (typos in
  comment and commit message) before moving it to "next", if at all.

or even sending the same message once I noticed it was merged into
"seen", instead of posting v3 out of "panic" that it boarded the
train without all the requested changes applied, yes?

If yes, then here's heads-up that there's still another non-critical
requested change to arrive in v4 (commit message wording), which I'll
send in few days, to allow for further comments to arrive.

Because resending the whole series (is that "reroll"?) for every
minor typo or wording change feels noisy and inappropriate to me.

Thanks again for the time and info.

avih

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