64115 2011-07-07 12:00:45 -0700 REGRESSION (r90564): webkitpy's Checkout class uses Executive inappropriately 2011-07-07 12:15:32 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 aroben aroben abarth eric oldest_to_newest 433634 0 aroben 2011-07-07 12:00:45 -0700 REGRESSION (r90564): webkitpy's Checkout class uses Executive inappropriately 433641 1 100015 aroben 2011-07-07 12:08:14 -0700 Created attachment 100015 Make Checkout use SCM's Executive instead of conjuring up its own 433643 2 100015 abarth 2011-07-07 12:10:42 -0700 Comment on attachment 100015 Make Checkout use SCM's Executive instead of conjuring up its own Thanks Adam. 433644 3 aroben 2011-07-07 12:14:14 -0700 Committed r90584: <http://trac.webkit.org/changeset/90584> 433645 4 100015 eric 2011-07-07 12:15:32 -0700 Comment on attachment 100015 Make Checkout use SCM's Executive instead of conjuring up its own View in context: https://bugs.webkit.org/attachment.cgi?id=100015&action=review > Tools/Scripts/webkitpy/common/checkout/checkout.py:123 > + message_text = self._scm.run([self._scm.script_path('commit-log-editor'), '--print-log'] + changelog_paths, return_stderr=False) SCM's run may do more than you want... like setting the cwd. But should be OK. I would have probably used self._scm._executive directly (even though grabbing at _ members is ugly too). Part of the trouble here is that scm.py is some of the vey very oldest code in webkitpy, and is poorly tested and factored. Checkout is pretty old itself. We've gotten better at this whole mocking thing since then. Thanks again for the fix. > Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:128 > + def mock_run(*args, **kwargs): > + # Note that we use a real Executive here, not a MockExecutive, so we can test that we're > + # invoking commit-log-editor correctly. > + return Executive().run_command(*args, **kwargs) > + I suspect what we wanted to do was make Checkout hold an self.executive pointer. But we can do that later when we go clean up the rest of Checkout too. 100015 2011-07-07 12:08:14 -0700 2011-07-07 12:15:32 -0700 Make Checkout use SCM's Executive instead of conjuring up its own bug-64115-20110707150812.patch text/plain 4178 aroben U3VidmVyc2lvbiBSZXZpc2lvbjogOTA1ODMKZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBi L1Rvb2xzL0NoYW5nZUxvZwppbmRleCA5MWYwN2VjNjQ3ZDU5MTAxZDYzZmI4Y2Y2ZGFjNzkxYWNj MTM1YWFlLi4xODkyZWYzMjJjYjkzOWY5MDQ4Yjc5MTg5MjNmNWNlYTMzMzk2NGM2IDEwMDY0NAot LS0gYS9Ub29scy9DaGFuZ2VMb2cKKysrIGIvVG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMjYg QEAKIDIwMTEtMDctMDcgIEFkYW0gUm9iZW4gIDxhcm9iZW5AYXBwbGUuY29tPgogCisgICAgICAg IE1ha2UgQ2hlY2tvdXQgdXNlIFNDTSdzIEV4ZWN1dGl2ZSBpbnN0ZWFkIG9mIGNvbmp1cmluZyB1 cCBpdHMgb3duCisKKyAgICAgICAgVGhpcyB3aWxsIGltcHJvdmUgaW50ZWdyYXRpb24gd2l0aCB0 aGUgcmVzdCBvZiB3ZWJraXRweSwgcGFydGljdWxhcmx5IHdoZW4gaW52b2tlZCB2aWEKKyAgICAg ICAgd2Via2l0LXBhdGNoLgorCisgICAgICAgIEZpeGVzIDxodHRwOi8vd2Via2l0Lm9yZy9iLzY0 MTE1PiBSRUdSRVNTSU9OIChyOTA1NjQpOiB3ZWJraXRweSdzIENoZWNrb3V0IGNsYXNzIHVzZXMK KyAgICAgICAgRXhlY3V0aXZlIGluYXBwcm9wcmlhdGVseQorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS9jb21tb24vY2hl Y2tvdXQvY2hlY2tvdXQucHk6CisgICAgICAgIChDaGVja291dC5jb21taXRfbWVzc2FnZV9mb3Jf dGhpc19jb21taXQpOiBVc2UgU0NNLnJ1biBpbnN0ZWFkIG9mIGNyZWF0aW5nIGFuIEV4ZWN1dGl2 ZSBmb3IKKyAgICAgICAgb3VyIG93biB1c2UuIFNDTSBtaWdodCBoYXZlIHNvbWUgc3VwZXItc3Bl Y2lhbCBFeGVjdXRpdmUgdGhhdCBpdCB1c2VzIHVuZGVyIHRoZSBjb3ZlcnMsIGFuZAorICAgICAg ICB3ZSB3YW50IHRvIHVzZSBpdCwgdG9vIQorCisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS9j b21tb24vY2hlY2tvdXQvY2hlY2tvdXRfdW5pdHRlc3QucHk6CisgICAgICAgIChDb21taXRNZXNz YWdlRm9yVGhpc0NvbW1pdFRlc3QudGVzdF9jb21taXRfbWVzc2FnZV9mb3JfdGhpc19jb21taXQp OiBNb2NrIHRoZSBTQ00ucnVuCisgICAgICAgIG1ldGhvZCB0byBjYWxsIHRocm91Z2ggdG8gRXhl Y3V0aXZlLnJ1bl9jb21tYW5kLgorCisyMDExLTA3LTA3ICBBZGFtIFJvYmVuICA8YXJvYmVuQGFw cGxlLmNvbT4KKwogICAgICAgICBNYWtlIFRlcm0vUmVhZEtleS5wbSBhbiBvcHRpb25hbCBkZXBl bmRlbmN5IG9mIGNvbW1pdC1sb2ctZWRpdG9yCiAKICAgICAgICAgV2hlbiBUZXJtL1JlYWRLZXku cG0gaXNuJ3QgcHJlc2VudCwgdGhlIC0tcmVnZW5lcmF0ZS1sb2cgc3dpdGNoIHdpbGwgYmUgbm9u LWZ1bmN0aW9uYWwuCmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9j aGVja291dC9jaGVja291dC5weSBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNr b3V0L2NoZWNrb3V0LnB5CmluZGV4IDIxYWFkNTA1Y2ZiN2YyOTZjNzRhZDQwY2FlMDU5ZDFiMDhk NGQwODIuLjA0M2VkZDE1M2ZhNzE0NWYyZTFmNWQwZjM0MDAzZjE1N2MxNzlhMmQgMTAwNjQ0Ci0t LSBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L2NoZWNrb3V0LnB5Cisr KyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L2NoZWNrb3V0LnB5CkBA IC0zNSw3ICszNSw3IEBAIGZyb20gd2Via2l0cHkuY29tbW9uLmNoZWNrb3V0LmNvbW1pdGluZm8g aW1wb3J0IENvbW1pdEluZm8KIGZyb20gd2Via2l0cHkuY29tbW9uLmNoZWNrb3V0LnNjbSBpbXBv cnQgQ29tbWl0TWVzc2FnZQogZnJvbSB3ZWJraXRweS5jb21tb24uY2hlY2tvdXQuZGVwcyBpbXBv cnQgREVQUwogZnJvbSB3ZWJraXRweS5jb21tb24ubWVtb2l6ZWQgaW1wb3J0IG1lbW9pemVkCi1m cm9tIHdlYmtpdHB5LmNvbW1vbi5zeXN0ZW0uZXhlY3V0aXZlIGltcG9ydCBFeGVjdXRpdmUsIHJ1 bl9jb21tYW5kLCBTY3JpcHRFcnJvcgorZnJvbSB3ZWJraXRweS5jb21tb24uc3lzdGVtLmV4ZWN1 dGl2ZSBpbXBvcnQgcnVuX2NvbW1hbmQsIFNjcmlwdEVycm9yCiBmcm9tIHdlYmtpdHB5LmNvbW1v bi5zeXN0ZW0uZGVwcmVjYXRlZF9sb2dnaW5nIGltcG9ydCBsb2cKIAogCkBAIC0xMjAsNyArMTIw LDcgQEAgY2xhc3MgQ2hlY2tvdXQob2JqZWN0KToKICAgICAgICAgICAgIHJhaXNlIFNjcmlwdEVy cm9yKG1lc3NhZ2U9IkZvdW5kIG5vIG1vZGlmaWVkIENoYW5nZUxvZ3MsIGNhbm5vdCBjcmVhdGUg YSBjb21taXQgbWVzc2FnZS5cbiIKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICJBbGwg Y2hhbmdlcyByZXF1aXJlIGEgQ2hhbmdlTG9nLiAgU2VlOlxuICVzIiAlIHVybHMuY29udHJpYnV0 aW9uX2d1aWRlbGluZXMpCiAKLSAgICAgICAgbWVzc2FnZV90ZXh0ID0gRXhlY3V0aXZlKCkucnVu X2NvbW1hbmQoW3NlbGYuX3NjbS5zY3JpcHRfcGF0aCgnY29tbWl0LWxvZy1lZGl0b3InKSwgJy0t cHJpbnQtbG9nJ10gKyBjaGFuZ2Vsb2dfcGF0aHMsIHJldHVybl9zdGRlcnI9RmFsc2UpCisgICAg ICAgIG1lc3NhZ2VfdGV4dCA9IHNlbGYuX3NjbS5ydW4oW3NlbGYuX3NjbS5zY3JpcHRfcGF0aCgn Y29tbWl0LWxvZy1lZGl0b3InKSwgJy0tcHJpbnQtbG9nJ10gKyBjaGFuZ2Vsb2dfcGF0aHMsIHJl dHVybl9zdGRlcnI9RmFsc2UpCiAgICAgICAgIHJldHVybiBDb21taXRNZXNzYWdlKG1lc3NhZ2Vf dGV4dC5zcGxpdGxpbmVzKCkpCiAKICAgICBkZWYgcmVjZW50X2NvbW1pdF9pbmZvc19mb3JfZmls ZXMoc2VsZiwgcGF0aHMpOgpkaWZmIC0tZ2l0IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9jb21t b24vY2hlY2tvdXQvY2hlY2tvdXRfdW5pdHRlc3QucHkgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5 L2NvbW1vbi9jaGVja291dC9jaGVja291dF91bml0dGVzdC5weQppbmRleCBmOGFhMDJhZTM0MTRm MTAzN2ZjZGIwM2U2MTkwMTExYjliODcwMTkyLi40ZjIyNmZmZWIyZDRkNzFhMDJiOWYyMWExYTM4 OWVlYzQ4NDg3ZTNkIDEwMDY0NAotLS0gYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9j aGVja291dC9jaGVja291dF91bml0dGVzdC5weQorKysgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5 L2NvbW1vbi9jaGVja291dC9jaGVja291dF91bml0dGVzdC5weQpAQCAtMzcsNyArMzcsNyBAQCBp bXBvcnQgdW5pdHRlc3QKIGZyb20gLmNoZWNrb3V0IGltcG9ydCBDaGVja291dAogZnJvbSAuY2hh bmdlbG9nIGltcG9ydCBDaGFuZ2VMb2dFbnRyeQogZnJvbSAuc2NtIGltcG9ydCBkZXRlY3Rfc2Nt X3N5c3RlbSwgQ29tbWl0TWVzc2FnZQotZnJvbSB3ZWJraXRweS5jb21tb24uc3lzdGVtLmV4ZWN1 dGl2ZSBpbXBvcnQgU2NyaXB0RXJyb3IKK2Zyb20gd2Via2l0cHkuY29tbW9uLnN5c3RlbS5leGVj dXRpdmUgaW1wb3J0IEV4ZWN1dGl2ZSwgU2NyaXB0RXJyb3IKIGZyb20gd2Via2l0cHkudGhpcmRw YXJ0eS5tb2NrIGltcG9ydCBNb2NrCiAKIApAQCAtMTIxLDkgKzEyMSwxNSBAQCBTZWNvbmQgcGFy dCBvZiB0aGlzIGNvbXBsaWNhdGVkIGNoYW5nZSBieSBtZSwgVG9yIEFybmUgVmVzdGJcdTAwZjgh CiAgICAgZGVmIHRlc3RfY29tbWl0X21lc3NhZ2VfZm9yX3RoaXNfY29tbWl0KHNlbGYpOgogICAg ICAgICBzY20gPSBNb2NrKCkKIAorICAgICAgICBkZWYgbW9ja19ydW4oKmFyZ3MsICoqa3dhcmdz KToKKyAgICAgICAgICAgICMgTm90ZSB0aGF0IHdlIHVzZSBhIHJlYWwgRXhlY3V0aXZlIGhlcmUs IG5vdCBhIE1vY2tFeGVjdXRpdmUsIHNvIHdlIGNhbiB0ZXN0IHRoYXQgd2UncmUKKyAgICAgICAg ICAgICMgaW52b2tpbmcgY29tbWl0LWxvZy1lZGl0b3IgY29ycmVjdGx5LgorICAgICAgICAgICAg cmV0dXJuIEV4ZWN1dGl2ZSgpLnJ1bl9jb21tYW5kKCphcmdzLCAqKmt3YXJncykKKwogICAgICAg ICBkZWYgbW9ja19zY3JpcHRfcGF0aChzY3JpcHQpOgogICAgICAgICAgICAgcmV0dXJuIG9zLnBh dGguYWJzcGF0aChvcy5wYXRoLmpvaW4ob3MucGF0aC5kaXJuYW1lKF9fZmlsZV9fKSwgJy4uJywg Jy4uJywgJy4uJywgc2NyaXB0KSkKIAorICAgICAgICBzY20ucnVuID0gbW9ja19ydW4KICAgICAg ICAgc2NtLnNjcmlwdF9wYXRoID0gbW9ja19zY3JpcHRfcGF0aAogCiAgICAgICAgIGNoZWNrb3V0 ID0gQ2hlY2tvdXQoc2NtKQo=