RESOLVED FIXED Bug 54918
PrettyPatch.rb doesn't handle diffs where lines matching /^--/ are missing properly 
https://bugs.webkit.org/show_bug.cgi?id=54918
Summary PrettyPatch.rb doesn't handle diffs where lines matching /^--/ are missing pr...
Dirk Pranke
Reported 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.
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+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Dirk Pranke
Comment 1 2011-02-21 17:37:06 PST
Created attachment 83244 [details] sample changed version of the test file that will cause PrettyPatch to crash
Dirk Pranke
Comment 2 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.
Dirk Pranke
Comment 3 2011-02-21 17:45:19 PST
Created attachment 83247 [details] Patch
Ojan Vafai
Comment 4 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?
Dirk Pranke
Comment 5 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.
Dirk Pranke
Comment 6 2011-02-21 19:45:06 PST
Committed r79281: <http://trac.webkit.org/changeset/79281>
Note You need to log in before you can comment on or make changes to this bug.