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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 20, 2023, 16:45 UTC
Message-ID: <xmqqmt321ozz.fsf@gitster.g>
In-Reply-To: <20230420074616.1642742-2-myoga.murase@gmail.com>

Koichi Murase <myoga.murase@gmail.com> writes:

Show 10 quoted lines

> From: Edwin Kofler <edwin@kofler.dev>
>
> 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.

Show 19 quoted lines

> Signed-off-by: Edwin Kofler <edwin@kofler.dev>
> Signed-off-by: Koichi Murase <myoga.murase@gmail.com>
> ---
>  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