[2/2] completion: suppress unwanted unescaping of `read`

Commit Message

Koichi Murase April 20, 2023, 7:46 a.m. UTC

From: Edwin Kofler <[e-mail]>

The function `__git_eread`, that reads the first line from the file,
calls the `read` builtin without passing the flag option `-r`.  When
the `read` builtin is called without the flag `-r`, it processes the
backslash escaping in the text that it reads.  We usually do not want
to process backslashes of the input but want to read the raw contents.

To make it read the first line as is, pass the flag `-r` to the `read`
builtin in the function `__git_eread`.

Signed-off-by: Edwin Kofler <[e-mail]>
Signed-off-by: Koichi Murase <[e-mail]>
---
 contrib/completion/git-prompt.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Junio C Hamano April 20, 2023, 4:45 p.m. UTC | #1

Koichi Murase <[e-mail]> writes:

> From: Edwin Kofler <[e-mail]>
>
> The function `__git_eread`, that reads the first line from the file,
> calls the `read` builtin without passing the flag option `-r`.  When
> the `read` builtin is called without the flag `-r`, it processes the
> backslash escaping in the text that it reads.  We usually do not want
> to process backslashes of the input but want to read the raw contents.
>
> To make it read the first line as is, pass the flag `-r` to the `read`
> builtin in the function `__git_eread`.

We USUALLY do not want?

If there were even a single caller that does rely on the usual
backslash processing happening in the current code, this patch is a
breaking change for them, so that phrase is not acceptable as a
justification for this change.  Perhaps

    After looking at ALL the existing callers of __git_eread, it
    turns out that they all want to read the first line of the file
    literally.  Worse yet, some files they read may record file
    paths, which may contain a backslash, so what they will read are
    corrupted unless we use `read -r`.

or something along that line would be more appropriate.

I did look at all the callers and I think they want to read things
literally, so I support the use of "read -r".  But I didn't validate
the other claim "may contain backslash"---the "... file paths, which
may contain ..." in the above example is a totally made up claim, as
I do not have access to a platform whose pathname separator is
backslash.  Please replace that part to describe the real world
problem you encountered that led to this fix, if there is one.

Other than that, a very well written description and good change.

Thanks.

> Signed-off-by: Edwin Kofler <[e-mail]>
> Signed-off-by: Koichi Murase <[e-mail]>
> ---
>  contrib/completion/git-prompt.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 9c10690a22..49dd69bb84 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -298,7 +298,7 @@ __git_ps1_colorize_gitstring ()
>  # variable, in that order.
>  __git_eread ()
>  {
> -	test -r "$1" && IFS=$'\r\n' read "$2" <"$1"
> +	test -r "$1" && IFS=$'\r\n' read -r "$2" <"$1"
>  }
>  
>  # see if a cherry-pick or revert is in progress, if the user has committed a

Koichi Murase April 20, 2023, 10:31 p.m. UTC | #2

Thank you for your review. The same as the other patch, this is also a
change submitted to our project and then forwarded here.

2023年4月21日(金) 1:45 Junio C Hamano <[e-mail]>:
> [...] But I didn't validate
> the other claim "may contain backslash"---the "... file paths, which
> may contain ..." in the above example is a totally made up claim, as
> I do not have access to a platform whose pathname separator is
> backslash.  Please replace that part to describe the real world
> problem you encountered that led to this fix, if there is one.

Because of the background of this patch (which was originally
submitted as a part of the consistent coding style in our project), I
wouldn't think this change is associated with a problem that has
happened in real use cases. But maybe Edwin has experienced actual
problems.

Edwin, did you have actual problems with the current implementation of
`__git_eread`?

For the record, I now installed Git for Windows in my Windows and
created a repository, but .git/HEAD contains something like "ref:
refs/heads/master" as usual.

Anyway, the default behavior of `read` without `-r` processing
backslashes is just a historical one, and it is generally considered
the best practice to always use `read -r` unless one intensionally
unescapes backslashes. Using `read` without `-r` is exceptional, and
doing so for no reason would be noisy in reading the code as the
intent is unclear. If flag -r in `__git_eread` would be rejected, I
would suggest adding code comments in `__git_eread` like "# We
intensionally process backslashes in the file because XXX".

I'll soon submit a new patch with an updated commit message in the next reply.

Patch

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 9c10690a22..49dd69bb84 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -298,7 +298,7 @@  __git_ps1_colorize_gitstring ()
 # variable, in that order.
 __git_eread ()
 {
-	test -r "$1" && IFS=$'\r\n' read "$2" <"$1"
+	test -r "$1" && IFS=$'\r\n' read -r "$2" <"$1"
 }
 
 # see if a cherry-pick or revert is in progress, if the user has committed a