threads / patch / 59622

patch, 2 partscompletion: quote arguments of test and [

Subject: [PATCH 1/2] completion: quote arguments of test and [

## tl;dr

10 messages between Apr 20, 2023 and Apr 24, 2023. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Koichi Murase· Apr 20, 2023, 07:46 UTC · lore

The raw command substitutions $v in the arguments of the test command and the [ command are subject to word splitting and pathname expansions. Even when it is ensured that the variable is not empty and does not contain whitespaces and glob characters, it can fail when IFS is set to non-trivial values containing letters and digits.

To prevent them from being unexpectedly processed by word splitting and pathname expansions, properly quote the unquoted command substituations in the arguments of the `test` and `[` builtins.

Co-authored-by: Edwin Kofler <edwin@kofler.dev>
Signed-off-by: Koichi Murase <myoga.murase@gmail.com>
Signed-off-by: Edwin Kofler <edwin@kofler.dev>
---
 contrib/completion/git-completion.bash | 62 +++++++++++++-------------
 contrib/completion/git-prompt.sh       |  8 ++--
 2 files changed, 35 insertions(+), 35 deletions(-)
Show changes to 2 files +35 −35

contrib/completion/git-completion.bash, contrib/completion/git-prompt.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index dc95c34cc8..6c110c223b 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -253,19 +253,19 @@ __git_reassemble_comp_words_by_ref()
 		# word separator characters to the current word.
 		first=t
 		while
-			[ $i -gt 0 ] &&
+			[ "$i" -gt 0 ] &&
 			[ -n "${COMP_WORDS[$i]}" ] &&
 			# word consists of excluded word separators
 			[ "${COMP_WORDS[$i]//[^$exclude]}" = "${COMP_WORDS[$i]}" ]
 		do
 			# Attach to the previous token,
 			# unless the previous token is the command name.
-			if [ $j -ge 2 ] && [ -n "$first" ]; then
+			if [ "$j" -ge 2 ] && [ -n "$first" ]; then
 				((j--))
 			fi
 			first=
 			words_[$j]=${words_[j]}${COMP_WORDS[i]}
-			if [ $i = $COMP_CWORD ]; then
+			if [ "$i" = "$COMP_CWORD" ]; then
 				cword_=$j
 			fi
 			if (($i < ${#COMP_WORDS[@]} - 1)); then
@@ -276,7 +276,7 @@ __git_reassemble_comp_words_by_ref()
 			fi
 		done
 		words_[$j]=${words_[j]}${COMP_WORDS[i]}
-		if [ $i = $COMP_CWORD ]; then
+		if [ "$i" = "$COMP_CWORD" ]; then
 			cword_=$j
 		fi
 	done
@@ -292,7 +292,7 @@ _get_comp_words_by_ref ()
 	fi
 	__git_reassemble_comp_words_by_ref "$exclude"
 	cur_=${words_[cword_]}
-	while [ $# -gt 0 ]; do
+	while [ "$#" -gt 0 ]; do
 		case "$1" in
 		cur)
 			cur=$cur_
@@ -848,7 +848,7 @@ __git_complete_refs ()
 {
 	local remote= dwim= pfx= cur_="$cur" sfx=" " mode="refs"
 
-	while test $# != 0; do
+	while test "$#" != 0; do
 		case "$1" in
 		--remote=*)	remote="${1##--remote=}" ;;
 		--dwim)		dwim="yes" ;;
@@ -1036,7 +1036,7 @@ __git_complete_remote_or_refspec ()
 	if [ "$cmd" = "remote" ]; then
 		((c++))
 	fi
-	while [ $c -lt $cword ]; do
+	while [ "$c" -lt "$cword" ]; do
 		i="${words[c]}"
 		case "$i" in
 		--mirror) [ "$cmd" = "push" ] && no_complete_refspec=1 ;;
@@ -1060,7 +1060,7 @@ __git_complete_remote_or_refspec ()
 		__gitcomp_nl "$(__git_remotes)"
 		return
 	fi
-	if [ $no_complete_refspec = 1 ]; then
+	if [ "$no_complete_refspec" = 1 ]; then
 		return
 	fi
 	[ "$remote" = "." ] && remote=
@@ -1080,21 +1080,21 @@ __git_complete_remote_or_refspec ()
 	esac
 	case "$cmd" in
 	fetch)
-		if [ $lhs = 1 ]; then
+		if [ "$lhs" = 1 ]; then
 			__git_complete_fetch_refspecs "$remote" "$pfx" "$cur_"
 		else
 			__git_complete_refs --pfx="$pfx" --cur="$cur_"
 		fi
 		;;
 	pull|remote)
-		if [ $lhs = 1 ]; then
+		if [ "$lhs" = 1 ]; then
 			__git_complete_refs --remote="$remote" --pfx="$pfx" --cur="$cur_"
 		else
 			__git_complete_refs --pfx="$pfx" --cur="$cur_"
 		fi
 		;;
 	push)
-		if [ $lhs = 1 ]; then
+		if [ "$lhs" = 1 ]; then
 			__git_complete_refs --pfx="$pfx" --cur="$cur_"
 		else
 			__git_complete_refs --remote="$remote" --pfx="$pfx" --cur="$cur_"
@@ -1203,7 +1203,7 @@ __git_find_on_cmdline ()
 {
 	local word c="$__git_cmd_idx" show_idx
 
-	while test $# -gt 1; do
+	while test "$#" -gt 1; do
 		case "$1" in
 		--show-idx)	show_idx=y ;;
 		*)		return 1 ;;
