diff --git a/src/applications/differential/controller/DifferentialController.php b/src/applications/differential/controller/DifferentialController.php --- a/src/applications/differential/controller/DifferentialController.php +++ b/src/applications/differential/controller/DifferentialController.php @@ -54,9 +54,7 @@ $toc_view->setAuthorityPackages($packages); } - // TODO: For Subversion, we should adjust these paths to be relative to - // the repository root where possible. - $paths = mpull($changesets, 'getFilename'); + $paths = mpull($changesets, 'getOwnersFilename'); $control_query = id(new PhabricatorOwnersPackageQuery()) ->setViewer($viewer) @@ -83,7 +81,7 @@ if ($have_owners) { $packages = $control_query->getControllingPackagesForPath( $repository_phid, - $changeset->getFilename()); + $changeset->getOwnersFilename()); $item->setPackages($packages); } diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1857,6 +1857,4 @@ $acting_phid); } - - } diff --git a/src/applications/differential/storage/DifferentialChangeset.php b/src/applications/differential/storage/DifferentialChangeset.php --- a/src/applications/differential/storage/DifferentialChangeset.php +++ b/src/applications/differential/storage/DifferentialChangeset.php @@ -75,6 +75,23 @@ return $name; } + public function getOwnersFilename() { + // TODO: For Subversion, we should adjust these paths to be relative to + // the repository root where possible. + + $path = $this->getFilename(); + + if (!isset($path[0])) { + return '/'; + } + + if ($path[0] != '/') { + $path = '/'.$path; + } + + return $path; + } + public function addUnsavedHunk(DifferentialHunk $hunk) { if ($this->hunks === self::ATTACHABLE) { $this->hunks = array(); diff --git a/src/applications/differential/storage/DifferentialReviewer.php b/src/applications/differential/storage/DifferentialReviewer.php --- a/src/applications/differential/storage/DifferentialReviewer.php +++ b/src/applications/differential/storage/DifferentialReviewer.php @@ -39,6 +39,11 @@ return (phid_get_type($this->getReviewerPHID()) == $user_type); } + public function isPackage() { + $package_type = PhabricatorOwnersPackagePHIDType::TYPECONST; + return (phid_get_type($this->getReviewerPHID()) == $package_type); + } + public function attachAuthority(PhabricatorUser $user, $has_authority) { $this->authority[$user->getCacheFragment()] = $has_authority; return $this; diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -48,6 +48,7 @@ private $customFields = self::ATTACHABLE; private $drafts = array(); private $flags = array(); + private $forceMap = array(); const TABLE_COMMIT = 'differential_commit'; @@ -245,6 +246,243 @@ return $this; } + public function canReviewerForceAccept( + PhabricatorUser $viewer, + DifferentialReviewer $reviewer) { + + if (!$reviewer->isPackage()) { + return false; + } + + $map = $this->getReviewerForceAcceptMap($viewer); + if (!$map) { + return false; + } + + if (isset($map[$reviewer->getReviewerPHID()])) { + return true; + } + + return false; + } + + private function getReviewerForceAcceptMap(PhabricatorUser $viewer) { + $fragment = $viewer->getCacheFragment(); + + if (!array_key_exists($fragment, $this->forceMap)) { + $map = $this->newReviewerForceAcceptMap($viewer); + $this->forceMap[$fragment] = $map; + } + + return $this->forceMap[$fragment]; + } + + private function newReviewerForceAcceptMap(PhabricatorUser $viewer) { + $diff = $this->getActiveDiff(); + if (!$diff) { + return null; + } + + $repository_phid = $diff->getRepositoryPHID(); + if (!$repository_phid) { + return null; + } + + $paths = array(); + + try { + $changesets = $diff->getChangesets(); + } catch (Exception $ex) { + $changesets = id(new DifferentialChangesetQuery()) + ->setViewer($viewer) + ->withDiffs(array($diff)) + ->execute(); + } + + foreach ($changesets as $changeset) { + $paths[] = $changeset->getOwnersFilename(); + } + + if (!$paths) { + return null; + } + + $reviewer_phids = array(); + foreach ($this->getReviewers() as $reviewer) { + if (!$reviewer->isPackage()) { + continue; + } + + $reviewer_phids[] = $reviewer->getReviewerPHID(); + } + + if (!$reviewer_phids) { + return null; + } + + // Load all the reviewing packages which have control over some of the + // paths in the change. These are packages which the actor may be able + // to force-accept on behalf of. + $control_query = id(new PhabricatorOwnersPackageQuery()) + ->setViewer($viewer) + ->withStatuses(array(PhabricatorOwnersPackage::STATUS_ACTIVE)) + ->withPHIDs($reviewer_phids) + ->withControl($repository_phid, $paths); + $control_packages = $control_query->execute(); + if (!$control_packages) { + return null; + } + + // Load all the packages which have potential control over some of the + // paths in the change and are owned by the actor. These are packages + // which the actor may be able to use their authority over to gain the + // ability to force-accept for other packages. This query doesn't apply + // dominion rules yet, and we'll bypass those rules later on. + $authority_query = id(new PhabricatorOwnersPackageQuery()) + ->setViewer($viewer) + ->withStatuses(array(PhabricatorOwnersPackage::STATUS_ACTIVE)) + ->withAuthorityPHIDs(array($viewer->getPHID())) + ->withControl($repository_phid, $paths); + $authority_packages = $authority_query->execute(); + if (!$authority_packages) { + return null; + } + $authority_packages = mpull($authority_packages, null, 'getPHID'); + + // Build a map from each path in the revision to the reviewer packages + // which control it. + $control_map = array(); + foreach ($paths as $path) { + $control_packages = $control_query->getControllingPackagesForPath( + $repository_phid, + $path); + + // Remove packages which the viewer has authority over. We don't need + // to check these for force-accept because they can just accept them + // normally. + $control_packages = mpull($control_packages, null, 'getPHID'); + foreach ($control_packages as $phid => $control_package) { + if (isset($authority_packages[$phid])) { + unset($control_packages[$phid]); + } + } + + if (!$control_packages) { + continue; + } + + $control_map[$path] = $control_packages; + } + + if (!$control_map) { + return null; + } + + // From here on out, we only care about paths which we have at least one + // controlling package for. + $paths = array_keys($control_map); + + // Now, build a map from each path to the packages which would control it + // if there were no dominion rules. + $authority_map = array(); + foreach ($paths as $path) { + $authority_packages = $authority_query->getControllingPackagesForPath( + $repository_phid, + $path, + $ignore_dominion = true); + + $authority_map[$path] = mpull($authority_packages, null, 'getPHID'); + } + + // For each path, find the most general package that the viewer has + // authority over. For example, we'll prefer a package that owns "/" to a + // package that owns "/src/". + $force_map = array(); + foreach ($authority_map as $path => $package_map) { + $path_fragments = PhabricatorOwnersPackage::splitPath($path); + $fragment_count = count($path_fragments); + + // Find the package that we have authority over which has the most + // general match for this path. + $best_match = null; + $best_package = null; + foreach ($package_map as $package_phid => $package) { + $package_paths = $package->getPathsForRepository($repository_phid); + foreach ($package_paths as $package_path) { + + // NOTE: A strength of 0 means "no match". A strength of 1 means + // that we matched "/", so we can not possibly find another stronger + // match. + + $strength = $package_path->getPathMatchStrength( + $path_fragments, + $fragment_count); + if (!$strength) { + continue; + } + + if ($strength < $best_match || !$best_package) { + $best_match = $strength; + $best_package = $package; + if ($strength == 1) { + break 2; + } + } + } + } + + if ($best_package) { + $force_map[$path] = array( + 'strength' => $best_match, + 'package' => $best_package, + ); + } + } + + // For each path which the viewer owns a package for, find other packages + // which that authority can be used to force-accept. Once we find a way to + // force-accept a package, we don't need to keep loooking. + $has_control = array(); + foreach ($force_map as $path => $spec) { + $path_fragments = PhabricatorOwnersPackage::splitPath($path); + $fragment_count = count($path_fragments); + + $authority_strength = $spec['strength']; + + $control_packages = $control_map[$path]; + foreach ($control_packages as $control_phid => $control_package) { + if (isset($has_control[$control_phid])) { + continue; + } + + $control_paths = $control_package->getPathsForRepository( + $repository_phid); + foreach ($control_paths as $control_path) { + $strength = $control_path->getPathMatchStrength( + $path_fragments, + $fragment_count); + + if (!$strength) { + continue; + } + + if ($strength > $authority_strength) { + $authority = $spec['package']; + $has_control[$control_phid] = array( + 'authority' => $authority, + 'phid' => $authority->getPHID(), + ); + break; + } + } + } + } + + // Return a map from packages which may be force accepted to the packages + // which permit that forced acceptance. + return ipull($has_control, 'phid'); + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php --- a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php @@ -84,11 +84,18 @@ } } + $default_unchecked = array(); foreach ($reviewers as $reviewer) { + $reviewer_phid = $reviewer->getReviewerPHID(); + if (!$reviewer->hasAuthority($viewer)) { // If the viewer doesn't have authority to act on behalf of a reviewer, - // don't include that reviewer as an option. - continue; + // we check if they can accept by force. + if ($revision->canReviewerForceAccept($viewer, $reviewer)) { + $default_unchecked[$reviewer_phid] = true; + } else { + continue; + } } if ($reviewer->isAccepted($diff_phid)) { @@ -97,20 +104,37 @@ continue; } - $reviewer_phid = $reviewer->getReviewerPHID(); $reviewer_phids[$reviewer_phid] = $reviewer_phid; } $handles = $viewer->loadHandles($reviewer_phids); + $head = array(); + $tail = array(); foreach ($reviewer_phids as $reviewer_phid) { - $options[$reviewer_phid] = pht( - 'Accept as %s', - $viewer->renderHandle($reviewer_phid)); + $is_force = isset($default_unchecked[$reviewer_phid]); - $value[] = $reviewer_phid; + if ($is_force) { + $tail[] = $reviewer_phid; + + $options[$reviewer_phid] = pht( + 'Force accept as %s', + $viewer->renderHandle($reviewer_phid)); + } else { + $head[] = $reviewer_phid; + $value[] = $reviewer_phid; + + $options[$reviewer_phid] = pht( + 'Accept as %s', + $viewer->renderHandle($reviewer_phid)); + } } + // Reorder reviewers so "force accept" reviewers come at the end. + $options = + array_select_keys($options, $head) + + array_select_keys($options, $tail); + return array($options, $value); } diff --git a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php --- a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php @@ -122,7 +122,12 @@ $field->setActionConflictKey('revision.action'); list($options, $value) = $this->getActionOptions($viewer, $revision); - if (count($options) > 1) { + + // Show the options if the user can select on behalf of two or more + // reviewers, or can force-accept on behalf of one or more reviewers. + $can_multi = (count($options) > 1); + $can_force = (count($value) < count($options)); + if ($can_multi || $can_force) { $field->setOptions($options); $field->setValue($value); } diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php --- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php @@ -109,7 +109,17 @@ // the desired set of states. foreach ($revision->getReviewers() as $reviewer) { if (!$reviewer->hasAuthority($viewer)) { - continue; + $can_force = false; + + if ($is_accepted) { + if ($revision->canReviewerForceAccept($viewer, $reviewer)) { + $can_force = true; + } + } + + if (!$can_force) { + continue; + } } $status = $reviewer->getReviewerStatus(); @@ -152,11 +162,21 @@ // reviewers you have authority for. When you resign, you only affect // yourself. $with_authority = ($status != DifferentialReviewerStatus::STATUS_RESIGNED); + $with_force = ($status == DifferentialReviewerStatus::STATUS_ACCEPTED); + if ($with_authority) { foreach ($revision->getReviewers() as $reviewer) { - if ($reviewer->hasAuthority($viewer)) { - $map[$reviewer->getReviewerPHID()] = $status; + if (!$reviewer->hasAuthority($viewer)) { + if (!$with_force) { + continue; + } + + if (!$revision->canReviewerForceAccept($viewer, $reviewer)) { + continue; + } } + + $map[$reviewer->getReviewerPHID()] = $status; } } diff --git a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php --- a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php +++ b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php @@ -348,7 +348,10 @@ * * @return list List of controlling packages. */ - public function getControllingPackagesForPath($repository_phid, $path) { + public function getControllingPackagesForPath( + $repository_phid, + $path, + $ignore_dominion = false) { $path = (string)$path; if (!isset($this->controlMap[$repository_phid][$path])) { @@ -382,9 +385,14 @@ } if ($best_match && $include) { + if ($ignore_dominion) { + $is_weak = false; + } else { + $is_weak = ($package->getDominion() == $weak_dominion); + } $matches[$package_id] = array( 'strength' => $best_match, - 'weak' => ($package->getDominion() == $weak_dominion), + 'weak' => $is_weak, 'package' => $package, ); }