{"thread":{"id":"29230","subject":"[PATCH] bash completion: use read -r everywhere","startedAt":"2011-12-21T15:54:14Z","lastAt":"2011-12-21T19:23:07Z","messageCount":4,"participants":["Thomas Rast","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"181580","messageId":"4502a0248bb843018335e9b5cdf70736c096ebe3.1324482693.git.trast@student.ethz.ch","threadId":"29230","inReplyTo":null,"subject":"[PATCH] bash completion: use read -r everywhere","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-21T15:54:14Z","receivedAt":"2011-12-21T15:54:14Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"POSIX specifies\n\n  The read utility shall read a single line from standard input.\n  By default, unless the -r option is specified, backslash ('\\')\n  shall act as an escape character...\n\nOur omission of -r breaks the loop reading refnames from\ngit-for-each-ref in __git_refs() if there are refnames such as\n\"foo'bar\", in which case for-each-ref helpfully quotes them as in\n\n  $ git update-ref \"refs/remotes/test/foo'bar\" HEAD\n  $ git for-each-ref --shell --format=\"ref=%(refname:short)\" \"refs/remotes\"\n  ref='test/foo'\\''bar'\n\nInterpolating the \\' here will read \"ref='test/foo'''bar'\" instead,\nand eval then chokes on the unbalanced quotes.\n\nHowever, since none of the read loops _want_ to have backslashes\ninterpolated, it's much safer to use read -r everywhere.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n contrib/completion/git-completion.bash |   12 ++++++------\n 1 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 78257ae..e7a39ef 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -111,7 +111,7 @@ __git_ps1_show_upstream ()\n \n \t# get some config options from git-config\n \tlocal output=\"$(git config -z --get-regexp '^(svn-remote\\..*\\.url|bash\\.showupstream)$' 2>/dev/null | tr '\\0\\n' '\\n ')\"\n-\twhile read key value; do\n+\twhile read -r key value; do\n \t\tcase \"$key\" in\n \t\tbash.showupstream)\n \t\t\tGIT_PS1_SHOWUPSTREAM=\"$value\"\n@@ -589,7 +589,7 @@ __git_refs ()\n \t\t\tlocal ref entry\n \t\t\tgit --git-dir=\"$dir\" for-each-ref --shell --format=\"ref=%(refname:short)\" \\\n \t\t\t\t\"refs/remotes/\" | \\\n-\t\t\twhile read entry; do\n+\t\t\twhile read -r entry; do\n \t\t\t\teval \"$entry\"\n \t\t\t\tref=\"${ref#*/}\"\n \t\t\t\tif [[ \"$ref\" == \"$cur\"* ]]; then\n@@ -602,7 +602,7 @@ __git_refs ()\n \tcase \"$cur\" in\n \trefs|refs/*)\n \t\tgit ls-remote \"$dir\" \"$cur*\" 2>/dev/null | \\\n-\t\twhile read hash i; do\n+\t\twhile read -r hash i; do\n \t\t\tcase \"$i\" in\n \t\t\t*^{}) ;;\n \t\t\t*) echo \"$i\" ;;\n@@ -611,7 +611,7 @@ __git_refs ()\n \t\t;;\n \t*)\n \t\tgit ls-remote \"$dir\" HEAD ORIG_HEAD 'refs/tags/*' 'refs/heads/*' 'refs/remotes/*' 2>/dev/null | \\\n-\t\twhile read hash i; do\n+\t\twhile read -r hash i; do\n \t\t\tcase \"$i\" in\n \t\t\t*^{}) ;;\n \t\t\trefs/*) echo \"${i#refs/*/}\" ;;\n@@ -636,7 +636,7 @@ __git_refs_remotes ()\n {\n \tlocal i hash\n \tgit ls-remote \"$1\" 'refs/heads/*' 2>/dev/null | \\\n-\twhile read hash i; do\n+\twhile read -r hash i; do\n \t\techo \"$i:refs/remotes/$1/${i#refs/heads/}\"\n \tdone\n }\n@@ -1863,7 +1863,7 @@ __git_config_get_set_variables ()\n \tdone\n \n \tgit --git-dir=\"$(__gitdir)\" config $config_file --list 2>/dev/null |\n-\twhile read line\n+\twhile read -r line\n \tdo\n \t\tcase \"$line\" in\n \t\t*.*=*)\n-- \n1.7.8.484.gdad4270\n"},{"id":"181589","messageId":"7vipl9hht4.fsf@alter.siamese.dyndns.org","threadId":"29230","inReplyTo":"4502a0248bb843018335e9b5cdf70736c096ebe3.1324482693.git.trast@student.ethz.ch","subject":"Re: [PATCH] bash completion: use read -r everywhere","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-21T18:59:35Z","receivedAt":"2011-12-21T18:59:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> POSIX specifies\n>\n>   The read utility shall read a single line from standard input.\n>   By default, unless the -r option is specified, backslash ('\\')\n>   shall act as an escape character...\n>\n> Our omission of -r breaks the loop reading refnames from\n> git-for-each-ref in __git_refs() if there are refnames such as\n> \"foo'bar\", in which case for-each-ref helpfully quotes them as in\n>\n>   $ git update-ref \"refs/remotes/test/foo'bar\" HEAD\n>   $ git for-each-ref --shell --format=\"ref=%(refname:short)\" \"refs/remotes\"\n>   ref='test/foo'\\''bar'\n>\n> Interpolating the \\' here will read \"ref='test/foo'''bar'\" instead,\n> and eval then chokes on the unbalanced quotes.\n>\n> However, since none of the read loops _want_ to have backslashes\n> interpolated, it's much safer to use read -r everywhere.\n>\n> Signed-off-by: Thomas Rast <trast@student.ethz.ch>\n\nThanks.\n\nAs this script is specific to bash, it is secondary importance what POSIX\nsays. The \"-r\" option is important only because \"bash\" happens to follow\nPOSIX in this case. I'd like to see the early part of the message reworded\nperhaps like this:\n\n\tAt various points in the script, we use \"read\" utility without\n\tgiving it the \"-r\" option that prevents a backslash ('\\')\n\tcharacter to act as an escape character. This breaks e.g. reading\n\trefnames from ...\n\nDoes this regress for zsh users in some ways, by the way?\n\n> ---\n>  contrib/completion/git-completion.bash |   12 ++++++------\n>  1 files changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 78257ae..e7a39ef 100755\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -111,7 +111,7 @@ __git_ps1_show_upstream ()\n>  \n>  \t# get some config options from git-config\n>  \tlocal output=\"$(git config -z --get-regexp '^(svn-remote\\..*\\.url|bash\\.showupstream)$' 2>/dev/null | tr '\\0\\n' '\\n ')\"\n> -\twhile read key value; do\n> +\twhile read -r key value; do\n>  \t\tcase \"$key\" in\n>  \t\tbash.showupstream)\n>  \t\t\tGIT_PS1_SHOWUPSTREAM=\"$value\"\n> @@ -589,7 +589,7 @@ __git_refs ()\n>  \t\t\tlocal ref entry\n>  \t\t\tgit --git-dir=\"$dir\" for-each-ref --shell --format=\"ref=%(refname:short)\" \\\n>  \t\t\t\t\"refs/remotes/\" | \\\n> -\t\t\twhile read entry; do\n> +\t\t\twhile read -r entry; do\n>  \t\t\t\teval \"$entry\"\n>  \t\t\t\tref=\"${ref#*/}\"\n>  \t\t\t\tif [[ \"$ref\" == \"$cur\"* ]]; then\n> @@ -602,7 +602,7 @@ __git_refs ()\n>  \tcase \"$cur\" in\n>  \trefs|refs/*)\n>  \t\tgit ls-remote \"$dir\" \"$cur*\" 2>/dev/null | \\\n> -\t\twhile read hash i; do\n> +\t\twhile read -r hash i; do\n>  \t\t\tcase \"$i\" in\n>  \t\t\t*^{}) ;;\n>  \t\t\t*) echo \"$i\" ;;\n> @@ -611,7 +611,7 @@ __git_refs ()\n>  \t\t;;\n>  \t*)\n>  \t\tgit ls-remote \"$dir\" HEAD ORIG_HEAD 'refs/tags/*' 'refs/heads/*' 'refs/remotes/*' 2>/dev/null | \\\n> -\t\twhile read hash i; do\n> +\t\twhile read -r hash i; do\n>  \t\t\tcase \"$i\" in\n>  \t\t\t*^{}) ;;\n>  \t\t\trefs/*) echo \"${i#refs/*/}\" ;;\n> @@ -636,7 +636,7 @@ __git_refs_remotes ()\n>  {\n>  \tlocal i hash\n>  \tgit ls-remote \"$1\" 'refs/heads/*' 2>/dev/null | \\\n> -\twhile read hash i; do\n> +\twhile read -r hash i; do\n>  \t\techo \"$i:refs/remotes/$1/${i#refs/heads/}\"\n>  \tdone\n>  }\n> @@ -1863,7 +1863,7 @@ __git_config_get_set_variables ()\n>  \tdone\n>  \n>  \tgit --git-dir=\"$(__gitdir)\" config $config_file --list 2>/dev/null |\n> -\twhile read line\n> +\twhile read -r line\n>  \tdo\n>  \t\tcase \"$line\" in\n>  \t\t*.*=*)\n"},{"id":"181591","messageId":"87wr9pkahw.fsf@thomas.inf.ethz.ch","threadId":"29230","inReplyTo":"7vipl9hht4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] bash completion: use read -r everywhere","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-21T19:09:15Z","receivedAt":"2011-12-21T19:09:15Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Thomas Rast <trast@student.ethz.ch> writes:\n>\n>> POSIX specifies\n>>\n>>   The read utility shall read a single line from standard input.\n>>   By default, unless the -r option is specified, backslash ('\\')\n>>   shall act as an escape character...\n>>\n>> Our omission of -r breaks the loop reading refnames from\n>> git-for-each-ref in __git_refs() if there are refnames such as\n>> \"foo'bar\", in which case for-each-ref helpfully quotes them as in\n[...]\n> Thanks.\n>\n> As this script is specific to bash, it is secondary importance what POSIX\n> says. The \"-r\" option is important only because \"bash\" happens to follow\n> POSIX in this case. I'd like to see the early part of the message reworded\n> perhaps like this:\n>\n> \tAt various points in the script, we use \"read\" utility without\n> \tgiving it the \"-r\" option that prevents a backslash ('\\')\n> \tcharacter to act as an escape character. This breaks e.g. reading\n> \trefnames from ...\n\nPerhaps we can then just fold it into the first paragraph after the\nPOSIX quote, like\n\n  We use the 'read' command without -r, so that it treats '\\' as an\n  escape character, in several places.  This breaks the loop reading\n  refnames from git-for-each-ref in __git_refs() if there are refnames\n  such as \"foo'bar\", in which case for-each-ref helpfully quotes them as\n  in\n\nOr some such.  Do you want me to resend?\n\n> Does this regress for zsh users in some ways, by the way?\n\nI'm not one of them, but a quick googling for \"zsh builtin read\" turns\nup that it has a dozen options, and -r means\n\n  -r\n      Raw mode: a \\ at the end of a line does not signify line continuation. \n\nI can't discern whether it treats \\ special at all with or without -r.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"181595","messageId":"7v1urxhgpw.fsf@alter.siamese.dyndns.org","threadId":"29230","inReplyTo":"87wr9pkahw.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH] bash completion: use read -r everywhere","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-21T19:23:07Z","receivedAt":"2011-12-21T19:23:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> Perhaps we can then just fold it into the first paragraph after the\n> POSIX quote, like\n\nWell, that is what I tried say so we are on the same page ;-).\n\n>   We use the 'read' command without -r, so that it treats '\\' as an\n>   escape character, in several places.  This breaks the loop reading\n>   refnames from git-for-each-ref in __git_refs() if there are refnames\n>   such as \"foo'bar\", in which case for-each-ref helpfully quotes them as\n>   in\n>\n> Or some such.  Do you want me to resend?\n\nNah, The above as-is is perfectly fine.\n\nBy the way, this is not a problem with the patch, but the for-each-ref\nloop is a poor example. Its --shell option is meant to produce a scriptlet\nthat can be evaled without the buggy processing loop you are fixing, i.e.\n\n\tscript=$(git for-each-ref --shell --format='\n\t\tref=%(refname:short)\n                ref=${ref#*/}\n                if [[ \"$ref\" == \"$cur\"* ]]\n                then\n                \t...\n\t' refs/remotes/) &&\n        eval \"$script\"\n\nis how it was designed to be used avoiding shell loops.\n\n>> Does this regress for zsh users in some ways, by the way?\n>\n> I'm not one of them, but...\n\nThanks, that was all I wanted to know before deciding if I should apply\nthis directly to 'master' or cook in 'next' to give real zsh users a\nchance to object or tweak it.\n"}]}