{"thread":{"id":"59622","subject":"[PATCH 1/2] completion: quote arguments of test and [","startedAt":"2023-04-20T07:46:50Z","lastAt":"2023-04-24T13:52:37Z","messageCount":10,"participants":["Koichi Murase","Junio C Hamano","Felipe Contreras"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"475730","messageId":"20230420074616.1642742-1-myoga.murase@gmail.com","threadId":"59622","inReplyTo":null,"subject":"[PATCH 1/2] completion: quote arguments of test and [","fromName":"Koichi Murase","fromEmail":"myoga.murase@gmail.com","sentAt":"2023-04-20T07:46:15Z","receivedAt":"2023-04-20T07:46:50Z","isPatch":true,"sender":{"key":"myoga.murase@gmail.com","avatar":"https://gravatar.com/avatar/02c724c6bd06bcd2cbe60143154638ac624d39ff80e76a595a9f51bc1b86f303?d=mp&s=160"},"body":"The raw command substitutions $v in the arguments of the test command\nand the [ command are subject to word splitting and pathname\nexpansions. Even when it is ensured that the variable is not empty and\ndoes not contain whitespaces and glob characters, it can fail when IFS\nis set to non-trivial values containing letters and digits.\n\nTo prevent them from being unexpectedly processed by word splitting\nand pathname expansions, properly quote the unquoted command\nsubstituations in the arguments of the `test` and `[` builtins.\n\nCo-authored-by: Edwin Kofler <edwin@kofler.dev>\nSigned-off-by: Koichi Murase <myoga.murase@gmail.com>\nSigned-off-by: Edwin Kofler <edwin@kofler.dev>\n---\n contrib/completion/git-completion.bash | 62 +++++++++++++-------------\n contrib/completion/git-prompt.sh       |  8 ++--\n 2 files changed, 35 insertions(+), 35 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex dc95c34cc8..6c110c223b 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -253,19 +253,19 @@ __git_reassemble_comp_words_by_ref()\n \t\t# word separator characters to the current word.\n \t\tfirst=t\n \t\twhile\n-\t\t\t[ $i -gt 0 ] &&\n+\t\t\t[ \"$i\" -gt 0 ] &&\n \t\t\t[ -n \"${COMP_WORDS[$i]}\" ] &&\n \t\t\t# word consists of excluded word separators\n \t\t\t[ \"${COMP_WORDS[$i]//[^$exclude]}\" = \"${COMP_WORDS[$i]}\" ]\n \t\tdo\n \t\t\t# Attach to the previous token,\n \t\t\t# unless the previous token is the command name.\n-\t\t\tif [ $j -ge 2 ] && [ -n \"$first\" ]; then\n+\t\t\tif [ \"$j\" -ge 2 ] && [ -n \"$first\" ]; then\n \t\t\t\t((j--))\n \t\t\tfi\n \t\t\tfirst=\n \t\t\twords_[$j]=${words_[j]}${COMP_WORDS[i]}\n-\t\t\tif [ $i = $COMP_CWORD ]; then\n+\t\t\tif [ \"$i\" = \"$COMP_CWORD\" ]; then\n \t\t\t\tcword_=$j\n \t\t\tfi\n \t\t\tif (($i < ${#COMP_WORDS[@]} - 1)); then\n@@ -276,7 +276,7 @@ __git_reassemble_comp_words_by_ref()\n \t\t\tfi\n \t\tdone\n \t\twords_[$j]=${words_[j]}${COMP_WORDS[i]}\n-\t\tif [ $i = $COMP_CWORD ]; then\n+\t\tif [ \"$i\" = \"$COMP_CWORD\" ]; then\n \t\t\tcword_=$j\n \t\tfi\n \tdone\n@@ -292,7 +292,7 @@ _get_comp_words_by_ref ()\n \tfi\n \t__git_reassemble_comp_words_by_ref \"$exclude\"\n \tcur_=${words_[cword_]}\n-\twhile [ $# -gt 0 ]; do\n+\twhile [ \"$#\" -gt 0 ]; do\n \t\tcase \"$1\" in\n \t\tcur)\n \t\t\tcur=$cur_\n@@ -848,7 +848,7 @@ __git_complete_refs ()\n {\n \tlocal remote= dwim= pfx= cur_=\"$cur\" sfx=\" \" mode=\"refs\"\n \n-\twhile test $# != 0; do\n+\twhile test \"$#\" != 0; do\n \t\tcase \"$1\" in\n \t\t--remote=*)\tremote=\"${1##--remote=}\" ;;\n \t\t--dwim)\t\tdwim=\"yes\" ;;\n@@ -1036,7 +1036,7 @@ __git_complete_remote_or_refspec ()\n \tif [ \"$cmd\" = \"remote\" ]; then\n \t\t((c++))\n \tfi\n-\twhile [ $c -lt $cword ]; do\n+\twhile [ \"$c\" -lt \"$cword\" ]; do\n \t\ti=\"${words[c]}\"\n \t\tcase \"$i\" in\n \t\t--mirror) [ \"$cmd\" = \"push\" ] && no_complete_refspec=1 ;;\n@@ -1060,7 +1060,7 @@ __git_complete_remote_or_refspec ()\n \t\t__gitcomp_nl \"$(__git_remotes)\"\n \t\treturn\n \tfi\n-\tif [ $no_complete_refspec = 1 ]; then\n+\tif [ \"$no_complete_refspec\" = 1 ]; then\n \t\treturn\n \tfi\n \t[ \"$remote\" = \".\" ] && remote=\n@@ -1080,21 +1080,21 @@ __git_complete_remote_or_refspec ()\n \tesac\n \tcase \"$cmd\" in\n \tfetch)\n-\t\tif [ $lhs = 1 ]; then\n+\t\tif [ \"$lhs\" = 1 ]; then\n \t\t\t__git_complete_fetch_refspecs \"$remote\" \"$pfx\" \"$cur_\"\n \t\telse\n \t\t\t__git_complete_refs --pfx=\"$pfx\" --cur=\"$cur_\"\n \t\tfi\n \t\t;;\n \tpull|remote)\n-\t\tif [ $lhs = 1 ]; then\n+\t\tif [ \"$lhs\" = 1 ]; then\n \t\t\t__git_complete_refs --remote=\"$remote\" --pfx=\"$pfx\" --cur=\"$cur_\"\n \t\telse\n \t\t\t__git_complete_refs --pfx=\"$pfx\" --cur=\"$cur_\"\n \t\tfi\n \t\t;;\n \tpush)\n-\t\tif [ $lhs = 1 ]; then\n+\t\tif [ \"$lhs\" = 1 ]; then\n \t\t\t__git_complete_refs --pfx=\"$pfx\" --cur=\"$cur_\"\n \t\telse\n \t\t\t__git_complete_refs --remote=\"$remote\" --pfx=\"$pfx\" --cur=\"$cur_\"\n@@ -1203,7 +1203,7 @@ __git_find_on_cmdline ()\n {\n \tlocal word c=\"$__git_cmd_idx\" show_idx\n \n-\twhile test $# -gt 1; do\n+\twhile test \"$#\" -gt 1; do\n \t\tcase \"$1\" in\n \t\t--show-idx)\tshow_idx=y ;;\n \t\t*)\t\treturn 1 ;;\n@@ -1212,7 +1212,7 @@ __git_find_on_cmdline ()\n \tdone\n \tlocal wordlist=\"$1\"\n \n-\twhile [ $c -lt $cword ]; do\n+\twhile [ \"$c\" -lt \"$cword\" ]; do\n \t\tfor word in $wordlist; do\n \t\t\tif [ \"$word\" = \"${words[c]}\" ]; then\n \t\t\t\tif [ -n \"${show_idx-}\" ]; then\n@@ -1237,7 +1237,7 @@ __git_find_last_on_cmdline ()\n {\n \tlocal word c=$cword show_idx\n \n-\twhile test $# -gt 1; do\n+\twhile test \"$#\" -gt 1; do\n \t\tcase \"$1\" in\n \t\t--show-idx)\tshow_idx=y ;;\n \t\t*)\t\treturn 1 ;;\n@@ -1246,7 +1246,7 @@ __git_find_last_on_cmdline ()\n \tdone\n \tlocal wordlist=\"$1\"\n \n-\twhile [ $c -gt \"$__git_cmd_idx\" ]; do\n+\twhile [ \"$c\" -gt \"$__git_cmd_idx\" ]; do\n \t\t((c--))\n \t\tfor word in $wordlist; do\n \t\t\tif [ \"$word\" = \"${words[c]}\" ]; then\n@@ -1286,7 +1286,7 @@ __git_get_option_value ()\n \tconfig_key=\"$4\"\n \n \t((c = $cword - 1))\n-\twhile [ $c -ge 0 ]; do\n+\twhile [ \"$c\" -ge 0 ]; do\n \t\tword=\"${words[c]}\"\n \t\tfor val in $values; do\n \t\t\tif [ \"$short_opt$val\" = \"$word\" ] ||\n@@ -1308,7 +1308,7 @@ __git_get_option_value ()\n __git_has_doubledash ()\n {\n \tlocal c=1\n-\twhile [ $c -lt $cword ]; do\n+\twhile [ \"$c\" -lt \"$cword\" ]; do\n \t\tif [ \"--\" = \"${words[c]}\" ]; then\n \t\t\treturn 0\n \t\tfi\n@@ -1474,7 +1474,7 @@ _git_branch ()\n {\n \tlocal i c=\"$__git_cmd_idx\" only_local_ref=\"n\" has_r=\"n\"\n \n-\twhile [ $c -lt $cword ]; do\n+\twhile [ \"$c\" -lt \"$cword\" ]; do\n \t\ti=\"${words[c]}\"\n \t\tcase \"$i\" in\n \t\t-d|-D|--delete|-m|-M|--move|-c|-C|--copy)\n@@ -1493,7 +1493,7 @@ _git_branch ()\n \t\t__gitcomp_builtin branch\n \t\t;;\n \t*)\n-\t\tif [ $only_local_ref = \"y\" -a $has_r = \"n\" ]; then\n+\t\tif [ \"$only_local_ref\" = \"y\" -a \"$has_r\" = \"n\" ]; then\n \t\t\t__gitcomp_direct \"$(__git_heads \"\" \"$cur\" \" \")\"\n \t\telse\n \t\t\t__git_complete_refs\n@@ -1897,7 +1897,7 @@ __git_match_ctag () {\n __git_complete_symbol () {\n \tlocal tags=tags pfx=\"\" cur_=\"${cur-}\" sfx=\" \"\n \n-\twhile test $# != 0; do\n+\twhile test \"$#\" != 0; do\n \t\tcase \"$1\" in\n \t\t--tags=*)\ttags=\"${1##--tags=}\" ;;\n \t\t--pfx=*)\tpfx=\"${1##--pfx=}\" ;;\n@@ -2166,7 +2166,7 @@ _git_mv ()\n \t\t;;\n \tesac\n \n-\tif [ $(__git_count_arguments \"mv\") -gt 0 ]; then\n+\tif [ \"$(__git_count_arguments \"mv\")\" -gt 0 ]; then\n \t\t# We need to show both cached and untracked files (including\n \t\t# empty directories) since this may not be the last argument.\n \t\t__git_complete_index_file \"--cached --others --directory\"\n@@ -2496,7 +2496,7 @@ _git_switch ()\n __git_config_get_set_variables ()\n {\n \tlocal prevword word config_file= c=$cword\n-\twhile [ $c -gt \"$__git_cmd_idx\" ]; do\n+\twhile [ \"$c\" -gt \"$__git_cmd_idx\" ]; do\n \t\tword=\"${words[c]}\"\n \t\tcase \"$word\" in\n \t\t--system|--global|--local|--file=*)\n@@ -2541,7 +2541,7 @@ __git_complete_config_variable_value ()\n {\n \tlocal varname=\"$prev\" cur_=\"$cur\"\n \n-\twhile test $# != 0; do\n+\twhile test \"$#\" != 0; do\n \t\tcase \"$1\" in\n \t\t--varname=*)\tvarname=\"${1##--varname=}\" ;;\n \t\t--cur=*)\tcur_=\"${1##--cur=}\" ;;\n@@ -2656,7 +2656,7 @@ __git_complete_config_variable_name ()\n {\n \tlocal cur_=\"$cur\" sfx\n \n-\twhile test $# != 0; do\n+\twhile test \"$#\" != 0; do\n \t\tcase \"$1\" in\n \t\t--cur=*)\tcur_=\"${1##--cur=}\" ;;\n \t\t--sfx=*)\tsfx=\"${1##--sfx=}\" ;;\n@@ -2756,7 +2756,7 @@ __git_complete_config_variable_name_and_value ()\n {\n \tlocal cur_=\"$cur\"\n \n-\twhile test $# != 0; do\n+\twhile test \"$#\" != 0; do\n \t\tcase \"$1\" in\n \t\t--cur=*)\tcur_=\"${1##--cur=}\" ;;\n \t\t*)\t\treturn 1 ;;\n@@ -3096,7 +3096,7 @@ _git_stash ()\n \t\t__gitcomp_builtin \"stash_$subcommand\"\n \t\t;;\n \tbranch,*)\n-\t\tif [ $cword -eq $((__git_cmd_idx+2)) ]; then\n+\t\tif [ \"$cword\" -eq \"$((__git_cmd_idx+2))\" ]; then\n \t\t\t__git_complete_refs\n \t\telse\n \t\t\t__gitcomp_nl \"$(__git stash list \\\n@@ -3261,7 +3261,7 @@ _git_svn ()\n _git_tag ()\n {\n \tlocal i c=\"$__git_cmd_idx\" f=0\n-\twhile [ $c -lt $cword ]; do\n+\twhile [ \"$c\" -lt \"$cword\" ]; do\n \t\ti=\"${words[c]}\"\n \t\tcase \"$i\" in\n \t\t-d|--delete|-v|--verify)\n@@ -3279,7 +3279,7 @@ _git_tag ()\n \t-m|-F)\n \t\t;;\n \t-*|tag)\n-\t\tif [ $f = 1 ]; then\n+\t\tif [ \"$f\" = 1 ]; then\n \t\t\t__gitcomp_direct \"$(__git_tags \"\" \"$cur\" \" \")\"\n \t\tfi\n \t\t;;\n@@ -3342,7 +3342,7 @@ _git_worktree ()\n \t\t\t# be either the 'add' subcommand, the unstuck\n \t\t\t# argument of an option (e.g. branch for -b|-B), or\n \t\t\t# the path for the new worktree.\n-\t\t\tif [ $cword -eq $((subcommand_idx+1)) ]; then\n+\t\t\tif [ \"$cword\" -eq \"$((subcommand_idx+1))\" ]; then\n \t\t\t\t# Right after the 'add' subcommand: have to\n \t\t\t\t# complete the path, so fall back to Bash\n \t\t\t\t# filename completion.\n@@ -3366,7 +3366,7 @@ _git_worktree ()\n \t\t__git_complete_worktree_paths\n \t\t;;\n \tmove,*)\n-\t\tif [ $cword -eq $((subcommand_idx+1)) ]; then\n+\t\tif [ \"$cword\" -eq \"$((subcommand_idx+1))\" ]; then\n \t\t\t# The first parameter must be an existing working\n \t\t\t# tree to be moved.\n \t\t\t__git_complete_worktree_paths\n@@ -3436,7 +3436,7 @@ __git_main ()\n \tlocal __git_C_args C_args_count=0\n \tlocal __git_cmd_idx\n \n-\twhile [ $c -lt $cword ]; do\n+\twhile [ \"$c\" -lt \"$cword\" ]; do\n \t\ti=\"${words[c]}\"\n \t\tcase \"$i\" in\n \t\t--git-dir=*)\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex 76ee4ab1e5..9c10690a22 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -232,7 +232,7 @@ __git_ps1_show_upstream ()\n \t\tif [[ -n \"$count\" && -n \"$name\" ]]; then\n \t\t\t__git_ps1_upstream_name=$(git rev-parse \\\n \t\t\t\t--abbrev-ref \"$upstream_type\" 2>/dev/null)\n-\t\t\tif [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then\n+\t\t\tif [ \"$pcmode\" = yes ] && [ \"$ps1_expanded\" = yes ]; then\n \t\t\t\tupstream=\"$upstream \\${__git_ps1_upstream_name}\"\n \t\t\telse\n \t\t\t\tupstream=\"$upstream ${__git_ps1_upstream_name}\"\n@@ -269,7 +269,7 @@ __git_ps1_colorize_gitstring ()\n \tlocal flags_color=\"$c_lblue\"\n \n \tlocal branch_color=\"\"\n-\tif [ $detached = no ]; then\n+\tif [ \"$detached\" = no ]; then\n \t\tbranch_color=\"$ok_color\"\n \telse\n \t\tbranch_color=\"$bad_color\"\n@@ -567,7 +567,7 @@ __git_ps1 ()\n \tlocal z=\"${GIT_PS1_STATESEPARATOR-\" \"}\"\n \n \tb=${b##refs/heads/}\n-\tif [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then\n+\tif [ \"$pcmode\" = yes ] && [ \"$ps1_expanded\" = yes ]; then\n \t\t__git_ps1_branch_name=$b\n \t\tb=\"\\${__git_ps1_branch_name}\"\n \tfi\n@@ -579,7 +579,7 @@ __git_ps1 ()\n \tlocal f=\"$h$w$i$s$u$p\"\n \tlocal gitstring=\"$c$b${f:+$z$f}${sparse}$r${upstream}${conflict}\"\n \n-\tif [ $pcmode = yes ]; then\n+\tif [ \"$pcmode\" = yes ]; then\n \t\tif [ \"${__git_printf_supports_v-}\" != yes ]; then\n \t\t\tgitstring=$(printf -- \"$printf_format\" \"$gitstring\")\n \t\telse\n-- \n2.39.0\n\n"},{"id":"475731","messageId":"20230420074616.1642742-2-myoga.murase@gmail.com","threadId":"59622","inReplyTo":"20230420074616.1642742-1-myoga.murase@gmail.com","subject":"[PATCH 2/2] completion: suppress unwanted unescaping of `read`","fromName":"Koichi Murase","fromEmail":"myoga.murase@gmail.com","sentAt":"2023-04-20T07:46:16Z","receivedAt":"2023-04-20T07:46:59Z","isPatch":true,"sender":{"key":"myoga.murase@gmail.com","avatar":"https://gravatar.com/avatar/02c724c6bd06bcd2cbe60143154638ac624d39ff80e76a595a9f51bc1b86f303?d=mp&s=160"},"body":"From: Edwin Kofler <edwin@kofler.dev>\n\nThe function `__git_eread`, that reads the first line from the file,\ncalls the `read` builtin without passing the flag option `-r`.  When\nthe `read` builtin is called without the flag `-r`, it processes the\nbackslash escaping in the text that it reads.  We usually do not want\nto process backslashes of the input but want to read the raw contents.\n\nTo make it read the first line as is, pass the flag `-r` to the `read`\nbuiltin in the function `__git_eread`.\n\nSigned-off-by: Edwin Kofler <edwin@kofler.dev>\nSigned-off-by: Koichi Murase <myoga.murase@gmail.com>\n---\n contrib/completion/git-prompt.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex 9c10690a22..49dd69bb84 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -298,7 +298,7 @@ __git_ps1_colorize_gitstring ()\n # variable, in that order.\n __git_eread ()\n {\n-\ttest -r \"$1\" && IFS=$'\\r\\n' read \"$2\" <\"$1\"\n+\ttest -r \"$1\" && IFS=$'\\r\\n' read -r \"$2\" <\"$1\"\n }\n \n # see if a cherry-pick or revert is in progress, if the user has committed a\n-- \n2.39.0\n\n"},{"id":"475740","messageId":"xmqqh6ta3485.fsf@gitster.g","threadId":"59622","inReplyTo":"20230420074616.1642742-1-myoga.murase@gmail.com","subject":"Re: [PATCH 1/2] completion: quote arguments of test and [","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-20T16:31:22Z","receivedAt":"2023-04-20T16:31:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Koichi Murase <myoga.murase@gmail.com> writes:\n\n> The raw command substitutions $v in the arguments of the test command\n> and the [ command are subject to word splitting and pathname\n> expansions. Even when it is ensured that the variable is not empty and\n> does not contain whitespaces and glob characters, it can fail when IFS\n> is set to non-trivial values containing letters and digits.\n\nThe above sounded good before I looked at the patch, and it still is\ngood in theory, but it start to look mostly academic especially with\nenclosing $# inside a pair of double-quotes, and the variable would\nhave only digits.  The same for $i and $j that appear in the loop\ncontrol \"for ((i=0, j=0; ...)); do\".  The story is pretty much the\nsame for local variables we set outselves to signal our findings,\nlike $pcmode that is only set to either 'yes' or 'no'.\n\nIs there a practical use case for an IFS that is set to split a run\nof digits somewhere in between (e.g. \"IFS=5\")?  Is there any\nrealistic setting of IFS that breaks the command line completion\nwithout this patch, but the IFS is usable without breaking all other\nthings people wrote as shell scripts and you use everyday?\n\nIf there is no such realistic setting of IFS, most, if not all, of\nthe changes presented here, while they may not be incorrect per-se,\nlook purely academic.\n\nIt's not like this patch was produced by enclosing each and every\nvariable reference machanically inside a pair of double-quotes,\nright?  If there were variable references that ought to be split at\nIFS whitespace, the patch would have left them alone.  The readers\nalso need to assume the opposite for those variable references that\nare touched by this patch while reviewing it.\n\nIs there any change in this patch that do fix a real problem with\nsome more realistic IFS setting?  Setting IFS to a digit does not\ncount.  If there is, then mixing such a real fix in many academic\nchanges is even worse, as the latter clutters the patch and makes it\nharder to assess the former.  It is like hiding a gem in a lot of\ncobblestones.\n\nIn other words, this patch looks way too noisy to be reviewed to\ndiscover its real worth.\n\nThanks.\n\n> To prevent them from being unexpectedly processed by word splitting\n> and pathname expansions, properly quote the unquoted command\n> substituations in the arguments of the `test` and `[` builtins.\n>\n> Co-authored-by: Edwin Kofler <edwin@kofler.dev>\n> Signed-off-by: Koichi Murase <myoga.murase@gmail.com>\n> Signed-off-by: Edwin Kofler <edwin@kofler.dev>\n> ---\n>  contrib/completion/git-completion.bash | 62 +++++++++++++-------------\n>  contrib/completion/git-prompt.sh       |  8 ++--\n>  2 files changed, 35 insertions(+), 35 deletions(-)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index dc95c34cc8..6c110c223b 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -253,19 +253,19 @@ __git_reassemble_comp_words_by_ref()\n>  \t\t# word separator characters to the current word.\n>  \t\tfirst=t\n>  \t\twhile\n> -\t\t\t[ $i -gt 0 ] &&\n> +\t\t\t[ \"$i\" -gt 0 ] &&\n>  \t\t\t[ -n \"${COMP_WORDS[$i]}\" ] &&\n>  \t\t\t# word consists of excluded word separators\n>  \t\t\t[ \"${COMP_WORDS[$i]//[^$exclude]}\" = \"${COMP_WORDS[$i]}\" ]\n>  \t\tdo\n>  \t\t\t# Attach to the previous token,\n>  \t\t\t# unless the previous token is the command name.\n> -\t\t\tif [ $j -ge 2 ] && [ -n \"$first\" ]; then\n> +\t\t\tif [ \"$j\" -ge 2 ] && [ -n \"$first\" ]; then\n>  \t\t\t\t((j--))\n>  \t\t\tfi\n>  \t\t\tfirst=\n>  \t\t\twords_[$j]=${words_[j]}${COMP_WORDS[i]}\n> -\t\t\tif [ $i = $COMP_CWORD ]; then\n> +\t\t\tif [ \"$i\" = \"$COMP_CWORD\" ]; then\n>  \t\t\t\tcword_=$j\n>  \t\t\tfi\n>  \t\t\tif (($i < ${#COMP_WORDS[@]} - 1)); then\n> @@ -276,7 +276,7 @@ __git_reassemble_comp_words_by_ref()\n>  \t\t\tfi\n>  \t\tdone\n>  \t\twords_[$j]=${words_[j]}${COMP_WORDS[i]}\n> -\t\tif [ $i = $COMP_CWORD ]; then\n> +\t\tif [ \"$i\" = \"$COMP_CWORD\" ]; then\n>  \t\t\tcword_=$j\n>  \t\tfi\n>  \tdone\n> @@ -292,7 +292,7 @@ _get_comp_words_by_ref ()\n>  \tfi\n>  \t__git_reassemble_comp_words_by_ref \"$exclude\"\n>  \tcur_=${words_[cword_]}\n> -\twhile [ $# -gt 0 ]; do\n> +\twhile [ \"$#\" -gt 0 ]; do\n>  \t\tcase \"$1\" in\n>  \t\tcur)\n>  \t\t\tcur=$cur_\n> @@ -848,7 +848,7 @@ __git_complete_refs ()\n>  {\n>  \tlocal remote= dwim= pfx= cur_=\"$cur\" sfx=\" \" mode=\"refs\"\n>  \n> -\twhile test $# != 0; do\n> +\twhile test \"$#\" != 0; do\n>  \t\tcase \"$1\" in\n>  \t\t--remote=*)\tremote=\"${1##--remote=}\" ;;\n>  \t\t--dwim)\t\tdwim=\"yes\" ;;\n> @@ -1036,7 +1036,7 @@ __git_complete_remote_or_refspec ()\n>  \tif [ \"$cmd\" = \"remote\" ]; then\n>  \t\t((c++))\n>  \tfi\n> -\twhile [ $c -lt $cword ]; do\n> +\twhile [ \"$c\" -lt \"$cword\" ]; do\n>  \t\ti=\"${words[c]}\"\n>  \t\tcase \"$i\" in\n>  \t\t--mirror) [ \"$cmd\" = \"push\" ] && no_complete_refspec=1 ;;\n> @@ -1060,7 +1060,7 @@ __git_complete_remote_or_refspec ()\n>  \t\t__gitcomp_nl \"$(__git_remotes)\"\n>  \t\treturn\n>  \tfi\n> -\tif [ $no_complete_refspec = 1 ]; then\n> +\tif [ \"$no_complete_refspec\" = 1 ]; then\n>  \t\treturn\n>  \tfi\n>  \t[ \"$remote\" = \".\" ] && remote=\n> @@ -1080,21 +1080,21 @@ __git_complete_remote_or_refspec ()\n>  \tesac\n>  \tcase \"$cmd\" in\n>  \tfetch)\n> -\t\tif [ $lhs = 1 ]; then\n> +\t\tif [ \"$lhs\" = 1 ]; then\n>  \t\t\t__git_complete_fetch_refspecs \"$remote\" \"$pfx\" \"$cur_\"\n>  \t\telse\n>  \t\t\t__git_complete_refs --pfx=\"$pfx\" --cur=\"$cur_\"\n>  \t\tfi\n>  \t\t;;\n>  \tpull|remote)\n> -\t\tif [ $lhs = 1 ]; then\n> +\t\tif [ \"$lhs\" = 1 ]; then\n>  \t\t\t__git_complete_refs --remote=\"$remote\" --pfx=\"$pfx\" --cur=\"$cur_\"\n>  \t\telse\n>  \t\t\t__git_complete_refs --pfx=\"$pfx\" --cur=\"$cur_\"\n>  \t\tfi\n>  \t\t;;\n>  \tpush)\n> -\t\tif [ $lhs = 1 ]; then\n> +\t\tif [ \"$lhs\" = 1 ]; then\n>  \t\t\t__git_complete_refs --pfx=\"$pfx\" --cur=\"$cur_\"\n>  \t\telse\n>  \t\t\t__git_complete_refs --remote=\"$remote\" --pfx=\"$pfx\" --cur=\"$cur_\"\n> @@ -1203,7 +1203,7 @@ __git_find_on_cmdline ()\n>  {\n>  \tlocal word c=\"$__git_cmd_idx\" show_idx\n>  \n> -\twhile test $# -gt 1; do\n> +\twhile test \"$#\" -gt 1; do\n>  \t\tcase \"$1\" in\n>  \t\t--show-idx)\tshow_idx=y ;;\n>  \t\t*)\t\treturn 1 ;;\n> @@ -1212,7 +1212,7 @@ __git_find_on_cmdline ()\n>  \tdone\n>  \tlocal wordlist=\"$1\"\n>  \n> -\twhile [ $c -lt $cword ]; do\n> +\twhile [ \"$c\" -lt \"$cword\" ]; do\n>  \t\tfor word in $wordlist; do\n>  \t\t\tif [ \"$word\" = \"${words[c]}\" ]; then\n>  \t\t\t\tif [ -n \"${show_idx-}\" ]; then\n> @@ -1237,7 +1237,7 @@ __git_find_last_on_cmdline ()\n>  {\n>  \tlocal word c=$cword show_idx\n>  \n> -\twhile test $# -gt 1; do\n> +\twhile test \"$#\" -gt 1; do\n>  \t\tcase \"$1\" in\n>  \t\t--show-idx)\tshow_idx=y ;;\n>  \t\t*)\t\treturn 1 ;;\n> @@ -1246,7 +1246,7 @@ __git_find_last_on_cmdline ()\n>  \tdone\n>  \tlocal wordlist=\"$1\"\n>  \n> -\twhile [ $c -gt \"$__git_cmd_idx\" ]; do\n> +\twhile [ \"$c\" -gt \"$__git_cmd_idx\" ]; do\n>  \t\t((c--))\n>  \t\tfor word in $wordlist; do\n>  \t\t\tif [ \"$word\" = \"${words[c]}\" ]; then\n> @@ -1286,7 +1286,7 @@ __git_get_option_value ()\n>  \tconfig_key=\"$4\"\n>  \n>  \t((c = $cword - 1))\n> -\twhile [ $c -ge 0 ]; do\n> +\twhile [ \"$c\" -ge 0 ]; do\n>  \t\tword=\"${words[c]}\"\n>  \t\tfor val in $values; do\n>  \t\t\tif [ \"$short_opt$val\" = \"$word\" ] ||\n> @@ -1308,7 +1308,7 @@ __git_get_option_value ()\n>  __git_has_doubledash ()\n>  {\n>  \tlocal c=1\n> -\twhile [ $c -lt $cword ]; do\n> +\twhile [ \"$c\" -lt \"$cword\" ]; do\n>  \t\tif [ \"--\" = \"${words[c]}\" ]; then\n>  \t\t\treturn 0\n>  \t\tfi\n> @@ -1474,7 +1474,7 @@ _git_branch ()\n>  {\n>  \tlocal i c=\"$__git_cmd_idx\" only_local_ref=\"n\" has_r=\"n\"\n>  \n> -\twhile [ $c -lt $cword ]; do\n> +\twhile [ \"$c\" -lt \"$cword\" ]; do\n>  \t\ti=\"${words[c]}\"\n>  \t\tcase \"$i\" in\n>  \t\t-d|-D|--delete|-m|-M|--move|-c|-C|--copy)\n> @@ -1493,7 +1493,7 @@ _git_branch ()\n>  \t\t__gitcomp_builtin branch\n>  \t\t;;\n>  \t*)\n> -\t\tif [ $only_local_ref = \"y\" -a $has_r = \"n\" ]; then\n> +\t\tif [ \"$only_local_ref\" = \"y\" -a \"$has_r\" = \"n\" ]; then\n>  \t\t\t__gitcomp_direct \"$(__git_heads \"\" \"$cur\" \" \")\"\n>  \t\telse\n>  \t\t\t__git_complete_refs\n> @@ -1897,7 +1897,7 @@ __git_match_ctag () {\n>  __git_complete_symbol () {\n>  \tlocal tags=tags pfx=\"\" cur_=\"${cur-}\" sfx=\" \"\n>  \n> -\twhile test $# != 0; do\n> +\twhile test \"$#\" != 0; do\n>  \t\tcase \"$1\" in\n>  \t\t--tags=*)\ttags=\"${1##--tags=}\" ;;\n>  \t\t--pfx=*)\tpfx=\"${1##--pfx=}\" ;;\n> @@ -2166,7 +2166,7 @@ _git_mv ()\n>  \t\t;;\n>  \tesac\n>  \n> -\tif [ $(__git_count_arguments \"mv\") -gt 0 ]; then\n> +\tif [ \"$(__git_count_arguments \"mv\")\" -gt 0 ]; then\n>  \t\t# We need to show both cached and untracked files (including\n>  \t\t# empty directories) since this may not be the last argument.\n>  \t\t__git_complete_index_file \"--cached --others --directory\"\n> @@ -2496,7 +2496,7 @@ _git_switch ()\n>  __git_config_get_set_variables ()\n>  {\n>  \tlocal prevword word config_file= c=$cword\n> -\twhile [ $c -gt \"$__git_cmd_idx\" ]; do\n> +\twhile [ \"$c\" -gt \"$__git_cmd_idx\" ]; do\n>  \t\tword=\"${words[c]}\"\n>  \t\tcase \"$word\" in\n>  \t\t--system|--global|--local|--file=*)\n> @@ -2541,7 +2541,7 @@ __git_complete_config_variable_value ()\n>  {\n>  \tlocal varname=\"$prev\" cur_=\"$cur\"\n>  \n> -\twhile test $# != 0; do\n> +\twhile test \"$#\" != 0; do\n>  \t\tcase \"$1\" in\n>  \t\t--varname=*)\tvarname=\"${1##--varname=}\" ;;\n>  \t\t--cur=*)\tcur_=\"${1##--cur=}\" ;;\n> @@ -2656,7 +2656,7 @@ __git_complete_config_variable_name ()\n>  {\n>  \tlocal cur_=\"$cur\" sfx\n>  \n> -\twhile test $# != 0; do\n> +\twhile test \"$#\" != 0; do\n>  \t\tcase \"$1\" in\n>  \t\t--cur=*)\tcur_=\"${1##--cur=}\" ;;\n>  \t\t--sfx=*)\tsfx=\"${1##--sfx=}\" ;;\n> @@ -2756,7 +2756,7 @@ __git_complete_config_variable_name_and_value ()\n>  {\n>  \tlocal cur_=\"$cur\"\n>  \n> -\twhile test $# != 0; do\n> +\twhile test \"$#\" != 0; do\n>  \t\tcase \"$1\" in\n>  \t\t--cur=*)\tcur_=\"${1##--cur=}\" ;;\n>  \t\t*)\t\treturn 1 ;;\n> @@ -3096,7 +3096,7 @@ _git_stash ()\n>  \t\t__gitcomp_builtin \"stash_$subcommand\"\n>  \t\t;;\n>  \tbranch,*)\n> -\t\tif [ $cword -eq $((__git_cmd_idx+2)) ]; then\n> +\t\tif [ \"$cword\" -eq \"$((__git_cmd_idx+2))\" ]; then\n>  \t\t\t__git_complete_refs\n>  \t\telse\n>  \t\t\t__gitcomp_nl \"$(__git stash list \\\n> @@ -3261,7 +3261,7 @@ _git_svn ()\n>  _git_tag ()\n>  {\n>  \tlocal i c=\"$__git_cmd_idx\" f=0\n> -\twhile [ $c -lt $cword ]; do\n> +\twhile [ \"$c\" -lt \"$cword\" ]; do\n>  \t\ti=\"${words[c]}\"\n>  \t\tcase \"$i\" in\n>  \t\t-d|--delete|-v|--verify)\n> @@ -3279,7 +3279,7 @@ _git_tag ()\n>  \t-m|-F)\n>  \t\t;;\n>  \t-*|tag)\n> -\t\tif [ $f = 1 ]; then\n> +\t\tif [ \"$f\" = 1 ]; then\n>  \t\t\t__gitcomp_direct \"$(__git_tags \"\" \"$cur\" \" \")\"\n>  \t\tfi\n>  \t\t;;\n> @@ -3342,7 +3342,7 @@ _git_worktree ()\n>  \t\t\t# be either the 'add' subcommand, the unstuck\n>  \t\t\t# argument of an option (e.g. branch for -b|-B), or\n>  \t\t\t# the path for the new worktree.\n> -\t\t\tif [ $cword -eq $((subcommand_idx+1)) ]; then\n> +\t\t\tif [ \"$cword\" -eq \"$((subcommand_idx+1))\" ]; then\n>  \t\t\t\t# Right after the 'add' subcommand: have to\n>  \t\t\t\t# complete the path, so fall back to Bash\n>  \t\t\t\t# filename completion.\n> @@ -3366,7 +3366,7 @@ _git_worktree ()\n>  \t\t__git_complete_worktree_paths\n>  \t\t;;\n>  \tmove,*)\n> -\t\tif [ $cword -eq $((subcommand_idx+1)) ]; then\n> +\t\tif [ \"$cword\" -eq \"$((subcommand_idx+1))\" ]; then\n>  \t\t\t# The first parameter must be an existing working\n>  \t\t\t# tree to be moved.\n>  \t\t\t__git_complete_worktree_paths\n> @@ -3436,7 +3436,7 @@ __git_main ()\n>  \tlocal __git_C_args C_args_count=0\n>  \tlocal __git_cmd_idx\n>  \n> -\twhile [ $c -lt $cword ]; do\n> +\twhile [ \"$c\" -lt \"$cword\" ]; do\n>  \t\ti=\"${words[c]}\"\n>  \t\tcase \"$i\" in\n>  \t\t--git-dir=*)\n> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n> index 76ee4ab1e5..9c10690a22 100644\n> --- a/contrib/completion/git-prompt.sh\n> +++ b/contrib/completion/git-prompt.sh\n> @@ -232,7 +232,7 @@ __git_ps1_show_upstream ()\n>  \t\tif [[ -n \"$count\" && -n \"$name\" ]]; then\n>  \t\t\t__git_ps1_upstream_name=$(git rev-parse \\\n>  \t\t\t\t--abbrev-ref \"$upstream_type\" 2>/dev/null)\n> -\t\t\tif [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then\n> +\t\t\tif [ \"$pcmode\" = yes ] && [ \"$ps1_expanded\" = yes ]; then\n>  \t\t\t\tupstream=\"$upstream \\${__git_ps1_upstream_name}\"\n>  \t\t\telse\n>  \t\t\t\tupstream=\"$upstream ${__git_ps1_upstream_name}\"\n> @@ -269,7 +269,7 @@ __git_ps1_colorize_gitstring ()\n>  \tlocal flags_color=\"$c_lblue\"\n>  \n>  \tlocal branch_color=\"\"\n> -\tif [ $detached = no ]; then\n> +\tif [ \"$detached\" = no ]; then\n>  \t\tbranch_color=\"$ok_color\"\n>  \telse\n>  \t\tbranch_color=\"$bad_color\"\n> @@ -567,7 +567,7 @@ __git_ps1 ()\n>  \tlocal z=\"${GIT_PS1_STATESEPARATOR-\" \"}\"\n>  \n>  \tb=${b##refs/heads/}\n> -\tif [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then\n> +\tif [ \"$pcmode\" = yes ] && [ \"$ps1_expanded\" = yes ]; then\n>  \t\t__git_ps1_branch_name=$b\n>  \t\tb=\"\\${__git_ps1_branch_name}\"\n>  \tfi\n> @@ -579,7 +579,7 @@ __git_ps1 ()\n>  \tlocal f=\"$h$w$i$s$u$p\"\n>  \tlocal gitstring=\"$c$b${f:+$z$f}${sparse}$r${upstream}${conflict}\"\n>  \n> -\tif [ $pcmode = yes ]; then\n> +\tif [ \"$pcmode\" = yes ]; then\n>  \t\tif [ \"${__git_printf_supports_v-}\" != yes ]; then\n>  \t\t\tgitstring=$(printf -- \"$printf_format\" \"$gitstring\")\n>  \t\telse\n"},{"id":"475742","messageId":"xmqqmt321ozz.fsf@gitster.g","threadId":"59622","inReplyTo":"20230420074616.1642742-2-myoga.murase@gmail.com","subject":"Re: [PATCH 2/2] completion: suppress unwanted unescaping of `read`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-20T16:45:36Z","receivedAt":"2023-04-20T16:45:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Koichi Murase <myoga.murase@gmail.com> writes:\n\n> From: Edwin Kofler <edwin@kofler.dev>\n>\n> The function `__git_eread`, that reads the first line from the file,\n> calls the `read` builtin without passing the flag option `-r`.  When\n> the `read` builtin is called without the flag `-r`, it processes the\n> backslash escaping in the text that it reads.  We usually do not want\n> to process backslashes of the input but want to read the raw contents.\n>\n> To make it read the first line as is, pass the flag `-r` to the `read`\n> builtin in the function `__git_eread`.\n\nWe USUALLY do not want?\n\nIf there were even a single caller that does rely on the usual\nbackslash processing happening in the current code, this patch is a\nbreaking change for them, so that phrase is not acceptable as a\njustification for this change.  Perhaps\n\n    After looking at ALL the existing callers of __git_eread, it\n    turns out that they all want to read the first line of the file\n    literally.  Worse yet, some files they read may record file\n    paths, which may contain a backslash, so what they will read are\n    corrupted unless we use `read -r`.\n\nor something along that line would be more appropriate.\n\nI did look at all the callers and I think they want to read things\nliterally, so I support the use of \"read -r\".  But I didn't validate\nthe other claim \"may contain backslash\"---the \"... file paths, which\nmay contain ...\" in the above example is a totally made up claim, as\nI do not have access to a platform whose pathname separator is\nbackslash.  Please replace that part to describe the real world\nproblem you encountered that led to this fix, if there is one.\n\nOther than that, a very well written description and good change.\n\nThanks.\n\n> Signed-off-by: Edwin Kofler <edwin@kofler.dev>\n> Signed-off-by: Koichi Murase <myoga.murase@gmail.com>\n> ---\n>  contrib/completion/git-prompt.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n> index 9c10690a22..49dd69bb84 100644\n> --- a/contrib/completion/git-prompt.sh\n> +++ b/contrib/completion/git-prompt.sh\n> @@ -298,7 +298,7 @@ __git_ps1_colorize_gitstring ()\n>  # variable, in that order.\n>  __git_eread ()\n>  {\n> -\ttest -r \"$1\" && IFS=$'\\r\\n' read \"$2\" <\"$1\"\n> +\ttest -r \"$1\" && IFS=$'\\r\\n' read -r \"$2\" <\"$1\"\n>  }\n>  \n>  # see if a cherry-pick or revert is in progress, if the user has committed a\n"},{"id":"475763","messageId":"CAFLRLk-uSOucgkHJSATkQVmYc_pxZO6qrJWYg2ofiWHVWPGRcw@mail.gmail.com","threadId":"59622","inReplyTo":"xmqqh6ta3485.fsf@gitster.g","subject":"Re: [PATCH 1/2] completion: quote arguments of test and [","fromName":"Koichi Murase","fromEmail":"myoga.murase@gmail.com","sentAt":"2023-04-20T20:59:21Z","receivedAt":"2023-04-20T21:00:17Z","isPatch":true,"sender":{"key":"myoga.murase@gmail.com","avatar":"https://gravatar.com/avatar/02c724c6bd06bcd2cbe60143154638ac624d39ff80e76a595a9f51bc1b86f303?d=mp&s=160"},"body":"Thank you for the review and comments.\n\n2023年4月21日(金) 1:31 Junio C Hamano <gitster@pobox.com>:\n> The above sounded good before I looked at the patch, and it still is\n> good in theory, but it start to look mostly academic especially with\n> enclosing $# inside a pair of double-quotes, and the variable would\n> have only digits.  The same for $i and $j that appear in the loop\n> control \"for ((i=0, j=0; ...)); do\".  The story is pretty much the\n> same for local variables we set outselves to signal our findings,\n> like $pcmode that is only set to either 'yes' or 'no'.\n\nI need to admit that this report is not associated with a problem that\nhas actually happened in real-world uses.  In that sense, maybe you\ncould regard these changes as more and less academic.  To be fair, I\nthink I first need to explain the background.  We first received the\nchange as a part of the commits for a consistent code style in our\nproject, which contains copies of git-completion.bash and\ngit-prompt.sh.  Since these files came from the upstream Git project\nand are not something we are maintaining in our project, we rejected\nthe changes to these two files.  However, I still think it's worth\nforwarding the contributions from Edwin to the upstream, so I expanded\nthe changes to similar cases in the same files and posted them here.\n\n> Is there a practical use case for an IFS that is set to split a run\n> of digits somewhere in between (e.g. \"IFS=5\")? Is there any\n> realistic setting of IFS that breaks the command line completion\n> without this patch,\n\nIf you are talking about the ``default'' settings of IFS in an\ninteractive session, I don't think there would be a practical use to\nset IFS to something other than <space><tab><newline>.  However, the\nexpected usage of IFS is to ``temporarily'' set a non-trivial value to\nIFS, run some commands referencing IFS (such as word splitting of\nunquoted variable expansions or the `read' builtin), and finally\nrestore IFS to the original state.  These three steps can be performed\nin three separate command lines in an interactive shell.  The problem\nis that git prompt and completion settings can be invoked between\nthese command lines, where IFS has the temporary value, regardless of\nwhether the user intends so.\n\n> but the IFS is usable without breaking all other\n> things people wrote as shell scripts and you use everyday?\n\nFirst, an independent shell script will never be affected by IFS in\ninteractive sessions because IFS is reset to <space><tab><newline> at\nthe startup of the shell as requested by POSIX [XCU 2.5.3 IFS ¶3], so\nthe only relevant scripts here are the settings of interactive shells.\nI assume you are not including random personal settings written by\nbeginners (where we can easily find broken settings), and then there\nare not too many projects of widely used Bash interactive settings.\nAmong such projects, recent projects are carefully written so that\nthey won't be affected by random IFS. An old project, bash-completion,\nstill contains the parts affected by random IFS, but the discussions\nfor possible solutions are ongoing (e.g. in the following refs):\n\nhttps://github.com/scop/bash-completion/issues/723\nhttps://github.com/scop/bash-completion/issues/720#issuecomment-1093764792\nhttps://github.com/scop/bash-completion/pull/739\n\n> If there is no such realistic setting of IFS, most, if not all, of\n> the changes presented here, while they may not be incorrect per-se,\n> look purely academic.\n>\n> It's not like this patch was produced by enclosing each and every\n> variable reference machanically inside a pair of double-quotes,\n> right?  If there were variable references that ought to be split at\n> IFS whitespace, the patch would have left them alone.  The readers\n> also need to assume the opposite for those variable references that\n> are touched by this patch while reviewing it.\n\nI think I miss the intent of this paragraph.  I made this patch\nmanually, i.e., edited each line of `test' and `[' by hand, but the\nprocedure is actually mechanical. There can be use cases to split\nwords by IFS for the other commands or shell constructs, but\nspecifically for the `test' and `[' builtins, the words should never\nbe split because it totally breaks the semantics that these builtins\nprocess.\n\n> Is there any change in this patch that do fix a real problem with\n> some more realistic IFS setting?\n\nNo, this patch focuses on solely the problems of `test' and `[' caused\nby custom IFS, (though I'm not convinced that the problem caused by\nthe custom IFS would be ``virtual'').\n\n> Setting IFS to a digit does not\n> count.  If there is, then mixing such a real fix in many academic\n> changes is even worse, as the latter clutters the patch and makes it\n> harder to assess the former.  It is like hiding a gem in a lot of\n> cobblestones.\n>\n> In other words, this patch looks way too noisy to be reviewed to\n> discover its real worth.\n>\n> Thanks.\n\nBy the way, in the original patch, I tried to minimize the changes so\njust continued to use `test' and `[', but a solution that is generally\nconsidered preferable would be actually to switch to `[[ ... ]]'. If\nrequested so, I can edit the patch.\n"},{"id":"475768","messageId":"CAFLRLk9aLXx-j=W++qqiu5-Z0TpqdnyfR35+QA5aMZM=YRZvGQ@mail.gmail.com","threadId":"59622","inReplyTo":"xmqqmt321ozz.fsf@gitster.g","subject":"Re: [PATCH 2/2] completion: suppress unwanted unescaping of `read`","fromName":"Koichi Murase","fromEmail":"myoga.murase@gmail.com","sentAt":"2023-04-20T22:31:33Z","receivedAt":"2023-04-20T22:33:12Z","isPatch":true,"sender":{"key":"myoga.murase@gmail.com","avatar":"https://gravatar.com/avatar/02c724c6bd06bcd2cbe60143154638ac624d39ff80e76a595a9f51bc1b86f303?d=mp&s=160"},"body":"Thank you for your review. The same as the other patch, this is also a\nchange submitted to our project and then forwarded here.\n\n2023年4月21日(金) 1:45 Junio C Hamano <gitster@pobox.com>:\n> [...] But I didn't validate\n> the other claim \"may contain backslash\"---the \"... file paths, which\n> may contain ...\" in the above example is a totally made up claim, as\n> I do not have access to a platform whose pathname separator is\n> backslash.  Please replace that part to describe the real world\n> problem you encountered that led to this fix, if there is one.\n\nBecause of the background of this patch (which was originally\nsubmitted as a part of the consistent coding style in our project), I\nwouldn't think this change is associated with a problem that has\nhappened in real use cases. But maybe Edwin has experienced actual\nproblems.\n\nEdwin, did you have actual problems with the current implementation of\n`__git_eread`?\n\nFor the record, I now installed Git for Windows in my Windows and\ncreated a repository, but .git/HEAD contains something like \"ref:\nrefs/heads/master\" as usual.\n\nAnyway, the default behavior of `read` without `-r` processing\nbackslashes is just a historical one, and it is generally considered\nthe best practice to always use `read -r` unless one intensionally\nunescapes backslashes. Using `read` without `-r` is exceptional, and\ndoing so for no reason would be noisy in reading the code as the\nintent is unclear. If flag -r in `__git_eread` would be rejected, I\nwould suggest adding code comments in `__git_eread` like \"# We\nintensionally process backslashes in the file because XXX\".\n\nI'll soon submit a new patch with an updated commit message in the next reply.\n"},{"id":"475770","messageId":"20230420223800.1698197-1-myoga.murase@gmail.com","threadId":"59622","inReplyTo":"CAFLRLk9aLXx-j=W++qqiu5-Z0TpqdnyfR35+QA5aMZM=YRZvGQ@mail.gmail.com","subject":"[PATCH] completion: suppress unwanted unescaping of `read`","fromName":"Koichi Murase","fromEmail":"myoga.murase@gmail.com","sentAt":"2023-04-20T22:38:00Z","receivedAt":"2023-04-20T22:38:35Z","isPatch":true,"sender":{"key":"myoga.murase@gmail.com","avatar":"https://gravatar.com/avatar/02c724c6bd06bcd2cbe60143154638ac624d39ff80e76a595a9f51bc1b86f303?d=mp&s=160"},"body":"From: Edwin Kofler <edwin@kofler.dev>\n\nThe function `__git_eread`, which reads the first line from the file,\ncalls the `read` builtin without passing the flag option `-r`.  When\nthe `read` builtin is called without the flag `-r`, it processes the\nbackslash escaping in the text that it reads.  For this reason, it is\ngenerally considered the best practice to always use the `read`\nbuiltin with flag `-r` unless one intensionally processes the\nbackslash escaping.  For the present case in git-prompt.sh, in fact,\nall the occurrences of the calls of `__git_eread` intend to read the\nliteral content of the first lines.\n\nTo make it read the first line literally, pass the flag `-r` to the\n`read` builtin in the function `__git_eread`.\n\nSigned-off-by: Edwin Kofler <edwin@kofler.dev>\nSigned-off-by: Koichi Murase <myoga.murase@gmail.com>\n---\n contrib/completion/git-prompt.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex 9c10690a22..49dd69bb84 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -298,7 +298,7 @@ __git_ps1_colorize_gitstring ()\n # variable, in that order.\n __git_eread ()\n {\n-\ttest -r \"$1\" && IFS=$'\\r\\n' read \"$2\" <\"$1\"\n+\ttest -r \"$1\" && IFS=$'\\r\\n' read -r \"$2\" <\"$1\"\n }\n \n # see if a cherry-pick or revert is in progress, if the user has committed a\n-- \n2.39.0\n\n"},{"id":"475771","messageId":"xmqqjzy6p3wf.fsf@gitster.g","threadId":"59622","inReplyTo":"20230420223800.1698197-1-myoga.murase@gmail.com","subject":"Re: [PATCH] completion: suppress unwanted unescaping of `read`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-20T22:47:28Z","receivedAt":"2023-04-20T22:47:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Koichi Murase <myoga.murase@gmail.com> writes:\n\n> From: Edwin Kofler <edwin@kofler.dev>\n>\n> The function `__git_eread`, which reads the first line from the file,\n> calls the `read` builtin without passing the flag option `-r`.  When\n> the `read` builtin is called without the flag `-r`, it processes the\n> backslash escaping in the text that it reads.  For this reason, it is\n> generally considered the best practice to always use the `read`\n> builtin with flag `-r` unless one intensionally processes the\n> backslash escaping.  For the present case in git-prompt.sh, in fact,\n> all the occurrences of the calls of `__git_eread` intend to read the\n> literal content of the first lines.\n>\n> To make it read the first line literally, pass the flag `-r` to the\n> `read` builtin in the function `__git_eread`.\n>\n> Signed-off-by: Edwin Kofler <edwin@kofler.dev>\n> Signed-off-by: Koichi Murase <myoga.murase@gmail.com>\n> ---\n>  contrib/completion/git-prompt.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n> index 9c10690a22..49dd69bb84 100644\n> --- a/contrib/completion/git-prompt.sh\n> +++ b/contrib/completion/git-prompt.sh\n> @@ -298,7 +298,7 @@ __git_ps1_colorize_gitstring ()\n>  # variable, in that order.\n>  __git_eread ()\n>  {\n> -\ttest -r \"$1\" && IFS=$'\\r\\n' read \"$2\" <\"$1\"\n> +\ttest -r \"$1\" && IFS=$'\\r\\n' read -r \"$2\" <\"$1\"\n>  }\n>  \n>  # see if a cherry-pick or revert is in progress, if the user has committed a\n\nPerfect.  Will queue.  Thanks.\n"},{"id":"475914","messageId":"6446876d2e22b_aba294a5@chronos.notmuch","threadId":"59622","inReplyTo":"xmqqh6ta3485.fsf@gitster.g","subject":"Re: [PATCH 1/2] completion: quote arguments of test and [","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-24T13:43:09Z","receivedAt":"2023-04-24T13:43:45Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Koichi Murase <myoga.murase@gmail.com> writes:\n> \n> > The raw command substitutions $v in the arguments of the test command\n> > and the [ command are subject to word splitting and pathname\n> > expansions. Even when it is ensured that the variable is not empty and\n> > does not contain whitespaces and glob characters, it can fail when IFS\n> > is set to non-trivial values containing letters and digits.\n> \n> The above sounded good before I looked at the patch, and it still is\n> good in theory, but it start to look mostly academic especially with\n> enclosing $# inside a pair of double-quotes, and the variable would\n> have only digits.  The same for $i and $j that appear in the loop\n> control \"for ((i=0, j=0; ...)); do\".  The story is pretty much the\n> same for local variables we set outselves to signal our findings,\n> like $pcmode that is only set to either 'yes' or 'no'.\n\nI do have the same opinion.\n\nAlthough the result seems more proper, I fail to see the actual value of doing\nall these changes everywhere.\n\nOn the other hand I do see the very real harm that they would break the\ngit-completion branch everywhere. Rebasing those 50-so patches would not be\nvery pleasant.\n\n> In other words, this patch looks way too noisy to be reviewed to\n> discover its real worth.\n\nAgreed.\n\n-- \nFelipe Contreras\n"},{"id":"475915","messageId":"6446898cd2dc3_aba294be@chronos.notmuch","threadId":"59622","inReplyTo":"20230420223800.1698197-1-myoga.murase@gmail.com","subject":"Re: [PATCH] completion: suppress unwanted unescaping of `read`","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-24T13:52:12Z","receivedAt":"2023-04-24T13:52:37Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Koichi Murase wrote:\n> From: Edwin Kofler <edwin@kofler.dev>\n> \n> The function `__git_eread`, which reads the first line from the file,\n> calls the `read` builtin without passing the flag option `-r`.  When\n> the `read` builtin is called without the flag `-r`, it processes the\n> backslash escaping in the text that it reads.  For this reason, it is\n> generally considered the best practice to always use the `read`\n> builtin with flag `-r` unless one intensionally processes the\n> backslash escaping.  For the present case in git-prompt.sh, in fact,\n> all the occurrences of the calls of `__git_eread` intend to read the\n> literal content of the first lines.\n\nThis is my undrstanding.\n\nI agree using `-r` is a good practice.\n\n> To make it read the first line literally, pass the flag `-r` to the\n> `read` builtin in the function `__git_eread`.\n> \n> Signed-off-by: Edwin Kofler <edwin@kofler.dev>\n> Signed-off-by: Koichi Murase <myoga.murase@gmail.com>\n\nAcked-by: Felipe Contreras <felipe.contreras@gmail.com>\n\n-- \nFelipe Contreras\n"}]}