Herald ignores “unpublished commits”

We have a rule in our Phabricator installation to send a notification e-mail for every commit. It works quite fine, but we have noticed recently Phabricator have stopped sending those e-mails for release commits. The difference seems to be that the release commit lands in the repository first as an out-of-branch tagged commit, only later the branch is pushed to include the release commit. And Herald seems to basically ignore such commits for the “Always” rule.

Reproduction Instructions

I created a test Phacility installation (expiring 2019-07-04) doing the following repro:

  • Create a new git repository and committed an initial commit to it.
  • Create a Herald rule which should fire for all commits; let’s say add a comment on each.
  • Commit and push a normal commit; everything works correctly, Herald sees the commit and comments so.
  • Commit a new commit, but do not push the master branch yet; instead, tag the commit and push the tag first (git commit -a; git tag r1; git push origin r1).
  • The commit lands in Phabricator (marked with a warning message Unpublished Commit · Learn More; Not On Permanent Ref: This commit is not an ancestor of any permanent ref.), but Herald does not comment the commit. There seem to be two Herald transcripts (unfortunately, I don’t know if they could be made public) for the push: HeraldPreCommitRefAdapter and HeraldPreCommitContentAdapter; both say No Herald rules applied to this object.
  • Afterwards, I pushed the master branch (git push). The commit does not warn about being unpublished anymore, and another HeraldPreCommitRefAdapter Herald transcript appeared, but still no Herald rules were applied, and Herald did not comment on the commit. (No that I would expect it at this time.)
  • Just to be sure, I committed and pushed another plain commit and Herald works correctly again, commenting on the commit.

Phabricator/Arcanist Version

ccfc74702f67c7c525c907d01c703a01ea8e5439 (Tue, Jun 25) (branched from ca56e8590a3df188af5710cf4ef7b3e9780dd780 on origin)
feb5f4d42c4fe0001e76428e80d5e88262308802 (Sat, Jun 22) (branched from d92fa96366c0ed50e4257508148aa75192d4fb1f on origin)
b675a06bf7419ff3b64448cb310e7557530446db (Sat, Jun 22) (branched from 8eaf1b38d41711112269b35bd2f083f6bc567d51 on origin)
3a1a1c2ec566d3188faf0104be9ef15f4c25e85f (Tue, Jun 25)
e9147e2871e1dd6d5bd305061304b7820d4ad3e3 (May 16 2019) (branched from 5424383159ac45981b9214a58b2cf4e67c1c89a9 on origin)
3.3 at /usr/bin/diff
2.22.0 at /usr/bin/git
2.8.2 at /usr/bin/hg
2.4.2 at /usr/local/bin/pygmentize
1.8.8 at /usr/bin/svn

The behavior you observe is a bug. Specifically, this part is not expected:

“Herald ignores commits that are ancestors of permanent refs if they were previously pushed to some other non-permanent ref”

It is likely a consequence of the new repository rules interacting with https://secure.phabricator.com/T13299. I’ll add a note there.

See also https://secure.phabricator.com/T13290 for general ongoing work here.