Paths

Table of Contentst

Differential D17507

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

Authored by epriestley on Mar 16 2017, 7:25 PM.

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.

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

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

Path

Size

src/

applications/

audit/

editor/

PhabricatorAuditEditor.php

12 lines

Diff 42104

View Options

src/applications/audit/editor/PhabricatorAuditEditor.php

Ignore "Auditors: author" when inferring auditors from commit messagesClosedPublicActions