| Summary: | More patches not handled by PrettyPatch.rb | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> |
| Component: | Tools / Tests | Assignee: | Adam Roben (:aroben) <aroben> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | aroben |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | All | ||
| OS: | All | ||
| Attachments: | |||
|
Description
David Kilzer (:ddkilzer)
2008-05-28 07:14:12 PDT
Bug 19043 https://bugs.webkit.org/attachment.cgi?id=21120&action=prettypatch https://bugs.webkit.org/attachment.cgi?id=21121&action=prettypatch Note that all patches listed in Comment #0 through Comment #4 (except Bug 18465) are from the same contributor. Created attachment 21388 [details]
print exceptions from PrettyPatch to stdout
Created attachment 21389 [details]
print exceptions from PrettyPatch to stdou
Fixed a typo
Comment on attachment 21389 [details] print exceptions from PrettyPatch to stdou Committed in r34173 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. Created attachment 21392 [details]
Fix most of the patches listed in this bug
Created attachment 21393 [details]
Fix most of the patches listed in this bug
Fixed a typo
Comment on attachment 21393 [details]
Fix most of the patches listed in this bug
r=me!
Comment on attachment 21393 [details] Fix most of the patches listed in this bug Committed in r34175 The only remaining broken patches are: Attachment 20510 [details] Attachment 20528 [details] Created attachment 21469 [details]
Fix remaining patches
Fix the remaining issue for patches with no "Index" or "diff" headers.
(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. 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?
(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). Comment on attachment 21469 [details]
Fix remaining patches
r=me
Comment on attachment 21469 [details] Fix remaining patches Sorry, Darin, I should have cleared the review? flag per Comment #17 and Comment #18. 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. Comment on attachment 21918 [details]
Fix remaining patches v2
r=me
Remaining patches should have been fixed by: Committed revision 34797. But it doesn't appear that they were. :( Comment on attachment 21918 [details] Fix remaining patches v2 Clearing review flag per Comment #23. (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! |