@@ -1212,7 +1212,7 @@ __git_find_on_cmdline ()
 	done
 	local wordlist="$1"
 
-	while [ $c -lt $cword ]; do
+	while [ "$c" -lt "$cword" ]; do
 		for word in $wordlist; do
 			if [ "$word" = "${words[c]}" ]; then
 				if [ -n "${show_idx-}" ]; then
@@ -1237,7 +1237,7 @@ __git_find_last_on_cmdline ()
 {
 	local word c=$cword show_idx
 
-	while test $# -gt 1; do
+	while test "$#" -gt 1; do
 		case "$1" in
 		--show-idx)	show_idx=y ;;
 		*)		return 1 ;;
@@ -1246,7 +1246,7 @@ __git_find_last_on_cmdline ()
 	done
 	local wordlist="$1"
 
-	while [ $c -gt "$__git_cmd_idx" ]; do
+	while [ "$c" -gt "$__git_cmd_idx" ]; do
 		((c--))
 		for word in $wordlist; do
 			if [ "$word" = "${words[c]}" ]; then
@@ -1286,7 +1286,7 @@ __git_get_option_value ()
 	config_key="$4"
 
 	((c = $cword - 1))
-	while [ $c -ge 0 ]; do
+	while [ "$c" -ge 0 ]; do
 		word="${words[c]}"
 		for val in $values; do
 			if [ "$short_opt$val" = "$word" ] ||
@@ -1308,7 +1308,7 @@ __git_get_option_value ()
 __git_has_doubledash ()
 {
 	local c=1
-	while [ $c -lt $cword ]; do
+	while [ "$c" -lt "$cword" ]; do
 		if [ "--" = "${words[c]}" ]; then
 			return 0
 		fi
@@ -1474,7 +1474,7 @@ _git_branch ()
 {
 	local i c="$__git_cmd_idx" only_local_ref="n" has_r="n"
 
-	while [ $c -lt $cword ]; do
+	while [ "$c" -lt "$cword" ]; do
 		i="${words[c]}"
 		case "$i" in
 		-d|-D|--delete|-m|-M|--move|-c|-C|--copy)
@@ -1493,7 +1493,7 @@ _git_branch ()
 		__gitcomp_builtin branch
 		;;
 	*)
-		if [ $only_local_ref = "y" -a $has_r = "n" ]; then
+		if [ "$only_local_ref" = "y" -a "$has_r" = "n" ]; then
 			__gitcomp_direct "$(__git_heads "" "$cur" " ")"
 		else
 			__git_complete_refs
@@ -1897,7 +1897,7 @@ __git_match_ctag () {
 __git_complete_symbol () {
 	local tags=tags pfx="" cur_="${cur-}" sfx=" "
 
-	while test $# != 0; do
+	while test "$#" != 0; do
 		case "$1" in
 		--tags=*)	tags="${1##--tags=}" ;;
 		--pfx=*)	pfx="${1##--pfx=}" ;;
@@ -2166,7 +2166,7 @@ _git_mv ()
 		;;
 	esac
 
-	if [ $(__git_count_arguments "mv") -gt 0 ]; then
+	if [ "$(__git_count_arguments "mv")" -gt 0 ]; then
 		# We need to show both cached and untracked files (including
 		# empty directories) since this may not be the last argument.
 		__git_complete_index_file "--cached --others --directory"
@@ -2496,7 +2496,7 @@ _git_switch ()
 __git_config_get_set_variables ()
 {
 	local prevword word config_file= c=$cword
-	while [ $c -gt "$__git_cmd_idx" ]; do
+	while [ "$c" -gt "$__git_cmd_idx" ]; do
 		word="${words[c]}"
 		case "$word" in
 		--system|--global|--local|--file=*)
@@ -2541,7 +2541,7 @@ __git_complete_config_variable_value ()
 {
 	local varname="$prev" cur_="$cur"
 
-	while test $# != 0; do
+	while test "$#" != 0; do
 		case "$1" in
 		--varname=*)	varname="${1##--varname=}" ;;
 		--cur=*)	cur_="${1##--cur=}" ;;
@@ -2656,7 +2656,7 @@ __git_complete_config_variable_name ()
 {
 	local cur_="$cur" sfx
 
-	while test $# != 0; do
+	while test "$#" != 0; do
 		case "$1" in
 		--cur=*)	cur_="${1##--cur=}" ;;
 		--sfx=*)	sfx="${1##--sfx=}" ;;
@@ -2756,7 +2756,7 @@ __git_complete_config_variable_name_and_value ()
 {
 	local cur_="$cur"
 
-	while test $# != 0; do
+	while test "$#" != 0; do
 		case "$1" in
 		--cur=*)	cur_="${1##--cur=}" ;;
 		*)		return 1 ;;
@@ -3096,7 +3096,7 @@ _git_stash ()
 		__gitcomp_builtin "stash_$subcommand"
 		;;
 	branch,*)
