Oh, maybe this isn't quite the same as T6522. Pushing another commit to the branch probably fires the rule?

I think this and T6522 will probably be resolved by the same set of changes, but maybe aren't quite the same problem.

I think it is the same as T6522?

Event 1: a new commit rXabc123 is created, on branch some-dev-branch. Herald runs, doesn't find it to contain master, doesn't apply action.
Event 2: master is updated to point to rXabc123. Herald doesn't run, because no commit was created.

We have some spooky magic around "autoclose" which might make the behavior for "Is on autoclose branch" different than "Is on <specific, named branch>". That said, I'm not sure if it's actually materially different, and these likely end up being more-or-less the same thing when we actually fix them.

Generally, I am leaning toward splitting the "Commit" rule into two rules, "Commit" and "Ref Update".

So then you would write something like this:

When:
[ Ref is an autoclose branch ][ is true ]
[ Any new commit does not have a corresponding revision ] [ is true ]
[ Author's projects ][ do not contain ][ Elite Developers ]
Take these actions:
[ Reject push with message: ][ Code must be reviewed if you are not elite enough. ]

...but I worry this will lead to a lot of really inflexible [ Any new commit <has some property> ][ is true ] rules, since I think a lot of these rules want to check if any of the newly pushed commits have (or do not have) some property.

For example, if you want to add a keyword like "@force" to let users push without review in an emergency, you have to do this:

[ Any new commit's combined message content ][ does not contain ] [ "@force" ]

...but if you want to require that each commit has a task ID, you have to do this:

[ Any new commit's individual message content ][ does not match ][ /some-pattern/ ]

Maybe we can do that by introducing a string-list field type with conditions like:

collectively contains
individually contains
collectively does not contain
individually do not contain
collectively matches
individually match

...but that gets really messy with the "Diff content" stuff.

More generally, I think 95% of this is mooted by supporting "push changes to the remote to backup or share your work" in a first-class way, via T8092/T5000, because almost all of this problem is created by people pushing "epriestley-tmp-backup3" branches into shared remotes. If git push blah.com/fork/epriestley/phabricator.git master:backup-3 just worked, most of this would likely vanish. I want to purse that before attempting to tackle this or T6522, since I think many of the issues we have around branch management today are rooted in Phabricator fundamentally believing "push = publish forever" and lots of users having a model like "push = save to disk".

FWIW, in this particular case our repository is hosted on GitHub, so lovely automagic in Phabricator wouldn't solve my specific case.

What are the barriers you face to hosting on Phabricator instead of on GitHub?

Purely organizational. I don't believe there's anything technically that Phabricator can't do, but we're just trying it out and don't want to move everything over before we're ready to commit.