Bug 27082

Summary: bugzilla-tool: Add --no-close switch to land-patches
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: Tools / TestsAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, levin
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 David Kilzer (:ddkilzer) 2009-07-08 10:34:07 PDT
Created attachment 32458 [details]
Patch v1

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 Darin Adler 2009-07-08 10:36:45 PDT
Comment on attachment 32458 [details]
Patch v1

rs=me -- I'm not a total expert on this, but it looks fine
Comment 2 Eric Seidel 2009-07-08 10:55:54 PDT
Comment on attachment 32458 [details]
Patch v1

Looks fine.  It should obsolete the one patch it landed instead of closing though.
Comment 3 Eric Seidel 2009-07-08 10:58:28 PDT
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 David Kilzer (:ddkilzer) 2009-07-08 14:10:51 PDT
Comment on attachment 32458 [details]
Patch v1

(In reply to comment #2)
> (From update of attachment 32458 [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 Eric Seidel 2009-07-08 14:15:37 PDT
Seems odd to not obsolete the patch as well.  But OK.  We should document these "practices" somewhere. ;)
Comment 6 David Kilzer (:ddkilzer) 2009-07-08 14:37:03 PDT
(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 David Kilzer (:ddkilzer) 2009-07-08 18:12:06 PDT
Created attachment 32495 [details]
Patch v2
Comment 8 David Kilzer (:ddkilzer) 2009-07-08 18:14:04 PDT
Comment on attachment 32495 [details]
Patch v2

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 Eric Seidel 2009-07-10 22:54:38 PDT
Comment on attachment 32495 [details]
Patch v2

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 David Kilzer (:ddkilzer) 2009-07-10 23:10:15 PDT
Comment on attachment 32495 [details]
Patch v2

(In reply to comment #9)
> (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?

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 Eric Seidel 2009-07-10 23:31:56 PDT
(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 David Kilzer (:ddkilzer) 2009-07-29 12:02:18 PDT
Created attachment 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 David Levin 2009-07-29 13:38:31 PDT
Comment on attachment 33728 [details]
Patch v3




> 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 David Kilzer (:ddkilzer) 2009-07-29 14:44:47 PDT
Created attachment 33744 [details]
Patch v4
Comment 15 David Kilzer (:ddkilzer) 2009-07-29 16:48:32 PDT
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