[PATCH] bash completion: use read -r everywhere

[PATCH] bash completion: use read -r everywhere

[Thomas Rast]

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(-)

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

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

[Junio C Hamano]

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
>
>   $ 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?

> ---
>  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
>  		*.*=*)

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

[Thomas Rast]

Junio C Hamano <gitster@pobox.com> writes:

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

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

[Junio C Hamano]

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

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