Bug 88689 - webkit-patch land-safely should set cq? if the patch author is not in committers.py
Summary: webkit-patch land-safely should set cq? if the patch author is not in committ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-08 14:39 PDT by Tony Chang
Modified: 2012-06-10 12:30 PDT (History)
6 users (show)

See Also:


Attachments
Adds the feature (5.60 KB, patch)
2012-06-08 17:53 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (5.69 KB, patch)
2012-06-08 18:13 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (6.22 KB, patch)
2012-06-08 18:22 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2012-06-08 14:39:29 PDT
I think it currently always does cq+, then the commit bot yells at them.
Comment 1 Ryosuke Niwa 2012-06-08 17:53:04 PDT
Created attachment 146670 [details]
Adds the feature
Comment 2 Dirk Pranke 2012-06-08 18:10:12 PDT
Comment on attachment 146670 [details]
Adds the feature

View in context: https://bugs.webkit.org/attachment.cgi?id=146670&action=review

r+ with some minor comments ...

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:518
>      def _commit_queue_flag(self, mark_for_landing, mark_for_commit_queue):

mark_for_landing and mark_for_commit_queue shouldn't be two different variables, since True/True doesn't make sense. Can we add a FIXME or something for this? Also it's not clear to me that 'mark_for_commit_queue' doesn't also mean "cq+"; a better name would be something like "commit_queue_requested"

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:520
> +            user_is_committer = self.committers.contributors_by_email_username(self.username)

can you change 'user_is_committer' to 'user' or 'account', since it's not a boolean, it's an object, and it can represent non-committers?
Comment 3 Ryosuke Niwa 2012-06-08 18:12:29 PDT
Comment on attachment 146670 [details]
Adds the feature

View in context: https://bugs.webkit.org/attachment.cgi?id=146670&action=review

>> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:518
>>      def _commit_queue_flag(self, mark_for_landing, mark_for_commit_queue):
> 
> mark_for_landing and mark_for_commit_queue shouldn't be two different variables, since True/True doesn't make sense. Can we add a FIXME or something for this? Also it's not clear to me that 'mark_for_commit_queue' doesn't also mean "cq+"; a better name would be something like "commit_queue_requested"

Will do.

>> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:520
>> +            user_is_committer = self.committers.contributors_by_email_username(self.username)
> 
> can you change 'user_is_committer' to 'user' or 'account', since it's not a boolean, it's an object, and it can represent non-committers?

Yeah, that's a good point. It was initially a boolean but then I realized it'll be better to spit out two different messages so I changed that to user.
Will fix.
Comment 4 Ryosuke Niwa 2012-06-08 18:13:46 PDT
Created attachment 146672 [details]
Patch for landing
Comment 5 Ryosuke Niwa 2012-06-08 18:22:21 PDT
Created attachment 146675 [details]
Patch for landing
Comment 6 WebKit Review Bot 2012-06-08 23:18:48 PDT
Comment on attachment 146675 [details]
Patch for landing

Clearing flags on attachment: 146675

Committed r119894: <http://trac.webkit.org/changeset/119894>
Comment 7 WebKit Review Bot 2012-06-08 23:18:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Adam Barth 2012-06-09 11:44:05 PDT
Thanks for making this change.  Should we make land-safely visible in the main help?  If folks use it a lot, it probably shouldn't be a hidden command.
Comment 9 Ojan Vafai 2012-06-09 12:22:16 PDT
(In reply to comment #8)
> Thanks for making this change.  Should we make land-safely visible in the main help?  If folks use it a lot, it probably shouldn't be a hidden command.

SGTM. I think people use it a ton.
Comment 10 Adam Barth 2012-06-10 12:30:53 PDT
> SGTM. I think people use it a ton.

Actually, looks like it already is in the main help.