threads / patch / 47334

patchgit-prompt: fix reading files with windows line endings

Subject: [PATCH] git-prompt: fix reading files with windows line endings

## tl;dr

25 messages between Nov 28, 2017 and Dec 5, 2017. Diffs are folded; open one to read it.

replies: 24people: 4as markdown or json

Robert Abel· Nov 28, 2017, 20:18 UTC · lore

git-prompt: fix reading files with windows line endings

I noticed today that my git prompt using msys-git on Windows got a bit broken. After investigating I found that the git-prompt doesn't handle the case when __git_eread reads Windows line endings \r\n. It will only strip \n, leaving the \r.

I noticed this when I created a repository with msys-git, did some tasks and later wanted to check the bare. Apparently, another tool on my PC went wild and replaced all line endings in all text files it could find, breaking my git prompt.

Robert Abel· Nov 28, 2017, 20:18 UTC · re: Robert Abel · lore

If any of the files read by __git_eread have \r\n line endings, read will only strip \n, leaving \r. This results in an ugly prompt, where instead of

    user@pc MINGW64 /path/to/repo (BARE:master)
the last parenthesis is printed over the beginning of the prompt like
    )ser@pc MINGW64 /path/to/repo (BARE:master
Signed-off-by: Robert Abel <rabel@robertabel.eu>
---
 contrib/completion/git-prompt.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to contrib/completion/git-prompt.sh +1 −1
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index c6cbef38c2..71a64e7959 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -282,7 +282,7 @@ __git_eread ()
 {
 	local f="$1"
 	shift
-	test -r "$f" && read "$@" <"$f"
+	test -r "$f" && read "$@" <"$f" && export $@="${!@%$'\r'}"
 }
 
 # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
-- 
2.13.0.windows.1
Johannes Schindelin· Nov 29, 2017, 14:27 UTC · re: Robert Abel · lore

Re: [PATCH] git-prompt: fix reading files with windows line endings

Hi Robert,
On Tue, 28 Nov 2017, Robert Abel wrote:
Show 9 quoted lines
> If any of the files read by __git_eread have \r\n line endings, read
> will only strip \n, leaving \r. This results in an ugly prompt, where
> instead of
> 
>     user@pc MINGW64 /path/to/repo (BARE:master)
> 
> the last parenthesis is printed over the beginning of the prompt like
> 
>     )ser@pc MINGW64 /path/to/repo (BARE:master
Thats' unfortunate, and obviously something to fix.
Show 10 quoted lines
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index c6cbef38c2..71a64e7959 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -282,7 +282,7 @@ __git_eread ()
>  {
>  	local f="$1"
>  	shift
> -	test -r "$f" && read "$@" <"$f"
> +	test -r "$f" && read "$@" <"$f" && export $@="${!@%$'\r'}"

As far as I understand, $'\r' is a Bash-only construct, and this file (git-prompt.sh) is targeting other Unix shells, too.

So how about using `tr -d '\r' <"$f" | read "$@"` instead?

Or maybe keep with the Bash construct, but guarded behind a test that we area actually running in Bash? Something like

	test -z "$BASH" || IFS=$' \t\r\n'

Ciao, Johannes

Robert Abel· Nov 29, 2017, 22:09 UTC · re: Johannes Schindelin · lore

Re: [PATCH] git-prompt: fix reading files with windows line endings

Hi Johannes,
On 29 Nov 2017 15:27, Johannes Schindelin wrote:
Show 13 quoted lines
>> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
>> index c6cbef38c2..71a64e7959 100644
>> --- a/contrib/completion/git-prompt.sh
>> +++ b/contrib/completion/git-prompt.sh
>> @@ -282,7 +282,7 @@ __git_eread ()
>>  {
>>  	local f="$1"
>>  	shift
>> -	test -r "$f" && read "$@" <"$f"
>> +	test -r "$f" && read "$@" <"$f" && export $@="${!@%$'\r'}"
> 
> As far as I understand, $'\r' is a Bash-only construct, and this file
> (git-prompt.sh) is targeting other Unix shells, too.

Sorry, I wasn't really aware about this bash-ism. I agree that a generic solution would be best.

> So how about using `tr -d '\r' <"$f" | read "$@"` instead?

That doesn't work for me. Apparently, the variable is always reset to "" and hence the prompt will always display the shortened sha1. Maybe it has something to do with variable scoping inside the backtick evaluation?

> Or maybe keep with the Bash construct, but guarded behind a test that we
> area actually running in Bash? Something like
> 
> 	test -z "$BASH" || IFS=$' \t\r\n'

Actually, this got me thinking and reading the POSIX.1-2008, specifically http://pubs.opengroup.org/onlinepubs/9699919799/utilities/read.html.

It seems POSIX states that IFS should be supported by read. This means that it should be okay to just do

> test -r "$f" && IFS=" \t\r\n" read "$@" < "$f"

This would also get rid of the export and avoid introducing backtick evaluation.

Regards,
Robert
Johannes Schindelin· Nov 30, 2017, 00:21 UTC · re: Robert Abel · lore

Re: [PATCH] git-prompt: fix reading files with windows line endings

Hi Robert,
On Wed, 29 Nov 2017, Robert Abel wrote:
Show 11 quoted lines
> On 29 Nov 2017 15:27, Johannes Schindelin wrote:
>
> > Or maybe keep with the Bash construct, but guarded behind a test that we
> > area actually running in Bash? Something like
> > 
> > 	test -z "$BASH" || IFS=$' \t\r\n'
> 
> Actually, this got me thinking and reading the POSIX.1-2008, specifically
> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/read.html.
> 
> It seems POSIX states that IFS should be supported by read.
Yes, that's what I meant: you could use IFS.
> This means that it should be okay to just do
> 
> > test -r "$f" && IFS=" \t\r\n" read "$@" < "$f"
I am afraid that this won't work: when I call
	printf '123\r\n' |
	while IFS=" \t\r\n" read line
	do
		printf '%s' "$line" |
		hexdump -C
	done
it prints
	00000000  31 32 33 0d                               |123.|
	00000004

If I replace the double-quoted IFS by the dollar-single-quoted one, it works again. I think the reason is that \t, \r and \n are used literally when double-quoted, not as <HT>, <CR> and <LF>.

Ciao, Johannes

Robert Abel· Nov 30, 2017, 06:22 UTC · re: Johannes Schindelin · lore

Re: [PATCH] git-prompt: fix reading files with windows line endings

Hi Johannes,
On 30 Nov 2017 01:21, Johannes Schindelin wrote:
Show 6 quoted lines
> On Wed, 29 Nov 2017, Robert Abel wrote:
>> This means that it should be okay to just do
>>
>>> test -r "$f" && IFS=" \t\r\n" read "$@" < "$f"
> 
> I am afraid that this won't work: when I call

I managed to trick myself with that one, yes... Apparently I had already converted my HEAD back to Unix line endings.

However, I noticed that the behavior of read is apparently ambiguous for the last (or a single) variable:

From POSIX.1-2008:
Show 7 quoted lines
> If there are fewer vars than fields, the last var shall be set to a
> value comprising the following elements:
> - The field that corresponds to the last var in the normal assignment
>   sequence described above
> - The delimiter(s) that follow the field corresponding to the last var
> - The remaining fields and their delimiters, with trailing IFS white
>   space ignored

I read that last "ignored" as "trailing IFS white space shall not be appended". Apparently, people implementing read read it as "trailing IFS while space shall not be processed further"

Thus, the behavior for trailing IFS white space is different in case of one or two variables:

    printf '123 456\r\n' | while IFS=$' \t\r\n' read foo bar
    do
        printf 'foo: %s' "$foo" | hexdump -C
        printf 'bar: %s' "$bar" | hexdump -C
    done
This works as expected trimming the trailing \r:
    00000000  66 6f 6f 3a 20 31 32 33                           |foo: 123|
    00000008
    00000000  62 61 72 3a 20 34 35 36                           |bar: 456|
    00000008
While doing the same just reading a single variable
    printf '123 456\r\n' | while IFS=$' \t\r\n' read foo
    do
        printf 'foo: %s' "$foo" | hexdump -C
        printf 'bar: %s' "$bar" | hexdump -C
    done
prints
    00000000  66 6f 6f 3a 20 31 32 33  20 34 35 36 0d           |foo:
123 456.|
    0000000d
    00000000  62 61 72 3a 20                                    |bar: |
    00000005
Notice the 0d at the end of foo, which didn't get trimmed.

So reading a dummy variable along with the actual content variable works for git-prompt:

    __git_eread ()
    {
        local f="$1"
        local dummy
        shift
        test -r "$f" && IFS=$'\r\n' read "$@" dummy < "$f"
    }
I feel like this would be the most readable solution thus far.
Regards,
Robert
Johannes Schindelin· Nov 30, 2017, 15:21 UTC · re: Robert Abel · lore

Re: [PATCH] git-prompt: fix reading files with windows line endings

Hi Robert,
On Thu, 30 Nov 2017, Robert Abel wrote:
Show 12 quoted lines
> So reading a dummy variable along with the actual content variable
> works for git-prompt:
> 
>     __git_eread ()
>     {
>         local f="$1"
>         local dummy
>         shift
>         test -r "$f" && IFS=$'\r\n' read "$@" dummy < "$f"
>     }
> 
> I feel like this would be the most readable solution thus far.

Hmm. I am just a little concerned about "dummy" swallowing the rest of the line, e.g. when reading "1 2 3" via `__git_eread line`... the way I read it, dummy would consume "2 3" and line would *not* receive "1 2 3" but only "1"...

Ciao, Johannes

Robert Abel· Nov 30, 2017, 18:01 UTC · re: Johannes Schindelin · lore

Re: [PATCH] git-prompt: fix reading files with windows line endings

Hi Johannes,
On 30 Nov 2017 16:21, Johannes Schindelin wrote:
Show 18 quoted lines
> On Thu, 30 Nov 2017, Robert Abel wrote:
>> So reading a dummy variable along with the actual content variable
>> works for git-prompt:
>>
>>     __git_eread ()
>>     {
>>         local f="$1"
>>         local dummy
>>         shift
>>         test -r "$f" && IFS=$'\r\n' read "$@" dummy < "$f"
>>     }
>>
>> I feel like this would be the most readable solution thus far.
> 
> Hmm. I am just a little concerned about "dummy" swallowing the rest of the
> line, e.g. when reading "1 2 3" via `__git_eread line`... the way I read
> it, dummy would consume "2 3" and line would *not* receive "1 2 3" but
> only "1"...

You missed that tab and space aren't field separator anymore, because IFS=$'\r\n'. The way I see it, __git_eread was never meant to split tokens. Its primary purpose was to test if a file exists and if so, read all its contents sans the newline into a variable.

That's how all call to __git_eread use it. And none of them are equipped to handle multi-line file contents or want to read more than one variable.

So this version does exactly that, but for CRLF line endings, too. I successfully use the above version now on two of my PCs.

If you agree and nobody else has any concerns, I'll resend an edited patch to accomodate for the changes and probably put a comment with usage info above __git_eread.

Regards,
Robert
Johannes Schindelin· Dec 1, 2017, 10:45 UTC · re: Robert Abel · lore

Re: [PATCH] git-prompt: fix reading files with windows line endings

Hi Robert,
On Thu, 30 Nov 2017, Robert Abel wrote:
Show 23 quoted lines
> On 30 Nov 2017 16:21, Johannes Schindelin wrote:
> > On Thu, 30 Nov 2017, Robert Abel wrote:
> >> So reading a dummy variable along with the actual content variable
> >> works for git-prompt:
> >>
> >>     __git_eread ()
> >>     {
> >>         local f="$1"
> >>         local dummy
> >>         shift
> >>         test -r "$f" && IFS=$'\r\n' read "$@" dummy < "$f"
> >>     }
> >>
> >> I feel like this would be the most readable solution thus far.
> > 
> > Hmm. I am just a little concerned about "dummy" swallowing the rest of the
> > line, e.g. when reading "1 2 3" via `__git_eread line`... the way I read
> > it, dummy would consume "2 3" and line would *not* receive "1 2 3" but
> > only "1"...
> You missed that tab and space aren't field separator anymore,
> because IFS=$'\r\n'. The way I see it, __git_eread was never meant to
> split tokens. Its primary purpose was to test if a file exists and if
> so, read all its contents sans the newline into a variable.

Ah. The "$@* put me on the wrong track. If you hard-code the expectation that __git_eread is not used to split tokens, maybe there should be a preparatory patch (after carefully ensuring that all callers pass only one argument) to change the "$@" to "$1"?

That will prevent future callers from expecting the token-splitting behavior that is promised by using "$@".

Ciao, Johannes

Robert Abel· Dec 1, 2017, 23:31 UTC · re: Johannes Schindelin · lore

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

__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.

Thus, add a comment and explicitly use $2 instead of shifting the args down and using $@.

Signed-off-by: Robert Abel <rabel@robertabel.eu>
---
 contrib/completion/git-prompt.sh | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
Show changes to contrib/completion/git-prompt.sh +4 −3
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)
-- 
2.15.1
Robert Abel· Dec 1, 2017, 23:31 UTC · re: Robert Abel · lore

[PATCH v2 2/2] git-prompt: fix reading files with windows line endings

If any of the files read by __git_eread have \r\n line endings, read will only strip \n, leaving \r. This results in an ugly prompt, where instead of

    user@pc MINGW64 /path/to/repo (BARE:master)
the last parenthesis is printed over the beginning of the prompt like
    )ser@pc MINGW64 /path/to/repo (BARE:master
Signed-off-by: Robert Abel <rabel@robertabel.eu>
---
 contrib/completion/git-prompt.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to contrib/completion/git-prompt.sh +1 −1
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 41a471957..983e419d2 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -283,7 +283,7 @@ __git_ps1_colorize_gitstring ()
 # variable, in that order.
 __git_eread ()
 {
-	test -r "$1" && read "$2" <"$1"
+	test -r "$1" && IFS=$'\r\n' read "$2" <"$1"
 }
 
 # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
-- 
2.15.1
Johannes Schindelin· Dec 4, 2017, 14:18 UTC · re: Robert Abel · lore

Re: [PATCH v2 2/2] git-prompt: fix reading files with windows line endings

Hi Robert,
1/2 looks very good.
On Sat, 2 Dec 2017, Robert Abel wrote:
Show 9 quoted lines
> If any of the files read by __git_eread have \r\n line endings, read
> will only strip \n, leaving \r. This results in an ugly prompt, where
> instead of
> 
>     user@pc MINGW64 /path/to/repo (BARE:master)
> 
> the last parenthesis is printed over the beginning of the prompt like
> 
>     )ser@pc MINGW64 /path/to/repo (BARE:master

Maybe mention explicitly what Gabór said about $'...' being supported by Bash and zsh, the only two intended users of git-prompt.sh (and there being precedent of that construct being used already e.g. in __git_ps1)?

Other than that, this looks very good to me.

Thanks, Johannes

Junio C Hamano· Dec 4, 2017, 17:58 UTC · re: Robert Abel · lore

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

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)
Robert Abel· Dec 4, 2017, 22:57 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On 04 Dec 2017 18:58, Junio C Hamano wrote:
Show 7 quoted lines
> 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. 

I'm aware of that. I was documenting current usage. The function is used to read file contents (which are expected to be a single line) into _a_ (i.e. single) variable.

None of the current users of the function expect tokens to be split, which is why I removed it in preparation of patch 2/2, which would break tokenizing file contents.

Regards,
Robert
Junio C Hamano· Dec 5, 2017, 00:27 UTC · re: Robert Abel · lore

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

Robert Abel <rabel@robertabel.eu> writes:
Show 18 quoted lines
> Hi Junio,
>
> On 04 Dec 2017 18:58, Junio C Hamano wrote:
>> 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. 
>
> I'm aware of that. I was documenting current usage. The function is used
> to read file contents (which are expected to be a single line) into
> _a_ (i.e. single) variable.
>
> None of the current users of the function expect tokens to be split,
> which is why I removed it in preparation of patch 2/2, which would
> break tokenizing file contents.

I know all of the above, but I think you misunderstood the point I wanted to raise, so let me try again. The thing is, none of what you just wrote changes the fact that lack of callers that want to do "multi-line" is IRRELEVANT. True, there is no caller that wants to read multiple lines---it is a true statement, but it is irrelevant statement. On the other hand, it is true and relevant that no caller expects to split a line into multiple variables.

By changing "$@" to "$2" there, you would have broken callers that wanted the helper function to read into multiple variables (if there were such callers). Explaining the current usage that nobody does so *IS* a valid justification for the change. It is relevant.

With or without that change, a caller that wanted to read multiple lines from the file would never have worked. It was just doing a single "read" built-in, so the only thing that would have been worked on is the first line of the file. Your change wouldn't have changed that---if a caller wanted to peek into the second line, your change wouldn't have helped such a caller. And it is not like your change would have broken such a caller that were happily reading the second and subsequent line. The original wouldn't allowed it to read the second line anyway.

Contrasting this with the above obsesrvation about possible breakage for multi-variable callers (if there were such callers---luckily there wasn't any), I hope that you can see why the lack of "multi-line" caller in the existing usage is totally irrelevant when analyzing this change and explaining why this is a good change.

HTH.
Robert Abel· Dec 5, 2017, 07:01 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On 05 Dec 2017 01:27, Junio C Hamano wrote:
> I know all of the above, but I think you misunderstood the point I
> wanted to raise, so let me try again.  The thing is, none of what
> you just wrote changes the fact that lack of callers that want to do
> "multi-line" is IRRELEVANT.

I disagree. The commit comment is meant to give context to the introduced changes. One change is the additional comment for __git_eread, which now clearly states that only a single line is read.

I'm well aware that I'm not breaking reading multiple lines, because that never worked in the first place. Thus, it was never the indented use for __git_eread as I see it. I explicitly want to include that information in my commit message to pay it forward to the next person working on the prompt.

Regards,
Robert
Junio C Hamano· Dec 5, 2017, 13:06 UTC · re: Robert Abel · lore

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

Robert Abel <rabel@robertabel.eu> writes:
Show 9 quoted lines
> On 05 Dec 2017 01:27, Junio C Hamano wrote:
>> I know all of the above, but I think you misunderstood the point I
>> wanted to raise, so let me try again.  The thing is, none of what
>> you just wrote changes the fact that lack of callers that want to do
>> "multi-line" is IRRELEVANT.
>
> I disagree. The commit comment is meant to give context to the
> introduced changes. One change is the  additional comment for
> __git_eread, which now clearly states that only a single line is read.

I still do not understand why you think the 'next' person would care about the (lack of )multi-line aspect of the helper.

Let's see how well the proposed log message gives the "context to the introduced changes" (from your v3).

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

The helper is about (1) reading the first line and (2) reading it as a whole into a single variable. Both are already covered by the first sentence, and there is no need to say 'and don't expect ...", unless you want to stress something.

And it places a stress on the former, which is a less relevant thing, WITHOUT giving the same treatment to the latter, which is a more relevant thing. After all, this patch is not about replacing an earlier implementation that did

    $2=$(cat "$1")
with
    read $2 <"$1"

If that were the case, _then_ the fact that the purpose of the helper is to read from a single-liner file (i.e. we do not expect the input file to have more than one line) is VERY relevant.

But this is not such a patch. And after readers read the above, they find this:

    Therefore, this patch removes the unused capability to split
    file conents into tokens by passing multiple variable names.

And because the previous paragraph placed an emphasis on a wrong aspect of the context of the calls to the helper function, this "Therefore" does not quite "click" in the readers' minds. The reason why it is OK to remove the multi-variable feature is because the callers of the helper want to always read the result into a single variable, but the "no need for multi-variable" that they read in the first sentence of the previous paragraph is less strong in their mind by now, because they read an irrelevant (for the purpose of this "Therefore") mention of "no need for multi-line" aspect of the helper.

Perhaps
    __git_eread is used to read the contents of a single-liner file
    into a single variable while dropping EOL.  It is misleading to
    use the "read" built-in with "$@", as if some callers would want
    the contents read from the file to be split into multiple
    variables.
    Explicitly use a single variable, and also document that the
    helper only reads the first line (simply because the input files
    are designed to be single-liner files).

would say it the same thing, but with emphasis on the right aspect of the facts.

I would also rephase the new in-code comment
    # Helper function to read the first line of a file into a variable.

to un-stress "the first line of a file" and place more stress on the fact that it is designed to read from a single-liner file (there is a subtle but important distinction between the two).

    # read the contents of a single-liner file into a variable,
    # while dropping the end-of-line from it.
or something like that, perhaps.
Robert Abel· Dec 5, 2017, 23:37 UTC · re: Junio C Hamano · lore

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

Dear Junio,

I'm amazed at how much time and energy you spend on correcting these essentially non-issues in my git commit messages for a quadruple-liner code change.

I'll resend both patches one last time addressing the grave issue of the informative mention of multi-line files.

Regards,
Robert
Robert Abel· Dec 5, 2017, 23:39 UTC · re: Robert Abel · lore

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

__git_eread is used to read a single line of a given file (if it exists) into a single variable stripping the EOL. This patch removes the unused capability to split file contents into tokens by passing multiple variable names. Add a comment and explicitly use $2 instead of misleading $@ as argument to the read builtin command.

Signed-off-by: Robert Abel <rabel@robertabel.eu>
---
 contrib/completion/git-prompt.sh | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
Show changes to contrib/completion/git-prompt.sh +4 −3
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index c6cbef38c2..41a471957a 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)
-- 
2.13.0.windows.1
Robert Abel· Dec 5, 2017, 23:39 UTC · re: Robert Abel · lore

[PATCH v4 2/2] git-prompt: fix reading files with windows line endings

If any of the files read by __git_eread have \r\n line endings, read will only strip \n, leaving \r. This results in an ugly prompt, where instead of

    user@pc MINGW64 /path/to/repo (BARE:master)
the last parenthesis is printed over the beginning of the prompt like
    )ser@pc MINGW64 /path/to/repo (BARE:master

This patch fixes the issue by changing the internal field separator variable IFS to $'\r\n' before using the read builtin command.

Note that ANSI-C Quoting/POSIX Quoting ($'...') is supported by bash as well as zsh, which are the current targets of git-prompt, cf. contrib/completion/git-prompt.sh.

Signed-off-by: Robert Abel <rabel@robertabel.eu>
---
 contrib/completion/git-prompt.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to contrib/completion/git-prompt.sh +1 −1
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 41a471957a..983e419d2b 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -283,7 +283,7 @@ __git_ps1_colorize_gitstring ()
 # variable, in that order.
 __git_eread ()
 {
-	test -r "$1" && read "$2" <"$1"
+	test -r "$1" && IFS=$'\r\n' read "$2" <"$1"
 }
 
 # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
-- 
2.13.0.windows.1
Robert Abel· Dec 4, 2017, 23:49 UTC · re: Robert Abel · lore

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

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

Therefore, this patch removes the unused capability to split file conents into tokens by passing multiple variable names. Add a comment and explicitly use $2 instead of $@ to read the file into one variable.

Signed-off-by: Robert Abel <rabel@robertabel.eu>
---
 contrib/completion/git-prompt.sh | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
Show changes to contrib/completion/git-prompt.sh +4 −3
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index c6cbef38c2..41a471957a 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)
-- 
2.13.0.windows.1
Robert Abel· Dec 4, 2017, 23:49 UTC · re: Robert Abel · lore

