Page MenuHomePhabricator
Files F15283230

D17569.id42267.diff
No OneTemporary
Actions

D17569.id42267.diff
View Options

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<PhabricatorOwnersPackage> 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,
);
}

File Metadata

Mime Type
text/plain
Expires
Wed, Mar 5, 6:18 AM (3 w, 5 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7221439
Default Alt Text
D17569.id42267.diff (16 KB)

Event Timeline

Phabricator · Built by Phacility