Page MenuHomePhabricator
Files F15302440

D18064.diff
No OneTemporary
Actions

D18064.diff
View Options

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
@@ -398,13 +398,36 @@
}
}
+ // At each strength level, drop weak packages if there are also strong
+ // packages of the same strength.
+ $strength_map = igroup($matches, 'strength');
+ foreach ($strength_map as $strength => $package_list) {
+ $any_strong = false;
+ foreach ($package_list as $package_id => $package) {
+ if (!$package['weak']) {
+ $any_strong = true;
+ break;
+ }
+ }
+ if ($any_strong) {
+ foreach ($package_list as $package_id => $package) {
+ if ($package['weak']) {
+ unset($matches[$package_id]);
+ }
+ }
+ }
+ }
+
$matches = isort($matches, 'strength');
$matches = array_reverse($matches);
- $first_id = null;
+ $strongest = null;
foreach ($matches as $package_id => $match) {
- if ($first_id === null) {
- $first_id = $package_id;
+ if ($strongest === null) {
+ $strongest = $match['strength'];
+ }
+
+ if ($match['strength'] === $strongest) {
continue;
}
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
@@ -297,27 +297,54 @@
// a more specific package.
if ($weak) {
foreach ($path_packages as $match => $packages) {
+
+ // Group packages by length.
+ $length_map = array();
+ foreach ($packages as $package_id => $package) {
+ $length_map[$package['length']][$package_id] = $package;
+ }
+
+ // For each path length, remove all weak packages if there are any
+ // strong packages of the same length. This makes sure that if there
+ // are one or more strong claims on a particular path, only those
+ // claims stand.
+ foreach ($length_map as $package_list) {
+ $any_strong = false;
+ foreach ($package_list as $package_id => $package) {
+ if (!isset($weak[$package_id])) {
+ $any_strong = true;
+ break;
+ }
+ }
+
+ if ($any_strong) {
+ foreach ($package_list as $package_id => $package) {
+ if (isset($weak[$package_id])) {
+ unset($packages[$package_id]);
+ }
+ }
+ }
+ }
+
$packages = isort($packages, 'length');
$packages = array_reverse($packages, true);
- $first = null;
+ $best_length = 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've encountered, note its length.
+ // We're iterating over the packages from longest to shortest match,
+ // so packages of this length always have the best claim on the path.
+ if ($best_length === null) {
+ $best_length = $package['length'];
}
- // If this is the first package we saw, its claim stands even if it
- // is a weak package.
- if ($first === $package_id) {
+ // If this package has the same length as the best length, its claim
+ // stands.
+ if ($package['length'] === $best_length) {
continue;
}
- // If this is a weak package and not the first package we saw,
+ // If this is a weak package and does not have the best length,
// cede its claim to the stronger package.
if (isset($weak[$package_id])) {
unset($packages[$package_id]);
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
@@ -100,6 +100,95 @@
PhabricatorOwnersPackage::findLongestPathsPerPackage($rows, $paths));
+ // Test cases where multiple packages own the same path, with various
+ // dominion rules.
+
+ $main_c = 'src/applications/main/main.c';
+
+ $rules = array(
+ // All claims strong.
+ array(
+ PhabricatorOwnersPackage::DOMINION_STRONG,
+ PhabricatorOwnersPackage::DOMINION_STRONG,
+ PhabricatorOwnersPackage::DOMINION_STRONG,
+ ),
+ // All claims weak.
+ array(
+ PhabricatorOwnersPackage::DOMINION_WEAK,
+ PhabricatorOwnersPackage::DOMINION_WEAK,
+ PhabricatorOwnersPackage::DOMINION_WEAK,
+ ),
+ // Mixture of strong and weak claims, strong first.
+ array(
+ PhabricatorOwnersPackage::DOMINION_STRONG,
+ PhabricatorOwnersPackage::DOMINION_STRONG,
+ PhabricatorOwnersPackage::DOMINION_WEAK,
+ ),
+ // Mixture of strong and weak claims, weak first.
+ array(
+ PhabricatorOwnersPackage::DOMINION_WEAK,
+ PhabricatorOwnersPackage::DOMINION_STRONG,
+ PhabricatorOwnersPackage::DOMINION_STRONG,
+ ),
+ );
+
+ foreach ($rules as $rule_idx => $rule) {
+ $rows = array(
+ array(
+ 'id' => 1,
+ 'excluded' => 0,
+ 'dominion' => $rule[0],
+ 'path' => $main_c,
+ ),
+ array(
+ 'id' => 2,
+ 'excluded' => 0,
+ 'dominion' => $rule[1],
+ 'path' => $main_c,
+ ),
+ array(
+ 'id' => 3,
+ 'excluded' => 0,
+ 'dominion' => $rule[2],
+ 'path' => $main_c,
+ ),
+ );
+
+ $paths = array(
+ $main_c => $pvalue,
+ );
+
+ // If one or more packages have strong dominion, they should own the
+ // path. If not, all the packages with weak dominion should own the
+ // path.
+ $strong = array();
+ $weak = array();
+ foreach ($rule as $idx => $dominion) {
+ if ($dominion == PhabricatorOwnersPackage::DOMINION_STRONG) {
+ $strong[] = $idx + 1;
+ } else {
+ $weak[] = $idx + 1;
+ }
+ }
+
+ if ($strong) {
+ $expect = $strong;
+ } else {
+ $expect = $weak;
+ }
+
+ $expect = array_fill_keys($expect, strlen($main_c));
+ $actual = PhabricatorOwnersPackage::findLongestPathsPerPackage(
+ $rows,
+ $paths);
+
+ ksort($actual);
+
+ $this->assertEqual(
+ $expect,
+ $actual,
+ pht('Ruleset "%s" for Identical Ownership', $rule_idx));
+ }
}
}

File Metadata

Mime Type
text/plain
Expires
Thu, Mar 6, 6:57 PM (13 h, 26 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7307603
Default Alt Text
D18064.diff (7 KB)

Event Timeline

Phabricator · Built by Phacility