Page MenuHomePhabricator

Support for "partially accepted" revisions
Closed, DuplicatePublic

Description

We frequently add multiple people to our reviews, and wish for more than one of them to accept the revision before we land it. However, we have discovered some anti patterns emerging recently:

When you mark a review as accepted, the whole review is accepted regardless of whether there are other reviewers as well. When the review is accepted, it vanishes from the home screen for everyone else, only to be found in the "Waiting for others" section under Differential. This is incidentally also where the other reviews currently in progress are found (for our most active developers, it is not uncommon to have more than 10 active revisions here), so people very rarely walk through them checking if they have done their duty.

People have been noticing this, so now some people have become reluctant to be the first to accept the revision, because they are aware of how much that reduces the chances of anyone else also looking over the revision. Some people end up mimicking mailing list rules and just post the comment "+1" instead, circumventing the whole reviewer status mechanism.

This phenomenon is especially visible on the more bigger and complex reviews; there is no good way to signal that "yes, I have looked through this and it looks good, but I don't feel confident enough to be the sole reviewer for this".

My suggestion for solving this: The reviewer status field is expanded to include something like STATUS_LOOKEDOVER (need a better name, but bear with me), and we get something similar to the +1/+2 responses I have seen used in other sites and mailing lists (here's one I found on http://wiki.typo3.org/Gerrit_Review_Workflow#Reviews_in_Gerrit)

Code Review:
+2: Verification approved
+1: Could commit, needs more approval
0: No opinion, just adding some comment
-1: Please do not commit
-2: Veto
Note that "+1" plus "+1" does not add to "+2". The +1 is only a indicator of how many people agree with the change (even anonymous reviewers). Only an active contributor can ultimately give a "+2", approving a change and unlocking the "Submit" button.

When only "LOOKEDOVER"-type responses have come in, the revision is marked as "partially accepted". Once someone marks it as accepted, it becomes accepted just like today.

For the home/differential screen, I then suggest adding a new Differential configuration option that lets you choose between the following behaviors:

  • Once accepted or partially accepted, the revision is moved to "waiting for others" (same as today's behavior)
  • Partially accepted revisions are still visible on home screens (and stay in "blocking" and "active" queues)

Event Timeline

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

Basically our thoughts are:

  • If someone shouldn't accept a commit, then just comment on it.
  • If someone should be required to review a commit, make them a blocking reviewer.
  • Herald/Audit/Owners to help enforce things are properly reviewed.

I'm not sure I even understand the difference between commenting "I took a quick look only, found these issues" vs. hitting a <select> that says the same thing. Overall this seems to be more a culture issue than one with Phabricator.

See also T731 and T1279 for our plan on better handling of multiple reviewer states, there are some known issues.

I'm going to merge this into T731. In addition to @chad's response, some other general thoughts:

  • The +1 / +2 terminology from Gerrit is confusing, because it seems like 1 + 1 = 2, but this isn't true. We are very unlikely to ever adopt this terminology.
  • Philosophically, we believe there should be a single responsible reviewer for every change. The most important review feedback on changes is big-picture, global feedback that accounts for the full context of a change. Having two people look at half of a change is not the same as having one person look at the whole thing. If no one is taking responsibility for reviewing the entire change, our philosophical stance is that no one is actually doing review properly.
  • We may add soft accept / soft reject eventually, but T731 covers this.
  • We vaguely plan to change some of the "what appears where" rules eventually, I think T1279 has some discussion somewhere.