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

phabricator
ccfc74702f67c7c525c907d01c703a01ea8e5439 (Tue, Jun 25) (branched from ca56e8590a3df188af5710cf4ef7b3e9780dd780 on origin)
arcanist
feb5f4d42c4fe0001e76428e80d5e88262308802 (Sat, Jun 22) (branched from d92fa96366c0ed50e4257508148aa75192d4fb1f on origin)
phutil
b675a06bf7419ff3b64448cb310e7557530446db (Sat, Jun 22) (branched from 8eaf1b38d41711112269b35bd2f083f6bc567d51 on origin)
libcore
3a1a1c2ec566d3188faf0104be9ef15f4c25e85f (Tue, Jun 25)
services
e9147e2871e1dd6d5bd305061304b7820d4ad3e3 (May 16 2019) (branched from 5424383159ac45981b9214a58b2cf4e67c1c89a9 on origin)
php
5.5.9-1ubuntu4.29
diff
3.3 at /usr/bin/diff
git
2.22.0 at /usr/bin/git
hg
2.8.2 at /usr/bin/hg
pygmentize
2.4.2 at /usr/local/bin/pygmentize
svn
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.