Bug 53625 – svn-apply and/or patch(1) has trouble applying patches that makes changes to files with Windows line endings

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Daniel Bates]

As observed in bug #53348 and #53607, svn-apply and/or patch(1) has trouble applying patches that make changes to files with Windows line endings.

[Daniel Bates]

Also observed in bug #53573.

[Daniel Bates]

I did not encounter any issues when using svn-apply to apply the patch for bug #53348, and bug #53607 (attachment 80936 [details] - https://bugs.webkit.org/attachment.cgi?id=80936).

I am using Mac OS 10.6.6 with patch(1) version 2.5.8.

I am curious as to the whether of patch(1) being used on the various EWS bots.

CC'ing Gustavo Silva with regards to the version patch(1) on the GTK EWS bot.

[Daniel Bates]

(In reply to comment #2)
> I am curious as to the whether of patch(1) being used on the various EWS bots.
> 
Wow, that came out terribly mangled.

I meant to write:

I am curious as to what version of patch(1) is installed on the various EWS bots.

[Daniel Bates]

On IRC today (02/02/11), Gustavo Silva wrote:
[14:17] <kov> dydx, hey, 2.6 is the patch I have on the EWS (it's got whatever is in debian unstable usually)

[Csaba Osztrogonác]

On Qt EWS now we use patch 2.5.9. 
After upgrading to Squeeze we will use 2.6. (in 2-3 weeks)

[Martin Robinson]

It seems the issue, at least in the case of the GTK+ EWS, is that some files are checked out with CRLF line endings and some with newlines. To ensure proper patch application, I believe that Unix machines need to be using newline line-endings for every file. There are a few knobs you can turn for git checkouts. Gustavo is looking into what the appropriate knob is.

[Eric Seidel (no email)]

I'm happy to set up any and all knobs on the commit bots.  Especially if they're documented in the WebKit Git instructions (on the wiki).

[Carol Szabo]

I am seeing issues on EWS with patching WebCore.vcproj on the mac based bots and GTK+.
Patching with non-matching line endings is a pain.
The way I addressed the issue in the EWS like systems that I built was partly by rules, partly by software:
The rules are:
1. All text files in the system would be required to have the svn:eol-style property set to native.
2. All files that appear as text files, but for whatever reason cannot have the svn:eol-style property set to native should have svn:mime-type property set to something like application/octet-stream.

The software would use flip or a similar tool to convert the line-endings in the patch to the configuration native to the architecture the buildbot runs in before application.

This should not be that hard to implement. I am volunteering to help.

[Gustavo Noronha (kov)]

I discussed my theory about this on IRC once, and just told dbates about it in an email reply, but thought I'd share it here. There is a .gitattributes file in our tree. It forces CRLF on some file types, including *.vcproj files:

$ grep crlf .gitattributes | wc -l
226
$ grep vcproj .gitattributes 
*.vcproj eol=crlf
$ 

Older checkouts (from before those rules being added) do not seem to suffer from the problem (i.e., my own checkout that runs on a system very much like the EWS bot works, as does Martin's). I'm also happy to apply any knobs, but reading docs and trying (not too much since my time's been quite limited) I didn't find anything that would guarantee the effect we want. Also, since it's not been mentioned explicitly in this bug yet, notice that the commit-queue suffers from the same issue - quite likely because it's got a new checkout itself.

[Daniel Bates]

(In reply to comment #9)
> I discussed my theory about this on IRC once, and just told dbates about it in an email reply, but thought I'd share it here. There is a .gitattributes file in our tree. It forces CRLF on some file types, including *.vcproj files:
> 
> $ grep crlf .gitattributes | wc -l
> 226
> $ grep vcproj .gitattributes 
> *.vcproj eol=crlf
> $ 
> 

Older versions of Git (e.g. 1.7.0.3) do not support the eol attribute. Instead, the line endings for checked out files are influenced by various factors, including the value of core.autocrlf.

From a brief check, our .vcproj files have svn:eol-style native. We should look to have both Git and SVN match. Additionally, we should either consider requiring a minimum version of Git (that support the eol attribute) and/or  teach svn-apply/svn-unapply how to compensate for older Git versions.

[David Levin]

Created attachment 88946 [details]
Patch

[Adam Barth]

Comment on attachment 88946 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88946&action=review

That's horrible, but also amazing.

> Tools/Scripts/svn-apply:335
> +            if (basename($fullPath) =~ "\.vcproj\$" || basename($fullPath) =~ "\.vsprops\$") {

I think there's some sneaky way of writing regular expressions in perl that avoids the need to \ the $.

[David Levin]

Comment on attachment 88946 [details]
Patch

Committed as http://trac.webkit.org/changeset/83394.

Also I found the way of not escaping the $ use // instead of "" (I don't speak perl colloquially.)

[Daniel Bates]

I know this patch has already been reviewed and landed in <http://trac.webkit.org/changeset/83394>.

I wanted to add some remarks. The patch is incorrect and will break svn-apply/unapply for all people with an SVN checkout on a Unix file system (this includes SVN under a default installation of Cygwin). This is because the Visual Studio project files have been checked into the repository with Unix line endings and have svn:eol-style: native. So, these files will have Unix new lines in an SVN checkout on a Unix file system.

Note, this patch will fix the issue for people who use Git version greater than 1.7.0.3. Since the commit/EWS bots seem to using this version of Git, this patch will resolve the issue. At the very least, there should be a FIXME comment.

[David Levin]

Reverted r83394 for reason:

Patch

Committed r83395: <http://trac.webkit.org/changeset/83395>

[Adam Barth]

It was foolish of me to review this patch without waiting for dbates.  I just got so excited about the prospect of fixing this bug.  Sorry Dan.

[David Levin]

(In reply to comment #14)
>  The patch is incorrect and will break svn-apply/unapply for all people with an SVN checkout on a Unix file system (this includes SVN under a default installation of Cygwin). ...

Thanks for the info. Rolled out. http://trac.webkit.org/changeset/83395

I suppose patch could be more sophisticated to look at the very first line ending of the file and use that to decided if we are dealing with DOS endings or not.

Would that work?

I really want to get this fixed as I hear about people hitting this often enough (and then I get pings to help land patches....)

[Daniel Bates]

(In reply to comment #17)
> (In reply to comment #14)
> >  The patch is incorrect and will break svn-apply/unapply for all people with an SVN checkout on a Unix file system (this includes SVN under a default installation of Cygwin). ...
> 
> Thanks for the info. Rolled out. http://trac.webkit.org/changeset/83395
> 
> I suppose patch could be more sophisticated to look at the very first line ending of the file and use that to decided if we are dealing with DOS endings or not.

I take it you mean svn-apply not patch. Yes, this is the approach I'm taking.

> 
> Would that work?

I believe it will work.

> 
> I really want to get this fixed as I hear about people hitting this often enough (and then I get pings to help land patches....)

I agree. For completeness, I am currently working on a fix for this issue and hope to post it tomorrow (04/10) / Monday. That being said, I would prefer we fix this as soon as possible. So, feel free to post an updated patch or a temporary workaround that we can use.

[David Levin]

(In reply to comment #18)
> I take it you mean svn-apply not patch. Yes, this is the approach I'm taking.
s/patch/my patch/ for my comment :)

[David Levin]

Created attachment 88947 [details]
Patch

[Daniel Bates]

Comment on attachment 88947 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88947&action=review

This patch is OK. That being said, we will also need to update svn-unapply and, if possible, add unit tests for this change. Towards this, we should look to extract the file EOL logic into a function in VCSUtils.pm so that it can be shared by both svn-apply/unapply.

I suggest that we add a FIXME comment above your change to indicate that we should add similar functionality to svn-unapply and/or extract some of this code and move it to VCSUtils.pm.

> Tools/Scripts/svn-apply:339
> +                # Think of it as escaping. Patch will convert \r\n to \n, so \r\r\n becomes \r\n.

Nit: Patch => Patch(1)

[David Levin]

Comment on attachment 88947 [details]
Patch

After discussing this more with Daniel, this patch would work ok, but has issues that should be addressed and he has a patch that would be more comprehensive (that should come on Monday and no one will probably hit this much during the weekend, so we'll ditch this version).

[Daniel Bates]

Created attachment 88978 [details]
Work-in-progress patch

Work-in-progress patch. Needs change log and some more unit tests. Will continue working on it today (04/11).

[Daniel Bates]

Created attachment 89055 [details]
Patch and unit tests

[WebKit Review Bot]

Attachment 89055 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/VCSUtils..." exit_code: 1

Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:66:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:67:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:74:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:75:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

(In reply to comment #25)
> [...]
> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:66:  Line contains tab character.  [whitespace/tab] [5]
> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:67:  Line contains tab character.  [whitespace/tab] [5]
> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:74:  Line contains tab character.  [whitespace/tab] [5]
> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:75:  Line contains tab character.  [whitespace/tab] [5]
> Total errors found: 4 in 5 files

The presence of these tab characters on these lines is expected since these lines are part of sample diff headers.

[Eric Seidel (no email)]

Comment on attachment 89055 [details]
Patch and unit tests

View in context: https://bugs.webkit.org/attachment.cgi?id=89055&action=review

> Tools/Scripts/VCSUtils.pm:454
> +sub parseEOL($)

Maybe parseFirstEOL?

> Tools/Scripts/VCSUtils.pm:872
> +sub parseDiff($$;$)

No clue what your fancy semi-colon does.

[Daniel Bates]

(In reply to comment #27)
> (From update of attachment 89055 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89055&action=review
> 
> > Tools/Scripts/VCSUtils.pm:454
> > +sub parseEOL($)
> 
> Maybe parseFirstEOL?

Will change.

> 
> > Tools/Scripts/VCSUtils.pm:872
> > +sub parseDiff($$;$)
> 
> No clue what your fancy semi-colon does.

As per <http://perldoc.perl.org/perlsub.html#Prototypes>: "A semicolon (;) separates mandatory arguments from optional arguments."

[Daniel Bates]

Created attachment 89081 [details]
Patch and unit tests

Renamed parseEOL() to parseFirstEOL() as suggested by Eric Seidel.

Also, renamed fileEOL() to firstEOLInFile() in VCSUtils.pm and parseEOLWithString() to firstEOLInString() in parseEOL.pl so as to be more descriptive.

[WebKit Review Bot]

Attachment 89081 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/VCSUtils..." exit_code: 1

Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:66:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:67:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:74:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:75:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 89082 [details]
Patch and unit tests

Renamed unit test file parseEOL.pl to parseFirstEOL.pl to reflect the name change of the function it's testing.

[WebKit Review Bot]

Attachment 89082 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/VCSUtils..." exit_code: 1

Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:66:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:67:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:74:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:75:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 89082 [details]
Patch and unit tests

I'm willing to rubber-stamp this.  If you need a more indepth review, you'll need to consult with Darin or Adam Roben or someone who speaks perl better than I.

[Daniel Bates]

Committed r83603: <http://trac.webkit.org/changeset/83603>