Bug 29100

Summary: svn-apply can't handle single-line binary file additions
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, commit-queue, darin, ddkilzer
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 28914, 29001    
Attachments:
Description Flags
reduced failing patch file
none
Patch v1
none
Patch v1
none
Better patch, now using non-greedy matching none

Eric Seidel (no email)
Reported 2009-09-09 11:54:38 PDT
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.
Attachments
reduced failing patch file (452 bytes, text/plain)
2009-09-09 11:58 PDT, Eric Seidel (no email) 		no flags
Details
Patch v1 (2.94 KB, patch)
2009-09-21 16:28 PDT, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
Patch v1 (2.94 KB, patch)
2009-09-21 16:38 PDT, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
Better patch, now using non-greedy matching (2.94 KB, patch)
2009-09-21 16:50 PDT, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2009-09-09 11:58:17 PDT
Created attachment 39287 [details] reduced failing patch file
Eric Seidel (no email)
Comment 2 2009-09-09 14:10:08 PDT
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)
Comment 3 2009-09-09 18:46:47 PDT
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)
Comment 4 2009-09-21 14:49:51 PDT
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)
Comment 5 2009-09-21 14:54:04 PDT
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)
Comment 6 2009-09-21 15:08:24 PDT
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)
Comment 7 2009-09-21 15:21:57 PDT
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)
Comment 8 2009-09-21 15:24:18 PDT
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)
Comment 9 2009-09-21 16:28:30 PDT
Created attachment 39892 [details] Patch v1
Eric Seidel (no email)
Comment 10 2009-09-21 16:32:55 PDT
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)
Comment 11 2009-09-21 16:38:45 PDT
Created attachment 39894 [details] Patch v1
Eric Seidel (no email)
Comment 12 2009-09-21 16:46:58 PDT
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)
Comment 13 2009-09-21 16:50:02 PDT
Created attachment 39895 [details] Better patch, now using non-greedy matching
Eric Seidel (no email)
Comment 14 2009-09-21 16:53:13 PDT
(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 15 2009-09-22 02:40:17 PDT
Comment on attachment 39895 [details] Better patch, now using non-greedy matching r=me
WebKit Commit Bot
Comment 16 2009-09-22 02:48:45 PDT
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
Comment 17 2009-09-22 02:48:50 PDT
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 18 2009-09-22 10:05:26 PDT
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. :)
Note You need to log in before you can comment on or make changes to this bug.