-		if [ $cword -eq $((__git_cmd_idx+2)) ]; then
+		if [ "$cword" -eq "$((__git_cmd_idx+2))" ]; then
 			__git_complete_refs
 		else
 			__gitcomp_nl "$(__git stash list \
@@ -3261,7 +3261,7 @@ _git_svn ()
 _git_tag ()
 {
 	local i c="$__git_cmd_idx" f=0
-	while [ $c -lt $cword ]; do
+	while [ "$c" -lt "$cword" ]; do
 		i="${words[c]}"
 		case "$i" in
 		-d|--delete|-v|--verify)
@@ -3279,7 +3279,7 @@ _git_tag ()
 	-m|-F)
 		;;
 	-*|tag)
-		if [ $f = 1 ]; then
+		if [ "$f" = 1 ]; then
 			__gitcomp_direct "$(__git_tags "" "$cur" " ")"
 		fi
 		;;
@@ -3342,7 +3342,7 @@ _git_worktree ()
 			# be either the 'add' subcommand, the unstuck
 			# argument of an option (e.g. branch for -b|-B), or
 			# the path for the new worktree.
-			if [ $cword -eq $((subcommand_idx+1)) ]; then
+			if [ "$cword" -eq "$((subcommand_idx+1))" ]; then
 				# Right after the 'add' subcommand: have to
 				# complete the path, so fall back to Bash
 				# filename completion.
@@ -3366,7 +3366,7 @@ _git_worktree ()
 		__git_complete_worktree_paths
 		;;
 	move,*)
-		if [ $cword -eq $((subcommand_idx+1)) ]; then
+		if [ "$cword" -eq "$((subcommand_idx+1))" ]; then
 			# The first parameter must be an existing working
 			# tree to be moved.
 			__git_complete_worktree_paths
@@ -3436,7 +3436,7 @@ __git_main ()
 	local __git_C_args C_args_count=0
 	local __git_cmd_idx
 
-	while [ $c -lt $cword ]; do
+	while [ "$c" -lt "$cword" ]; do
 		i="${words[c]}"
 		case "$i" in
 		--git-dir=*)
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 76ee4ab1e5..9c10690a22 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -232,7 +232,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}"
@@ -269,7 +269,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"
@@ -567,7 +567,7 @@ __git_ps1 ()
 	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
@@ -579,7 +579,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
-- 
2.39.0
Koichi Murase· Apr 20, 2023, 07:46 UTC · re: Koichi Murase · lore

[PATCH 2/2] completion: suppress unwanted unescaping of `read`

From: Edwin Kofler <edwin@kofler.dev>

The function `__git_eread`, that reads the first line from the file, calls the `read` builtin without passing the flag option `-r`. When the `read` builtin is called without the flag `-r`, it processes the backslash escaping in the text that it reads. We usually do not want to process backslashes of the input but want to read the raw contents.

To make it read the first line as is, pass the flag `-r` to the `read` builtin in the function `__git_eread`.

Signed-off-by: Edwin Kofler <edwin@kofler.dev>
Signed-off-by: Koichi Murase <myoga.murase@gmail.com>
---
 contrib/completion/git-prompt.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to contrib/completion/git-prompt.sh +1 −1
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 9c10690a22..49dd69bb84 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -298,7 +298,7 @@ __git_ps1_colorize_gitstring ()
 # variable, in that order.
 __git_eread ()
 {
-	test -r "$1" && IFS=$'\r\n' read "$2" <"$1"
+	test -r "$1" && IFS=$'\r\n' read -r "$2" <"$1"
 }
 
 # see if a cherry-pick or revert is in progress, if the user has committed a
-- 
2.39.0
Junio C Hamano· Apr 20, 2023, 16:45 UTC · re: Koichi Murase · lore

Re: [PATCH 2/2] completion: suppress unwanted unescaping of `read`

Koichi Murase <myoga.murase@gmail.com> writes:
Show 10 quoted lines
> From: Edwin Kofler <edwin@kofler.dev>
>
> The function `__git_eread`, that reads the first line from the file,
> calls the `read` builtin without passing the flag option `-r`.  When
> the `read` builtin is called without the flag `-r`, it processes the
> backslash escaping in the text that it reads.  We usually do not want
> to process backslashes of the input but want to read the raw contents.
>
> To make it read the first line as is, pass the flag `-r` to the `read`
> builtin in the function `__git_eread`.
We USUALLY do not want?

If there were even a single caller that does rely on the usual backslash processing happening in the current code, this patch is a breaking change for them, so that phrase is not acceptable as a justification for this change. Perhaps

    After looking at ALL the existing callers of __git_eread, it
    turns out that they all want to read the first line of the file
    literally.  Worse yet, some files they read may record file
    paths, which may contain a backslash, so what they will read are
    corrupted unless we use `read -r`.
or something along that line would be more appropriate.

I did look at all the callers and I think they want to read things literally, so I support the use of "read -r". But I didn't validate the other claim "may contain backslash"---the "... file paths, which may contain ..." in the above example is a totally made up claim, as I do not have access to a platform whose pathname separator is backslash. Please replace that part to describe the real world problem you encountered that led to this fix, if there is one.

