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.
Created attachment 39287 [details] reduced failing patch file
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.
Hit this again trying to land attachment 29065 [details]. I'll try rolling out r48027 and see if it's related.
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.
I reverted my WebKitTools/Scripts directory to r42053 and the problem went away. :) So this is definitely a recent regression. Investigating.
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.
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)#) {
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.
Created attachment 39892 [details] Patch v1
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.
Created attachment 39894 [details] Patch v1
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.
Created attachment 39895 [details] Better patch, now using non-greedy matching
(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);
Comment on attachment 39895 [details] Better patch, now using non-greedy matching r=me
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>
All reviewed patches have been landed. Closing bug.
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. :)