Page MenuHomePhabricator

Implement "Author's packages", "Added project tags", and "Removed project tags" Herald fields
Closed, ResolvedPublic


See PHI1609. This issue effectively requests an [ Author's packages ] field in Herald, to write this kind of rule for reviewer management in Differential:

[ Affected packages ][ include ][ X ]
[ Author's packages ][ do not include ][ X ]

See PHI1553. See also Previously, see T13283.

These are broadly requests for "Added project tags" and "Removed project tags" fields. I'm satisfied that T13283 is sticking so these are now reasonable to implement.

Event Timeline

epriestley triaged this task as Wishlist priority.Jan 17 2020, 5:05 PM
epriestley created this task.

D20947 does not implement "Author's packages" as a "Commit Content" field, nor as a "Commit Content (Hook)" field. The reason for this is that getting the modern authorPHID in both cases is somewhat complicated.

  • "Commit Content" fields currently rely on authorPHID on CommitData. This isn't completely effective given that author identities now exist.
  • "Commit Content (Hook)" fields currently rely on an explicit inline identity query, but bypass the actual identity table. So they'll find the "naive" answer, but may get the wrong answer if an identity has been remapped. This should be updated.

Both cases are somewhat buggy for other "Author ..." fields today; the pathways should be sorted out before new fields are added.

Remaining work:

  • HeraldPreCommitContentAdapter->lookupUser() should use DiffusionRepositoryIdentityEngine to resolve an identity, not DiffusionResolveUserQuery. Currently, the wrong author will be looked up when an identity exists and is explicitly bound to a non-default user.
  • HeraldPreCommitContentAdapter->lookupUser() should probably cache results (<raw author string -> resolved identity> on the Adapter is likely an appropriate scope).
  • HeraldCommitAdapter should load identities by default.
    • It looks like this should happen in setObject()?
    • setObject() has no prototype, but the root class calls it. Yikes.
  • Existing rules which use author or committer PHIDs should use the identity versions.
  • Once this is all sorted out, "Author's packages" fields should be implemented for both Commit flavors.