Other than that, a very well written description and good change.
Thanks.
Show 19 quoted lines
> Signed-off-by: Edwin Kofler <edwin@kofler.dev>
> Signed-off-by: Koichi Murase <myoga.murase@gmail.com>
> ---
>  contrib/completion/git-prompt.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 9c10690a22..49dd69bb84 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -298,7 +298,7 @@ __git_ps1_colorize_gitstring ()
>  # variable, in that order.
>  __git_eread ()
>  {
> -	test -r "$1" && IFS=$'\r\n' read "$2" <"$1"
> +	test -r "$1" && IFS=$'\r\n' read -r "$2" <"$1"
>  }
>  
>  # see if a cherry-pick or revert is in progress, if the user has committed a
Koichi Murase· Apr 20, 2023, 22:31 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] completion: suppress unwanted unescaping of `read`

Thank you for your review. The same as the other patch, this is also a change submitted to our project and then forwarded here.

2023年4月21日(金) 1:45 Junio C Hamano <gitster@pobox.com>:
Show 6 quoted lines
> [...] But I didn't validate
> the other claim "may contain backslash"---the "... file paths, which
> may contain ..." in the above example is a totally made up claim, as
> I do not have access to a platform whose pathname separator is
> backslash.  Please replace that part to describe the real world
> problem you encountered that led to this fix, if there is one.

Because of the background of this patch (which was originally submitted as a part of the consistent coding style in our project), I wouldn't think this change is associated with a problem that has happened in real use cases. But maybe Edwin has experienced actual problems.

Edwin, did you have actual problems with the current implementation of `__git_eread`?

For the record, I now installed Git for Windows in my Windows and created a repository, but .git/HEAD contains something like "ref: refs/heads/master" as usual.

Anyway, the default behavior of `read` without `-r` processing backslashes is just a historical one, and it is generally considered the best practice to always use `read -r` unless one intensionally unescapes backslashes. Using `read` without `-r` is exceptional, and doing so for no reason would be noisy in reading the code as the intent is unclear. If flag -r in `__git_eread` would be rejected, I would suggest adding code comments in `__git_eread` like "# We intensionally process backslashes in the file because XXX".

I'll soon submit a new patch with an updated commit message in the next reply.
Koichi Murase· Apr 20, 2023, 22:38 UTC · re: Koichi Murase · lore

[PATCH] completion: suppress unwanted unescaping of `read`

From: Edwin Kofler <edwin@kofler.dev>

The function `__git_eread`, which reads the first line from the file, calls the `read` builtin without passing the flag option `-r`. When the `read` builtin is called without the flag `-r`, it processes the backslash escaping in the text that it reads. For this reason, it is generally considered the best practice to always use the `read` builtin with flag `-r` unless one intensionally processes the backslash escaping. For the present case in git-prompt.sh, in fact, all the occurrences of the calls of `__git_eread` intend to read the literal content of the first lines.

To make it read the first line literally, pass the flag `-r` to the `read` builtin in the function `__git_eread`.

Signed-off-by: Edwin Kofler <edwin@kofler.dev>
Signed-off-by: Koichi Murase <myoga.murase@gmail.com>
---
 contrib/completion/git-prompt.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to contrib/completion/git-prompt.sh +1 −1
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 9c10690a22..49dd69bb84 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -298,7 +298,7 @@ __git_ps1_colorize_gitstring ()
 # variable, in that order.
 __git_eread ()
 {
-	test -r "$1" && IFS=$'\r\n' read "$2" <"$1"
+	test -r "$1" && IFS=$'\r\n' read -r "$2" <"$1"
 }
 
 # see if a cherry-pick or revert is in progress, if the user has committed a
-- 
2.39.0
Junio C Hamano· Apr 20, 2023, 22:47 UTC · re: Koichi Murase · lore

Re: [PATCH] completion: suppress unwanted unescaping of `read`

Koichi Murase <myoga.murase@gmail.com> writes:
Show 34 quoted lines
> From: Edwin Kofler <edwin@kofler.dev>
>
> The function `__git_eread`, which reads the first line from the file,
> calls the `read` builtin without passing the flag option `-r`.  When
> the `read` builtin is called without the flag `-r`, it processes the
> backslash escaping in the text that it reads.  For this reason, it is
> generally considered the best practice to always use the `read`
> builtin with flag `-r` unless one intensionally processes the
> backslash escaping.  For the present case in git-prompt.sh, in fact,
> all the occurrences of the calls of `__git_eread` intend to read the
> literal content of the first lines.
>
> To make it read the first line literally, pass the flag `-r` to the
> `read` builtin in the function `__git_eread`.
>
> Signed-off-by: Edwin Kofler <edwin@kofler.dev>
> Signed-off-by: Koichi Murase <myoga.murase@gmail.com>
> ---
>  contrib/completion/git-prompt.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 9c10690a22..49dd69bb84 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -298,7 +298,7 @@ __git_ps1_colorize_gitstring ()
>  # variable, in that order.
>  __git_eread ()
>  {
> -	test -r "$1" && IFS=$'\r\n' read "$2" <"$1"
> +	test -r "$1" && IFS=$'\r\n' read -r "$2" <"$1"
>  }
>  
>  # see if a cherry-pick or revert is in progress, if the user has committed a
Perfect.  Will queue.  Thanks.
Felipe Contreras· Apr 24, 2023, 13:52 UTC · re: Koichi Murase · lore

Re: [PATCH] completion: suppress unwanted unescaping of `read`

