[webkit-reviews] review denied: [Bug 27082] bugzilla-tool: Add --no-close switch to land-patches : [Attachment 33728] Patch v3

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied David Kilzer (ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 27082: bugzilla-tool: Add --no-close switch to land-patches
https://bugs.webkit.org/show_bug.cgi?id=27082

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

------- Additional Comments from David Levin <levin at chromium.org>



> diff --git a/WebKitTools/Scripts/bugzilla-tool
b/WebKitTools/Scripts/bugzilla-tool


> @@ -314,20 +315,21 @@ class LandPatchesFromBugs(Command):
>      @classmethod
>      def land_patches(cls, bug_id, patches, options, tool):
>	   try:
> +	       # Clear the review flag on each bug if we're not closing the bug
or if there is more than one patch.

Consider:
 # Clear the review flag on patches when needed for adding the commit revision
number and/or moving out of the commit queue.

> +	       should_clear_review_flag = not options.close_bug or len(patches)
> 1
...
> +		   if should_clear_review_flag:
> +		       tool.bugs.clear_attachment_review_flag(patch['id'])

Why isn't comment_text passed in?

I really like the practice of putting the revision # corresponding to the patch
commitment in the bug.



> diff --git a/WebKitTools/Scripts/modules/bugzilla.py
b/WebKitTools/Scripts/modules/bugzilla.py
> +    def clear_attachment_review_flag(self, attachment_id):
...
> +	   self.browser['comment'] = "Clearing review+ flag."