Bug 88689 – webkit-patch land-safely should set cq? if the patch author is not in committers.py

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

[Tony Chang]

I think it currently always does cq+, then the commit bot yells at them.

[Ryosuke Niwa]

Created attachment 146670 [details]
Adds the feature

[Dirk Pranke]

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?

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 146672 [details]
Patch for landing

[Ryosuke Niwa]

Created attachment 146675 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 146675 [details]
Patch for landing

Clearing flags on attachment: 146675

Committed r119894: <http://trac.webkit.org/changeset/119894>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

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.

[Ojan Vafai]

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

[Adam Barth]

> SGTM. I think people use it a ton.

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