diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php index 61a615950d..3f41226b9f 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php @@ -1,144 +1,168 @@ triggerOwnerAudits($repository, $commit); $commit->writeImportStatusFlag( PhabricatorRepositoryCommit::IMPORTED_OWNERS); if ($this->shouldQueueFollowupTasks()) { $this->queueTask( 'PhabricatorRepositoryCommitHeraldWorker', array( 'commitID' => $commit->getID(), )); } } private function triggerOwnerAudits( PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit) { if (!$repository->shouldPublish()) { return; } $affected_paths = PhabricatorOwnerPathQuery::loadAffectedPaths( $repository, $commit, PhabricatorUser::getOmnipotentUser()); + $affected_packages = PhabricatorOwnersPackage::loadAffectedPackages( $repository, $affected_paths); - if ($affected_packages) { - $requests = id(new PhabricatorRepositoryAuditRequest()) - ->loadAllWhere( - 'commitPHID = %s', - $commit->getPHID()); - $requests = mpull($requests, null, 'getAuditorPHID'); - - foreach ($affected_packages as $package) { - $request = idx($requests, $package->getPHID()); - if ($request) { - // Don't update request if it exists already. - continue; - } + if (!$affected_packages) { + return; + } - if ($package->isArchived()) { - // Don't trigger audits if the package is archived. - continue; - } + $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere( + 'commitID = %d', + $commit->getID()); + $commit->attachCommitData($data); - if ($package->getAuditingEnabled()) { - $reasons = $this->checkAuditReasons($commit, $package); - if ($reasons) { - $audit_status = - PhabricatorAuditStatusConstants::AUDIT_REQUIRED; - } else { - $audit_status = - PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED; - } - } else { - $reasons = array(); - $audit_status = PhabricatorAuditStatusConstants::NONE; - } + $author_phid = $data->getCommitDetail('authorPHID'); + $revision_id = $data->getCommitDetail('differential.revisionID'); + if ($revision_id) { + $revision = id(new DifferentialRevisionQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withIDs(array($revision_id)) + ->needReviewerStatus(true) + ->executeOne(); + } else { + $revision = null; + } - $relationship = new PhabricatorRepositoryAuditRequest(); - $relationship->setAuditorPHID($package->getPHID()); - $relationship->setCommitPHID($commit->getPHID()); - $relationship->setAuditReasons($reasons); - $relationship->setAuditStatus($audit_status); + $requests = id(new PhabricatorRepositoryAuditRequest()) + ->loadAllWhere( + 'commitPHID = %s', + $commit->getPHID()); + $requests = mpull($requests, null, 'getAuditorPHID'); - $relationship->save(); - $requests[$package->getPHID()] = $relationship; + foreach ($affected_packages as $package) { + $request = idx($requests, $package->getPHID()); + if ($request) { + // Don't update request if it exists already. + continue; } - $commit->updateAuditStatus($requests); - $commit->save(); + if ($package->isArchived()) { + // Don't trigger audits if the package is archived. + continue; + } + + if ($package->getAuditingEnabled()) { + $reasons = $this->checkAuditReasons( + $commit, + $package, + $author_phid, + $revision); + + if ($reasons) { + $audit_status = PhabricatorAuditStatusConstants::AUDIT_REQUIRED; + } else { + $audit_status = PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED; + } + } else { + $reasons = array(); + $audit_status = PhabricatorAuditStatusConstants::NONE; + } + + $relationship = new PhabricatorRepositoryAuditRequest(); + $relationship->setAuditorPHID($package->getPHID()); + $relationship->setCommitPHID($commit->getPHID()); + $relationship->setAuditReasons($reasons); + $relationship->setAuditStatus($audit_status); + + $relationship->save(); + + $requests[$package->getPHID()] = $relationship; } + + $commit->updateAuditStatus($requests); + $commit->save(); } private function checkAuditReasons( PhabricatorRepositoryCommit $commit, - PhabricatorOwnersPackage $package) { + PhabricatorOwnersPackage $package, + $author_phid, + $revision) { - $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere( - 'commitID = %d', - $commit->getID()); + $owner_phids = PhabricatorOwnersOwner::loadAffiliatedUserPHIDs( + array( + $package->getID(), + )); + $owner_phids = array_fuse($owner_phids); $reasons = array(); - if ($data->getCommitDetail('vsDiff')) { - $reasons[] = pht('Changed After Revision Was Accepted'); + if (!$author_phid) { + $reasons[] = pht('Commit Author Not Recognized'); + } else if (isset($owner_phids[$author_phid])) { + return $reasons; } - $commit_author_phid = $data->getCommitDetail('authorPHID'); - if (!$commit_author_phid) { - $reasons[] = pht('Commit Author Not Recognized'); + if (!$revision) { + $reasons[] = pht('No Revision Specified'); + return $reasons; } - $revision_id = $data->getCommitDetail('differential.revisionID'); + $accepted_statuses = array( + DifferentialReviewerStatus::STATUS_ACCEPTED, + DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER, + ); + $accepted_statuses = array_fuse($accepted_statuses); - $revision_author_phid = null; - $commit_reviewedby_phid = null; + $found_accept = false; + foreach ($revision->getReviewerStatus() as $reviewer) { + $reviewer_phid = $reviewer->getReviewerPHID(); - if ($revision_id) { - $revision = id(new DifferentialRevisionQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withIDs(array($revision_id)) - ->executeOne(); - if ($revision) { - $revision_author_phid = $revision->getAuthorPHID(); - $commit_reviewedby_phid = $data->getCommitDetail('reviewerPHID'); - if ($revision_author_phid !== $commit_author_phid) { - $reasons[] = pht('Author Not Matching with Revision'); - } - } else { - $reasons[] = pht('Revision Not Found'); + // If this reviewer isn't a package owner, just ignore them. + if (empty($owner_phids[$reviewer_phid])) { + continue; } - } else { - $reasons[] = pht('No Revision Specified'); + // If this reviewer accepted the revision and owns the package, we're + // all clear and do not need to trigger an audit. + if (isset($accepted_statuses[$reviewer->getStatus()])) { + $found_accept = true; + break; + } } - $owners_phids = PhabricatorOwnersOwner::loadAffiliatedUserPHIDs( - array($package->getID())); - - if (!($commit_author_phid && in_array($commit_author_phid, $owners_phids) || - $commit_reviewedby_phid && in_array($commit_reviewedby_phid, - $owners_phids))) { + if (!$found_accept) { $reasons[] = pht('Owners Not Involved'); } return $reasons; } } diff --git a/src/docs/user/userguide/owners.diviner b/src/docs/user/userguide/owners.diviner index 4aa6a29f2d..a9a52bfab8 100644 --- a/src/docs/user/userguide/owners.diviner +++ b/src/docs/user/userguide/owners.diviner @@ -1,130 +1,154 @@ @title Owners User Guide @group userguide Group files in a codebase into packages and define ownership. Overview ======== The Owners application allows you to group files in a codebase (or across codebases) into packages. This can make it easier to reference a module or subsystem in other applications, like Herald. Creating a Package ================== To create a package, choose a name and add some files which belong to the package. For example, you might define an "iOS Application" package by including these paths: /conf/ios/ /src/ios/ /shared/assets/mobile/ Any files in those directories are considered to be part of the package, and you can now conveniently refer to them (for example, in a Herald rule) by refering to the package instead of copy/pasting a huge regular expression into a bunch of places. If new source files are later added, or the scope of the package otherwise expands or contracts, you can edit the package definition to keep things updated. You can use "exclude" paths to ignore subdirectories which would otherwise be considered part of the package. For example, you might exclude a path like this: /conf/ios/generated/ Perhaps that directory contains some generated configuration which frequently changes, and which you aren't concerned about. After creating a package, files the package contains will be identified as belonging to the package when you look at them in Diffusion, or look at changes which affect them in Diffusion or Differential. Dominion ======== The **Dominion** option allows you to control how ownership cascades when multiple packages own a path. The dominion rules are: **Strong Dominion.** This is the default. In this mode, the package will always own all files matching its configured paths, even if another package also owns them. For example, if the package owns `a/`, it will always own `a/b/c.z` even if another package owns `a/b/`. In this case, both packages will own `a/b/c.z`. This mode prevents users from stealing files away from the package by defining more narrow ownership rules in new packages, but enforces hierarchical ownership rules. **Weak Dominion.** In this mode, the package will only own files which do not match a more specific path in another package. For example, if the package owns `a/` but another package owns `a/b/`, the package will no longer consider `a/b/c.z` to be a file it owns because another package matches the path with a more specific rule. This mode lets you to define rules without implicit hierarchical ownership, but allows users to steal files away from a package by defining a more specific package. For more details on files which match multiple packages, see "Files in Multiple Packages", below. Auto Review =========== You can configure **Auto Review** for packages. When a new code review is created in Differential which affects code in a package, the package can automatically be added as a subscriber or reviewer. The available settings are: - **No Autoreview**: This package will not be added to new reviews. - **Subscribe to Changes**: This package will be added to reviews as a subscriber. Owners will be notified of changes, but not required to act. - **Review Changes**: This package will be added to reviews as a reviewer. Reviews will appear on the dashboards of package owners. - **Review Changes (Blocking)** This package will be added to reviews as a blocking reviewer. A package owner will be required to accept changes before they may land. NOTE: These rules **do not trigger** if the change author is a package owner. They only apply to changes made by users who aren't already owners. +These rules also do not trigger if the package has been archived. + The intent of this feature is to make it easy to configure simple, reasonable behaviors. If you want more tailored or specific triggers, you can write more powerful rules by using Herald. +Auditing +======== + +You can automatically trigger audits on unreviewed code by configuring +**Auditing**. The available settings are: + + - **Disabled**: Do not trigger audits. + - **Enabled**: Trigger audits. + +When enabled, audits are triggered for commits which: + + - affect code owned by the package; + - were not authored by a package owner; and + - were not accepted by a package owner. + +Audits do not trigger if the package has been archived. + +The intent of this feature is to make it easy to configure simple auditing +behavior. If you want more powerful auditing behavior, you can use Herald to +write more sophisticated rules. + + Files in Multiple Packages ========================== Multiple packages may own the same file. For example, both the "Android Application" and the "iOS Application" packages might own a path like this, containing resources used by both: /shared/assets/mobile/ If both packages own this directory, files in the directory are considered to be part of both packages. Packages do not need to have claims of equal specificity to own files. For example, if you have a "Design Assets" package which owns this path: /shared/assets/ ...it will //also// own all of the files in the `mobile/` subdirectory. In this configuration, these files are part of three packages: "iOS Application", "Android Application", and "Design Assets". (You can use an "exclude" rule if you want to make a different package with a more specific claim the owner of a file or subdirectory. You can also change the **Dominion** setting for a package to let it give up ownership of paths owned by another package.)