[webkit-reviews] review denied: [Bug 33197] bugzilla-tool submit-patch mistakenly picks up bug URLs in non-ChangeLog files : [Attachment 46247] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Adam Barth
<abarth at webkit.org>'s request for review:
Bug 33197: bugzilla-tool submit-patch mistakenly picks up bug URLs in
non-ChangeLog files
https://bugs.webkit.org/show_bug.cgi?id=33197

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

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
> +	   Also, fixed a wide-spread typo.

This should be a separate patch.  rs=me to fix the typo separately.

> diff --git a/WebKitTools/Scripts/webkitpy/commands/download.py
b/WebKitTools/Scripts/webkitpy/commands/download.py
> @@ -268,6 +269,7 @@ Commits the revert and updates the bug (including
re-opening the bug if necessar
>      @staticmethod
>      def _parse_bug_id_from_revision_diff(tool, revision):
>	   original_diff = tool.scm().diff_for_revision(revision)
> +	   # FIXME: This is wrong because it picks up bug numbers outside of
ChangeLogs
>	   return parse_bug_id(original_diff)

Why wasn't this instance fixed at the same time?  At minimum, this should be
explained in the ChangeLog.

> diff --git a/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py
b/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py
> index f8392aa..ee5a3c8 100644
> --- a/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py
> +++ b/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py
> @@ -156,11 +156,14 @@ class MockSCM(Mock):
>      def commit_ids_from_commitish_arguments(self, args):
>	   return ["Commitish1", "Commitish2"]
>  
> +    def commit_message_for_this_commit(self):
> +	   return CommitMessage(["CommitMessage1",
"https://bugs.webkit.org/show_bug.cgi?id=93"])
> +
>      def commit_message_for_local_commit(self, commit_id):
>	   if commit_id == "Commitish1":
> -	       return
CommitMessage("CommitMessage1\nhttps://bugs.example.org/show_bug.cgi?id=42\n")
> +	       return CommitMessage(["CommitMessage1",
"https://bugs.webkit.org/show_bug.cgi?id=42"])
>	   if commit_id == "Commitish2":
> -	       return
CommitMessage("CommitMessage2\nhttps://bugs.example.org/show_bug.cgi?id=75\n")
> +	       return CommitMessage(["CommitMessage2",
"https://bugs.webkit.org/show_bug.cgi?id=75"])
>	   raise Exception("Bogus commit_id in
commit_message_for_local_commit.")

Why didn't test results change in response to changing these text inputs? 
Shouldn't there be tests for grabbing the bug id from a local commit?  (Or to
reverse the question, why change the URLs if test results didn't change?)

r- to split the renames into a different patch and to answer the questions
above.