Regression: awk doesn't strip \r

[orgads]

• I was not able to find an open or closed issue matching what I'm seeing

Setup

• Which version of Git for Windows are you using? Is it 32-bit or 64-bit?

$ git --version --build-options

git version 2.16.2.windows.1
cpu: x86_64
built from commit: e1848984d1004040ec5199e749b5f282ddf4bb09
sizeof-long: 4

• Which version of Windows are you running? Vista, 7, 8, 10? Is it 32-bit or 64-bit?

$ cmd.exe /c ver

Microsoft Windows [Version 6.1.7601]

• What options did you set as part of the installation? Or did you choose the
  defaults?

# One of the following:
> type "C:\Program Files\Git\etc\install-options.txt"
> type "C:\Program Files (x86)\Git\etc\install-options.txt"
> type "%USERPROFILE%\AppData\Local\Programs\Git\etc\install-options.txt"
$ cat /etc/install-options.txt

Editor Option: VIM
Path Option: Cmd
SSH Option: OpenSSH
CURL Option: WinSSL
CRLF Option: CRLFAlways
Bash Terminal Option: MinTTY
Performance Tweaks FSCache: Enabled
Use Credential Manager: Enabled
Enable Symlinks: Enabled

Details

• Which terminal/shell are you running Git from? e.g Bash/CMD/PowerShell/other

Bash

• What commands did you run to trigger this issue? If you can provide a
  Minimal, Complete, and Verifiable example
  this will help us understand the issue.

echo -en "foo\r\n\r\nbar\r\n" > foo.txt
awk '/^$/ { print "found" }' foo.txt # This worked before 2.16 and doesn't work with 2.16
awk '/^\r$/ { print "found" }' foo.txt # This works with 2.16 and doesn't work with previous versions

• What did you expect to occur after running these commands?
• What actually happened instead?

See comments

Awk version in 2.15.1:

GNU Awk 4.1.4, API: 1.1 (GNU MPFR 3.1.5-p2, GNU MP 6.1.2)

In 2.16:

GNU Awk 4.2.0, API: 2.0 (GNU MPFR 4.0.1, GNU MP 6.1.2)

This affects gerrit's commit-msg hook, which now adds an empty line between the footer and the Change-Id line. It also affects our own scripts.

[orgads]

Workaround: Set RS="\r?\n" in BEGIN:

awk 'BEGIN {RS="\r?\n"}; /^$/ { print "found" }' foo.txt

[dscho]

Hmm. I cannot spot any obvious culprit in msys2/MSYS2-packages@316e772 nor in http://git.savannah.gnu.org/cgit/gawk.git/tree/ChangeLog?h=gawk-4.2.1&id=bd8a8ad0c258d2db31e420eec81932cf15bf9702. Can you?

[orgads]

Bisected to 5db38f775d9ba239e125d81dff2010a2ddacb48e:

(* gawkmisc.c (cygwin_premain0, cygwin_premain2): Remove.
No longer needed).

:)

[dscho]

I uploaded new packages to Git for Windows' Pacman repositories, and verified that the indicated commands now work as expected again.

[orgads]

Thanks!

[dscho]

@orgads oh, and BTW, there is a new snapshot at https://wingit.blob.core.windows.net/files/index.html that should have the fix... Do you want to test?

[orgads]

I already tested with your packages in the sdk. 👍

[dscho]

:-)

[dscho]

For the record, I re-tested with the 64-bit portable Git of the latest snapshot and it does fix the regression. So if you need any semi-official installer until the next official Git for Windows version is released, there you go (the snapshot installers are pretty close to the official versions, the binaries are also code-signed by me).

[orgads]

Thank you.

[orgads]

It breaks compatibility of existing scripts, so I consider this a bug.

[dscho]

Um, this is NOT a bug

@svnpenn if the software behaved in one way for ages, so much so that users started relying on it, you can go down the streets shouting from the top of your lungs that this is not a bug, and it will still be wrong.

All the Open Source maintainers of this world could form a choir and sing that this is not a bug, and they still would be all wrong.

