RESOLVED FIXED 45424
svn-apply tries to delete directories it shouldn't 
https://bugs.webkit.org/show_bug.cgi?id=45424
Summary svn-apply tries to delete directories it shouldn't
Mihai Parparita
Reported 2010-09-08 17:18:25 PDT
svn-apply tries to delete directories it shouldn't
Attachments
Patch (1.44 KB, patch)
2010-09-08 17:32 PDT, Mihai Parparita 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Mihai Parparita
Comment 1 2010-09-08 17:29:43 PDT
Eric, can you review this? This is why the EWS failed for bug 45307 (see output at https://webkit-commit-queue.appspot.com/results/3961246). It called isDirectoryEmptyForRemoval on LayoutTests/platform/mac/http. That only has one item in it, the tests subdirectory. We ended up calling scmWillDeleteFile("LayoutTests/platform/mac/http/tests"), which returned true since there were files deleted in it (and for the git case we just look for a leading D).
Mihai Parparita
Comment 2 2010-09-08 17:32:46 PDT
Created attachment 66971 [details] Patch
Eric Seidel (no email)
Comment 3 2010-09-09 12:11:56 PDT
Dan is the man.
Eric Seidel (no email)
Comment 4 2010-09-09 12:12:42 PDT
Comment on attachment 66971 [details] Patch We've been trying to unittest our perl code more. I'm not sure we have a way to test code which is still in svn-apply directly (instead of being in a seprate perl module), but Dan would know. What about svn-unapply?
Daniel Bates
Comment 5 2010-09-09 13:55:06 PDT
(In reply to comment #4) > (From update of attachment 66971 [details]) > We've been trying to unittest our perl code more. I'm not sure we have a way to test code which is still in svn-apply directly (instead of being in a seprate perl module), but Dan would know. What about svn-unapply? Unfortunately, we don't have a way to test code in svn-apply and svn-unapply directly at this time. This function is specific to svn-apply.
Daniel Bates
Comment 6 2010-09-09 13:55:34 PDT
Comment on attachment 66971 [details] Patch r=me.
Mihai Parparita
Comment 7 2010-09-09 13:58:33 PDT
(In reply to comment #6) > (From update of attachment 66971 [details]) > r=me. Thanks. Mind landing this (I don't have committer access yet)?
Daniel Bates
Comment 8 2010-09-09 14:00:27 PDT
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 66971 [details] [details]) > > r=me. > > Thanks. Mind landing this (I don't have committer access yet)? Sure.
Daniel Bates
Comment 9 2010-09-09 14:08:00 PDT
Comment on attachment 66971 [details] Patch Clearing flags on attachment: 66971 Committed r67115: <http://trac.webkit.org/changeset/67115>
Daniel Bates
Comment 10 2010-09-09 14:08:09 PDT
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 11 2010-09-09 14:28:40 PDT
You can also always set cq? :) Thanks dan for the info and landage!
Daniel Bates
Comment 12 2010-09-09 14:30:30 PDT
(In reply to comment #11) > You can also always set cq? :) Thanks dan for the info and landage! Yes, I normally would cq+ such a patch, but the queue looked pretty full at the time.
Note You need to log in before you can comment on or make changes to this bug.