* [PATCH] bash completion: use read -r everywhere
@ 2011-12-21 15:54 Thomas Rast
2011-12-21 18:59 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Rast @ 2011-12-21 15:54 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kevin Ballard
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] bash completion: use read -r everywhere
2011-12-21 15:54 [PATCH] bash completion: use read -r everywhere Thomas Rast
@ 2011-12-21 18:59 ` Junio C Hamano
2011-12-21 19:09 ` Thomas Rast
0 siblings, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2011-12-21 18:59 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Kevin Ballard
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
> *.*=*)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] bash completion: use read -r everywhere
2011-12-21 18:59 ` Junio C Hamano
@ 2011-12-21 19:09 ` Thomas Rast
2011-12-21 19:23 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Rast @ 2011-12-21 19:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kevin Ballard
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] bash completion: use read -r everywhere
2011-12-21 19:09 ` Thomas Rast
@ 2011-12-21 19:23 ` Junio C Hamano
0 siblings, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2011-12-21 19:23 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Kevin Ballard
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-12-21 19:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-12-21 15:54 [PATCH] bash completion: use read -r everywhere Thomas Rast
2011-12-21 18:59 ` Junio C Hamano
2011-12-21 19:09 ` Thomas Rast
2011-12-21 19:23 ` Junio C Hamano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.