[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--- 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 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
[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@gmail.com>. Signed-off-by: Robert Abel--- 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 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
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: > 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
[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--- 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 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
Re: [PATCH] git-prompt: fix reading files with windows line endings
Hi Robert, On Thu, 30 Nov 2017, Robert Abel wrote: > 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
Re: [PATCH] git-prompt: fix reading files with windows line endings
On Thu, Nov 30, 2017 at 2:51 AM, Johannes Schindelinwrote: > 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
Re: [PATCH] git-prompt: fix reading files with windows line endings
Hi Johannes, 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. 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
Re: [PATCH] git-prompt: fix reading files with windows line endings
Hi Robert, 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"... Ciao, Johannes
Re: [PATCH] git-prompt: fix reading files with windows line endings
Hi Johannes, On 30 Nov 2017 01:21, Johannes Schindelin wrote: > 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: > 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: 66 6f 6f 3a 20 31 32 33 |foo: 123| 0008 62 61 72 3a 20 34 35 36 |bar: 456| 0008 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 66 6f 6f 3a 20 31 32 33 20 34 35 36 0d |foo: 123 456.| 000d 62 61 72 3a 20|bar: | 0005 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
Re: [PATCH] git-prompt: fix reading files with windows line endings
Hi Gábor, 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, Johannes
Re: [PATCH] git-prompt: fix reading files with windows line endings
> > 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
Re: [PATCH] git-prompt: fix reading files with windows line endings
Hi Robert, On Wed, 29 Nov 2017, Robert Abel wrote: > 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 31 32 33 0d |123.| 0004 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 , and . Ciao, Johannes
Re: [PATCH] git-prompt: fix reading files with windows line endings
Hi Johannes, On 29 Nov 2017 15:27, Johannes Schindelin 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'}" > > 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
Re: [PATCH] git-prompt: fix reading files with windows line endings
Hi Robert, On Tue, 28 Nov 2017, Robert Abel wrote: > 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. > 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
[PATCH] 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--- 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 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
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.