| Summary: | webkitpy's clean_working_directory has a confusing name | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | WebKit Review Bot <webkit.review.bot> | ||||||||||
| Component: | New Bugs | Assignee: | Tim 'mithro' Ansell <mithro> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, buildbot, dglazkov, dpranke, eric, gtk-ews, mithro, peter+ews, philn, rniwa, webkit-ews, xan.lopez | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 106429 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
WebKit Review Bot
2012-12-05 18:03:34 PST
Created attachment 182517 [details]
Patch
Comment on attachment 182517 [details] Patch Attachment 182517 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15867363 Comment on attachment 182517 [details] Patch Attachment 182517 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15870396 Comment on attachment 182517 [details] Patch Attachment 182517 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15869383 Attachment 182517 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1
/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/pylint/lint.py:393: DeprecationWarning: disable-msg is deprecated, replace it by disable (/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/common/checkout/scm/svn.py, line # ignore false positives for methods implemented in the mixee class. pylint: disable-msg=E1101
)
DeprecationWarning)
Tools/Scripts/webkitpy/common/checkout/scm/scm_mock.py:49: [MockSCM.has_working_directory_changes] An attribute affected in webkitpy.tool.steps.cleanworkingdirectory_unittest line 41 hide this method [pylint/E0202] [5]
/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/pylint/lint.py:393: DeprecationWarning: disable-msg is deprecated, replace it by disable (/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/tool/commands/download.py, line # pylint: disable-msg=E1101
)
DeprecationWarning)
Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:333: [SCMTest._shared_test_added_files] Instance of 'SCMTest' has no 'scm' member [pylint/E1101] [5]
Total errors found: 2 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 182517 [details] Patch Attachment 182517 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15853412 Comment on attachment 182517 [details] Patch Attachment 182517 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15836013 Comment on attachment 182517 [details] Patch Attachment 182517 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15838376 Looks like this patch would break the bots. I think the problem is that you removed the --no-clean option. Comment on attachment 182517 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182517&action=review > Tools/Scripts/webkitpy/common/checkout/scm/git.py:152 > + return self._run_git(['diff', 'HEAD', '--no-renames', '--name-only']) == "" should this be "!=" instead of "=="? Otherwise True == no changes, which is the opposite of what the method name implies. > Tools/Scripts/webkitpy/common/checkout/scm/git.py:361 > + # Haven't given us which commits, so use the difference to remote base. We generally try to make our comments grammatically correct, and this is a bit too far off for me :) I would probably just delete the comment anyway, the code is pretty clear as-is. > Tools/Scripts/webkitpy/common/checkout/scm/git.py:367 > + force_squash = squash and squash.lower() == "true" I'd probably collapse this to: force_squash = force_squash or Git.read_git_config('webkit-patch.commit-should-always-squash', cwd=self.checkout-root).lower() == 'true'. > Tools/Scripts/webkitpy/common/checkout/scm/git.py:370 > + num_changes = len(self.local_commits()) + int(not has_working_directory_changes) I'm not a big fan of the bool->int conversion; it's very unpythonic to me. perhaps (1 if has_working_directory_changes else 0) ? Also, seems like you don't want the 'not' here (cf. the comment in has_working_directory_changes). Also, if you collapse the lines above, you can probably get rid of the comment here as well (the whole thing will only be three lines and the logic will be obvious). > Tools/Scripts/webkitpy/common/checkout/scm/scm.py:93 > + probably don't need this line. > Tools/Scripts/webkitpy/common/checkout/scm/scm.py:210 > + #-------------------------------------------------------------------------- We don't tend to add divider lines like this ... > Tools/Scripts/webkitpy/common/checkout/scm/scm.py:218 > + #-------------------------------------------------------------------------- ditto. Created attachment 182640 [details]
Patch
Comment on attachment 182640 [details] Patch Attachment 182640 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15867649 Attachment 182640 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1
/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/pylint/lint.py:393: DeprecationWarning: disable-msg is deprecated, replace it by disable (/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/common/checkout/scm/svn.py, line # ignore false positives for methods implemented in the mixee class. pylint: disable-msg=E1101
)
DeprecationWarning)
Tools/Scripts/webkitpy/common/checkout/scm/scm_mock.py:49: [MockSCM.has_working_directory_changes] An attribute affected in webkitpy.tool.steps.cleanworkingdirectory_unittest line 41 hide this method [pylint/E0202] [5]
/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/pylint/lint.py:393: DeprecationWarning: disable-msg is deprecated, replace it by disable (/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/tool/commands/download.py, line # pylint: disable-msg=E1101
)
DeprecationWarning)
Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:333: [SCMTest._shared_test_added_files] Instance of 'SCMTest' has no 'scm' member [pylint/E1101] [5]
Total errors found: 2 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 182640 [details] Patch Attachment 182640 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15869676 Comment on attachment 182640 [details] Patch Attachment 182640 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15870699 Comment on attachment 182640 [details] Patch Attachment 182640 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15853727 Comment on attachment 182640 [details] Patch Attachment 182640 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15870703 Comment on attachment 182640 [details] Patch Attachment 182640 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15874634 Comment on attachment 182640 [details] Patch Attachment 182640 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/15860691 Comment on attachment 182640 [details]
Patch
This patch appears to break all the bots:
webkit-patch: error: no such option: --no-clean
Created attachment 182673 [details]
Patch
Comment on attachment 182673 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182673&action=review Looks good. > Tools/Scripts/webkitpy/common/checkout/scm/git.py:53 > + Exception.__init__(self, "Found %s local commits and the working directory is %s" % ( > + num_local_commits, ["clean", "not clean"][has_working_directory_changes])) I think this part landed in a separate patch. Created attachment 182680 [details]
Patch
Attachment 182680 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1
/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/pylint/lint.py:393: DeprecationWarning: disable-msg is deprecated, replace it by disable (/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/common/checkout/scm/svn.py, line # ignore false positives for methods implemented in the mixee class. pylint: disable-msg=E1101
)
DeprecationWarning)
Tools/Scripts/webkitpy/common/checkout/scm/scm_mock.py:49: [MockSCM.has_working_directory_changes] An attribute affected in webkitpy.tool.steps.cleanworkingdirectory_unittest line 42 hide this method [pylint/E0202] [5]
Tools/Scripts/webkitpy/common/checkout/scm/scm_mock.py:58: [MockSCM.has_local_commits] An attribute affected in webkitpy.tool.steps.cleanworkingdirectory_unittest line 51 hide this method [pylint/E0202] [5]
Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:333: [SCMTest._shared_test_added_files] Instance of 'SCMTest' has no 'scm' member [pylint/E1101] [5]
Total errors found: 3 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 182680 [details]
Patch
ok. let's give it a try.
Comment on attachment 182680 [details] Patch Clearing flags on attachment: 182680 Committed r139712: <http://trac.webkit.org/changeset/139712> All reviewed patches have been landed. Closing bug. Still need to fix the "CleanWorkingDirectory" step to make it clear that it is actually doing a "DiscardLocalChanges". Please use a new bug. We prefer to have one patch per bug. |