[webkit-reviews] review denied: [Bug 106402] Adding tool to check local changes have been successfully landed via a third-party (such as the commit queue). : [Attachment 183727] 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 106402: Adding tool to check local changes have been successfully landed
via a third-party (such as the commit queue).
https://bugs.webkit.org/show_bug.cgi?id=106402

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

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=183727&action=review


I'm basically okay with this patch and approach. You should fix (or suppress)
the style warnings; otherwise, I'm just looking at minor issues.

> Tools/Scripts/webkitpy/common/net/bugzilla/bug.py:136
> +	   # If we don't find a revision, return None

nit: this comment doesn't add much.

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:69
> +	   # Now this is there it gets complicated, we need to compare our diff
to the diff at landed_revision.

typo: "there" -> "where"?

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:84
> +	       ["interdiff", diff1_patch.name, diff2_patch.name],
decode_output=False)

I'm still not fond of just raising an uncaught exception if interdiff isn't
available. Tools shouldn't just return hard-to-follow stack traces to the user.
Can you catch this somewhere and print a more helpful message?

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:89
> +	       return True

Should we log some sort of a message in this case rather than silently
succeeding?

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:118
> +	   self._tool.scm().discard_local_changes()

Do you want this line?