Koichi Murase wrote:
Show 11 quoted lines
> From: Edwin Kofler <edwin@kofler.dev>
> 
> The function `__git_eread`, which reads the first line from the file,
> calls the `read` builtin without passing the flag option `-r`.  When
> the `read` builtin is called without the flag `-r`, it processes the
> backslash escaping in the text that it reads.  For this reason, it is
> generally considered the best practice to always use the `read`
> builtin with flag `-r` unless one intensionally processes the
> backslash escaping.  For the present case in git-prompt.sh, in fact,
> all the occurrences of the calls of `__git_eread` intend to read the
> literal content of the first lines.
This is my undrstanding.
I agree using `-r` is a good practice.
Show 5 quoted lines
> To make it read the first line literally, pass the flag `-r` to the
> `read` builtin in the function `__git_eread`.
> 
> Signed-off-by: Edwin Kofler <edwin@kofler.dev>
> Signed-off-by: Koichi Murase <myoga.murase@gmail.com>
Acked-by: Felipe Contreras <felipe.contreras@gmail.com>
-- 
Felipe Contreras
Junio C Hamano· Apr 20, 2023, 16:31 UTC · re: Koichi Murase · lore

Re: [PATCH 1/2] completion: quote arguments of test and [

Koichi Murase <myoga.murase@gmail.com> writes:
Show 5 quoted lines
> The raw command substitutions $v in the arguments of the test command
> and the [ command are subject to word splitting and pathname
> expansions. Even when it is ensured that the variable is not empty and
> does not contain whitespaces and glob characters, it can fail when IFS
> is set to non-trivial values containing letters and digits.

The above sounded good before I looked at the patch, and it still is good in theory, but it start to look mostly academic especially with enclosing $# inside a pair of double-quotes, and the variable would have only digits. The same for $i and $j that appear in the loop control "for ((i=0, j=0; ...)); do". The story is pretty much the same for local variables we set outselves to signal our findings, like $pcmode that is only set to either 'yes' or 'no'.

Is there a practical use case for an IFS that is set to split a run of digits somewhere in between (e.g. "IFS=5")? Is there any realistic setting of IFS that breaks the command line completion without this patch, but the IFS is usable without breaking all other things people wrote as shell scripts and you use everyday?

If there is no such realistic setting of IFS, most, if not all, of the changes presented here, while they may not be incorrect per-se, look purely academic.

It's not like this patch was produced by enclosing each and every variable reference machanically inside a pair of double-quotes, right? If there were variable references that ought to be split at IFS whitespace, the patch would have left them alone. The readers also need to assume the opposite for those variable references that are touched by this patch while reviewing it.

Is there any change in this patch that do fix a real problem with some more realistic IFS setting? Setting IFS to a digit does not count. If there is, then mixing such a real fix in many academic changes is even worse, as the latter clutters the patch and makes it harder to assess the former. It is like hiding a gem in a lot of cobblestones.

In other words, this patch looks way too noisy to be reviewed to discover its real worth.

Thanks.
Show 329 quoted lines
> To prevent them from being unexpectedly processed by word splitting
> and pathname expansions, properly quote the unquoted command
> substituations in the arguments of the `test` and `[` builtins.
>
> Co-authored-by: Edwin Kofler <edwin@kofler.dev>
> Signed-off-by: Koichi Murase <myoga.murase@gmail.com>
> Signed-off-by: Edwin Kofler <edwin@kofler.dev>
> ---
>  contrib/completion/git-completion.bash | 62 +++++++++++++-------------
>  contrib/completion/git-prompt.sh       |  8 ++--
>  2 files changed, 35 insertions(+), 35 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index dc95c34cc8..6c110c223b 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -253,19 +253,19 @@ __git_reassemble_comp_words_by_ref()
>  		# word separator characters to the current word.
>  		first=t
>  		while
> -			[ $i -gt 0 ] &&
> +			[ "$i" -gt 0 ] &&
>  			[ -n "${COMP_WORDS[$i]}" ] &&
>  			# word consists of excluded word separators
>  			[ "${COMP_WORDS[$i]//[^$exclude]}" = "${COMP_WORDS[$i]}" ]
>  		do
>  			# Attach to the previous token,
>  			# unless the previous token is the command name.
> -			if [ $j -ge 2 ] && [ -n "$first" ]; then
> +			if [ "$j" -ge 2 ] && [ -n "$first" ]; then
>  				((j--))
>  			fi
>  			first=
>  			words_[$j]=${words_[j]}${COMP_WORDS[i]}
> -			if [ $i = $COMP_CWORD ]; then
> +			if [ "$i" = "$COMP_CWORD" ]; then
>  				cword_=$j
>  			fi
>  			if (($i < ${#COMP_WORDS[@]} - 1)); then
> @@ -276,7 +276,7 @@ __git_reassemble_comp_words_by_ref()
>  			fi
>  		done
>  		words_[$j]=${words_[j]}${COMP_WORDS[i]}
> -		if [ $i = $COMP_CWORD ]; then
> +		if [ "$i" = "$COMP_CWORD" ]; then
>  			cword_=$j
>  		fi
>  	done
> @@ -292,7 +292,7 @@ _get_comp_words_by_ref ()
>  	fi
>  	__git_reassemble_comp_words_by_ref "$exclude"
>  	cur_=${words_[cword_]}
> -	while [ $# -gt 0 ]; do
> +	while [ "$#" -gt 0 ]; do
>  		case "$1" in
>  		cur)
>  			cur=$cur_
> @@ -848,7 +848,7 @@ __git_complete_refs ()
>  {
>  	local remote= dwim= pfx= cur_="$cur" sfx=" " mode="refs"
>  
> -	while test $# != 0; do
> +	while test "$#" != 0; do
>  		case "$1" in
>  		--remote=*)	remote="${1##--remote=}" ;;
>  		--dwim)		dwim="yes" ;;
> @@ -1036,7 +1036,7 @@ __git_complete_remote_or_refspec ()
>  	if [ "$cmd" = "remote" ]; then
>  		((c++))
>  	fi
> -	while [ $c -lt $cword ]; do
> +	while [ "$c" -lt "$cword" ]; do
>  		i="${words[c]}"
>  		case "$i" in
>  		--mirror) [ "$cmd" = "push" ] && no_complete_refspec=1 ;;
> @@ -1060,7 +1060,7 @@ __git_complete_remote_or_refspec ()
>  		__gitcomp_nl "$(__git_remotes)"
>  		return
>  	fi
> -	if [ $no_complete_refspec = 1 ]; then
> +	if [ "$no_complete_refspec" = 1 ]; then
>  		return
>  	fi
>  	[ "$remote" = "." ] && remote=
> @@ -1080,21 +1080,21 @@ __git_complete_remote_or_refspec ()
>  	esac
>  	case "$cmd" in
>  	fetch)
> -		if [ $lhs = 1 ]; then
> +		if [ "$lhs" = 1 ]; then
>  			__git_complete_fetch_refspecs "$remote" "$pfx" "$cur_"
>  		else
>  			__git_complete_refs --pfx="$pfx" --cur="$cur_"
>  		fi
>  		;;
>  	pull|remote)
> -		if [ $lhs = 1 ]; then
> +		if [ "$lhs" = 1 ]; then
>  			__git_complete_refs --remote="$remote" --pfx="$pfx" --cur="$cur_"
>  		else
>  			__git_complete_refs --pfx="$pfx" --cur="$cur_"
>  		fi
>  		;;
>  	push)
> -		if [ $lhs = 1 ]; then
> +		if [ "$lhs" = 1 ]; then
>  			__git_complete_refs --pfx="$pfx" --cur="$cur_"
>  		else
>  			__git_complete_refs --remote="$remote" --pfx="$pfx" --cur="$cur_"
> @@ -1203,7 +1203,7 @@ __git_find_on_cmdline ()
>  {
>  	local word c="$__git_cmd_idx" show_idx
>  
> -	while test $# -gt 1; do
> +	while test "$#" -gt 1; do
>  		case "$1" in
>  		--show-idx)	show_idx=y ;;
>  		*)		return 1 ;;
> @@ -1212,7 +1212,7 @@ __git_find_on_cmdline ()
>  	done
>  	local wordlist="$1"
>  
> -	while [ $c -lt $cword ]; do
> +	while [ "$c" -lt "$cword" ]; do
>  		for word in $wordlist; do
>  			if [ "$word" = "${words[c]}" ]; then
>  				if [ -n "${show_idx-}" ]; then
> @@ -1237,7 +1237,7 @@ __git_find_last_on_cmdline ()
>  {
>  	local word c=$cword show_idx
>  
> -	while test $# -gt 1; do
> +	while test "$#" -gt 1; do
>  		case "$1" in
>  		--show-idx)	show_idx=y ;;
>  		*)		return 1 ;;
> @@ -1246,7 +1246,7 @@ __git_find_last_on_cmdline ()
>  	done
>  	local wordlist="$1"
>  
> -	while [ $c -gt "$__git_cmd_idx" ]; do
> +	while [ "$c" -gt "$__git_cmd_idx" ]; do
>  		((c--))
>  		for word in $wordlist; do
>  			if [ "$word" = "${words[c]}" ]; then
> @@ -1286,7 +1286,7 @@ __git_get_option_value ()
>  	config_key="$4"
>  
>  	((c = $cword - 1))
> -	while [ $c -ge 0 ]; do
> +	while [ "$c" -ge 0 ]; do
>  		word="${words[c]}"
>  		for val in $values; do
>  			if [ "$short_opt$val" = "$word" ] ||
> @@ -1308,7 +1308,7 @@ __git_get_option_value ()
>  __git_has_doubledash ()
>  {
>  	local c=1
> -	while [ $c -lt $cword ]; do
> +	while [ "$c" -lt "$cword" ]; do
>  		if [ "--" = "${words[c]}" ]; then
>  			return 0
>  		fi
> @@ -1474,7 +1474,7 @@ _git_branch ()
>  {
>  	local i c="$__git_cmd_idx" only_local_ref="n" has_r="n"
>  
> -	while [ $c -lt $cword ]; do
> +	while [ "$c" -lt "$cword" ]; do
>  		i="${words[c]}"
>  		case "$i" in
>  		-d|-D|--delete|-m|-M|--move|-c|-C|--copy)
> @@ -1493,7 +1493,7 @@ _git_branch ()
>  		__gitcomp_builtin branch
>  		;;
>  	*)
> -		if [ $only_local_ref = "y" -a $has_r = "n" ]; then
> +		if [ "$only_local_ref" = "y" -a "$has_r" = "n" ]; then
>  			__gitcomp_direct "$(__git_heads "" "$cur" " ")"
>  		else
>  			__git_complete_refs
> @@ -1897,7 +1897,7 @@ __git_match_ctag () {
>  __git_complete_symbol () {
>  	local tags=tags pfx="" cur_="${cur-}" sfx=" "
>  
> -	while test $# != 0; do
> +	while test "$#" != 0; do
>  		case "$1" in
>  		--tags=*)	tags="${1##--tags=}" ;;
>  		--pfx=*)	pfx="${1##--pfx=}" ;;
> @@ -2166,7 +2166,7 @@ _git_mv ()
>  		;;
>  	esac
>  
> -	if [ $(__git_count_arguments "mv") -gt 0 ]; then
> +	if [ "$(__git_count_arguments "mv")" -gt 0 ]; then
>  		# We need to show both cached and untracked files (including
>  		# empty directories) since this may not be the last argument.
>  		__git_complete_index_file "--cached --others --directory"
> @@ -2496,7 +2496,7 @@ _git_switch ()
>  __git_config_get_set_variables ()
>  {
>  	local prevword word config_file= c=$cword
> -	while [ $c -gt "$__git_cmd_idx" ]; do
> +	while [ "$c" -gt "$__git_cmd_idx" ]; do
>  		word="${words[c]}"
>  		case "$word" in
>  		--system|--global|--local|--file=*)
> @@ -2541,7 +2541,7 @@ __git_complete_config_variable_value ()
>  {
>  	local varname="$prev" cur_="$cur"
>  
> -	while test $# != 0; do
> +	while test "$#" != 0; do
>  		case "$1" in
>  		--varname=*)	varname="${1##--varname=}" ;;
>  		--cur=*)	cur_="${1##--cur=}" ;;
> @@ -2656,7 +2656,7 @@ __git_complete_config_variable_name ()
>  {
>  	local cur_="$cur" sfx
>  
> -	while test $# != 0; do
> +	while test "$#" != 0; do
>  		case "$1" in
>  		--cur=*)	cur_="${1##--cur=}" ;;
>  		--sfx=*)	sfx="${1##--sfx=}" ;;
> @@ -2756,7 +2756,7 @@ __git_complete_config_variable_name_and_value ()
>  {
>  	local cur_="$cur"
>  
> -	while test $# != 0; do
> +	while test "$#" != 0; do
>  		case "$1" in
>  		--cur=*)	cur_="${1##--cur=}" ;;
>  		*)		return 1 ;;
> @@ -3096,7 +3096,7 @@ _git_stash ()
>  		__gitcomp_builtin "stash_$subcommand"
>  		;;
>  	branch,*)
> -		if [ $cword -eq $((__git_cmd_idx+2)) ]; then
> +		if [ "$cword" -eq "$((__git_cmd_idx+2))" ]; then
>  			__git_complete_refs
>  		else
>  			__gitcomp_nl "$(__git stash list \
> @@ -3261,7 +3261,7 @@ _git_svn ()
>  _git_tag ()
>  {
>  	local i c="$__git_cmd_idx" f=0
> -	while [ $c -lt $cword ]; do
> +	while [ "$c" -lt "$cword" ]; do
>  		i="${words[c]}"
>  		case "$i" in
>  		-d|--delete|-v|--verify)
> @@ -3279,7 +3279,7 @@ _git_tag ()
>  	-m|-F)
>  		;;
>  	-*|tag)
> -		if [ $f = 1 ]; then
> +		if [ "$f" = 1 ]; then
>  			__gitcomp_direct "$(__git_tags "" "$cur" " ")"
>  		fi
>  		;;
> @@ -3342,7 +3342,7 @@ _git_worktree ()
>  			# be either the 'add' subcommand, the unstuck
>  			# argument of an option (e.g. branch for -b|-B), or
>  			# the path for the new worktree.
> -			if [ $cword -eq $((subcommand_idx+1)) ]; then
> +			if [ "$cword" -eq "$((subcommand_idx+1))" ]; then
>  				# Right after the 'add' subcommand: have to
>  				# complete the path, so fall back to Bash
>  				# filename completion.
> @@ -3366,7 +3366,7 @@ _git_worktree ()
>  		__git_complete_worktree_paths
>  		;;
>  	move,*)
> -		if [ $cword -eq $((subcommand_idx+1)) ]; then
> +		if [ "$cword" -eq "$((subcommand_idx+1))" ]; then
>  			# The first parameter must be an existing working
>  			# tree to be moved.
>  			__git_complete_worktree_paths
> @@ -3436,7 +3436,7 @@ __git_main ()
>  	local __git_C_args C_args_count=0
>  	local __git_cmd_idx
>  
> -	while [ $c -lt $cword ]; do
> +	while [ "$c" -lt "$cword" ]; do
>  		i="${words[c]}"
>  		case "$i" in
>  		--git-dir=*)
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 76ee4ab1e5..9c10690a22 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -232,7 +232,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}"
> @@ -269,7 +269,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"
> @@ -567,7 +567,7 @@ __git_ps1 ()
>  	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
> @@ -579,7 +579,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
Koichi Murase· Apr 20, 2023, 20:59 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] completion: quote arguments of test and [

Thank you for the review and comments.
2023年4月21日(金) 1:31 Junio C Hamano <gitster@pobox.com>:
Show 7 quoted lines
> The above sounded good before I looked at the patch, and it still is
> good in theory, but it start to look mostly academic especially with
> enclosing $# inside a pair of double-quotes, and the variable would
> have only digits.  The same for $i and $j that appear in the loop
> control "for ((i=0, j=0; ...)); do".  The story is pretty much the
> same for local variables we set outselves to signal our findings,
> like $pcmode that is only set to either 'yes' or 'no'.

I need to admit that this report is not associated with a problem that has actually happened in real-world uses. In that sense, maybe you could regard these changes as more and less academic. To be fair, I think I first need to explain the background. We first received the change as a part of the commits for a consistent code style in our project, which contains copies of git-completion.bash and git-prompt.sh. Since these files came from the upstream Git project and are not something we are maintaining in our project, we rejected the changes to these two files. However, I still think it's worth forwarding the contributions from Edwin to the upstream, so I expanded the changes to similar cases in the same files and posted them here.

> Is there a practical use case for an IFS that is set to split a run
> of digits somewhere in between (e.g. "IFS=5")? Is there any
> realistic setting of IFS that breaks the command line completion
> without this patch,

If you are talking about the ``default'' settings of IFS in an interactive session, I don't think there would be a practical use to set IFS to something other than <space><tab><newline>. However, the expected usage of IFS is to ``temporarily'' set a non-trivial value to IFS, run some commands referencing IFS (such as word splitting of unquoted variable expansions or the `read' builtin), and finally restore IFS to the original state. These three steps can be performed in three separate command lines in an interactive shell. The problem is that git prompt and completion settings can be invoked between these command lines, where IFS has the temporary value, regardless of whether the user intends so.

