Bug 27082 – bugzilla-tool: Add --no-close switch to land-patches

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[David Kilzer (:ddkilzer)]

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(-)

[Darin Adler]

Comment on attachment 32458 [details]
Patch v1

rs=me -- I'm not a total expert on this, but it looks fine

[Eric Seidel (no email)]

Comment on attachment 32458 [details]
Patch v1

Looks fine.  It should obsolete the one patch it landed instead of closing though.

[Eric Seidel (no email)]

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)

[David Kilzer (:ddkilzer)]

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.

[Eric Seidel (no email)]

Seems odd to not obsolete the patch as well.  But OK.  We should document these "practices" somewhere. ;)

[David Kilzer (:ddkilzer)]

(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+).

[David Kilzer (:ddkilzer)]

Created attachment 32495 [details]
Patch v2

[David Kilzer (:ddkilzer)]

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.

[Eric Seidel (no email)]

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?

[David Kilzer (:ddkilzer)]

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!

[Eric Seidel (no email)]

(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. :)

[David Kilzer (:ddkilzer)]

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(-)

[David Levin]

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."

[David Kilzer (:ddkilzer)]

Created attachment 33744 [details]
Patch v4

[David Kilzer (:ddkilzer)]

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