Page MenuHomePhorge

Herald runs even for non-permanent refs
Closed, ResolvedPublic

Asked by smith on May 22 2023, 09:33.

Details

This guide suggests that commits pushed to non-permanent branches do not trigger Herald rules.

Scenario:

  • Create a repository.
  • Configure permanent branches in repository settings.
  • Make global Herald rule that blocks commits (that don't have accepted Differential revision, for example).
  • Push commit to a branch that doesn't match any of the entries in the configured permanent branches.

Expectations:
Commit is not audited, because it is pushed to a non-permanent branch.

Reality:
Commit is audited and blocked.

Answers

avivey
Updated 555 Days Ago

EDIT: I've "graduated" this to a task , so I'm closing this question now.
Feel free to continue the discussion in T15449!


I didn't try to reproduce this, but this sounds like an actual bug - either in the implementation or in the docs.

Technically, this happens because the commit-hook Herald rules run in a completely different flow then all the other rules (including "commit" rules); Most herald rules react to objects after they are "discovered", and run after a change has happened.
"Commit hook" rules run in the git hook, while the action ("push") is taking place.

The "unpublished branches" feature prevents Herald from discovering the unpublished commits, so Herald doesn't run at all.
For the commit-hook rules, the rule is invoked directly, without waiting for any discovery to take place, so this filtering doesn't happen.

Making the rule only consider "published" branches might be harder then it sounds:

  • Technically, I think a commit may be pushed without being in any ref?
  • A single push command might include a single commit in many refs
  • An "illegal" commit might be pushed to a non-published branch, and later become published by updating the published ref (this would make it very easy to bypass such a rule)

It might be hard to define the precise behavior of making a commit-hook rule respect "non-published branches". So I'm inclined to just update the docs, stating that commit-hooks are not effected by unpublished refs.

WDYT?

New Answer

Answer

This question has been marked as closed, but you can still leave a new answer.