Page MenuHomePhabricator

Arc land is not enforcing Herald Rule
Closed, InvalidPublic

Description

Let me know if you have any questions or difficulty reproducing. Thanks!

Reproduction steps:

  • make new branch
  • commit code changes
  • arc diff for a code review
  • have code review approved
  • commit more changes to the branch
  • arc land

Expected result:

  • push fails due to Herald Rule (H2) not being satisfied for the commit to be pushed

Actual result:

  • All code (reviewed and unreviewed) is pushed to develop

Our Herald Rule(H2):
When all of these conditions are met:

  • Branches matches regexp @^develop$@

Take these actions every time this rule matches:

  • Block push with message: Please do a code review before pushing to origin/develop.
  • Send an email to: user x and user y

Referenced Herald Rule(H9):
When all of these conditions are met:

  • Differential reviewers include any of ntouran, gmalmgren
  • Accepted Differential revision exists

Take these actions every time this rule matches:

  • Do nothing.
NameVersion
phabricator0426ce73f0e63f1900f1cc285cfa1465ea72317e (Jan 13 2017) (branched from 7276af6a81f49bbdc14ace064aab50afbeb79cfc on origin)
arcanist9503b941cc02be637d967bb50cfb25f852e071e4 (Jan 6 2017) (branched from ade25facfdf22aed1c1e20fed3e58e60c0be3c2b on origin)
phutil10963f771f118baa338aacd3172aaede695cde62 (Jan 13 2017) (branched from 9d85dfab0f532d50c2343719e92d574a4827341b on origin)

Event Timeline

monufer created this task.Mar 1 2017, 1:31 AM
monufer updated the task description. (Show Details)Mar 1 2017, 1:36 AM
chad added a subscriber: chad.Mar 1 2017, 2:13 AM

Please update your install to a current version of Phabricator (we require all bug reports be against HEAD of stable or master).

chad added a comment.Mar 1 2017, 2:14 AM

Alternatively, you can use a test instance at Phacility to verify your reproduction steps against a current version.

chad updated the task description. (Show Details)Mar 1 2017, 6:29 AM
chad renamed this task from Arc Land is not Enforcing Herold Rule to Arc land is not enforcing Herald Rule.
avivey added a subscriber: avivey.Mar 1 2017, 9:27 AM

That's the expected behavior - see https://secure.phabricator.com/book/phabricator/article/differential_faq/

arc land squashes all the revision's changes into a single commit, marks it with the revision's message, and pushes that; Herald then identifies the single commit as "reviewed as revision X", and accepts it.

@avivey Thank you for the explanation!

We would like all code being pushed to show up in the code review,. Our main concern with this behavior is that these additional commits can be pushed to develop with arc land without actually showing up in the code review that approved the land.

Obviously best practice to address this concern is to arc diff anytime between making a commit and using arc land, but people are not perfect.

Do you know if there is a different Herald Rule we could use or setting we could change that would enforce all the code being pushed showing up in the associated code review?

chad closed this task as Invalid.Mar 1 2017, 5:55 PM

(Going to close since this isn't a bug report. My mistake on reading the reproduction steps, @avivey has better eyes than I)

chad added a comment.Mar 1 2017, 6:11 PM

arc land mutates the code (adds details, squashes), so it's not possible. I'd recommend restricting who can land code and have the accepted reviewers manually land with "Land Revision" from the web ui. In general we expect people to trust co-workers, or use Audit for post-commit coverage.

avivey added a comment.Mar 1 2017, 9:03 PM

I don't think there's any way to check for that via Herald. There's the Land Revision button( T182), which I ❤, but it's still a prototype and has some surprising behavior built into it (If you're doing anything complex, it can sometime land stuff you didn't want it to).

You can set history.immutable to true in your repository to prevent arc from performing mutation operations (amends, squash-merges).

If you set this option, arc will use a merge commit to merge instead of a squash-merge. But this won't really solve your problem.

Particularly, this will not guarantee that what lands is what was reviewed. Most often, users may need to resolve conflicts during the merge, which will cause additional changes to be contained in the merge commit. The conflict resolutions are not reviewed, and they are free to resolve the conflicts however they want, including by introducing entirely new code, and we can not prevent this (there is no automatic way to tell when a merge is faithful vs malicious). This will also prevent them from merging master to their branch first without getting re-review, because the merge commit will be a new commit which is not part of the revision.

If we did exactly what you ask more generally (prevent code from landing unless it is exactly identical to reviewed changes) users would need to re-review for every conflict, rebase, and merge, because rebasing and merging [almost] always introduces new code.

T182 discusses the idea of "chain of custody", which allows you to make this guarantee by having Phabricator perform merges. In this case, we know the merge is faithful because we perform it faithfully ourselves. This is the only way to guarantee that reviewed code and committed code are the same while still allowing merges and rebases (that is, guarantee that reviewed code and published code differ by only faithful merge/rebase actions).

This isn't a priority because installs generally have not been interested in it.

Thanks for the information @epriestley.

To be clear I am not requesting that users should have to re-review every conflict, rebase, and merge. We do not hold our users to that standard.

For example, if you want to push directly (not land) with the Herald Rule we are using, you just need to arc diff any new changes since the approval before pushing, but the code review does not need re-approval. I agree that that re-approval would be too arduous.

I think our solution is to proceduralize users to always arc diff before landing and have the reviewer close the review after looking over the changes since the last review iteration. This code will already have been pushed, of course, but at least they can make a ticket if they see any problems. We could also use audit more as @chad suggested.

monufer updated the task description. (Show Details)Jul 16 2018, 5:00 PM