53569 2011-02-01 20:28:06 -0800 include svn revisions in git diffs for the code review tool to use 2011-02-07 20:30:13 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 1 ojan ojan abarth aroben eric oldest_to_newest 343800 0 ojan 2011-02-01 20:28:06 -0800 include svn revisions in git diffs for the code review tool to use 343801 1 80879 ojan 2011-02-01 20:28:51 -0800 Created attachment 80879 Patch 343802 2 80879 abarth 2011-02-01 20:36:17 -0800 Comment on attachment 80879 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80879&action=review The webkitpy parts look great. > Tools/Scripts/webkitpy/common/checkout/scm.py:751 > + num_tries = 0 nit: We prefer to use full words in variable names. 343803 3 abarth 2011-02-01 20:37:10 -0800 So, unrelated to this bug, but I'm going to spam it here anyway. When you load up a review, the page jumps down one line after a moment because it's filling in the "j/k" help message. Can we put that on the same line as the basic help message (and maybe gray it out also)? 343805 4 ojan 2011-02-01 20:39:18 -0800 (In reply to comment #3) > So, unrelated to this bug, but I'm going to spam it here anyway. When you load up a review, the page jumps down one line after a moment because it's filling in the "j/k" help message. Can we put that on the same line as the basic help message (and maybe gray it out also)? lol. That was your doing! Happy to fix that though. 343806 5 ojan 2011-02-01 20:39:40 -0800 aroben, you mind looking over the ruby side of this patch? 343814 6 abarth 2011-02-01 20:51:12 -0800 > lol. That was your doing! Happy to fix that though. Yeah, well... Um... Thanks. :) 343949 7 80879 aroben 2011-02-02 05:21:02 -0800 Comment on attachment 80879 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80879&action=review > Tools/Scripts/webkitpy/common/checkout/scm.py:758 > + while not revision and num_tries < 10: > + # If the git checkout is not tracking an SVN repo, then svn_revision_from_git_commit throws. > + try: > + revision = self.svn_revision_from_git_commit('HEAD~' + str(num_tries)) > + except: > + return diff > + num_tries += 1 Would using "git svn info" be faster than this loop? > Tools/Scripts/webkitpy/common/checkout/scm.py:763 > + return "SVN Revision: " + str(revision) + '\n' + diff Would "Subversion" be better than "SVN"? > Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:22 > + match = /^SVN\ Revision: (.*)$/.match(line) Would it be better to use \d instead of .? > Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:23 > + if (not match.nil?) I don't think you need the parentheses here. You can also just use "unless" instead of "if not". 347040 8 80879 ojan 2011-02-07 20:26:44 -0800 Comment on attachment 80879 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80879&action=review Thanks for the review. >> Tools/Scripts/webkitpy/common/checkout/scm.py:758 >> + num_tries += 1 > > Would using "git svn info" be faster than this loop? It certainly would be faster. But that doesn't necessarily tell us which svn revision this branch is merged to, right? >> Tools/Scripts/webkitpy/common/checkout/scm.py:763 > > Would "Subversion" be better than "SVN"? Done >> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:22 >> + match = /^SVN\ Revision: (.*)$/.match(line) > > Would it be better to use \d instead of .? yes! :) >> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:23 >> + if (not match.nil?) > > I don't think you need the parentheses here. You can also just use "unless" instead of "if not". crazy language. thx. 347042 9 ojan 2011-02-07 20:30:13 -0800 Committed r77889: <http://trac.webkit.org/changeset/77889> 80879 2011-02-01 20:28:51 -0800 2011-02-07 20:26:44 -0800 Patch bug-53569-20110202152849.patch text/plain 5334 ojan U1ZOIFJldmlzaW9uOiA3NzM0MApkaWZmIC0tZ2l0IGEvVG9vbHMvQ2hhbmdlTG9nIGIvVG9vbHMv Q2hhbmdlTG9nCmluZGV4IGQ1ZTFmNmI4Y2VlZjAzYjZhYzdmMDc5YzViYmE2ZmZkZDlhNjk0Mjcu LmQ1ZGZhZGI2ZDRhNTY0OWY1MGQyZDA0MjQ4NTYzZGFiYTBjYTYyYTYgMTAwNjQ0Ci0tLSBhL1Rv b2xzL0NoYW5nZUxvZworKysgYi9Ub29scy9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxMyBAQAorMjAx MS0wMi0wMSAgT2phbiBWYWZhaSAgPG9qYW5AY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIGluY2x1ZGUgc3ZuIHJldmlzaW9ucyBp biBnaXQgZGlmZnMgZm9yIHRoZSBjb2RlIHJldmlldyB0b29sIHRvIHVzZQorICAgICAgICBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTM1NjkKKworICAgICAgICAqIFNj cmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L3NjbS5weToKKyAgICAgICAgKiBTY3JpcHRz L3dlYmtpdHB5L2NvbW1vbi9jaGVja291dC9zY21fdW5pdHRlc3QucHk6CisKIDIwMTEtMDItMDEg IE1paGFpIFBhcnBhcml0YSAgPG1paGFpcEBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3 ZWQgYnkgSmFtZXMgUm9iaW5zb24uCmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5 L2NvbW1vbi9jaGVja291dC9zY20ucHkgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9j aGVja291dC9zY20ucHkKaW5kZXggNDIxYzBkY2ZlOGY2ZmYzM2FlMzcwNDg5MWM1OTVkMTlkZmZh NDFiYS4uYjhiZTY5MDc2NTczOTQwYjgwZjUzNzRmYjkxNWUyN2QzOGVjYjNmZSAxMDA2NDQKLS0t IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9jb21tb24vY2hlY2tvdXQvc2NtLnB5CisrKyBiL1Rv b2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L3NjbS5weQpAQCAtNzQ2LDYgKzc0 NiwyMiBAQCBjbGFzcyBHaXQoU0NNKToKICAgICBkZWYgZGlzcGxheV9uYW1lKHNlbGYpOgogICAg ICAgICByZXR1cm4gImdpdCIKIAorICAgIGRlZiBwcmVwZW5kX3N2bl9yZXZpc2lvbihzZWxmLCBk aWZmKToKKyAgICAgICAgcmV2aXNpb24gPSBOb25lCisgICAgICAgIG51bV90cmllcyA9IDAKKyAg ICAgICAgd2hpbGUgbm90IHJldmlzaW9uIGFuZCBudW1fdHJpZXMgPCAxMDoKKyAgICAgICAgICAg ICMgSWYgdGhlIGdpdCBjaGVja291dCBpcyBub3QgdHJhY2tpbmcgYW4gU1ZOIHJlcG8sIHRoZW4g c3ZuX3JldmlzaW9uX2Zyb21fZ2l0X2NvbW1pdCB0aHJvd3MuCisgICAgICAgICAgICB0cnk6Cisg ICAgICAgICAgICAgICAgcmV2aXNpb24gPSBzZWxmLnN2bl9yZXZpc2lvbl9mcm9tX2dpdF9jb21t aXQoJ0hFQUR+JyArIHN0cihudW1fdHJpZXMpKQorICAgICAgICAgICAgZXhjZXB0OgorICAgICAg ICAgICAgICAgIHJldHVybiBkaWZmCisgICAgICAgICAgICBudW1fdHJpZXMgKz0gMQorCisgICAg ICAgIGlmIG5vdCByZXZpc2lvbjoKKyAgICAgICAgICAgIHJldHVybiBkaWZmCisKKyAgICAgICAg cmV0dXJuICJTVk4gUmV2aXNpb246ICIgKyBzdHIocmV2aXNpb24pICsgJ1xuJyArIGRpZmYKKwog ICAgIGRlZiBjcmVhdGVfcGF0Y2goc2VsZiwgZ2l0X2NvbW1pdD1Ob25lLCBjaGFuZ2VkX2ZpbGVz PU5vbmUpOgogICAgICAgICAiIiJSZXR1cm5zIGEgYnl0ZSBhcnJheSAoc3RyKCkpIHJlcHJlc2Vu dGluZyB0aGUgcGF0Y2ggZmlsZS4KICAgICAgICAgUGF0Y2ggZmlsZXMgYXJlIGVmZmVjdGl2ZWx5 IGJpbmFyeSBzaW5jZSB0aGV5IG1heSBjb250YWluCkBAIC03NTMsNyArNzY5LDcgQEAgY2xhc3Mg R2l0KFNDTSk6CiAgICAgICAgIGNvbW1hbmQgPSBbJ2dpdCcsICdkaWZmJywgJy0tYmluYXJ5Jywg Ii0tbm8tZXh0LWRpZmYiLCAiLS1mdWxsLWluZGV4IiwgIi1NIiwgc2VsZi5tZXJnZV9iYXNlKGdp dF9jb21taXQpLCAiLS0iXQogICAgICAgICBpZiBjaGFuZ2VkX2ZpbGVzOgogICAgICAgICAgICAg Y29tbWFuZCArPSBjaGFuZ2VkX2ZpbGVzCi0gICAgICAgIHJldHVybiBzZWxmLnJ1bihjb21tYW5k LCBkZWNvZGVfb3V0cHV0PUZhbHNlLCBjd2Q9c2VsZi5jaGVja291dF9yb290KQorICAgICAgICBy ZXR1cm4gc2VsZi5wcmVwZW5kX3N2bl9yZXZpc2lvbihzZWxmLnJ1bihjb21tYW5kLCBkZWNvZGVf b3V0cHV0PUZhbHNlLCBjd2Q9c2VsZi5jaGVja291dF9yb290KSkKIAogICAgIGRlZiBfcnVuX2dp dF9zdm5fZmluZF9yZXYoc2VsZiwgYXJnKToKICAgICAgICAgIyBnaXQgc3ZuIGZpbmQtcmV2IGFs d2F5cyBleGl0cyAwLCBldmVuIHdoZW4gdGhlIHJldmlzaW9uIG9yIGNvbW1pdCBpcyBub3QgZm91 bmQuCmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9jaGVja291dC9z Y21fdW5pdHRlc3QucHkgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9jaGVja291dC9z Y21fdW5pdHRlc3QucHkKaW5kZXggNjQxMjJiNDI2ZGRmNDI3NTM2YzllNjRjZTRiN2U4YjJkNTY0 MzliOS4uZjhjNGMyMjExNDA2YzU2ODM3NGU3NTYyNDdkZjU4MWVmMzE4YmZhNSAxMDA2NDQKLS0t IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9jb21tb24vY2hlY2tvdXQvc2NtX3VuaXR0ZXN0LnB5 CisrKyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L3NjbV91bml0dGVz dC5weQpAQCAtODEwLDYgKzgxMCwxNiBAQCBjbGFzcyBHaXRUZXN0KFNDTVRlc3QpOgogICAgICAg ICBydW5fY29tbWFuZChbJ2dpdCcsICdjb25maWcnLCAnLS1hZGQnLCAnc3ZuLXJlbW90ZS5zdm4u ZmV0Y2gnLCAndHJ1bms6cmVtb3RlMiddKQogICAgICAgICBzZWxmLmFzc2VydEVxdWFsKHNlbGYu dHJhY2tpbmdfc2NtLnJlbW90ZV9icmFuY2hfcmVmKCksICdyZW1vdGUxJykKIAorICAgIGRlZiB0 ZXN0X2NyZWF0ZV9wYXRjaChzZWxmKToKKyAgICAgICAgd3JpdGVfaW50b19maWxlX2F0X3BhdGgo J3Rlc3RfZmlsZV9jb21taXQxJywgJ2NvbnRlbnRzJykKKyAgICAgICAgcnVuX2NvbW1hbmQoWydn aXQnLCAnYWRkJywgJ3Rlc3RfZmlsZV9jb21taXQxJ10pCisgICAgICAgIHNjbSA9IGRldGVjdF9z Y21fc3lzdGVtKHNlbGYudW50cmFja2luZ19jaGVja291dF9wYXRoKQorICAgICAgICBzY20uY29t bWl0X2xvY2FsbHlfd2l0aF9tZXNzYWdlKCdtZXNzYWdlJykKKworICAgICAgICBwYXRjaCA9IHNj bS5jcmVhdGVfcGF0Y2goKQorICAgICAgICBzZWxmLmFzc2VydEZhbHNlKHJlLnNlYXJjaChyJ1NW TiBSZXZpc2lvbjonLCBwYXRjaCkpCisKKwogY2xhc3MgR2l0U1ZOVGVzdChTQ01UZXN0KToKIAog ICAgIGRlZiBfc2V0dXBfZ2l0X2NoZWNrb3V0KHNlbGYpOgpAQCAtMTEyNiw2ICsxMTM2LDcgQEAg Y2xhc3MgR2l0U1ZOVGVzdChTQ01UZXN0KToKICAgICAgICAgcGF0Y2ggPSBzY20uY3JlYXRlX3Bh dGNoKCkKICAgICAgICAgc2VsZi5hc3NlcnRUcnVlKHJlLnNlYXJjaChyJ3Rlc3RfZmlsZV9jb21t aXQyJywgcGF0Y2gpKQogICAgICAgICBzZWxmLmFzc2VydFRydWUocmUuc2VhcmNoKHIndGVzdF9m aWxlX2NvbW1pdDEnLCBwYXRjaCkpCisgICAgICAgIHNlbGYuYXNzZXJ0VHJ1ZShyZS5zZWFyY2go cidTVk4gUmV2aXNpb246IDUnLCBwYXRjaCkpCiAKICAgICBkZWYgdGVzdF9jcmVhdGVfcGF0Y2hf d2l0aF9jaGFuZ2VkX2ZpbGVzKHNlbGYpOgogICAgICAgICBzZWxmLl9vbmVfbG9jYWxfY29tbWl0 X3BsdXNfd29ya2luZ19jb3B5X2NoYW5nZXMoKQpkaWZmIC0tZ2l0IGEvV2Vic2l0ZXMvYnVncy53 ZWJraXQub3JnL0NoYW5nZUxvZyBiL1dlYnNpdGVzL2J1Z3Mud2Via2l0Lm9yZy9DaGFuZ2VMb2cK aW5kZXggOWYwMWE3MGRiNGU1OWMxYjI0Y2FiOTUyYjQ2NTg2MjZmZGRjYTI5My4uOWE3YWZjNDA1 MjFiZGIwNTNhYTEyNjM1YTk3OGM4NDcxMjYwZGY1ZiAxMDA2NDQKLS0tIGEvV2Vic2l0ZXMvYnVn cy53ZWJraXQub3JnL0NoYW5nZUxvZworKysgYi9XZWJzaXRlcy9idWdzLndlYmtpdC5vcmcvQ2hh bmdlTG9nCkBAIC0xLDUgKzEsMTQgQEAKIDIwMTEtMDItMDEgIE9qYW4gVmFmYWkgIDxvamFuQGNo cm9taXVtLm9yZz4KIAorICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBpbmNsdWRlIHN2biByZXZpc2lvbnMgaW4gZ2l0IGRpZmZzIGZvciB0aGUgY29kZSByZXZp ZXcgdG9vbCB0byB1c2UKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTUzNTY5CisKKyAgICAgICAgKiBQcmV0dHlQYXRjaC9QcmV0dHlQYXRjaC5yYjoKKwor MjAxMS0wMi0wMSAgT2phbiBWYWZhaSAgPG9qYW5AY2hyb21pdW0ub3JnPgorCiAgICAgICAgIFJl dmlld2VkIGJ5IE1paGFpIFBhcnBhcml0YS4KIAogICAgICAgICBbY29kZXJldmlld3Rvb2xdIGV4 dHJhIHNwYWNlIGF0IHRoZSBiZWdpbm5pbmcgb2YgbGluZXMgaW4gc2lkZS1ieS1zaWRlIHZpZXcK ZGlmZiAtLWdpdCBhL1dlYnNpdGVzL2J1Z3Mud2Via2l0Lm9yZy9QcmV0dHlQYXRjaC9QcmV0dHlQ YXRjaC5yYiBiL1dlYnNpdGVzL2J1Z3Mud2Via2l0Lm9yZy9QcmV0dHlQYXRjaC9QcmV0dHlQYXRj aC5yYgppbmRleCA1NTNkZGVkNmUzZGFiNjM3NTUwY2U4YmI4NjIzNzUxZjNlNTI4MmE5Li5lODkw MTE1OTJiMGFkZDgyNGVhODU2MWQzOWNkNDUwMDE2YWQxMGEzIDEwMDY0NAotLS0gYS9XZWJzaXRl cy9idWdzLndlYmtpdC5vcmcvUHJldHR5UGF0Y2gvUHJldHR5UGF0Y2gucmIKKysrIGIvV2Vic2l0 ZXMvYnVncy53ZWJraXQub3JnL1ByZXR0eVBhdGNoL1ByZXR0eVBhdGNoLnJiCkBAIC0xNSw2ICsx NSwxNyBAQCBwdWJsaWMKICAgICAgICAgZmlsZURpZmZzID0gRmlsZURpZmYucGFyc2Uoc3RyaW5n KQogCiAgICAgICAgIHN0ciA9IEhFQURFUiArICJcbiIKKworICAgICAgICAjIEp1c3QgbG9vayBh dCB0aGUgZmlyc3QgbGluZSB0byBzZWUgaWYgaXQgaXMgYW4gU1ZOIHJldmlzaW9uIG51bWJlciBh cyBhZGRlZAorICAgICAgICAjIGJ5IHdlYmtpdC1wYXRjaCBmb3IgZ2l0IGNoZWNrb3V0cy4KKyAg ICAgICAgc3RyaW5nLmVhY2hfbGluZSBkbyB8bGluZXwKKyAgICAgICAgICAgIG1hdGNoID0gL15T Vk5cIFJldmlzaW9uOiAoLiopJC8ubWF0Y2gobGluZSkKKyAgICAgICAgICAgIGlmIChub3QgbWF0 Y2gubmlsPykKKyAgICAgICAgICAgICAgc3RyICs9ICI8c3BhbiBjbGFzcz0ncmV2aXNpb24nPiIg KyBtYXRjaFsxXSArICI8L3NwYW4+IgorICAgICAgICAgICAgZW5kCisgICAgICAgICAgICBicmVh aworICAgICAgICBlbmQKKwogICAgICAgICBzdHIgKz0gZmlsZURpZmZzLmNvbGxlY3R7IHxkaWZm fCBkaWZmLnRvX2h0bWwgfS5qb2luCiAgICAgZW5kCiAK