Bug 19290 – More patches not handled by PrettyPatch.rb

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[David Kilzer (:ddkilzer)]

More patches that aren't handled by PrettyPatch.rb:

Bug 19041
https://bugs.webkit.org/attachment.cgi?id=21119&action=prettypatch

Bug 18465
https://bugs.webkit.org/attachment.cgi?id=20510&action=prettypatch
https://bugs.webkit.org/attachment.cgi?id=20528&action=prettypatch

[David Kilzer (:ddkilzer)]

Bug 19043
https://bugs.webkit.org/attachment.cgi?id=21120&action=prettypatch
https://bugs.webkit.org/attachment.cgi?id=21121&action=prettypatch

[David Kilzer (:ddkilzer)]

Bug 19067
https://bugs.webkit.org/attachment.cgi?id=21151&action=prettypatch

[David Kilzer (:ddkilzer)]

Bug 19068
https://bugs.webkit.org/attachment.cgi?id=21152&action=prettypatch

[David Kilzer (:ddkilzer)]

Bug 19069
https://bugs.webkit.org/attachment.cgi?id=21153&action=prettypatch

[David Kilzer (:ddkilzer)]

Note that all patches listed in Comment #0 through Comment #4 (except Bug 18465) are from the same contributor.

[Adam Roben (:aroben)]

Created attachment 21388 [details]
print exceptions from PrettyPatch to stdout

[Adam Roben (:aroben)]

Created attachment 21389 [details]
print exceptions from PrettyPatch to stdou

Fixed a typo

[Adam Roben (:aroben)]

Comment on attachment 21389 [details]
print exceptions from PrettyPatch to stdou

Committed in r34173

[Adam Roben (:aroben)]

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)]

Created attachment 21392 [details]
Fix most of the patches listed in this bug

[Adam Roben (:aroben)]

Created attachment 21393 [details]
Fix most of the patches listed in this bug

Fixed a typo

[David Kilzer (:ddkilzer)]

Comment on attachment 21393 [details]
Fix most of the patches listed in this bug

r=me!

[Adam Roben (:aroben)]

Comment on attachment 21393 [details]
Fix most of the patches listed in this bug

Committed in r34175

[Adam Roben (:aroben)]

The only remaining broken patches are:

Attachment 20510 [details]
Attachment 20528 [details]

[David Kilzer (:ddkilzer)]

Created attachment 21469 [details]
Fix remaining patches

Fix the remaining issue for patches with no "Index" or "diff" headers.

[David Kilzer (:ddkilzer)]

(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 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)]

(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 on attachment 21469 [details]
Fix remaining patches

r=me

[David Kilzer (:ddkilzer)]

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)]

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 on attachment 21918 [details]
Fix remaining patches v2

r=me

[David Kilzer (:ddkilzer)]

Remaining patches should have been fixed by:

Committed revision 34797.

But it doesn't appear that they were.  :(

[David Kilzer (:ddkilzer)]

Comment on attachment 21918 [details]
Fix remaining patches v2

Clearing review flag per Comment #23.

[David Kilzer (:ddkilzer)]

(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!