Bug 54918 - PrettyPatch.rb doesn't handle diffs where lines matching /^--/ are missing properly
Summary: PrettyPatch.rb doesn't handle diffs where lines matching /^--/ are missing pr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-21 17:34 PST by Dirk Pranke
Modified: 2011-02-21 19:45 PST (History)
2 users (show)

See Also:


Attachments
sample changed version of the test file that will cause PrettyPatch to crash (412 bytes, text/html)
2011-02-21 17:37 PST, Dirk Pranke
no flags Details
the diff produced by the test in the first attachment. PrettyPatch will crash on this directly. (961 bytes, text/plain)
2011-02-21 17:37 PST, Dirk Pranke
no flags Details
Patch (2.68 KB, patch)
2011-02-21 17:45 PST, Dirk Pranke
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-02-21 17:34:50 PST
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.

In the meantime, we could also just change the test and the expected output to not contain lines starting with "--" :). I'll attach a patch to do that as well.
Comment 1 Dirk Pranke 2011-02-21 17:37:06 PST
Created attachment 83244 [details]
sample changed version of the test file that will cause PrettyPatch to crash
Comment 2 Dirk Pranke 2011-02-21 17:37:33 PST
Created attachment 83245 [details]
the diff produced by the test in the first attachment. PrettyPatch will crash on this directly.
Comment 3 Dirk Pranke 2011-02-21 17:45:19 PST
Created attachment 83247 [details]
Patch
Comment 4 Ojan Vafai 2011-02-21 19:39:47 PST
Comment on attachment 83247 [details]
Patch

I'd eventually like to see prettypatch fixed. Can you file a separate bug for that and make this bug specifically about the layout test?
Comment 5 Dirk Pranke 2011-02-21 19:44:13 PST
(In reply to comment #4)
> (From update of attachment 83247 [details])
> I'd eventually like to see prettypatch fixed. Can you file a separate bug for that and make this bug specifically about the layout test?

Sure.
Comment 6 Dirk Pranke 2011-02-21 19:45:06 PST
Committed r79281: <http://trac.webkit.org/changeset/79281>