1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-19 05:12:41 +01:00

Add "Dominion" rules for Owners Packages

Summary:
Ref T10939. This supports two settings for packages (although they can't be configured yet):

  - **Strong Dominion**: If the package owns `a/`, it always owns every subpath, even if another package also owns the subpath. For example, if I own `src/differential/`, I always own it even if someone else claims `src/differential/js/` as part of the "Javascript" package. This is the current behavior, and the default.
  - **Weak Dominion**: If the package owns `a/`, but another package owns `a/b/`, the package gives up control of those paths and no longer owns paths in `a/b/`. This is a new behavior which can make defining some types of packages easier.

In the next change, I'll allow users to switch these modes and document what they mean.

Test Plan:
  - Ran existing unit tests.
  - Added new unit tests.

Reviewers: chad

Reviewed By: chad

Subscribers: joel

Maniphest Tasks: T10939

Differential Revision: https://secure.phabricator.com/D15935
This commit is contained in:
epriestley 2016-05-16 18:57:34 -07:00
parent 29a060d7f1
commit 6cb2bde48d
2 changed files with 165 additions and 26 deletions

View file

@ -34,6 +34,9 @@ final class PhabricatorOwnersPackage
const AUTOREVIEW_REVIEW = 'review'; const AUTOREVIEW_REVIEW = 'review';
const AUTOREVIEW_BLOCK = 'block'; const AUTOREVIEW_BLOCK = 'block';
const DOMINION_STRONG = 'strong';
const DOMINION_WEAK = 'weak';
public static function initializeNewPackage(PhabricatorUser $actor) { public static function initializeNewPackage(PhabricatorUser $actor) {
$app = id(new PhabricatorApplicationQuery()) $app = id(new PhabricatorApplicationQuery())
->setViewer($actor) ->setViewer($actor)
@ -190,7 +193,7 @@ final class PhabricatorOwnersPackage
foreach (array_chunk(array_keys($fragments), 128) as $chunk) { foreach (array_chunk(array_keys($fragments), 128) as $chunk) {
$rows[] = queryfx_all( $rows[] = queryfx_all(
$conn, $conn,
'SELECT pkg.id, p.excluded, p.path 'SELECT pkg.id, "strong" dominion, p.excluded, p.path
FROM %T pkg JOIN %T p ON p.packageID = pkg.id FROM %T pkg JOIN %T p ON p.packageID = pkg.id
WHERE p.path IN (%Ls) %Q', WHERE p.path IN (%Ls) %Q',
$package->getTableName(), $package->getTableName(),
@ -232,35 +235,100 @@ final class PhabricatorOwnersPackage
} }
public static function findLongestPathsPerPackage(array $rows, array $paths) { public static function findLongestPathsPerPackage(array $rows, array $paths) {
$ids = array();
foreach (igroup($rows, 'id') as $id => $package_paths) { // Build a map from each path to all the package paths which match it.
$relevant_paths = array_select_keys( $path_hits = array();
$paths, $weak = array();
ipull($package_paths, 'path')); foreach ($rows as $row) {
$id = $row['id'];
$path = $row['path'];
$length = strlen($path);
$excluded = $row['excluded'];
// For every package, remove all excluded paths. if ($row['dominion'] === self::DOMINION_WEAK) {
$remove = array(); $weak[$id] = true;
foreach ($package_paths as $package_path) { }
if ($package_path['excluded']) {
$remove += idx($relevant_paths, $package_path['path'], array()); $matches = $paths[$path];
unset($relevant_paths[$package_path['path']]); foreach ($matches as $match => $ignored) {
$path_hits[$match][] = array(
'id' => $id,
'excluded' => $excluded,
'length' => $length,
);
} }
} }
if ($remove) { // For each path, process the matching package paths to figure out which
foreach ($relevant_paths as $fragment => $fragment_paths) { // packages actually own it.
$relevant_paths[$fragment] = array_diff_key($fragment_paths, $remove); $path_packages = array();
foreach ($path_hits as $match => $hits) {
$hits = isort($hits, 'length');
$packages = array();
foreach ($hits as $hit) {
$package_id = $hit['id'];
if ($hit['excluded']) {
unset($packages[$package_id]);
} else {
$packages[$package_id] = $hit;
} }
} }
$relevant_paths = array_filter($relevant_paths); $path_packages[$match] = $packages;
if ($relevant_paths) { }
$ids[$id] = max(array_map('strlen', array_keys($relevant_paths)));
// Remove packages with weak dominion rules that should cede control to
// a more specific package.
if ($weak) {
foreach ($path_packages as $match => $packages) {
$packages = isort($packages, 'length');
$packages = array_reverse($packages, true);
$first = null;
foreach ($packages as $package_id => $package) {
// If this is the first package we've encountered, note it and
// continue. We're iterating over the packages from longest to
// shortest match, so this package always has the strongest claim
// on the path.
if ($first === null) {
$first = $package_id;
continue;
}
// If this is the first package we saw, its claim stands even if it
// is a weak package.
if ($first === $package_id) {
continue;
}
// If this is a weak package and not the first package we saw,
// cede its claim to the stronger package.
if (isset($weak[$package_id])) {
unset($packages[$package_id]);
} }
} }
return $ids; $path_packages[$match] = $packages;
}
}
// For each package that owns at least one path, identify the longest
// path it owns.
$package_lengths = array();
foreach ($path_packages as $match => $hits) {
foreach ($hits as $hit) {
$length = $hit['length'];
$id = $hit['id'];
if (empty($package_lengths[$id])) {
$package_lengths[$id] = $length;
} else {
$package_lengths[$id] = max($package_lengths[$id], $length);
}
}
}
return $package_lengths;
} }
public static function splitPath($path) { public static function splitPath($path) {

View file

@ -4,9 +4,24 @@ final class PhabricatorOwnersPackageTestCase extends PhabricatorTestCase {
public function testFindLongestPathsPerPackage() { public function testFindLongestPathsPerPackage() {
$rows = array( $rows = array(
array('id' => 1, 'excluded' => 0, 'path' => 'src/'), array(
array('id' => 1, 'excluded' => 1, 'path' => 'src/releeph/'), 'id' => 1,
array('id' => 2, 'excluded' => 0, 'path' => 'src/releeph/'), 'excluded' => 0,
'dominion' => PhabricatorOwnersPackage::DOMINION_STRONG,
'path' => 'src/',
),
array(
'id' => 1,
'excluded' => 1,
'dominion' => PhabricatorOwnersPackage::DOMINION_STRONG,
'path' => 'src/releeph/',
),
array(
'id' => 2,
'excluded' => 0,
'dominion' => PhabricatorOwnersPackage::DOMINION_STRONG,
'path' => 'src/releeph/',
),
); );
$paths = array( $paths = array(
@ -29,6 +44,62 @@ final class PhabricatorOwnersPackageTestCase extends PhabricatorTestCase {
2 => strlen('src/releeph/'), 2 => strlen('src/releeph/'),
), ),
PhabricatorOwnersPackage::findLongestPathsPerPackage($rows, $paths)); PhabricatorOwnersPackage::findLongestPathsPerPackage($rows, $paths));
// Test packages with weak dominion. Here, only package #2 should own the
// path. Package #1's claim is ceded to Package #2 because it uses weak
// rules. Package #2 gets the claim even though it also has weak rules
// because there is no more-specific package.
$rows = array(
array(
'id' => 1,
'excluded' => 0,
'dominion' => PhabricatorOwnersPackage::DOMINION_WEAK,
'path' => 'src/',
),
array(
'id' => 2,
'excluded' => 0,
'dominion' => PhabricatorOwnersPackage::DOMINION_WEAK,
'path' => 'src/applications/',
),
);
$pvalue = array('src/applications/main/main.c' => true);
$paths = array(
'src/' => $pvalue,
'src/applications/' => $pvalue,
);
$this->assertEqual(
array(
2 => strlen('src/applications/'),
),
PhabricatorOwnersPackage::findLongestPathsPerPackage($rows, $paths));
// Now, add a more specific path to Package #1. This tests nested ownership
// in packages with weak dominion rules. This time, Package #1 should end
// up back on top, with Package #2 cedeing control to its more specific
// path.
$rows[] = array(
'id' => 1,
'excluded' => 0,
'dominion' => PhabricatorOwnersPackage::DOMINION_WEAK,
'path' => 'src/applications/main/',
);
$paths['src/applications/main/'] = $pvalue;
$this->assertEqual(
array(
1 => strlen('src/applications/main/'),
),
PhabricatorOwnersPackage::findLongestPathsPerPackage($rows, $paths));
} }
} }