threads / patch / 29230

patchbash completion: use read -r everywhere

Subject: [PATCH] bash completion: use read -r everywhere

## tl;dr

4 messages between Dec 21, 2011 and Dec 21, 2011. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Thomas Rast· Dec 21, 2011, 15:54 UTC · lore
POSIX specifies
  The read utility shall read a single line from standard input.
  By default, unless the -r option is specified, backslash ('\')
  shall act as an escape character...

Our omission of -r breaks the loop reading refnames from git-for-each-ref in __git_refs() if there are refnames such as "foo'bar", in which case for-each-ref helpfully quotes them as in

  $ git update-ref "refs/remotes/test/foo'bar" HEAD
  $ git for-each-ref --shell --format="ref=%(refname:short)" "refs/remotes"
  ref='test/foo'\''bar'

Interpolating the \' here will read "ref='test/foo'''bar'" instead, and eval then chokes on the unbalanced quotes.

However, since none of the read loops _want_ to have backslashes interpolated, it's much safer to use read -r everywhere.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 contrib/completion/git-completion.bash |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
Show changes to contrib/completion/git-completion.bash +6 −6
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 78257ae..e7a39ef 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -111,7 +111,7 @@ __git_ps1_show_upstream ()
 
 	# get some config options from git-config
 	local output="$(git config -z --get-regexp '^(svn-remote\..*\.url|bash\.showupstream)$' 2>/dev/null | tr '\0\n' '\n ')"
-	while read key value; do
+	while read -r key value; do
 		case "$key" in
 		bash.showupstream)
 			GIT_PS1_SHOWUPSTREAM="$value"
@@ -589,7 +589,7 @@ __git_refs ()
 			local ref entry
 			git --git-dir="$dir" for-each-ref --shell --format="ref=%(refname:short)" \
 				"refs/remotes/" | \
-			while read entry; do
+			while read -r entry; do
 				eval "$entry"
 				ref="${ref#*/}"
 				if [[ "$ref" == "$cur"* ]]; then
@@ -602,7 +602,7 @@ __git_refs ()
 	case "$cur" in
 	refs|refs/*)
 		git ls-remote "$dir" "$cur*" 2>/dev/null | \
-		while read hash i; do
+		while read -r hash i; do
 			case "$i" in
 			*^{}) ;;
 			*) echo "$i" ;;
@@ -611,7 +611,7 @@ __git_refs ()
 		;;
 	*)
 		git ls-remote "$dir" HEAD ORIG_HEAD 'refs/tags/*' 'refs/heads/*' 'refs/remotes/*' 2>/dev/null | \
-		while read hash i; do
+		while read -r hash i; do
 			case "$i" in
 			*^{}) ;;
 			refs/*) echo "${i#refs/*/}" ;;
@@ -636,7 +636,7 @@ __git_refs_remotes ()
 {
 	local i hash
 	git ls-remote "$1" 'refs/heads/*' 2>/dev/null | \
-	while read hash i; do
+	while read -r hash i; do
 		echo "$i:refs/remotes/$1/${i#refs/heads/}"
 	done
 }
@@ -1863,7 +1863,7 @@ __git_config_get_set_variables ()
 	done
 
 	git --git-dir="$(__gitdir)" config $config_file --list 2>/dev/null |
-	while read line
+	while read -r line
 	do
 		case "$line" in
 		*.*=*)

-- 
1.7.8.484.gdad4270
Junio C Hamano· Dec 21, 2011, 18:59 UTC · re: Thomas Rast · lore

Re: [PATCH] bash completion: use read -r everywhere

Thomas Rast <trast@student.ethz.ch> writes:
Show 21 quoted lines
> POSIX specifies
>
>   The read utility shall read a single line from standard input.
>   By default, unless the -r option is specified, backslash ('\')
>   shall act as an escape character...
>
> Our omission of -r breaks the loop reading refnames from
> git-for-each-ref in __git_refs() if there are refnames such as
> "foo'bar", in which case for-each-ref helpfully quotes them as in
>
>   $ git update-ref "refs/remotes/test/foo'bar" HEAD
>   $ git for-each-ref --shell --format="ref=%(refname:short)" "refs/remotes"
>   ref='test/foo'\''bar'
>
> Interpolating the \' here will read "ref='test/foo'''bar'" instead,
> and eval then chokes on the unbalanced quotes.
>
> However, since none of the read loops _want_ to have backslashes
> interpolated, it's much safer to use read -r everywhere.
>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
Thanks.

As this script is specific to bash, it is secondary importance what POSIX says. The "-r" option is important only because "bash" happens to follow POSIX in this case. I'd like to see the early part of the message reworded perhaps like this:

	At various points in the script, we use "read" utility without
	giving it the "-r" option that prevents a backslash ('\')
	character to act as an escape character. This breaks e.g. reading
	refnames from ...
Does this regress for zsh users in some ways, by the way?
Show 62 quoted lines
> ---
>  contrib/completion/git-completion.bash |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 78257ae..e7a39ef 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -111,7 +111,7 @@ __git_ps1_show_upstream ()
>  
>  	# get some config options from git-config
>  	local output="$(git config -z --get-regexp '^(svn-remote\..*\.url|bash\.showupstream)$' 2>/dev/null | tr '\0\n' '\n ')"
> -	while read key value; do
> +	while read -r key value; do
>  		case "$key" in
>  		bash.showupstream)
>  			GIT_PS1_SHOWUPSTREAM="$value"
> @@ -589,7 +589,7 @@ __git_refs ()
>  			local ref entry
>  			git --git-dir="$dir" for-each-ref --shell --format="ref=%(refname:short)" \
>  				"refs/remotes/" | \
> -			while read entry; do
> +			while read -r entry; do
>  				eval "$entry"
>  				ref="${ref#*/}"
>  				if [[ "$ref" == "$cur"* ]]; then
> @@ -602,7 +602,7 @@ __git_refs ()
>  	case "$cur" in
>  	refs|refs/*)
>  		git ls-remote "$dir" "$cur*" 2>/dev/null | \
> -		while read hash i; do
> +		while read -r hash i; do
>  			case "$i" in
>  			*^{}) ;;
>  			*) echo "$i" ;;
> @@ -611,7 +611,7 @@ __git_refs ()
>  		;;
>  	*)
>  		git ls-remote "$dir" HEAD ORIG_HEAD 'refs/tags/*' 'refs/heads/*' 'refs/remotes/*' 2>/dev/null | \
> -		while read hash i; do
> +		while read -r hash i; do
>  			case "$i" in
>  			*^{}) ;;
>  			refs/*) echo "${i#refs/*/}" ;;
> @@ -636,7 +636,7 @@ __git_refs_remotes ()
>  {
>  	local i hash
>  	git ls-remote "$1" 'refs/heads/*' 2>/dev/null | \
> -	while read hash i; do
> +	while read -r hash i; do
>  		echo "$i:refs/remotes/$1/${i#refs/heads/}"
>  	done
>  }
> @@ -1863,7 +1863,7 @@ __git_config_get_set_variables ()
>  	done
>  
>  	git --git-dir="$(__gitdir)" config $config_file --list 2>/dev/null |
> -	while read line
> +	while read -r line
>  	do
>  		case "$line" in
>  		*.*=*)
Thomas Rast· Dec 21, 2011, 19:09 UTC · re: Junio C Hamano · lore

