Page MenuHomePhabricator

Audits are no longer triggered using singular "Auditor:"
Closed, ResolvedPublic

Description

Should these commits 10178aa480cb, 88c20f7ea075 have triggered an audit? I noticed after I updated my install on Master to resolve another issue that the parser no longer seemed to work when specifying "Auditor: ..." or "Auditors: ...".

It looks like this is a result of this commit bc41c3f5a5b9. I'm not sure if this workflow has now changed.

Replicated on secure.phabricator.com, but my version information is:

phabricator 9b92e56dfc033d298393617fcbd59357d5dd1e7b (Wed, Feb 1)
arcanist ade25facfdf22aed1c1e20fed3e58e60c0be3c2b (Fri, Jan 6)
phutil 9d85dfab0f532d50c2343719e92d574a4827341b (Fri, Jan 13)

Event Timeline

Those commits are not expected to trigger an audit after D17122 / T10312. The first line of commit messages is now always parsed as a title, even if it begins "Auditors: ...", "Tests: ...", "Reviewers: ...", etc.

A commit like rGITTEST702cd23a, which has "Auditors" elsewhere than the title line, is expected to trigger an audit; it did so.

Does that explain what you're seeing locally?

No, not yet... In those examples, I just happened to have Auditors on the first line. Usually, it's not on the first line for us.

I'm trying to see if I can replicate on secure.phabricator.com. Locally, this is with an SVN repo. I just did this rSVNTEST9, and it worked (despite it taking quite a while to process). I'll try a few more things...

Ahhh... Before both "Auditor: ..." and "Auditors: ..." worked. Now it appears that only "Auditors: ..." works. I must not have been trying it with "Auditors: ..." at all.

Side note: are all of the recognized labels documented in one place?

Thanks

Thanks! I think this is the timeline of events:

  • Until recently, Differential and Diffusion had different code to parse "Auditors:".
  • I think Differential never accepted "Auditor", while Diffusion did.
  • Somewhat recently, in connection with T11114, code for parsing commit messages was made more general.
  • Very recently, in D17262, the custom ad-hoc code for parsing "Auditors" was unified, so both Differential and Diffusion use the same code. As a side effect, "Auditor" stopped being recognized.

D17306 should fix this by restoring "Auditor" as an alias for the "Auditors" field.

Side note: are all of the recognized labels documented in one place?

Not currently. Which labels are available depends on configuration and third-party code, so we'd have to write a dynamic page showing labels for the current viewer/install/config, not just a static piece of documentation. This would rarely help solve problems and there isn't a great place to shove it in the UI. T9713 may improve clarity here while making more concrete UI improvements.

Gotcha... Thanks for the explanation. Initially, I wasn't exactly sure what was going on... I sent an email out telling folks to use "Auditor: ..." in their commit messages, upgraded to clean up what I mentioned here T12181: Bulk Deleting Audits does not remove "Audit Required" from Commit, and then the "Auditor: ..." tag stopped working when our team started using it. Appreciate the fix!

epriestley renamed this task from Audits are no longer triggered using Auditor: or Auditors: to Audits are no longer triggered using singular "Auditor:".Feb 3 2017, 3:01 PM
epriestley claimed this task.
epriestley triaged this task as Normal priority.
epriestley added a project: Audit.

This should now be resolved at HEAD of master, and should promote to stable later today.

Note that this isn't retroactive: after upgrading, "Auditor" should work for new commits, but won't apply to existing commits made before the upgrade.

Let us know if this doesn't fix things or you're still seeing issues. Thanks for the report!