Page MenuHomePhabricator
Differential D17507

Ignore "Auditors: author" when inferring auditors from commit messages
ClosedPublic
Actions

Authored by epriestley on Mar 16 2017, 7:25 PM.
Tags
None
Referenced Files
F13237285: D17507.id.diff
Tue, May 21, 1:09 PM
F13233810: D17507.diff
Tue, May 21, 2:35 AM
F13226150: D17507.id.diff
Sun, May 19, 7:18 PM
F13188887: D17507.diff
Sat, May 11, 5:34 AM
Unknown Object (File)
Tue, May 7, 9:18 AM
Unknown Object (File)
Fri, May 3, 9:05 AM
Unknown Object (File)
Wed, May 1, 5:21 AM
Unknown Object (File)
Thu, Apr 25, 2:45 AM
View All Files
Subscribers
alexmv

Details

Reviewers
chad
Maniphest Tasks
T12406: Importing commits which name themselves as the auditor can cause stuck tasks
Commits
rP65de9e9f5e7f: Ignore "Auditors: author" when inferring auditors from commit messages
Summary

Fixes T12406. When importing commits, we automatically add auditors if the message lists "Auditors: username".

If the list of auditors includes the commit author, this edit fails because you can't audit your own commits (previously, you sometimes could and/or we didn't validate).

Instead, just ignore "Auditors: author".

Test Plan
  • Made a commit with "Auditors: epriestley".
  • Pushed it.
  • Saw the HeraldWorker get stuck with the error in T12406.
  • Applied the change; worker now succeeded.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley created this revision.Mar 16 2017, 7:25 PM
epriestley mentioned this in T12406: Importing commits which name themselves as the auditor can cause stuck tasks.Mar 16 2017, 7:26 PM
Harbormaster completed remote builds in B16008: Diff 42100.Mar 16 2017, 7:26 PM
alexmv added a subscriber: alexmv.Mar 16 2017, 8:03 PM
alexmv added inline comments.
src/applications/audit/editor/PhabricatorAuditEditor.php
310–313

It's not clear what this short-circuit accomplishes -- if the array is empty, the foreach does no iterations, and the equivalent short-circuit below it kicks in.

epriestley added a comment.Mar 16 2017, 8:18 PM

Oh -- it avoids foreach (null as ...) for commits with no "Auditors" field, which emits a warning since foreach (...) wants an array.

chad accepted this revision.Mar 16 2017, 8:30 PM
This revision is now accepted and ready to land.Mar 16 2017, 8:30 PM
alexmv added a comment.Mar 16 2017, 8:37 PM
In D17507#209885, @epriestley wrote:

Oh -- it avoids foreach (null as ...) for commits with no "Auditors" field, which emits a warning since foreach (...) wants an array.

Ah -- it's to catch the null case. I was just thinking in terms of the empty list case.

Thanks!

Closed by commit rP65de9e9f5e7f: Ignore "Auditors: author" when inferring auditors from commit messages (authored by epriestley, committed by epriestley). · Explain WhyMar 16 2017, 8:57 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
src/
applications/
audit/
editor/
12 lines

Diff 42104

View Options

src/applications/audit/editor/PhabricatorAuditEditor.php

Loading...
Phabricator · Built by Phacility