Re: [PATCH v2 1/2] git-prompt: make __git_eread intended use explicit

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 4, 2017, 17:58 UTC
Message-ID: <xmqqindmml25.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171201233133.30011-1-rabel@robertabel.eu>

Robert Abel <rabel@robertabel.eu> writes:

> __git_eread is used to read a single line of a given file (if it exists)
> into a variable without the EOL. All six current users of __git_eread
> use it that way and don't expect multi-line content.

Changing $@ to $2 does not change whether this is about "multi-line" or not. What you are changing is that the original was prepared to be given two or more variable names, and split an input line at IFS into multiple tokens to be assigned to these variables, but with this change, the caller can only use one variable and this function will not split the line and store it into that single variable.

The above can easily be fixed with a bit of rewording, perhaps like:

    ... that way.  We do not need to split the line into tokens and
    assign them to multiple variables---reading only into a single
    variable needs to be supported.

While reviewing this patch, I also wondered if the "read" wants to become "read -r" or something that is even safer than simply avoiding tokenization, but after scanning to see exactly which files __git_eread is used to read from, I do not think it matters (the input will not have a backslash that would want to be protected from 'read'), so this should be OK.

Show 25 quoted lines

> Signed-off-by: Robert Abel <rabel@robertabel.eu>
> ---
>  contrib/completion/git-prompt.sh | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index c6cbef38c..41a471957 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -278,11 +278,12 @@ __git_ps1_colorize_gitstring ()
>  	r="$c_clear$r"
>  }
>  
> +# Helper function to read the first line of a file into a variable.
> +# __git_eread requires 2 arguments, the file path and the name of the
> +# variable, in that order.
>  __git_eread ()
>  {
> -	local f="$1"
> -	shift
> -	test -r "$f" && read "$@" <"$f"
> +	test -r "$1" && read "$2" <"$1"
>  }
>  
>  # __git_ps1 accepts 0 or 1 arguments (i.e., format string)