[PATCH v3 2/2] git-prompt: fix reading files with windows line endings

If any of the files read by __git_eread have \r\n line endings, read will only strip \n, leaving \r. This results in an ugly prompt, where instead of

    user@pc MINGW64 /path/to/repo (BARE:master)
the last parenthesis is printed over the beginning of the prompt like
    )ser@pc MINGW64 /path/to/repo (BARE:master

This patch fixes the issue by setting the IFS to $'\r\n' for the read operation. Note that ANSI-C Quoting ($'...') is supported by bash as well as zsh, which are the current targets of git-prompt.sh, cf. <20171130010811.17369-1-szeder.dev@gmail.com>.

Signed-off-by: Robert Abel <rabel@robertabel.eu>
---
 contrib/completion/git-prompt.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to contrib/completion/git-prompt.sh +1 −1
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 41a471957a..983e419d2b 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -283,7 +283,7 @@ __git_ps1_colorize_gitstring ()
 # variable, in that order.
 __git_eread ()
 {
-	test -r "$1" && read "$2" <"$1"
+	test -r "$1" && IFS=$'\r\n' read "$2" <"$1"
 }
 
 # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
-- 
2.13.0.windows.1
SZEDER Gábor· Nov 30, 2017, 01:08 UTC · re: Johannes Schindelin · lore

Re: [PATCH] git-prompt: fix reading files with windows line endings

Show 10 quoted lines
> > diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> > index c6cbef38c2..71a64e7959 100644
> > --- a/contrib/completion/git-prompt.sh
> > +++ b/contrib/completion/git-prompt.sh
> > @@ -282,7 +282,7 @@ __git_eread ()
> >  {
> >  	local f="$1"
> >  	shift
> > -	test -r "$f" && read "$@" <"$f"
> > +	test -r "$f" && read "$@" <"$f" && export $@="${!@%$'\r'}"
I don't think that export is necessary here.
> As far as I understand, $'\r' is a Bash-only construct, and this file
> (git-prompt.sh) is targeting other Unix shells, too.

The only other shell the prompt (and completion) script is targeting is ZSH, and ZSH understands this construct. We already use this construct to set IFS in several places in both scripts for a long time, so it should be fine here, too.

Gábor
Johannes Schindelin· Nov 30, 2017, 01:51 UTC · re: SZEDER Gábor · lore

Re: [PATCH] git-prompt: fix reading files with windows line endings

Hi Gábor,
On Thu, 30 Nov 2017, SZEDER Gábor wrote:
Show 20 quoted lines
> > > diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> > > index c6cbef38c2..71a64e7959 100644
> > > --- a/contrib/completion/git-prompt.sh
> > > +++ b/contrib/completion/git-prompt.sh
> > > @@ -282,7 +282,7 @@ __git_eread ()
> > >  {
> > >  	local f="$1"
> > >  	shift
> > > -	test -r "$f" && read "$@" <"$f"
> > > +	test -r "$f" && read "$@" <"$f" && export $@="${!@%$'\r'}"
> 
> I don't think that export is necessary here.
> 
> > As far as I understand, $'\r' is a Bash-only construct, and this file
> > (git-prompt.sh) is targeting other Unix shells, too.
> 
> The only other shell the prompt (and completion) script is targeting
> is ZSH, and ZSH understands this construct.  We already use this
> construct to set IFS in several places in both scripts for a long
> time, so it should be fine here, too.
That's good to know! I should have `git grep`ped...

Sorry for the noise, Johannes

SZEDER Gábor· Nov 30, 2017, 23:45 UTC · re: Johannes Schindelin · lore

Re: [PATCH] git-prompt: fix reading files with windows line endings

On Thu, Nov 30, 2017 at 2:51 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 26 quoted lines
> On Thu, 30 Nov 2017, SZEDER Gábor wrote:
>
>> > > diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
>> > > index c6cbef38c2..71a64e7959 100644
>> > > --- a/contrib/completion/git-prompt.sh
>> > > +++ b/contrib/completion/git-prompt.sh
>> > > @@ -282,7 +282,7 @@ __git_eread ()
>> > >  {
>> > >   local f="$1"
>> > >   shift
>> > > - test -r "$f" && read "$@" <"$f"
>> > > + test -r "$f" && read "$@" <"$f" && export $@="${!@%$'\r'}"
>>
>> I don't think that export is necessary here.
>>
>> > As far as I understand, $'\r' is a Bash-only construct, and this file
>> > (git-prompt.sh) is targeting other Unix shells, too.
>>
>> The only other shell the prompt (and completion) script is targeting
>> is ZSH, and ZSH understands this construct.  We already use this
>> construct to set IFS in several places in both scripts for a long
>> time, so it should be fine here, too.
>
> That's good to know! I should have `git grep`ped...
>
> Sorry for the noise,

No, no, your concern is justified, you just happened to pick the wrong construct :)

It's the ${!var} indirect expansion construct that ZSH doesn't know, it uses a different syntax for that.

Gábor

← back to recent threads