Bug 19290

Summary: More patches not handled by PrettyPatch.rb
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: Tools / TestsAssignee: Adam Roben (:aroben) <aroben>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
print exceptions from PrettyPatch to stdout
none
print exceptions from PrettyPatch to stdou
none
Fix most of the patches listed in this bug
none
Fix most of the patches listed in this bug
none
Fix remaining patches
none
Fix remaining patches v2 none

Attachments
print exceptions from PrettyPatch to stdout (3.20 KB, patch)
2008-05-28 08:40 PDT, Adam Roben (:aroben)
no flags
print exceptions from PrettyPatch to stdou (3.21 KB, patch)
2008-05-28 08:41 PDT, Adam Roben (:aroben)
no flags
Fix most of the patches listed in this bug (3.41 KB, patch)
2008-05-28 10:19 PDT, Adam Roben (:aroben)
no flags
Fix most of the patches listed in this bug (3.21 KB, patch)
2008-05-28 10:20 PDT, Adam Roben (:aroben)
no flags
Fix remaining patches (2.63 KB, patch)
2008-06-02 15:10 PDT, David Kilzer (:ddkilzer)
no flags
Fix remaining patches v2 (2.84 KB, patch)
2008-06-24 17:35 PDT, David Kilzer (:ddkilzer)
no flags
David Kilzer (:ddkilzer)
Comment 5 2008-05-28 07:58:06 PDT
Note that all patches listed in Comment #0 through Comment #4 (except Bug 18465) are from the same contributor.
Adam Roben (:aroben)
Comment 6 2008-05-28 08:40:22 PDT
Created attachment 21388 [details] print exceptions from PrettyPatch to stdout
Adam Roben (:aroben)
Comment 7 2008-05-28 08:41:56 PDT
Created attachment 21389 [details] print exceptions from PrettyPatch to stdou Fixed a typo
Adam Roben (:aroben)
Comment 8 2008-05-28 08:52:42 PDT
Comment on attachment 21389 [details] print exceptions from PrettyPatch to stdou Committed in r34173
Adam Roben (:aroben)
Comment 9 2008-05-28 08:55:52 PDT
It looks like attachment 20510 [details] and attachment 20528 [details] are missing the "Index: " line that PrettyPatch uses to find the start of the diff. All the other attachments are causing PrettyPatch trouble because they have a leading / in the path of the files. This should be easy to fix.
Adam Roben (:aroben)
Comment 10 2008-05-28 10:19:05 PDT
Created attachment 21392 [details] Fix most of the patches listed in this bug
Adam Roben (:aroben)
Comment 11 2008-05-28 10:20:53 PDT
Created attachment 21393 [details] Fix most of the patches listed in this bug Fixed a typo
David Kilzer (:ddkilzer)
Comment 12 2008-05-28 10:28:10 PDT
Comment on attachment 21393 [details] Fix most of the patches listed in this bug r=me!
Adam Roben (:aroben)
Comment 13 2008-05-28 10:33:04 PDT
Comment on attachment 21393 [details] Fix most of the patches listed in this bug Committed in r34175
Adam Roben (:aroben)
Comment 14 2008-05-28 10:33:48 PDT
The only remaining broken patches are: Attachment 20510 [details] Attachment 20528 [details]
David Kilzer (:ddkilzer)
Comment 15 2008-06-02 15:10:17 PDT
Created attachment 21469 [details] Fix remaining patches Fix the remaining issue for patches with no "Index" or "diff" headers.
David Kilzer (:ddkilzer)
Comment 16 2008-06-02 15:12:05 PDT
(In reply to comment #15) > Created an attachment (id=21469) [edit] > Fix remaining patches > > Fix the remaining issue for patches with no "Index" or "diff" headers. I should note that this change will NOT handle a patch with a mix of diffs that do and do not contain headers. I don't expect that case to ever happen, though.
Adam Roben (:aroben)
Comment 17 2008-06-03 09:20:44 PDT
Comment on attachment 21469 [details] Fix remaining patches I wonder if we should just always look for the "---" and "+++" lines instead of looking for "Index:" or "diff --git" lines? I think that's what the patch utility does anyway. It seems like it would simplify this code quite a bit. Care to take a stab at that, Dave?
David Kilzer (:ddkilzer)
Comment 18 2008-06-03 10:59:06 PDT
(In reply to comment #17) > (From update of attachment 21469 [details] [edit]) > I wonder if we should just always look for the "---" and "+++" lines instead of > looking for "Index:" or "diff --git" lines? I think that's what the patch > utility does anyway. It seems like it would simplify this code quite a bit. > > Care to take a stab at that, Dave? Yes, maybe today. Bug mail is awfully slow (or Y!Mail delivery has slowed way down).
Darin Adler
Comment 19 2008-06-08 14:03:57 PDT
Comment on attachment 21469 [details] Fix remaining patches r=me
David Kilzer (:ddkilzer)
Comment 20 2008-06-08 14:05:43 PDT
Comment on attachment 21469 [details] Fix remaining patches Sorry, Darin, I should have cleared the review? flag per Comment #17 and Comment #18.
David Kilzer (:ddkilzer)
Comment 21 2008-06-24 17:35:32 PDT
Created attachment 21918 [details] Fix remaining patches v2 (In reply to comment #17) > (From update of attachment 21469 [details] [edit]) > I wonder if we should just always look for the "---" and "+++" lines instead of > looking for "Index:" or "diff --git" lines? I think that's what the patch > utility does anyway. It seems like it would simplify this code quite a bit. > > Care to take a stab at that, Dave? I don't think ignoring the "Index:" and "diff --git" lines will work. There are some patches with an "Index:" line that don't have "---" or "+++" lines (like binary patches), so this approach won't work with those cases. I did manage to simplify the patch from the previous attempt, at least for the first two changes.
Adam Roben (:aroben)
Comment 22 2008-06-25 07:49:29 PDT
Comment on attachment 21918 [details] Fix remaining patches v2 r=me
David Kilzer (:ddkilzer)
Comment 23 2008-06-25 12:47:05 PDT
Remaining patches should have been fixed by: Committed revision 34797. But it doesn't appear that they were. :(
David Kilzer (:ddkilzer)
Comment 24 2008-06-25 12:47:26 PDT
Comment on attachment 21918 [details] Fix remaining patches v2 Clearing review flag per Comment #23.
David Kilzer (:ddkilzer)
Comment 25 2008-06-25 13:58:09 PDT
(In reply to comment #23) > Remaining patches should have been fixed by: > > Committed revision 34797. > > But it doesn't appear that they were. :( This was a server update issue. The issue is now resolved!
Note You need to log in before you can comment on or make changes to this bug.