@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator

When an Owners package accepts a revision, count that as an "involved owner" for the purposes of audit

Summary:
Depends on D20129. Ref T13244. See PHI1058. When a revision has an "Accept" from a package, count the owners as "involved" in the change whether or not any actual human owners are actually accepting reviewers.

If a user owns "/" and uses "force accept" to cause "/src/javascript" to accept, or a user who legitimately owns "/src/javascript" accepts on behalf of the package but not on behalf of themselves (for whatever reason), it generally makes practical sense that these changes have owners involved in them (i.e., that's what a normal user would expect in both cases) and don't need to trigger audits under "no involvement" rules.

Test Plan: Used `bin/repository reparse --force --owners <commit>` to trigger audit logic. Saw a commit owned by `O1` with a revision counted as "involved" when `O1` had accepted the revision, even though no actual human owner had accepted it.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20130

+12 -4
+10 -3
src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
··· 200 200 )); 201 201 $owner_phids = array_fuse($owner_phids); 202 202 203 + // For the purposes of deciding whether the owners were involved in the 204 + // revision or not, consider a review by the package itself to count as 205 + // involvement. This can happen when human reviewers force-accept on 206 + // behalf of packages they don't own but have authority over. 207 + $owner_phids[$package->getPHID()] = $package->getPHID(); 208 + 203 209 // If the commit author is identifiable and a package owner, they're 204 210 // involved. 205 211 if ($author_phid) { ··· 225 231 foreach ($revision->getReviewers() as $reviewer) { 226 232 $reviewer_phid = $reviewer->getReviewerPHID(); 227 233 228 - // If this reviewer isn't a package owner, just ignore them. 234 + // If this reviewer isn't a package owner or the package itself, 235 + // just ignore them. 229 236 if (empty($owner_phids[$reviewer_phid])) { 230 237 continue; 231 238 } 232 239 233 - // If this reviewer accepted the revision and owns the package, we've 234 - // found an involved owner. 240 + // If this reviewer accepted the revision and owns the package (or is 241 + // the package), we've found an involved owner. 235 242 if (isset($accepted_statuses[$reviewer->getReviewerStatus()])) { 236 243 $found_accept = true; 237 244 break;
+2 -1
src/docs/user/userguide/owners.diviner
··· 129 129 130 130 - affect code owned by the package; 131 131 - were not authored by a package owner; and 132 - - were not accepted (in Differential) by a package owner. 132 + - were not accepted (in Differential) by a package owner or the package 133 + itself. 133 134 134 135 Audits do not trigger if the package has been archived. 135 136