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)); + } } }