Bug 51960

Summary:

review tool formatted diff doesn't match the uploaded diff

Product:

WebKit

Reporter:

Ojan Vafai <ojan>

Component:

Tools / Tests

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, aroben, eric, koz

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	none
Patch	abarth: review+

Ojan Vafai

Reported 2011-01-05 15:05:29 PST

https://bugs.webkit.org/attachment.cgi?id=78048&action=review doesn't match https://bug-51959-attachments.webkit.org/attachment.cgi?id=78048. The diff on code-review.js has an extra blank line at the end that doesn't exist in the diff or in the file in svn. Other than just being wrong, this also causes the expand logic to be unable to apply the diff.

Attachments
Patch (1.83 KB, patch) 2011-01-11 18:57 PST, Ojan Vafai	no flags	Details Formatted Diff Diff
Patch (1.36 KB, patch) 2011-01-12 15:33 PST, Ojan Vafai	abarth: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Ojan Vafai

Comment 1 2011-01-06 12:29:08 PST

Another case that hits this. The last diff on the page has an extra blank line that is not in the uploaded diff https://bugs.webkit.org/attachment.cgi?id=78143&action=review

Ojan Vafai

Comment 2 2011-01-11 18:37:46 PST

Adam, do you know the pretty patch code well enough to identify what's going wrong here? I'm kinda scared to touch it.

Ojan Vafai

Comment 3 2011-01-11 18:41:37 PST

Oh, I think I know what's going wrong. It's putting a blank line after the final newline in the diff.

Ojan Vafai

Comment 4 2011-01-11 18:44:33 PST

Maybe I should just fix this on the JS side then? I assume the problem is around here: http://trac.webkit.org/browser/trunk/Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb#L576.

Ojan Vafai

Comment 5 2011-01-11 18:57:22 PST

Created attachment 78639 [details] Patch

Adam Barth

Comment 6 2011-01-12 00:18:57 PST

Comment on attachment 78639 [details] Patch I think we should fix this on the server side. aroben can help us.

Adam Roben (:aroben)

Comment 7 2011-01-12 10:52:41 PST

Just running prettify.rb on the patch doesn't seem to result in the extra line being added. So maybe the JS is adding the extra line?

Ojan Vafai

Comment 8 2011-01-12 11:04:43 PST

(In reply to comment #7) > Just running prettify.rb on the patch doesn't seem to result in the extra line being added. So maybe the JS is adding the extra line? I don't think it's the JS. If I delete all the JS, it's still there. But yeah, I can verify that prettify.rb doesn't add the extra line. A problem on the bugzilla side maybe?

Adam Roben (:aroben)

Comment 9 2011-01-12 11:18:09 PST

(In reply to comment #8) > (In reply to comment #7) > > Just running prettify.rb on the patch doesn't seem to result in the extra line being added. So maybe the JS is adding the extra line? > > I don't think it's the JS. If I delete all the JS, it's still there. But yeah, I can verify that prettify.rb doesn't add the extra line. A problem on the bugzilla side maybe? Oooh: http://trac.webkit.org/browser/trunk/Websites/bugs.webkit.org/attachment.cgi?rev=75500#L399

Ojan Vafai

Comment 10 2011-01-12 11:21:14 PST

(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > Just running prettify.rb on the patch doesn't seem to result in the extra line being added. So maybe the JS is adding the extra line? > > > > I don't think it's the JS. If I delete all the JS, it's still there. But yeah, I can verify that prettify.rb doesn't add the extra line. A problem on the bugzilla side maybe? > > Oooh: http://trac.webkit.org/browser/trunk/Websites/bugs.webkit.org/attachment.cgi?rev=75500#L399 Lol. Nice find. Presumably we can just delete that line? It seems to only be used for the PrettyPatch code.

Ojan Vafai

Comment 11 2011-01-12 11:22:28 PST

Actually, I wonder if there are cases where uploaded patches don't end in a newline? We should probably change that code to add a newline only if it doesn't already end in one?

Adam Roben (:aroben)

Comment 12 2011-01-12 11:34:08 PST

(In reply to comment #11) > Actually, I wonder if there are cases where uploaded patches don't end in a newline? We should probably change that code to add a newline only if it doesn't already end in one? It should be easy to test what happens when there's no trailing newline. If PrettyPatch behaves itself then we can just get rid of the ' . "\n"' part of that line.

Ojan Vafai

Comment 13 2011-01-12 15:29:31 PST

(In reply to comment #12) > It should be easy to test what happens when there's no trailing newline. If PrettyPatch behaves itself then we can just get rid of the ' . "\n"' part of that line. It seems to work the same without the trailing newline in the patch.

Ojan Vafai

Comment 14 2011-01-12 15:33:30 PST

Created attachment 78747 [details] Patch

Adam Barth

Comment 15 2011-01-12 15:36:09 PST

Comment on attachment 78747 [details] Patch Maybe the buffer doesn't get flushed if there's no trailing newline? I'm trying to imagine the disaster scenarios... close() should flush though...

Ojan Vafai

Comment 16 2011-01-12 15:49:10 PST

Committed r75650: <http://trac.webkit.org/changeset/75650>

Note You need to log in before you can comment on or make changes to this bug.