Bug 54928 – PrettyPatch.rb doesn't handle diffs where lines matching /^--/ are missing properly

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

[Dirk Pranke]

This bug is cloned from bug 54918 - the test that was causing the problem has been patched, but this bug will track the root cause.

From bug 54918:
> LayoutTests/platform/mac/fast/objc/dom-html-select-activate.html has a few statements of the 
> form log("-- foo") in it, which causes a text line containing two dashes at the start of the line to be
> output in dumpAsText() mode.
>
> If the test fails in such a way that those lines aren't output (as it does on chromium-mac at the
> moment), then the unified diff contains a line with "---" at the start. 
>
> PrettyPatch.rb interprets that as the start of a new file for diffing, and gets confused when
> there's no filename, and errors out. 
>
> I think, in FileDiff.parse, it needs to be taught to understand the "@@ -1,26 +1,6 @@" line and
> skip over the appropriate number of lines of input correctly, rather than just looking for a line 
> containing "---". 
> My Ruby is weak, and I am lazy, and PrettyPatch appears to have no unit tests, so I haven't actually
> submitted a patch to do this. If, however, you change dom-html-select-activate to the contents
> of the first attachment, you'll get the diff in the second attachment, and old-run-webkit-tests
> will keel over with PrettyPatch failing.

[Dirk Pranke]

Created attachment 83258 [details]
modified version of the test file that will fail, producing the diff file in the next attachment that will cause PrettyPatch to crash.

[Dirk Pranke]

Created attachment 83259 [details]
diff file that will cause PrettyPatch to crash

[Dániel Bátyai]

Created attachment 219074 [details]
Proposed patch

[Dániel Bátyai]

Created attachment 219075 [details]
Proposed patch

[Adam Roben (:aroben)]

Comment on attachment 219075 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=219075&action=review

> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:665
> +            line_array = string.each_line.to_a

I believe it's more idiomatic to use string.lines.to_a here instead of string.each_line.to_a

[Dániel Bátyai]

Created attachment 219316 [details]
Proposed patch

[Adam Roben (:aroben)]

Comment on attachment 219316 [details]
Proposed patch

r=me

[WebKit Commit Bot]

Comment on attachment 219316 [details]
Proposed patch

Clearing flags on attachment: 219316

Committed r160642: <http://trac.webkit.org/changeset/160642>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.