Page MenuHomePhabricator

Differential shouldn't require an initial reviewer to accept a revision
Closed, InvalidPublic

Description

If multiple reviewers are added when a revision is created, then only one reviewer has to accept it before it can be landed.

However, if some reviewer(s) are added after the revision is created, then arc land refuses to land unless one of the initial reviewers accepts it.

Why should there be a difference? Let us not discriminate.

Event Timeline

adityar7 raised the priority of this task from to Needs Triage.
adityar7 updated the task description. (Show Details)
adityar7 added a project: Differential.
adityar7 added a subscriber: adityar7.

I'll see if I can reproduce this; this is not expected.

What is the expected behavior i.e. who has to accept before the revision can be landed? Usually till date what we've encountered is that one reviewer needs to accept it.

But in the last few weeks we've often been adding reviewers after the revision is created, and several times different people have been unable to land the patch until the initial reviewer also accepted it. Maybe some sequence of actions (request changes, add reviewer, accept) causes this.

Admittedly I was a bit lazy and didn't reproduce this from scratch. If you can't reproduce this or don't see anything that should cause this, then I'll get back with more info the next time we encounter it.

The expectation is that a revision becomes accepted when it has one or more "accepting" reviewers, and zero "rejecting" or "blocking review" users.

adityar7 claimed this task.

Ok, thanks, that explains it. Reviewer1 requests changes and asks the author to add Reviewer2 and leave it to Reviewer2 to accept it. Reviewer2 accepts it but Reviewer1 had blocked it.

We may add some sort of softer version of "Request Changes" which goes away after the revision is updated (e.g., "Request One Change" or something, although hopefully we can find more clear wording).

Before T1279, a reject did not block a later accept, but this was mostly a technical limitation (it simply could not: we only had one field for revision status). However, rejects usually seemed to be treated socially as somewhat blocking (e.g., users would rarely accept when another user had raised objections, instead writing something like "this looks good to me, but I'll let <that other guy> confirm that this addresses his concerns"). The new behavior is thus relatively consistent with the old behavior in practice, in my experience, but is not equivalent technically.

I want to finish up more of the T1279 stuff before adding new statuses (and maybe T2222, too), but expect to take another look at a soft/nonblocking reject afterward.