Bug 51960 – review tool formatted diff doesn't match the uploaded diff

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

[Ojan Vafai]

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.

[Ojan Vafai]

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]

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]

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

[Ojan Vafai]

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]

Created attachment 78639 [details]
Patch

[Adam Barth]

Comment on attachment 78639 [details]
Patch

I think we should fix this on the server side.  aroben can help us.

[Adam Roben (:aroben)]

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]

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

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

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

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

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

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

Created attachment 78747 [details]
Patch

[Adam Barth]

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]

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