Bug 104198 - webkitpy's clean_working_directory has a confusing name
Summary: webkitpy's clean_working_directory has a confusing name
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim 'mithro' Ansell
URL:
Keywords:
Depends on:
Blocks: 106429
  Show dependency treegraph
 
Reported: 2012-12-05 18:03 PST by WebKit Review Bot
Modified: 2013-01-14 21:24 PST (History)
12 users (show)

See Also:


Attachments
Patch (35.04 KB, patch)
2013-01-13 23:27 PST, Tim 'mithro' Ansell
no flags Details | Formatted Diff | Diff
Patch (34.77 KB, patch)
2013-01-14 15:50 PST, Tim 'mithro' Ansell
no flags Details | Formatted Diff | Diff
Patch (19.39 KB, patch)
2013-01-14 18:24 PST, Tim 'mithro' Ansell
no flags Details | Formatted Diff | Diff
Patch (19.53 KB, patch)
2013-01-14 19:09 PST, Tim 'mithro' Ansell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description WebKit Review Bot 2012-12-05 18:03:34 PST
webkitpy's clean_working_directory has a confusing name
Requested by abarth on #webkit.
Comment 1 Tim 'mithro' Ansell 2013-01-13 23:27:57 PST
Created attachment 182517 [details]
Patch
Comment 2 WebKit Review Bot 2013-01-13 23:29:32 PST
Comment on attachment 182517 [details]
Patch

Attachment 182517 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15867363
Comment 3 EFL EWS Bot 2013-01-13 23:29:57 PST
Comment on attachment 182517 [details]
Patch

Attachment 182517 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15870396
Comment 4 Build Bot 2013-01-13 23:30:28 PST
Comment on attachment 182517 [details]
Patch

Attachment 182517 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15869383
Comment 5 WebKit Review Bot 2013-01-13 23:30:58 PST
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 6 Early Warning System Bot 2013-01-13 23:31:17 PST
Comment on attachment 182517 [details]
Patch

Attachment 182517 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15853412
Comment 7 Early Warning System Bot 2013-01-13 23:31:51 PST
Comment on attachment 182517 [details]
Patch

Attachment 182517 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15836013
Comment 8 Peter Beverloo (cr-android ews) 2013-01-14 00:11:44 PST
Comment on attachment 182517 [details]
Patch

Attachment 182517 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15838376
Comment 9 Adam Barth 2013-01-14 10:28:42 PST
Looks like this patch would break the bots.  I think the problem is that you removed the --no-clean option.
Comment 10 Dirk Pranke 2013-01-14 13:22:31 PST
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.
Comment 11 Tim 'mithro' Ansell 2013-01-14 15:50:04 PST
Created attachment 182640 [details]
Patch
Comment 12 WebKit Review Bot 2013-01-14 15:52:03 PST
Comment on attachment 182640 [details]
Patch

Attachment 182640 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15867649
Comment 13 WebKit Review Bot 2013-01-14 15:53:16 PST
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 14 Early Warning System Bot 2013-01-14 15:53:36 PST
Comment on attachment 182640 [details]
Patch

Attachment 182640 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15869676
Comment 15 Early Warning System Bot 2013-01-14 15:54:14 PST
Comment on attachment 182640 [details]
Patch

Attachment 182640 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15870699
Comment 16 EFL EWS Bot 2013-01-14 15:56:32 PST
Comment on attachment 182640 [details]
Patch

Attachment 182640 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15853727
Comment 17 Peter Beverloo (cr-android ews) 2013-01-14 16:00:50 PST
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 18 Build Bot 2013-01-14 16:09:21 PST
Comment on attachment 182640 [details]
Patch

Attachment 182640 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15874634
Comment 19 kov's GTK+ EWS bot 2013-01-14 16:24:48 PST
Comment on attachment 182640 [details]
Patch

Attachment 182640 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15860691
Comment 20 Adam Barth 2013-01-14 16:31:53 PST
Comment on attachment 182640 [details]
Patch

This patch appears to break all the bots:

webkit-patch: error: no such option: --no-clean
Comment 21 Tim 'mithro' Ansell 2013-01-14 18:24:05 PST
Created attachment 182673 [details]
Patch
Comment 22 Adam Barth 2013-01-14 18:41:25 PST
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.
Comment 23 Tim 'mithro' Ansell 2013-01-14 19:09:26 PST
Created attachment 182680 [details]
Patch
Comment 24 WebKit Review Bot 2013-01-14 19:11:44 PST
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 25 Adam Barth 2013-01-14 21:10:51 PST
Comment on attachment 182680 [details]
Patch

ok.  let's give it a try.
Comment 26 WebKit Review Bot 2013-01-14 21:17:50 PST
Comment on attachment 182680 [details]
Patch

Clearing flags on attachment: 182680

Committed r139712: <http://trac.webkit.org/changeset/139712>
Comment 27 WebKit Review Bot 2013-01-14 21:17:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Tim 'mithro' Ansell 2013-01-14 21:22:00 PST
Still need to fix the "CleanWorkingDirectory" step to make it clear that it is actually doing a "DiscardLocalChanges".
Comment 29 Adam Barth 2013-01-14 21:24:09 PST
Please use a new bug.  We prefer to have one patch per bug.