Re: [PATCH] bash completion: use read -r everywhere

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> Thomas Rast <trast@student.ethz.ch> writes:
>
>> POSIX specifies
>>
>>   The read utility shall read a single line from standard input.
>>   By default, unless the -r option is specified, backslash ('\')
>>   shall act as an escape character...
>>
>> Our omission of -r breaks the loop reading refnames from
>> git-for-each-ref in __git_refs() if there are refnames such as
>> "foo'bar", in which case for-each-ref helpfully quotes them as in
[...]
Show 11 quoted lines
> Thanks.
>
> As this script is specific to bash, it is secondary importance what POSIX
> says. The "-r" option is important only because "bash" happens to follow
> POSIX in this case. I'd like to see the early part of the message reworded
> perhaps like this:
>
> 	At various points in the script, we use "read" utility without
> 	giving it the "-r" option that prevents a backslash ('\')
> 	character to act as an escape character. This breaks e.g. reading
> 	refnames from ...

Perhaps we can then just fold it into the first paragraph after the POSIX quote, like

  We use the 'read' command without -r, so that it treats '\' as an
  escape character, in several places.  This breaks the loop reading
  refnames from git-for-each-ref in __git_refs() if there are refnames
  such as "foo'bar", in which case for-each-ref helpfully quotes them as
  in
Or some such.  Do you want me to resend?
> Does this regress for zsh users in some ways, by the way?

I'm not one of them, but a quick googling for "zsh builtin read" turns up that it has a dozen options, and -r means

  -r
      Raw mode: a \ at the end of a line does not signify line continuation. 
I can't discern whether it treats \ special at all with or without -r.
-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Junio C Hamano· Dec 21, 2011, 19:23 UTC · re: Thomas Rast · lore

Re: [PATCH] bash completion: use read -r everywhere

Thomas Rast <trast@student.ethz.ch> writes:
> Perhaps we can then just fold it into the first paragraph after the
> POSIX quote, like
Well, that is what I tried say so we are on the same page ;-).
Show 7 quoted lines
>   We use the 'read' command without -r, so that it treats '\' as an
>   escape character, in several places.  This breaks the loop reading
>   refnames from git-for-each-ref in __git_refs() if there are refnames
>   such as "foo'bar", in which case for-each-ref helpfully quotes them as
>   in
>
> Or some such.  Do you want me to resend?
Nah, The above as-is is perfectly fine.

By the way, this is not a problem with the patch, but the for-each-ref loop is a poor example. Its --shell option is meant to produce a scriptlet that can be evaled without the buggy processing loop you are fixing, i.e.

	script=$(git for-each-ref --shell --format='
		ref=%(refname:short)
                ref=${ref#*/}
                if [[ "$ref" == "$cur"* ]]
                then
                	...
	' refs/remotes/) &&
        eval "$script"
is how it was designed to be used avoiding shell loops.
>> Does this regress for zsh users in some ways, by the way?
>
> I'm not one of them, but...

Thanks, that was all I wanted to know before deciding if I should apply this directly to 'master' or cook in 'next' to give real zsh users a chance to object or tweak it.

← back to recent threads