Bug 29100 – svn-apply can't handle single-line binary file additions

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

[Eric Seidel (no email)]

REGRESSION: svn-apply exit(0) on patch failure

This is really bad, and means the commit-queue could land partial patches.  I think I will need to stop the queue until this is fixed.

Example:
> curl --location "https://bugs.webkit.org/attachment.cgi?id=39280" | svn-apply 
patching file LayoutTests/ChangeLog
patching file LayoutTests/plugins/iframe-shims-expected.txt
patching file LayoutTests/plugins/iframe-shims.html
fatal: pathspec 'LayoutTests/plugins/resources/simple_blank.swf' did not match any files
> echo $?
0

It's possible this is not a regression and that my fix in http://trac.webkit.org/changeset/45153 was incomplete.

It's also possible this is a regression from http://trac.webkit.org/changeset/48027.

Will investigate.

[Eric Seidel (no email)]

Created attachment 39287 [details]
reduced failing patch file

[Eric Seidel (no email)]

Ok, looks like it's hitting the scmRemove case.  This is going down the handleBinaryChange codepath but:
    if ($contents =~ m#((\n[A-Za-z0-9+/]{76})+\n[A-Za-z0-9+/=]{4,76}\n)#) {
must be failing.

Probably because this has a Property change before the binary output.  I wonder if this is a valid patch file from svn-create-patch.

[Eric Seidel (no email)]

Hit this again trying to land attachment 29065 [details].  I'll try rolling out r48027 and see if it's related.

[Eric Seidel (no email)]

This reproduced for me by adding a binary file to my local svn checkout and running svn-apply:

> cd WebKitSVN
> curl -O "http://www.google.com/logos/hgwells09.gif"
> svn add hgwells09.gif
> svn-create-patch > test.patch
> svn-unapply test.patch
Reverted 'hgwells09.gif'

> svn-apply test.patch
svn: 'hgwells09.gif' does not exist

Bad news bears.

[Eric Seidel (no email)]

I reverted my WebKitTools/Scripts directory to r42053 and the problem went away. :)  So this is definitely a recent regression.  Investigating.

[Eric Seidel (no email)]

OK.  I've further reduced the issue.  My testing was in error before, I used a large .gif file for testing old revisions and a small one for new revisions.  It turns out that this issue is related to the size of the binary file rather than the revision of svn-apply/svn-create-patch we're using.

Still investigating.

[Eric Seidel (no email)]

Yeah, I expect it's newline troubles with:
    if ($contents =~ m#((\n[A-Za-z0-9+/]{76})+\n[A-Za-z0-9+/=]{4,76}\n)#) {

[Eric Seidel (no email)]

Yeah, the regexp simply fails for something as short as:

"Q1dTBx0AAAB42itg4GlgYJjGwMDDyODMxMDw34GBgQEAJPQDJA=="

if ($contents =~ m#((\n[A-Za-z0-9+/]{76})+\n[A-Za-z0-9+/=]{4,76}\n)#) {

Clearly expects multiple lines.  I'm not quite sure what the regexp should be changed to yet.

[Eric Seidel (no email)]

Created attachment 39892 [details]
Patch v1

[Eric Seidel (no email)]

This patch fixes the application failure we were seeing, but does not fix the general issue of svn-apply exiting 0 when it should not.  I've filed bug 29622 to track the latter.

[Eric Seidel (no email)]

Created attachment 39894 [details]
Patch v1

[Eric Seidel (no email)]

Actually, I'm not confident in these regexp changes.  I'm not sure I like the design of guessing based on content.  Assuming we're entering this path only for binary patch fragments, then this is OK, otherwise we'd match adding a file with just "me" as a base64 file.

[Eric Seidel (no email)]

Created attachment 39895 [details]
Better patch, now using non-greedy matching

[Eric Seidel (no email)]

(In reply to comment #12)
> Actually, I'm not confident in these regexp changes.  I'm not sure I like the
> design of guessing based on content.  Assuming we're entering this path only
> for binary patch fragments, then this is OK, otherwise we'd match adding a file
> with just "me" as a base64 file.

Looks like we only enter this for patch fragments marked as binary, so this should be OK.
        if ($isBinary) {
            # Binary change
            handleBinaryChange($fullPath, $patch);

[David Kilzer (:ddkilzer)]

Comment on attachment 39895 [details]
Better patch, now using non-greedy matching

r=me

[WebKit Commit Bot]

Comment on attachment 39895 [details]
Better patch, now using non-greedy matching

Clearing flags on attachment: 39895

Committed r48622: <http://trac.webkit.org/changeset/48622>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Thanks Dave!  And thanks for setting cq+ even though I forgot to set cq?  I need to add a --commit-queue option to post-diff so I don't have to remember each time. :)