51960 2011-01-05 15:05:29 -0800 review tool formatted diff doesn't match the uploaded diff 2011-01-12 15:49:10 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 ojan webkit-unassigned abarth aroben eric koz oldest_to_newest 329813 0 ojan 2011-01-05 15:05:29 -0800 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. 330308 1 ojan 2011-01-06 12:29:08 -0800 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 332853 2 ojan 2011-01-11 18:37:46 -0800 Adam, do you know the pretty patch code well enough to identify what's going wrong here? I'm kinda scared to touch it. 332855 3 ojan 2011-01-11 18:41:37 -0800 Oh, I think I know what's going wrong. It's putting a blank line after the final newline in the diff. 332857 4 ojan 2011-01-11 18:44:33 -0800 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. 332860 5 78639 ojan 2011-01-11 18:57:22 -0800 Created attachment 78639 Patch 332946 6 78639 abarth 2011-01-12 00:18:57 -0800 Comment on attachment 78639 Patch I think we should fix this on the server side. aroben can help us. 333160 7 aroben 2011-01-12 10:52:41 -0800 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? 333170 8 ojan 2011-01-12 11:04:43 -0800 (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? 333182 9 aroben 2011-01-12 11:18:09 -0800 (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 333185 10 ojan 2011-01-12 11:21:14 -0800 (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. 333187 11 ojan 2011-01-12 11:22:28 -0800 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? 333193 12 aroben 2011-01-12 11:34:08 -0800 (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. 333357 13 ojan 2011-01-12 15:29:31 -0800 (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. 333359 14 78747 ojan 2011-01-12 15:33:30 -0800 Created attachment 78747 Patch 333360 15 78747 abarth 2011-01-12 15:36:09 -0800 Comment on attachment 78747 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... 333374 16 ojan 2011-01-12 15:49:10 -0800 Committed r75650: <http://trac.webkit.org/changeset/75650> 78639 2011-01-11 18:57:22 -0800 2011-01-12 15:33:26 -0800 Patch bug-51960-20110111185721.patch text/plain 1874 ojan ZGlmZiAtLWdpdCBhL1dlYnNpdGVzL2J1Z3Mud2Via2l0Lm9yZy9DaGFuZ2VMb2cgYi9XZWJzaXRl cy9idWdzLndlYmtpdC5vcmcvQ2hhbmdlTG9nCmluZGV4IDhkOWRmOTVjZGEzYzllODNjOWM0ZDA0 Nzc1ZmZmODIwMDFmNWE5NGEuLjgyZDEwZGQ4OTU4YzkyOWU0ZDcwMDg4OWY4NjIyZjI2MzY1Yzg1 NzMgMTAwNjQ0Ci0tLSBhL1dlYnNpdGVzL2J1Z3Mud2Via2l0Lm9yZy9DaGFuZ2VMb2cKKysrIGIv V2Vic2l0ZXMvYnVncy53ZWJraXQub3JnL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE2IEBACiAyMDEx LTAxLTExICBPamFuIFZhZmFpICA8b2phbkBjaHJvbWl1bS5vcmc+CiAKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgcmV2aWV3IHRvb2wgZm9ybWF0dGVkIGRp ZmYgZG9lc24ndCBtYXRjaCB0aGUgdXBsb2FkZWQgZGlmZgorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTE5NjAKKworICAgICAgICBXb3JrYXJvdW5kIHRo ZSBidWcgb24gdGhlIEpTIHNpZGUuCisKKyAgICAgICAgKiBjb2RlLXJldmlldy5qczoKKworMjAx MS0wMS0xMSAgT2phbiBWYWZhaSAgPG9qYW5AY2hyb21pdW0ub3JnPgorCiAgICAgICAgIFJldmll d2VkIGJ5IEFkYW0gQmFydGguCiAKICAgICAgICAgZml4IGRpZmZsaW5rIGNlbnRlcmluZyB0byBi ZSB2ZXJ0aWNhbCwgbm90IGhvcml6b250YWwKZGlmZiAtLWdpdCBhL1dlYnNpdGVzL2J1Z3Mud2Vi a2l0Lm9yZy9jb2RlLXJldmlldy5qcyBiL1dlYnNpdGVzL2J1Z3Mud2Via2l0Lm9yZy9jb2RlLXJl dmlldy5qcwppbmRleCBlNmFlMjIwMGNhNGVmMzY3ODM5YzY0M2M0NzIxMzI5YjdmY2ZjOGFlLi4x NWRmNTFkN2I2MGJmOTliMDg1ZDZlYmUzZjUzOWMzNWEzNjhmMDMwIDEwMDY0NAotLS0gYS9XZWJz aXRlcy9idWdzLndlYmtpdC5vcmcvY29kZS1yZXZpZXcuanMKKysrIGIvV2Vic2l0ZXMvYnVncy53 ZWJraXQub3JnL2NvZGUtcmV2aWV3LmpzCkBAIC03MDcsOCArNzA3LDkgQEAKICAgfQogCiAgICQo ZG9jdW1lbnQpLnJlYWR5KGZ1bmN0aW9uKCkgewotICAgIGNyYXdsRGlmZigpOwogICAgIGZldGNo SGlzdG9yeSgpOworICAgIHJlbW92ZUV4dHJhbmVvdXNOZXdsaW5lKCk7CisgICAgY3Jhd2xEaWZm KCk7CiAgICAgJChkb2N1bWVudC5ib2R5KS5wcmVwZW5kKCc8ZGl2IGlkPSJtZXNzYWdlIj4nICsK ICAgICAgICAgJzxkaXYgY2xhc3M9ImhlbHAiPlNlbGVjdCBsaW5lIG51bWJlcnMgdG8gYWRkIGEg Y29tbWVudC4nICsKICAgICAgICAgICBkaWZmTGlua3NIdG1sKCdMaW5rQ29udGFpbmVyJykgKwpA QCAtNzQ0LDYgKzc0NSwxNyBAQAogICAgIGxvYWREaWZmU3RhdGUoKTsKICAgfSk7CiAKKyAgLy8g V29ya2Fyb3VuZCBhIGJ1ZyBpbiBQcmV0dHlQYXRjaC5yYiB3aGVyZSBpdCBpbmNsdWRlcyBhbiBl eHRyYSBsaW5lIGFmdGVyIHRoZQorICAvLyBmaW5hbCBuZXdsaW5lIGluIHRoZSBkaWZmLgorICBm dW5jdGlvbiByZW1vdmVFeHRyYW5lb3VzTmV3bGluZSgpIHsKKyAgICB2YXIgbGFzdCA9ICQoJy5M aW5lJykubGFzdCgpOworICAgIGlmICgkKCcudGV4dCcsIGxhc3QpLnRleHQoKSAhPSAnJykgewor ICAgICAgY29uc29sZS5sb2coJ0V4cGVjdGVkIGxhc3QgbGluZSB0byBiZSBlbXB0eSBhbmQgaXQg d2FzIG5vdC4nKTsKKyAgICAgIHJldHVybjsKKyAgICB9CisgICAgbGFzdC5kZXRhY2goKTsKKyAg fQorCiAgIGZ1bmN0aW9uIGxvYWREaWZmU3RhdGUoKSB7CiAgICAgdmFyIGRpZmZzdGF0ZSA9IGxv Y2FsU3RvcmFnZS5nZXRJdGVtKCdjb2RlLXJldmlldy1kaWZmc3RhdGUnKTsKICAgICBpZiAoZGlm ZnN0YXRlICE9ICdzaWRlYnlzaWRlJyAmJiBkaWZmc3RhdGUgIT0gJ3VuaWZpZWQnKQo= 78747 2011-01-12 15:33:30 -0800 2011-01-12 15:36:09 -0800 Patch bug-51960-20110112153329.patch text/plain 1395 ojan ZGlmZiAtLWdpdCBhL1dlYnNpdGVzL2J1Z3Mud2Via2l0Lm9yZy9DaGFuZ2VMb2cgYi9XZWJzaXRl cy9idWdzLndlYmtpdC5vcmcvQ2hhbmdlTG9nCmluZGV4IDZiNWY2NzhiMjFiOGQ2N2M0ZjI3ZTUz ZjQxMTBhNWE3NTJjY2QzZGUuLjdjNmI4ZDc3ODgzZGQzOWYwNDg4YzA3OWY1YTk4MWNkZTU1YWNl MjYgMTAwNjQ0Ci0tLSBhL1dlYnNpdGVzL2J1Z3Mud2Via2l0Lm9yZy9DaGFuZ2VMb2cKKysrIGIv V2Vic2l0ZXMvYnVncy53ZWJraXQub3JnL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE2IEBACiAyMDEx LTAxLTEyICBPamFuIFZhZmFpICA8b2phbkBjaHJvbWl1bS5vcmc+CiAKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgcmV2aWV3IHRvb2wgZm9ybWF0dGVkIGRp ZmYgZG9lc24ndCBtYXRjaCB0aGUgdXBsb2FkZWQgZGlmZgorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTE5NjAKKworICAgICAgICBQcmV0dHlQYXRjaCB3 b3JrcyBqdXN0IGZpbmUgaWYgdGhlcmUgaXMgbm8gbmV3bGluZSBhdCB0aGUgZW5kIG9mIHRoZSBm aWxlLgorCisgICAgICAgICogYXR0YWNobWVudC5jZ2k6CisKKzIwMTEtMDEtMTIgIE9qYW4gVmFm YWkgIDxvamFuQGNocm9taXVtLm9yZz4KKwogICAgICAgICBSZXZpZXdlZCBieSBBZGFtIEJhcnRo LgogCiAgICAgICAgIEJ1Z3ppbGxhOiBBZGQga2V5Ym9hcmQgc2hvcnRjdXRzIHRvIGp1bXAgdG8g bmV4dCBjaGFuZ2UKZGlmZiAtLWdpdCBhL1dlYnNpdGVzL2J1Z3Mud2Via2l0Lm9yZy9hdHRhY2ht ZW50LmNnaSBiL1dlYnNpdGVzL2J1Z3Mud2Via2l0Lm9yZy9hdHRhY2htZW50LmNnaQppbmRleCA5 MGMyNzlkYzRhNGRjMDc1NWZlNjQwZWEyN2RjYWQ5Yjg0NWE2MTMzLi4yNDI3YjU5YmEyY2NmNTgx OWYzOGVjN2NjYjE4YzgyNDc4NzJhMTE0IDEwMDc1NQotLS0gYS9XZWJzaXRlcy9idWdzLndlYmtp dC5vcmcvYXR0YWNobWVudC5jZ2kKKysrIGIvV2Vic2l0ZXMvYnVncy53ZWJraXQub3JnL2F0dGFj aG1lbnQuY2dpCkBAIC0zOTYsNyArMzk2LDcgQEAgc3ViIHByZXR0eVBhdGNoCiAgICAgJEVOVnsn UEFUSCd9ID0gIi9vcHQvbG9jYWwvYmluOiIgLiAkRU5WeydQQVRIJ307CiAgICAgb3BlbjIoXCpP VVQsIFwqSU4sICIvdXNyL2Jpbi9ydWJ5IiwgIi1JIiwgIlByZXR0eVBhdGNoIiwgIlByZXR0eVBh dGNoL3ByZXR0aWZ5LnJiIiwgIi0taHRtbC1leGNlcHRpb25zIik7CiAgICAgJEVOVnsnUEFUSCd9 ID0gJG9yaWdfcGF0aDsKLSAgICBwcmludCBJTiAkYXR0YWNobWVudC0+ZGF0YSAuICJcbiI7Cisg ICAgcHJpbnQgSU4gJGF0dGFjaG1lbnQtPmRhdGE7CiAgICAgY2xvc2UoSU4pOwogICAgIHdoaWxl ICg8T1VUPikgewogICAgICAgICBwcmludDsK