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