Page MenuHomePhabricator

[lit] Fix internal diff's --strip-trailing-cr and use it
ClosedPublic

Authored by jdenny on Oct 10 2019, 3:14 PM.

Details

Summary

Using GNU diff, --strip-trailing-cr removes a \r appearing before
a \n at the end of a line. Without this patch, lit's internal diff
only removes \r if it appears as the last character. That seems
useless. This patch fixes that.

This patch also adds --strip-trailing-cr to some tests that fail on
Windows bots when D68664 is applied. Based on what I see in the bot
logs, I think the following is happening. In each test there, lit
diff is comparing a file with \r\n line endings to a file with \n
line endings. Without D68664, lit diff reads those files in text
mode, which in Windows causes \r\n to be replaced with \n.
However, with D68664, lit diff reads the files in binary mode instead
and thus reports that every line is different, just as GNU diff does
(at least under Ubuntu). Adding --strip-trailing-cr to those tests
restores the previous behavior while permitting the behavior of lit
diff to be more like GNU diff.

Diff Detail

Event Timeline

jdenny created this revision.Oct 10 2019, 3:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2019, 3:14 PM
Herald added a subscriber: delcypher. · View Herald Transcript
rnk accepted this revision.Oct 11 2019, 1:29 PM

I looked into these tests, and it seems that they would've failed if it were not for Python's universal newline translator thing. When I diff the files in question with gnu diff from git bash, they appear to be different without -w or --strip-trailing-cr. So, your fix makes lit's diff more like gnu diff, and fixes the tests to work in that mode.

The only downside is that this is one more way for tests to pass on Linux but fail on Windows out of the box. However, if we want to address that, I think we should fix it by adding a lit substitution for "\bdiff\b " to add --strip-trailing-cr on Windows.

lgtm

This revision is now accepted and ready to land.Oct 11 2019, 1:29 PM
In D68839#1706504, @rnk wrote:

I looked into these tests, and it seems that they would've failed if it were not for Python's universal newline translator thing. When I diff the files in question with gnu diff from git bash, they appear to be different without -w or --strip-trailing-cr. So, your fix makes lit's diff more like gnu diff, and fixes the tests to work in that mode.

Thanks for verifying. I only had access to logs, so I was doing some guesswork.

The only downside is that this is one more way for tests to pass on Linux but fail on Windows out of the box.

Yes, I debated whether the old behavior was desirable, but ultimately I concluded that consistency with external diffs is more important to avoid surprising behavior.

However, if we want to address that, I think we should fix it by adding a lit substitution for "\bdiff\b " to add --strip-trailing-cr on Windows.

Agreed.

jdenny updated this revision to Diff 224947.Oct 14 2019, 6:50 PM
  • Added --strip-trailing-cr to another test that needs it.
  • Removed some accidental FileCheck options.
  • Rebased onto master so it doesn't depend on D66574 anymore. Thus, it modifies TestRunner.py instead of diff.py.

Another review shouldn't be necessary. I plan to push tomorrow.

jdenny updated this revision to Diff 225139.Tue, Oct 15, 5:47 PM
jdenny edited the summary of this revision. (Show Details)

Rebased onto D68664, which I previously forgot this patch requires. Otherwise, when using Windows or when using Python 3, a lit test introduced here fails because it expects \r\n and \n to be different while, when using Windows or Python 3, \r\n is read as \n in text mode. D68664 reads in binary mode and thus prevents that problem.

Adjusted the commit log not to confuse Python's universal newlines support (the default in Python 3 but not 2), with the \r\n -> \n behavior of text mode under even Python 2 on Windows.