diff --git a/resources/sql/autopatches/20160516.owners.01.dominion.sql b/resources/sql/autopatches/20160516.owners.01.dominion.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20160516.owners.01.dominion.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_owners.owners_package + ADD dominion VARCHAR(32) NOT NULL COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20160516.owners.02.dominionstrong.sql b/resources/sql/autopatches/20160516.owners.02.dominionstrong.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20160516.owners.02.dominionstrong.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_owners.owners_package + SET dominion = 'strong' WHERE dominion = ''; 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 @@ -1723,6 +1723,10 @@ $paths[] = $path_prefix.'/'.$changeset->getFilename(); } + // Save the affected paths; we'll use them later to query Owners. This + // uses the un-expanded paths. + $this->affectedPaths = $paths; + // Mark this as also touching all parent paths, so you can see all pending // changes to any file within a directory. $all_paths = array(); @@ -1733,9 +1737,6 @@ } $all_paths = array_keys($all_paths); - // Save the affected paths; we'll use them later to query Owners. - $this->affectedPaths = $all_paths; - $path_ids = PhabricatorRepositoryCommitChangeParserWorker::lookupOrCreatePaths( $all_paths); diff --git a/src/applications/owners/controller/PhabricatorOwnersDetailController.php b/src/applications/owners/controller/PhabricatorOwnersDetailController.php --- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php +++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php @@ -184,6 +184,13 @@ } $view->addProperty(pht('Owners'), $owner_list); + + $dominion = $package->getDominion(); + $dominion_map = PhabricatorOwnersPackage::getDominionOptionsMap(); + $spec = idx($dominion_map, $dominion, array()); + $name = idx($spec, 'short', $dominion); + $view->addProperty(pht('Dominion'), $name); + $auto = $package->getAutoReview(); $autoreview_map = PhabricatorOwnersPackage::getAutoreviewOptionsMap(); $spec = idx($autoreview_map, $auto, array()); diff --git a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php --- a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php +++ b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php @@ -87,6 +87,9 @@ $autoreview_map = PhabricatorOwnersPackage::getAutoreviewOptionsMap(); $autoreview_map = ipull($autoreview_map, 'name'); + $dominion_map = PhabricatorOwnersPackage::getDominionOptionsMap(); + $dominion_map = ipull($dominion_map, 'name'); + return array( id(new PhabricatorTextEditField()) ->setKey('name') @@ -104,6 +107,16 @@ ->setIsCopyable(true) ->setValue($object->getOwnerPHIDs()), id(new PhabricatorSelectEditField()) + ->setKey('dominion') + ->setLabel(pht('Dominion')) + ->setDescription( + pht('Change package dominion rules.')) + ->setTransactionType( + PhabricatorOwnersPackageTransaction::TYPE_DOMINION) + ->setIsCopyable(true) + ->setValue($object->getDominion()) + ->setOptions($dominion_map), + id(new PhabricatorSelectEditField()) ->setKey('autoReview') ->setLabel(pht('Auto Review')) ->setDescription( diff --git a/src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php b/src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php --- a/src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php +++ b/src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php @@ -21,6 +21,7 @@ $types[] = PhabricatorOwnersPackageTransaction::TYPE_PATHS; $types[] = PhabricatorOwnersPackageTransaction::TYPE_STATUS; $types[] = PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW; + $types[] = PhabricatorOwnersPackageTransaction::TYPE_DOMINION; $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY; $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY; @@ -50,6 +51,8 @@ return $object->getStatus(); case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW: return $object->getAutoReview(); + case PhabricatorOwnersPackageTransaction::TYPE_DOMINION: + return $object->getDominion(); } } @@ -62,6 +65,7 @@ case PhabricatorOwnersPackageTransaction::TYPE_DESCRIPTION: case PhabricatorOwnersPackageTransaction::TYPE_STATUS: case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW: + case PhabricatorOwnersPackageTransaction::TYPE_DOMINION: return $xaction->getNewValue(); case PhabricatorOwnersPackageTransaction::TYPE_PATHS: $new = $xaction->getNewValue(); @@ -120,6 +124,9 @@ case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW: $object->setAutoReview($xaction->getNewValue()); return; + case PhabricatorOwnersPackageTransaction::TYPE_DOMINION: + $object->setDominion($xaction->getNewValue()); + return; } return parent::applyCustomInternalTransaction($object, $xaction); @@ -135,6 +142,7 @@ case PhabricatorOwnersPackageTransaction::TYPE_AUDITING: case PhabricatorOwnersPackageTransaction::TYPE_STATUS: case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW: + case PhabricatorOwnersPackageTransaction::TYPE_DOMINION: return; case PhabricatorOwnersPackageTransaction::TYPE_OWNERS: $old = $xaction->getOldValue(); @@ -249,6 +257,26 @@ } } break; + case PhabricatorOwnersPackageTransaction::TYPE_DOMINION: + $map = PhabricatorOwnersPackage::getDominionOptionsMap(); + foreach ($xactions as $xaction) { + $new = $xaction->getNewValue(); + + if (empty($map[$new])) { + $valid = array_keys($map); + + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht( + 'Dominion setting "%s" is not valid. '. + 'Valid settings are: %s.', + $new, + implode(', ', $valid)), + $xaction); + } + } + break; case PhabricatorOwnersPackageTransaction::TYPE_PATHS: if (!$xactions) { continue; 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 @@ -351,6 +351,7 @@ } $packages = $this->controlResults; + $weak_dominion = PhabricatorOwnersPackage::DOMINION_WEAK; $matches = array(); foreach ($packages as $package_id => $package) { @@ -373,6 +374,7 @@ if ($best_match && $include) { $matches[$package_id] = array( 'strength' => $best_match, + 'weak' => ($package->getDominion() == $weak_dominion), 'package' => $package, ); } @@ -381,6 +383,18 @@ $matches = isort($matches, 'strength'); $matches = array_reverse($matches); + $first_id = null; + foreach ($matches as $package_id => $match) { + if ($first_id === null) { + $first_id = $package_id; + continue; + } + + if ($match['weak']) { + unset($matches[$package_id]); + } + } + return array_values(ipull($matches, 'package')); } diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php --- a/src/applications/owners/storage/PhabricatorOwnersPackage.php +++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php @@ -21,6 +21,7 @@ protected $status; protected $viewPolicy; protected $editPolicy; + protected $dominion; private $paths = self::ATTACHABLE; private $owners = self::ATTACHABLE; @@ -51,6 +52,7 @@ return id(new PhabricatorOwnersPackage()) ->setAuditingEnabled(0) ->setAutoReview(self::AUTOREVIEW_NONE) + ->setDominion(self::DOMINION_STRONG) ->setViewPolicy($view_policy) ->setEditPolicy($edit_policy) ->attachPaths(array()) @@ -83,6 +85,19 @@ ); } + public static function getDominionOptionsMap() { + return array( + self::DOMINION_STRONG => array( + 'name' => pht('Strong (Control All Paths)'), + 'short' => pht('Strong'), + ), + self::DOMINION_WEAK => array( + 'name' => pht('Weak (Control Unowned Paths)'), + 'short' => pht('Weak'), + ), + ); + } + protected function getConfiguration() { return array( // This information is better available from the history table. @@ -97,6 +112,7 @@ 'mailKey' => 'bytes20', 'status' => 'text32', 'autoReview' => 'text32', + 'dominion' => 'text32', ), ) + parent::getConfiguration(); } @@ -193,7 +209,7 @@ foreach (array_chunk(array_keys($fragments), 128) as $chunk) { $rows[] = queryfx_all( $conn, - 'SELECT pkg.id, "strong" dominion, p.excluded, p.path + 'SELECT pkg.id, pkg.dominion, p.excluded, p.path FROM %T pkg JOIN %T p ON p.packageID = pkg.id WHERE p.path IN (%Ls) %Q', $package->getTableName(), diff --git a/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php b/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php --- a/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php +++ b/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php @@ -11,6 +11,7 @@ const TYPE_PATHS = 'owners.paths'; const TYPE_STATUS = 'owners.status'; const TYPE_AUTOREVIEW = 'owners.autoreview'; + const TYPE_DOMINION = 'owners.dominion'; public function getApplicationName() { return 'owners'; @@ -156,6 +157,18 @@ $this->renderHandleLink($author_phid), $old, $new); + case self::TYPE_DOMINION: + $map = PhabricatorOwnersPackage::getDominionOptionsMap(); + $map = ipull($map, 'short'); + + $old = idx($map, $old, $old); + $new = idx($map, $new, $new); + + return pht( + '%s adjusted package dominion rules from "%s" to "%s".', + $this->renderHandleLink($author_phid), + $old, + $new); } return parent::getTitle(); diff --git a/src/docs/user/userguide/owners.diviner b/src/docs/user/userguide/owners.diviner --- a/src/docs/user/userguide/owners.diviner +++ b/src/docs/user/userguide/owners.diviner @@ -45,6 +45,38 @@ 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 =========== @@ -93,4 +125,6 @@ "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.) +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.)