Herald "Is Merge Commit" Error (?)

#1

Observed Behavior

Herald is not enforcing “is merge commit is false” condition. Herald allows regular (non-merge) commits through when commits are fast-forwards to an already-published commit on another branch.

Expected Behavior

If a Herald rule is set up to check “is merge commit is false” on a certain branch of a certain repository, all commits of any kind to that branch should be denied unless they are indeed merge commits.

Comments

  • This may well be intended behavior due to the “Permanent Ref” paradigm. My best-guess is that Herald isn’t even triggering due to the fact that a fast-forward is occurring. If that’s so, it makes the repro steps below irrelevant.
  • If that’s the case, is there another Herald method for properly enforcing merge-only into a branch?
  • This behavior currently allows a dev to skirt “Requires Review” Herald triggers by fast-forwarding from a non-review-required branch.

Phabricator Version

  • phabricator: 5dc4e76eb9f8f883e9d3846e084a2ac06da1ef3d (Thu, May 2)
  • arcanist: 9830c9316d38988b2dc283ac1a124b73bc8e6c5f (Mar 7 2019)
  • phutil: 639e4b9cae284717b1ed717dd1e4d11c70744b86 (Fri, Apr 12)

Reproduction Steps

  1. Set up & activate a new Git repository named “REPO” (all basic defaults / no settings changes).
  2. Clone the repository locally & enter it.
  3. Touch a file, “ReadMe.txt”.
  4. Add, commit, and push upstream the file to the default branch “master”.
    a. git add .
    b. git commit -m "Verbosely using Git commands."
    c. git push
  5. Branch locally to a new branch “develop”.
    a. git checkout -b develop
  6. Alter “ReadMe.txt”.
  7. Add, commit, and push upstream the new branch “develop”.
    a. git add .
    b. git commit -m "Developing."
    c. git push --set-upstream origin develop
  8. Create a Herald rule to enforce merge-only into “master”. Go to Herald and set up:
    a. Commit Hook: Commit Content
    b. Global
    c. Condition: Repository is any of “REPO”
    d. Condition: Branches contains “master”
    e. Condition: Is merge commit “is false”
    f. Action: Block push with message “Merges only, please.”
    g. Save rule.
  9. Locally, checkout “master” again.
    a. git checkout master
  10. Alter “ReadMe.txt”.
  11. Add, commit, and push to “master.” Herald rule will apply and the push is denied with “Merges only, please.” message. Good.
    a. git add .
    b. git commit -m "Changes to be denied."
    c. git push
    d. Hail the evil dragon bureaucrats.
    e. The rule is functional.
  12. Reset “master” locally:
    a. git reset --soft HEAD~1
    b. git reset --hard HEAD
  13. Checkout “develop” again.
    a. git checkout develop
  14. Alter “ReadMe.txt”.
  15. Add, commit, and push to “develop”. Herald rule will not apply and push will succeed. Good.
    a. git add .
    b. git commit -m "This will go through."
    c. git push
    d. The rule is still functional.
  16. Checkout “master” again.
    a. git checkout master
  17. Merge in “develop”:
    a. git merge develop
    b. By default Git will use a fast-forward for this “merge”. If it does not, clear your global Git config (remove e.g. “merge.ff = no”) temporarily or otherwise force a fast-forward.
  18. Push. Herald will not block the commit; you have bypassed the “merges-only” condition for “master”. Right?

Additional Step

To further verify the fast-forward is both not a merge commit and not being caught by Herald, invert the Herald rule:

  1. Repeat steps 13 to 17 (don’t push yet) to queue up another fast-forward on “master”.
  2. Edit the Herald rule from “is merge commit is false” to “is merge commit is true”.
  3. Now the rule blocks merges instead of allows them.
  4. If step 18 was actually being treated as a merge commit (and that’s why it went through), then this repeat attempt (with the rule now inverted) should be blocked.
  5. Push.
  6. Herald again doesn’t block the commit. The fast-forward is treated as neither a merge or not-a-merge.
#2

There’s currently no way to enforce “merge commits only” with Herald if users may push commits into a temporary branch which does not have this restriction and later fast-forward the intended “merge-only” branch. Each Herald stage runs against commits only once:

  • Pre-commit rules run the first time a commit is pushed to any ref.
  • Post-commit rules run the first time a commit becomes an ancestor of a permanent ref.

If Herald enabled this rule to be enforced, it would mean that creating a temporary branch like this:

$ git checkout master
$ git checkout -b temp123
$ git push origin temp123

…would require Herald to evaluate every commit on master synchronously inside the git push for the new branch, because it would become possible to write Herald rules like “branches matching ‘temp.*’ may not have any ancestor commit with the word ‘quack’ in the commit message” which would necessitate completely evaluating the entire history.

At least naively, this would effectively break branch creation for all installs with large repositories and any Herald rules.

Put another way, Herald does not evaluate when a <ref, commit> reachability pair changes, mostly because there are simply too many of these events: pushing a copy of branch X with a million commits creates a million new <ref, commit> reachability pairs.

Although the desired rule is reasonable, I don’t see a practical way to make Herald evaluate on <ref, commit> pairs. Possible approaches which could solve this problem (“disallow non-merge commits”) include:

(1) Add “Newly Reachable Commits Include Merge/Non-Merge Commit” to “Ref” rules.
(2) Add repository-level “merge-only branches”, similar to the existing “Filesize Limit” and “Touched Paths Limit”.
(3) Implement this hook manually by adding a hook to hooks/pre-receive-phabricator.d/ (see: https://secure.phabricator.com/book/phabricator/article/diffusion_hooks/).
(4) Try to navigate <ref, commit> hooks, perhaps by allowing a repository to be configured into a mode where branches are created explicitly through some other UI or certain virtualized views of the repository are subjected to pair rules and others aren’t.

In the case of (1), this rule can be evaluated relatively efficiently, and careful construction allows Herald to avoid evaluating it when it isn’t relevant. Two problems are: it’s very limited (no good if the next user shows up and actually wants to stop “quack” from appearing in any commit message reachable from “master”) and the error can’t tell the pushing user what they did wrong (it can only say “no merge commits allowed”, not “commit abcd is a merge commit”).

In the case of (2), we fix the error message but end up with an even less flexible tool.

For (3), this will work, but you’re on your own.

For (4), I don’t see a clear pathway forward. Maybe this will become more clear in the future as virtual refs evolve.

Upshot: The desired rule can not be implemented because it is technically difficult to implement with Herald. It’s plausible we may support this rule in the future, but there’s no clearly-good pathway forward and no customer interest in this today, so it’s not clear when (or if) this will happen.

#3

(Also, the desired rule is more like “no new ancestor of master reachable by following only first parents is not a merge commit”, so even if we evaluated <ref, commit> rules and magically bypassed the “create a new branch” problem, that wouldn’t allow this rule to be implemented.)

#4

A possible approach here is “forbid non-review-required branches”, i.e. the root problem might be “git push = save changes”. Virtual refs and/or arc save might fix this problem, allow elimination of non-review branches, and moot everything else.

#6

^ Replied too soon; somehow missed your excellent technical responses above the one I was responding to. That’s a great amalgamation of the many “Why isn’t Phabricator GitHub?” posts I came across while trying to answer this for myself.

We are already walking down the hooks path, which works well for us as it is easy to maintain repository-to-repository and doesn’t break the Phabricator review-publish flow.

Really appreciate your time, @epriestley!