How to prevent sneaky changes pushed after review is approved?
Event Timeline
Not currently possible. Two quick thoughts.
- We feel this is more efficient for users on day to day interactions. That is, it's simple to say "just change X and this is good" and accept the diff. This keeps engineers working. And if someone is abusive about it, they need to have a talk to their manager about expectations.
- I think thiiiink Drydock is needed to "land this absolutely as is, don't give the user access to the direct repository".
Thanks for your response, and I agree. The rare inconvience of reverting a change that someone put in abusing the system is far less then the annoyance of having to wait for a minor change to complete the review or waiting for reviewers to approve as I make a minor change.
I'm in the middle of a discussion with some other developers about adopting either Phabricator or Gerrit, and this was the one behavior of Gerrit that I couldn't emulate with Phabricator.
The upstream discussion of this issue starts with "Chain of Custody" here: T182#133871
See also "Sticky Accept" here:
https://secure.phabricator.com/book/phabricator/article/differential_faq/
In practice, although we plan to complete the "Land Revision" feature in a way that allows it to be configured and used to establish a chain of custody, no install has ever actually asked us for this. The defaults even encourage changes after accept ("caught a couple of typos, see inline + accept") and this workflow seems to work well in reality. There's generally a strong implicit social contract around not getting A reviewed and landing B, and our experience to date is that engineers generally do the right thing if we make it easy to say "yep, fixed those typos, here's the new diff" or "oops, caught one more issue at the end, see inline" without requiring re-review.