[webkit-reviews] review denied: [Bug 104198] webkitpy's clean_working_directory has a confusing name : [Attachment 182517] Patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Tim 'mithro' Ansell
<mithro at mithis.com>'s request for review:
Bug 104198: webkitpy's clean_working_directory has a confusing name
https://bugs.webkit.org/show_bug.cgi?id=104198

Attachment 182517: Patch
https://bugs.webkit.org/attachment.cgi?id=182517&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
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.