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
Unknown Object (File)
Fri, Dec 13, 2:24 AM
Unknown Object (File)
Fri, Dec 6, 1:26 PM
Unknown Object (File)
Fri, Dec 6, 8:30 AM
Unknown Object (File)
Fri, Dec 6, 4:55 AM
Unknown Object (File)
Wed, Nov 27, 10:21 PM
Unknown Object (File)
Tue, Nov 26, 2:45 PM
Unknown Object (File)
Nov 23 2024, 12:19 AM
Unknown Object (File)
Nov 19 2024, 5:15 PM
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
Branch
audit1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 16008
Build 21225: Run Core Tests
Build 21224: arc lint + arc unit

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 42100

View Options

src/applications/audit/editor/PhabricatorAuditEditor.php

Loading...
Phabricator · Built by Phacility