Page MenuHomePhabricator

"differential.sticky-accept" isn't working because of overbroad accept downgrading
Closed, ResolvedPublic


See D17590 for an example. Changes in T10967 are still a little overbroad: accepts currently get voided on update because of reject downgrades. Voiding needs to apply to accept/reject more narrowly so that sticky accept works.

Event Timeline

I've gotten way behind this week so I'm going to cut and deploy the release now to try to catch up, then fix this afterwards and deploy it separately.

I cherry-picked this to stable and will plan to push it to the cluster tomorrow morning.

I'm not sure if what I'm seeing is a bug or expected (or even if related to this). We have sticky accept set to true (just as default).

  1. @cspeckmim creates a diff with reviewers @epriestley and @chad
  2. @epriestley requests changes to the revision
  3. @cspeckmim makes changes
  4. @chad accepts the revision
  5. @cspeckmim makes one last change
  6. Revision is in status Needs Review

Is this expected? I thought it was a bug at first but then saw that in the history someone previously requested changes and never updated their status to accept, so wasn't sure what it should be placed in.

(Currently on 517372a1e0c48d0bc1961af06e132b8537776565)

That's not expected, but sounds like D17653. Did all the actions happen at rP517372a1?

Oh wait. That is expected. The assumption is that the "one last change" in (5) is a response to my reject in (2), and that I need to look at it again.

The revision was created on 56dd1b297c3e5cdbb477acc7435d6aa5749f33f2. While at that same revision I had updated the diff.

The Request Changes, Acceptance, and further updates were all done on 517372a1e0c48d0bc1961af06e132b8537776565.

That is, the expected state at (6) is:

  • @chad is "Accepted".
  • @epriestley is "Rejected".
  • Revision "Needs Review" from @epriestley, and is in "Needs Review" (not "Needs Revision") because the author was the last one to touch it. Presumption: they've responded to feedback from me.

I believe you can arc diff --plan-changes if your intent with (3) and (5) is to partially address feedback but not return it to me for review.

So by going through a Plan Changes status it would "clear" or "address" the prior Request Changes status?

The state transitions are a bit tricky. "Plan Changes" temporarily sets the status to "Plan Changes", but does not "address" (internally, "void") individual reviewer state on its own.

The reason for this is so that this sequence works:

  • Reviewer accepts.
  • Author plans changes.
  • Author requests review ("oops, I actually didn't need to make any changes, this is good as-is").
  • Desired result: revision transitions to "Accepted", because the reviewer previously accepted exactly this set of changes.

Normally, when you update, that does void individual reviewer state. "Accept" becomes "Accepted Older" and "Reject" becomes "Rejected Older". Then we re-run the reviewer state rules. Since all "Reject" have been turned into "Rejected Older", the revision transitions to "Needs Review". The workflow analog is "The author has addressed all the feedback and updated the revision."

The expectation is that authors normally do not update revisions until they want more feedback from reviewers.