[ghost]

@dscho no, its the people relying on non portable scripts that are wrong

http://cygwin.com/ml/cygwin/2017-02/msg00155.html

this "fix" you all have merged needs to be reverted ASAP - and people need to just write proper scripts - AKA awk something | tr -d '\r'

[orgads]

@dscho no, its the people relying on non portable scripts that are wrong

http://cygwin.com/ml/cygwin/2017-02/msg00155.html

this "fix" you all have merged needs to be reverted ASAP - and people need to just write proper scripts - AKA awk something | tr -d '\r'

It's the other way around: tr -d '\r' | awk something...

Or add RS="\r?\n". Both are ugly, and require changes to existing scripts that relied on this behavior. I already gave as an example gerrit's commit-msg hook, and we had at least 2 scripts that I had to adapt (so far).

[dscho]

it seems you cant see past your own nose here

@svnpenn this type of comment is not at all welcome here. Please do not force me to block you from the project as a poisonous person.

[dscho]

the issue I linked to - and will now link again

http://cygwin.com/ml/cygwin/2017-02/msg00155.html

was a critical Awk issue that fixed this "bug" you are now reporting - while convenient for you that you no longer have to adapt your script - youve now reintroduced that bug - one that cant be fixed by modifying scripts

I agree that we are stuck between a rock and a hard place because gawk featured a certain behavior for such a long time that it became the de facto standard. However, I disagree that we have to cause even more pain in order to solve what some may consider a bug by breaking existing setups. That is just such a no-go that I am a bit surprised I even have to mention it.

[ghost]

@dscho you do have to mention it

"That‘s the way it‘s always been done" - is not a good reason to keep doing something - scripts that assumed Awk would strip CR - are not and never were portable - you should also note that this "errant" behavior falsely reported is now the default with Sed and Grep too

so if you are going to break Awk, youre going to need to break Sed and Grep too:

• http://cygwin.com/ml/cygwin-announce/2017-02/msg00036.html
• http://cygwin.com/ml/cygwin-announce/2017-02/msg00035.html

[dscho]

"That‘s the way it‘s always been done" - is not a good reason to keep doing something - scripts that assumed Awk would strip CR - are not and never were portable - you should also note that this "errant" behavior falsely reported is now the default with Sed and Grep too

@svnpenn Let's stop misquoting, alright?

To continue doing things as they always have been done just for the heck of it is obviously not what we do here. Otherwise there would be no commits in this org. Zero.

So I would appreciate it if this needling way of phrasing things would stop, like, right now. Or even better: earlier than that.

The problem pointed out both by @orgads and by myself is that you advocate for breaking existing setups without warning.

To put this into a context that I have a suspicion will be more familiar to you: can you imagine coming to your house tonight, finding that there is a digger there that already started tearing it down? And the workers will gladly tell you that a new highway will be built, and there is really no more sensible route than right through your house. And it really is the most sensible route, even if it happens to require the house to be torn down that you have gotten used to live in.

If you still think that you have a convincing argument for breaking existing setups without prior warning, let's hear it. If you wish to do that, you are welcome to do so, in a polite manner.

[ghost]

@dscho no thank you

i wont be arguing with a brick wall

any who support portable scripts will understand that the current cygwin behavior is correct - the opposite side of that argument is those who do not care about portable scripts - but only care to not change their existing non portable scripts - this argument has been had out already and the winning side was portable scripts as far as the cygwin community is concerned

if you wish to deviate from that path it is your call - but i urge you not to - take my request to revert this commit seriously - i do not make it lightly

http://cygwin.com/ml/cygwin/2017-08/msg00033.html

[dscho]

@svnpenn the number one priority in this project are the users. If you break users' existing setups, you introduce a bug. That's as simple as I can phrase it.

Saying "I won't be arguing with a brick wall" (grammar fixed for you) is just disrespectful, and I think we will have to lock down this ticket.

The last word should be, though, that we care about users here, and the change we made was necessary to take care of users' broken setups. There is little one can sensibly say to the contrary.