Arc land accepts unapproved revisions

We use Phab and Arc. We want to make sure that all revisions are approved before being landed. Normally, this works. Eg, if I make an arc diff and then try to arc land it immediately, it fails because the diff hasn’t been approved in Phab.

However, one of my coworkers discovered a corner case:

  1. Submit a revision (arc diff)
  2. Have someone accept the revision in the Phab UI
  3. Commit another change locally. DO NOT run arc diff to update Phab.
  4. Run arc land.

Now, the changes that were committed locally get landed even though they were never approved (indeed, Phab never even know about the changes at all).

I assume that there’s some config flag we just need to set, but I can’t find any documentation about it.

Any ideas?

Thanks!

There is/was (my install is a little old now) a setting for that!

Try:
your_url/config/group/differential/
and look for the setting:
differential.sticky-accept
You’ll want to set it to false.

(On my team have it set to true b/c usually folks are good about only making naming/comment modifications in response to a “conditional accept” and no other additional changes. YMMV.)

Edit: I should note, this will simply warn the person landing that their changes have not been approved (as if they had made a PFR and no one approved it). They can still respond “y” and land anyway. In that case, our team has “audit” rules set up (via herald) to catch unapproved changes.

We have sticky accept set to false, but I think that’s a different setting. Sticky accept makes it so that when you run arc diff again to upload changes to Phab, Phab will no longer count the change as accepted.

In the edge case I’m describing above, Phab never knows about the new changes (eg, there wasn’t a second arc diff), so it still counts as accepted even though unapproved changes are landing.

There’s no real way to prevent this, other than some variant of T182 and disallow all other pushes.

It’s not possible, in the general case, to tell the difference between “rebased my code and now it’s slightly different from the revision” and “I’m sending a different piece of code than what we agreed on”.

Wouldn’t it be possible to see if the diff is the same? Rebasing shouldn’t change that, and if it does, that probably does merit the author resubmitting the change since it means two people edited the same lines.

Or would it be best to just make a Herald rule that checks for this or something like that?

It sounds possible; I think this request (“Don’t let users push code that doesn’t exactly match the revision”) has came up several times in the past, and was always declined because it was not always possible in the general case (eg - binary files, changes in the context part of the diff, confusing whitespace changes…); I think this always ends up being rolled into T182, which solves this problem by only allowing differential make code pushes.

I can’t find any relevant discussion right now.