Bug 115281 – WebKit tools cannot apply git patches for filenames containing spaces

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Tools / Tests

[Simon Cooper]

The tools cannot handle patches for files that have spaces in their names.

See here for a patch that fails to apply...

https://bug-115142-attachments.webkit.org/attachment.cgi?id=199861

It was trying to patch the file,

Source/WebKit2/Resources/PlugInSandboxProfiles/com.macromedia.Flash Player.plugin.sb

To see the tool failure...

    https://webkit-queues.appspot.com/patch/199570

You can see the output here....

https://webkit-queues.appspot.com/results/34412
https://webkit-queues.appspot.com/results/191141
https://webkit-queues.appspot.com/results/108267
https://webkit-queues.appspot.com/results/80803
https://webkit-queues.appspot.com/results/66244
https://webkit-queues.appspot.com/results/97422
https://webkit-queues.appspot.com/results/73329
https://webkit-queues.appspot.com/results/108270
https://webkit-queues.appspot.com/results/108276

[Alexey Proskuryakov]

Looks like this is an issue with git patches, but not with svn ones.

[Alexey Proskuryakov]

The script that needs to be fixed in likely Tools/Scripts/svn-apply.

[Simon Cooper]

There were two files that had spaces in their names in this patch...

Source/WebKit2/Resources/PlugInSandboxProfiles/com.macromedia.QuckTime Plugin.plugin.sb
Source/WebKit2/Resources/PlugInSandboxProfiles/com.macromedia.Flash Player.plugin.sb

It was the first one that appears in the logs for the failing patch application.

[Simon Cooper]

What does a svn patch look like then?

How is it different from the git patch?

[Alexey Proskuryakov]

Created attachment 199867 [details]
An example of svn patch

Attaching the same patch, re-made with svn.

[Mark Rowe (bdash)]

The difficulty is in parsing this line:

diff --git a/Source/WebKit2/Resources/PlugInSandboxProfiles/com.apple.QuickTime Plugin.plugin.sb b/Source/WebKit2/Resources/PlugInSandboxProfiles/com.apple.QuickTime Plugin.plugin.sb

There's no obvious delimiter between the two filenames.

[Simon Cooper]

There is a faulty regex:  $gitDiffStartRegEx. 

It is not sufficient to parse arguments containing spaces.  The logic in parseGitDiffHeader() doesn’t help, because it assumes the regex actually can parse all filenames.  That isn’t how it is done for SVN.

There are perfectly good and easily parseable filenames on the line following the “diff” line.

However, the following diff will parse my patch (although there may be issues with other tools) — I just debugged this to the point where it can parse the diffs.

diff --git a/Tools/Scripts/VCSUtils.pm b/Tools/Scripts/VCSUtils.pm
index cf87db3..fb6380a 100644
--- a/Tools/Scripts/VCSUtils.pm
+++ b/Tools/Scripts/VCSUtils.pm
@@ -107,7 +107,7 @@ my $svnVersion;
 # Project time zone for Cupertino, CA, US
 my $changeLogTimeZone = "PST8PDT";
 
-my $gitDiffStartRegEx = qr#^diff --git (\w/)?(.+) (\w/)?([^\r\n]+)#;
+my $gitDiffStartRegEx = qr#^diff --git (\w/)(.+) (\w/)([^\r\n]+?)$#;
 my $svnDiffStartRegEx = qr#^Index: ([^\r\n]+)#;
 my $svnPropertiesStartRegEx = qr#^Property changes on: ([^\r\n]+)#; # $1 is normally the same as the index path.
 my $svnPropertyStartRegEx = qr#^(Modified|Name|Added|Deleted): ([^\r\n]+)#; # $2 is the name of the property.


The fix works by making sure the starting a/ or b/ are not optional, the 2nd regex is *not* greedy and the whole regex needs to be terminally anchored (to force the consumption of the whole string).

[Mark Rowe (bdash)]

The leading a/ b/ were intentionally made optional back in r58741 / bug 32438.

[Simon Cooper]

Then parseGitDiffHeader() has to be rewritten to get the filenames from the ^--- and ^+++ lines.

[Simon Cooper]

svn-apply also mangles git patches in a way that “patch” cannot apply them to files with spaces.  It is not clear what is going on, but patch does do something special when there is an “Index:” line.

I’m giving up on looking at this.

[Alexey Proskuryakov]

Created attachment 201923 [details]
proposed fix

I don't really know what I'm doing here (neither Perl nor the structure of patches). Please review carefully.

I think that the trick here was that +++ and --- lines are really of this format:

--- Source/WebKit2/Resources/PlugInSandboxProfiles/com.macromedia.Flash Player.plugin.sb<TAB>(revision 149145)

When there are no spaces, a path alone suffices, but if there are spaces, at least the tab is desired.

[Alexey Proskuryakov]

Created attachment 201924 [details]
proposed patch

Actually, looks like git patches do the same thing with tabs, so let's preserve these more carefully.

[David Kilzer (:ddkilzer)]

Comment on attachment 201924 [details]
proposed patch

I'm overriding darin's r+ with an r- because this doesn't have a test added for the changes.

Tests for this method are in Tools/Scripts/webkitperl/VCSUtils_unittest/parseGitDiffHeader.pl.

To run the tests, use:  ./Tools/Scripts/test-webkitperl

[Alexey Proskuryakov]

I didn't know that we had secret regression tests for this tool! Looks like the patch is not good.

Svn and git "+++" and "---" lines are different - looks like one likes to put /dev/null there when adding or deleting files, and another does not. Unsure how to fix.

[Alexey Proskuryakov]

<rdar://problem/13925627>

[Daniel Bates]

*** This bug has been marked as a duplicate of bug 111066 ***