Page MenuHomePhabricator
Create Task
Maniphest T12496

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

Description

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.

Revisions and Commits

rP Phabricator
D17605rP520d3b939204 (stable) Don't downgrade accepts on update (fix "sticky accept")
D17605rP9ebb5f8cda56 Don't downgrade accepts on update (fix "sticky accept")

Related Objects
Search...

Event Timeline

epriestley created this task.Apr 3 2017, 11:10 AM
Herald added a subscriber: eadler. · View Herald TranscriptApr 3 2017, 11:10 AM
epriestley added a parent task: T10967: Move Differential reviewers back to a dedicated database storage table.Apr 3 2017, 11:10 AM

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.

epriestley mentioned this in 2017 Week 13 (Very Early April).Apr 3 2017, 11:22 AM
epriestley added a revision: D17605: Don't downgrade accepts on update (fix "sticky accept").Apr 3 2017, 3:38 PM
epriestley closed this task as Resolved by committing rP9ebb5f8cda56: Don't downgrade accepts on update (fix "sticky accept").Apr 3 2017, 4:55 PM
epriestley added a commit: rP9ebb5f8cda56: Don't downgrade accepts on update (fix "sticky accept").
epriestley added a comment.Apr 3 2017, 4:58 PM

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

epriestley added a commit: rP520d3b939204: (stable) Don't downgrade accepts on update (fix "sticky accept").Apr 5 2017, 1:35 AM
cspeckmim mentioned this in Z1336: General Chat.Apr 21 2017, 2:50 PM
cspeckmim added subscribers: cspeckmim, chad.Apr 21 2017, 3:23 PM

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)

epriestley added a comment.Apr 21 2017, 3:29 PM

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

epriestley added a comment.Apr 21 2017, 3:31 PM

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.

cspeckmim added a comment.Apr 21 2017, 3:32 PM

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.

cspeckmim added a comment.Apr 21 2017, 3:32 PM

Okay - wanted to double check

epriestley added a comment.Apr 21 2017, 3:33 PM

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.
epriestley added a comment.Apr 21 2017, 3:34 PM

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.

cspeckmim added a comment.Apr 21 2017, 3:40 PM

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

epriestley added a comment.Apr 21 2017, 3:45 PM

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.

Phabricator · Built by Phacility