Page MenuHomePhabricator
Files F15458512

D15935.id38366.diff
No OneTemporary
Actions

D15935.id38366.diff
View Options

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
@@ -34,6 +34,9 @@
const AUTOREVIEW_REVIEW = 'review';
const AUTOREVIEW_BLOCK = 'block';
+ const DOMINION_STRONG = 'strong';
+ const DOMINION_WEAK = 'weak';
+
public static function initializeNewPackage(PhabricatorUser $actor) {
$app = id(new PhabricatorApplicationQuery())
->setViewer($actor)
@@ -190,7 +193,7 @@
foreach (array_chunk(array_keys($fragments), 128) as $chunk) {
$rows[] = queryfx_all(
$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
WHERE p.path IN (%Ls) %Q',
$package->getTableName(),
@@ -232,35 +235,100 @@
}
public static function findLongestPathsPerPackage(array $rows, array $paths) {
- $ids = array();
-
- foreach (igroup($rows, 'id') as $id => $package_paths) {
- $relevant_paths = array_select_keys(
- $paths,
- ipull($package_paths, 'path'));
-
- // For every package, remove all excluded paths.
- $remove = array();
- foreach ($package_paths as $package_path) {
- if ($package_path['excluded']) {
- $remove += idx($relevant_paths, $package_path['path'], array());
- unset($relevant_paths[$package_path['path']]);
+
+ // Build a map from each path to all the package paths which match it.
+ $path_hits = array();
+ $weak = array();
+ foreach ($rows as $row) {
+ $id = $row['id'];
+ $path = $row['path'];
+ $length = strlen($path);
+ $excluded = $row['excluded'];
+
+ if ($row['dominion'] === self::DOMINION_WEAK) {
+ $weak[$id] = true;
+ }
+
+ $matches = $paths[$path];
+ foreach ($matches as $match => $ignored) {
+ $path_hits[$match][] = array(
+ 'id' => $id,
+ 'excluded' => $excluded,
+ 'length' => $length,
+ );
+ }
+ }
+
+ // For each path, process the matching package paths to figure out which
+ // packages actually own it.
+ $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;
}
}
- if ($remove) {
- foreach ($relevant_paths as $fragment => $fragment_paths) {
- $relevant_paths[$fragment] = array_diff_key($fragment_paths, $remove);
+ $path_packages[$match] = $packages;
+ }
+
+ // 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]);
+ }
}
+
+ $path_packages[$match] = $packages;
}
+ }
- $relevant_paths = array_filter($relevant_paths);
- if ($relevant_paths) {
- $ids[$id] = max(array_map('strlen', array_keys($relevant_paths)));
+ // 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 $ids;
+ return $package_lengths;
}
public static function splitPath($path) {
diff --git a/src/applications/owners/storage/__tests__/PhabricatorOwnersPackageTestCase.php b/src/applications/owners/storage/__tests__/PhabricatorOwnersPackageTestCase.php
--- a/src/applications/owners/storage/__tests__/PhabricatorOwnersPackageTestCase.php
+++ b/src/applications/owners/storage/__tests__/PhabricatorOwnersPackageTestCase.php
@@ -4,9 +4,24 @@
public function testFindLongestPathsPerPackage() {
$rows = array(
- array('id' => 1, 'excluded' => 0, 'path' => 'src/'),
- array('id' => 1, 'excluded' => 1, 'path' => 'src/releeph/'),
- array('id' => 2, 'excluded' => 0, 'path' => 'src/releeph/'),
+ array(
+ 'id' => 1,
+ '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(
@@ -29,6 +44,62 @@
2 => strlen('src/releeph/'),
),
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));
+
+
}
}

File Metadata

Mime Type
text/plain
Expires
Tue, Apr 1, 4:11 AM (2 w, 1 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7594988
Default Alt Text
D15935.id38366.diff (7 KB)

Event Timeline

Phabricator · Built by Phacility