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.
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
Adam, do you know the pretty patch code well enough to identify what's going wrong here? I'm kinda scared to touch it.
Oh, I think I know what's going wrong. It's putting a blank line after the final newline in the diff.
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.
Created attachment 78639 [details] Patch
Comment on attachment 78639 [details] Patch I think we should fix this on the server side. aroben can help us.
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?
(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?
(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
(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.
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?
(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.
(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.
Created attachment 78747 [details] Patch
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...
Committed r75650: <http://trac.webkit.org/changeset/75650>