> but the IFS is usable without breaking all other
> things people wrote as shell scripts and you use everyday?

First, an independent shell script will never be affected by IFS in interactive sessions because IFS is reset to <space><tab><newline> at the startup of the shell as requested by POSIX [XCU 2.5.3 IFS ¶3], so the only relevant scripts here are the settings of interactive shells. I assume you are not including random personal settings written by beginners (where we can easily find broken settings), and then there are not too many projects of widely used Bash interactive settings. Among such projects, recent projects are carefully written so that they won't be affected by random IFS. An old project, bash-completion, still contains the parts affected by random IFS, but the discussions for possible solutions are ongoing (e.g. in the following refs):

https://github.com/scop/bash-completion/issues/723 https://github.com/scop/bash-completion/issues/720#issuecomment-1093764792 https://github.com/scop/bash-completion/pull/739

Show 10 quoted lines
> If there is no such realistic setting of IFS, most, if not all, of
> the changes presented here, while they may not be incorrect per-se,
> look purely academic.
>
> It's not like this patch was produced by enclosing each and every
> variable reference machanically inside a pair of double-quotes,
> right?  If there were variable references that ought to be split at
> IFS whitespace, the patch would have left them alone.  The readers
> also need to assume the opposite for those variable references that
> are touched by this patch while reviewing it.

