Bug 104198 – webkitpy's clean_working_directory has a confusing name

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[WebKit Review Bot]

webkitpy's clean_working_directory has a confusing name
Requested by abarth on #webkit.

[Tim 'mithro' Ansell]

Created attachment 182517 [details]
Patch

[WebKit Review Bot]

Comment on attachment 182517 [details]
Patch

Attachment 182517 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15867363

[EFL EWS Bot]

Comment on attachment 182517 [details]
Patch

Attachment 182517 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15870396

[Build Bot]

Comment on attachment 182517 [details]
Patch

Attachment 182517 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15869383

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 182517 [details]
Patch

Attachment 182517 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15853412

[Early Warning System Bot]

Comment on attachment 182517 [details]
Patch

Attachment 182517 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15836013

[Peter Beverloo (cr-android ews)]

Comment on attachment 182517 [details]
Patch

Attachment 182517 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15838376

[Adam Barth]

Looks like this patch would break the bots.  I think the problem is that you removed the --no-clean option.

[Dirk Pranke]

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.

[Tim 'mithro' Ansell]

Created attachment 182640 [details]
Patch

[WebKit Review Bot]

Comment on attachment 182640 [details]
Patch

Attachment 182640 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15867649

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 182640 [details]
Patch

Attachment 182640 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15869676

[Early Warning System Bot]

Comment on attachment 182640 [details]
Patch

Attachment 182640 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15870699

[EFL EWS Bot]

Comment on attachment 182640 [details]
Patch

Attachment 182640 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15853727

[Peter Beverloo (cr-android ews)]

Comment on attachment 182640 [details]
Patch

Attachment 182640 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15870703

[Build Bot]

Comment on attachment 182640 [details]
Patch

Attachment 182640 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15874634

[kov's GTK+ EWS bot]

Comment on attachment 182640 [details]
Patch

Attachment 182640 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15860691

[Adam Barth]

Comment on attachment 182640 [details]
Patch

This patch appears to break all the bots:

webkit-patch: error: no such option: --no-clean

[Tim 'mithro' Ansell]

Created attachment 182673 [details]
Patch

[Adam Barth]

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.

[Tim 'mithro' Ansell]

Created attachment 182680 [details]
Patch

[WebKit Review Bot]

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.

[Adam Barth]

Comment on attachment 182680 [details]
Patch

ok.  let's give it a try.

[WebKit Review Bot]

Comment on attachment 182680 [details]
Patch

Clearing flags on attachment: 182680

Committed r139712: <http://trac.webkit.org/changeset/139712>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tim 'mithro' Ansell]

Still need to fix the "CleanWorkingDirectory" step to make it clear that it is actually doing a "DiscardLocalChanges".

[Adam Barth]

Please use a new bug.  We prefer to have one patch per bug.