1234468 - Should not set review request for patch with r=XXX

Status: RESOLVED INVALID

(MozReview Graveyard :: General, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

It seems the current behavior is if you have r?XXX, it sets the r? flag, and if you have r=XXX, it also sets r? flag, and warns the pusher that they are supposed to set r?XXX instead.

I think this setting doesn't make much sense. The program should not make assumption on what people intend to do in this case.

People may push reviewed patches to MozReview because of various reasons, e.g. previously reviewed but dropped, or previously reviewed as normal patch, etc. In those cases, setting all of them to request new review is extremely annoying.

Comment 1

[Dan Minor [:dminor]]

The current behaviour is to request review for r= unless that person has already granted a "ship it". This was intended as a convenience when we introduced r? as pretty much everyone used r= and we were afraid that people would end up with reviews with no reviewers and not notice.

If r? has sufficient traction, then I think it makes sense to use r? to indicate request a review (even if that person has already granted a "ship it" that would be carried forward) and to do what you suggest and ignore r= completely.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Even if people continue using r=, I think it is still better not to set review request automatically.

I personally never use any r flag to specify the reviewer, but prefer to set them manually after submitting, and I only add r= if a patch has been reviewed. It seems to me this is the workflow of at least quite a few people, and the current behavior doesn't make sense for us, only sometimes adds trouble.

Comment 3

[BMO Automation]

MozReview is now obsolete. Please use Phabricator instead. Closing this bug.