Block unreviewed code from landing onto master

Question
#1

Hi,

We recently realized that even with “sticky-accept” disabled, one can introduce and land unreviewed code when manually resolving merge conflicts after rebasing accepted diff before landing.

Is there any way to restrict any unreviewed code from landing? Thanks!

$ arc version
arcanist 83661809e532c3fe444a8bf7c7d6936e6377691b (26 Oct 2018)
libphutil f9a65ebb0e0c70940321e20c1ee5c5df6573822f (27 Oct 2018)

Regards,

Peter

Block land process before accepted
#2

See https://secure.phabricator.com/T182 for discussion.

The only way you can guarantee that code which lands differs from code which was reviewed by only an automatic merge is to configure “Land Revision” according to this document:

https://secure.phabricator.com/book/phabricator/article/differential_land/

…and turn off push access for everyone so all merges and pushes are performed by Phabricator using the reviewed code state.

1 Like
#3

Thanks for your response. Using the "Land Revision " button, is it possible to only disable arc land cli and keep push access to non-master branch?

#4

If your repository is hosted by Phabricator, you can write a Herald rule like this:

  • Commit Hook: Branches/Tags/Bookmarks
  • Global Rule
  • When [ Ref name ][ is ][ master ] and [ Pusher ][ is not ][ your-pusher-bot-account ], take actions [ Block push with message ][ “You can not push directly to master, use “Land Revision” from the web UI instead.” ]

If your repository is not hosted by Phabricator, you may be able to use a raw Git commit hook or some other control mechanism to achieve the same effect.

Note that arc land will still run in this case, it will just fail when it attempts to push. A future version of arc land may, in effect, click “Land Revision” for you when things are configured like this, but this isn’t currently a priority.

1 Like
#5

On this subject (the other thread was closed, sorry to piggy-back on this one):

I remember a similar thread where someone mentioned “This would basically require comparing the output of the git diff that would be caused by the new commit vs the diff output that we have after an arc patch of the corresponding revision.”

We’re considering that option (which we would prefer to the ‘land’ button option because the whole workflow would start and end on the CLI): are there any fundamental problems with it?

#6

There’s no fundamental problem, but if the only goal is to end on the CLI, it may be easier to implement arc pretend-i-opened-my-browser-and-clicked-the-land-revision-button (which is the intended upstream pathway forward) than a repository hook that regenerates merges and compares them.

#7

The “pretend” path seems reasonable, so obviously I had to try the other one :slight_smile:

https://gist.github.com/Shastick/dfdc42b49a4191e82ce1ed2853b561a4 has a working hook, which incidentally includes a small go file to compare patches (too lazy to go the full “patch + compare” way, that’s for a future streak of boredom)

It’s dirty but currently does what we need it to do. (There still are leftovers from when I though I could just read environment variables from the hook. I’ll clean up at some point)

Thanks a lot for the great work on phabricator! Happy holidays to y’all.

#8

Wouldn’t this still allow arbitrary code to be landed, just with extra steps? Nothing ties the staging area to the diff uploaded to Phabricator, so why would it guarantee that?

#9

Sorry, my phrasing was misleading: those steps are necessary, but not sufficient. You’re correct that additional steps which provide the guarantee that the staging area and visible diff are the same (or some equivalent guarantee) are necessary. See https://secure.phabricator.com/T182 for more detailed dicussion.

1 Like