This is a pot-pourri of copy-pastes from chats and links to the phab discourse regarding the subject of:
How do I ensure that what someone lands is what was reviewed in the corresponding diff while also letting them fix typos or implement minor comments?
Here is an older discussion with some background, as well as some input from Evan.
Options some orgs have considered (non-exhaustive)
arc land does not push anything: it triggers a bot somewhere to run arc patch and as many other checks one would like before handling the push.
This has some similarities with gitlab's release trains.
Pre-receive commit hook
Here, a pre-receive commit hook does the following:
- finds out the diff belonging to the commit,
- arc patches the diff to a separate, clean working copy,
- compares the diff of the patch with the diff of the commit,
- rejects the push if they differ.
This forces the dev to update his diff when what he would push differs from it. Then we have some rules around what areas of the repo have a sticky accept or not.
It does not solve for "diff breaks master after rebase" but at least you work around the problem of malicious or unintentionally borked pushes.
Here is a gist for a partial implementation (it still attempts to compare patch files vs applying the changesets and comparing the output of git diff.
Diff Similarity settings
Landbod and pre-receive hook give us a place where to put some logic, but this still leaves the question as to how one decides where the limit is between a change that warrants a new review and one that does not.
The way we currently handle this is that:
- some owner packages have stickiness disabled
- if a diff suddenly involves a new package owner, it will need to be reviewed
And we manage this via a custom webhook that phabricator calls upon Diff update.
It would probably be worth thinking a bit about what uses cases we want to cover, but at least we can quickly have some hooks that are easy to install for users that know what they are doing.