Page MenuHomePhabricator

Herald rule for commits on autoclose branch didn't trigger when a commit was first pushed to a branch, then to master (fast-forward)
Closed, ResolvedPublic


To reproduce:

  1. Create a Herald rule that triggers for "commit is on autoclose branch".
  2. Create a commit on a non-autoclose branch that has the head of an autoclose branch as its parent, and push it. The herald transcript should show the commit as not matching the rule (since it isn't on an autoclose branch).
  3. Update (fast-forward) the head of the autoclose branch to point at the new commit.
  4. Push the autoclose branch.

Herald doesn't trigger for this commit, because no commit was created (only a branch was updated).

I'm working around this issue for now by tracking only autoclose branches.

Event Timeline

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:

[ 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 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.

epriestley edited projects, added Diffusion (v3); removed Diffusion.
epriestley edited projects, added Diffusion; removed Diffusion (v3).
epriestley claimed this task.

This is effectively mooted by changes in T13277.

We no longer fire Herald rules for commits not reachable from a permanent ref, so all "Commit" Herald rules essentially have an implicit "Commit is reachable from permanent ref / Commit is on autoclose branch" rule now, and Herald rules will always evaluate the first time a commit becomes reachable from a permanent ref.