I think I miss the intent of this paragraph. I made this patch manually, i.e., edited each line of `test' and `[' by hand, but the procedure is actually mechanical. There can be use cases to split words by IFS for the other commands or shell constructs, but specifically for the `test' and `[' builtins, the words should never be split because it totally breaks the semantics that these builtins process.

> Is there any change in this patch that do fix a real problem with
> some more realistic IFS setting?

No, this patch focuses on solely the problems of `test' and `[' caused by custom IFS, (though I'm not convinced that the problem caused by the custom IFS would be ``virtual'').

Show 10 quoted lines
> Setting IFS to a digit does not
> count.  If there is, then mixing such a real fix in many academic
> changes is even worse, as the latter clutters the patch and makes it
> harder to assess the former.  It is like hiding a gem in a lot of
> cobblestones.
>
> In other words, this patch looks way too noisy to be reviewed to
> discover its real worth.
>
> Thanks.

By the way, in the original patch, I tried to minimize the changes so just continued to use `test' and `[', but a solution that is generally considered preferable would be actually to switch to `[[ ... ]]'. If requested so, I can edit the patch.

Felipe Contreras· Apr 24, 2023, 13:43 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] completion: quote arguments of test and [

Junio C Hamano wrote:
Show 15 quoted lines
> Koichi Murase <myoga.murase@gmail.com> writes:
> 
> > The raw command substitutions $v in the arguments of the test command
> > and the [ command are subject to word splitting and pathname
> > expansions. Even when it is ensured that the variable is not empty and
> > does not contain whitespaces and glob characters, it can fail when IFS
> > is set to non-trivial values containing letters and digits.
> 
> The above sounded good before I looked at the patch, and it still is
> good in theory, but it start to look mostly academic especially with
> enclosing $# inside a pair of double-quotes, and the variable would
> have only digits.  The same for $i and $j that appear in the loop
> control "for ((i=0, j=0; ...)); do".  The story is pretty much the
> same for local variables we set outselves to signal our findings,
> like $pcmode that is only set to either 'yes' or 'no'.
I do have the same opinion.

Although the result seems more proper, I fail to see the actual value of doing all these changes everywhere.

On the other hand I do see the very real harm that they would break the git-completion branch everywhere. Rebasing those 50-so patches would not be very pleasant.

> In other words, this patch looks way too noisy to be reviewed to
> discover its real worth.
Agreed.
-- 
Felipe Contreras

← back to recent threads