Bug 27082

Summary: bugzilla-tool: Add --no-close switch to land-patches
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer@webkit.org>
Component: Tools / TestsAssignee: David Kilzer (:ddkilzer) <ddkilzer@webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: eric@webkit.org, levin@chromium.org
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Mac OS X 10.5   
Attachments:
Description Flags
Patch v1
none
Patch v2
none
Patch v3
none
Patch v4 levin: review+

Description From 2009-07-08 10:34:07 PST
Created an attachment (id=32458) [details]
        bugzilla-tool: Add --no-close switch to land-patches

Reviewed by NOBODY (OOPS!).

        * Scripts/bugzilla-tool:
        (LandPatchesFromBugs.__init__): Added --no-close switch.
        (LandPatchesFromBugs.land_patches): Don't close the bug if the
        --no-close switch was used.
---
 2 files changed, 15 insertions(+), 2 deletions(-)
------- Comment #1 From 2009-07-08 10:36:45 PST -------
(From update of attachment 32458 [details])
rs=me -- I'm not a total expert on this, but it looks fine
------- Comment #2 From 2009-07-08 10:55:54 PST -------
(From update of attachment 32458 [details])
Looks fine.  It should obsolete the one patch it landed instead of closing though.
------- Comment #3 From 2009-07-08 10:58:28 PST -------
This current change will just cause it to leave the attachment r+'d after landing the bug, and leave it open.  In the --no-close we can unconditionally obsolete first patch as we go, instead of trying to close the bug in that case.

                # If we're commiting more than one patch, update the bug as we go.
                if len(patches) > 1:
                    tool.bugs.obsolete_attachment(patch['id'], comment_text)
------- Comment #4 From 2009-07-08 14:10:51 PST -------
(From update of attachment 32458 [details])
(In reply to comment #2)
> (From update of attachment 32458 [details] [details])
> Looks fine.  It should obsolete the one patch it landed instead of closing
> though.

The current practice is to clear the review flag, not obsolete the patch.   I'll make it do that instead.
------- Comment #5 From 2009-07-08 14:15:37 PST -------
Seems odd to not obsolete the patch as well.  But OK.  We should document these "practices" somewhere. ;)
------- Comment #6 From 2009-07-08 14:37:03 PST -------
(In reply to comment #5)
> Seems odd to not obsolete the patch as well.  But OK.  We should document these
> "practices" somewhere. ;)

When you're done committing all the patches, you're left with a set of patches that are obsolete (which means they were superseded by another patch attached), and a list of patches that have been committed with their review flags cleared (and maybe the last one set to r+).
------- Comment #7 From 2009-07-08 18:12:06 PST -------
Created an attachment (id=32495) [details]
        Bug 27082: bugzilla-tool: Add --no-close switch to land-patches
------- Comment #8 From 2009-07-08 18:14:04 PST -------
(From update of attachment 32495 [details])
Changes since v1:

- Clear review+ flag on attachments after they've landed if the --no-close switch is used so they won't show up in the commit queue.
------- Comment #9 From 2009-07-10 22:54:38 PST -------
(From update of attachment 32495 [details])
So I'm confused by why we obsolete patches one at a time when landing them when there is more than one to land. But we don't obsolete them when landing with the --no-close option.

I guess I would prefer we always obsoleted the patch after landing.  But I think you and I have different personal preferences here. :)

Either way, we should make them consistent, no?
------- Comment #10 From 2009-07-10 23:10:15 PST -------
(From update of attachment 32495 [details])
(In reply to comment #9)
> (From update of attachment 32495 [details] [details])
> So I'm confused by why we obsolete patches one at a time when landing them when
> there is more than one to land. But we don't obsolete them when landing with
> the --no-close option.
> 
> I guess I would prefer we always obsoleted the patch after landing.  But I
> think you and I have different personal preferences here. :)
> 
> Either way, we should make them consistent, no?

My view of the world:

Obsolete patch:  A patch that has never and will never land.
Patch with cleared review flag:  A patch that has landed on a bug with more than one patch up for review (which must necessarily stay open until all patches have landed).

What's your view?

I'll make the change to be consistent in behavior and post another patch.  Thanks!
------- Comment #11 From 2009-07-10 23:31:56 PST -------
(In reply to comment #10)
> My view of the world:
> 
> Obsolete patch:  A patch that has never and will never land.
> Patch with cleared review flag:  A patch that has landed on a bug with more
> than one patch up for review (which must necessarily stay open until all
> patches have landed).
> 
> What's your view?

I guess "obsolete" to me means "we're done with this", it "no longer applies".  where as the non-obsolete stuff is stuff I have to take action with.  I think it would be a mixed of flagged, obsolete, and unflagged patches on a bug, the unflagged indicating that they had landed, the obsolete that they were bad/never landed.

But honestly I don't feel strongly about this in either case.  We can always change it later. :)
------- Comment #12 From 2009-07-29 12:02:18 PST -------
Created an attachment (id=33728) [details]
Patch v3

Reviewed by NOBODY (OOPS!).

* Scripts/bugzilla-tool:
(LandPatchesFromBugs.__init__): Added --no-close switch.
(LandPatchesFromBugs.land_patches): Don't close the bug if the
--no-close switch was used, but call
Bugzilla.clear_attachment_review_flag() on the attachment so it
no longer shows up in the commit queue.  Also changed the
behavior when landing multiple patches from obsoleting patches
to clearing each patch's review flag.
* Scripts/modules/bugzilla.py:
(Bugzilla.clear_attachment_review_flag): Added.
---
 3 files changed, 38 insertions(+), 6 deletions(-)
------- Comment #13 From 2009-07-29 13:38:31 PST -------
(From update of attachment 33728 [details])



> 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."
------- Comment #14 From 2009-07-29 14:44:47 PST -------
Created an attachment (id=33744) [details]
Patch v4
------- Comment #15 From 2009-07-29 16:48:32 PST -------
Committing to http://svn.webkit.org/repository/webkit/trunk ...
    M    WebKitTools/ChangeLog
    M    WebKitTools/Scripts/bugzilla-tool
    M    WebKitTools/Scripts/modules/bugzilla.py
Committed r46563
    M    WebKitTools/ChangeLog
    M    WebKitTools/Scripts/modules/bugzilla.py
    M    WebKitTools/Scripts/bugzilla-tool
r46563 = 5477449a50731e3e4b6637af4cd8943fe46cf5d9 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46563