From 4f615ad2a936a59ac64bcb71bea253c74e3bb5b9 Mon Sep 17 00:00:00 2001 From: vrana Date: Thu, 6 Dec 2012 17:23:56 -0800 Subject: [PATCH] Allow excluding paths from package Summary: Resolves T2149. Test Plan: $ bin/storage upgrade # /owners/ - saw + # /owners/package/1/ - saw + # /owners/edit/1/ - added exclude paths, saw correct e-mail # /rPabc123 - included paths are still highlighted and excluded not # /owners/view/search/?path=/included/ - found # /owners/view/search/?path=/excluded/ - not found # owners.query - path: /included/ # owners.query - path: /excluded/ # new unit test PhabricatorOwnersPackage::loadAffectedPackages( $repository, array('/excluded/b.php')); PhabricatorOwnersPackage::loadAffectedPackages( $repository, array('/included/a.php')); Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin Maniphest Tasks: T2149 Differential Revision: https://secure.phabricator.com/D4102 --- resources/sql/patches/owners-exclude.sql | 2 + src/__celerity_resource_map__.php | 5 +- src/__phutil_library_map__.php | 2 + .../controller/DiffusionLintController.php | 7 +- .../view/DiffusionCommitChangeTableView.php | 6 +- .../PhabricatorOwnersDetailController.php | 4 +- .../PhabricatorOwnersEditController.php | 3 + .../PhabricatorOwnersListController.php | 13 ++- src/applications/owners/mail/PackageMail.php | 11 ++- .../storage/PhabricatorOwnersPackage.php | 96 ++++++++++++------- .../owners/storage/PhabricatorOwnersPath.php | 1 + .../PhabricatorOwnersPackageTestCase.php | 34 +++++++ .../patch/PhabricatorBuiltinPatchList.php | 4 + .../application/owners/owners-path-editor.css | 4 +- .../js/application/owners/OwnersPathEditor.js | 12 ++- 15 files changed, 156 insertions(+), 48 deletions(-) create mode 100644 resources/sql/patches/owners-exclude.sql create mode 100644 src/applications/owners/storage/__tests__/PhabricatorOwnersPackageTestCase.php diff --git a/resources/sql/patches/owners-exclude.sql b/resources/sql/patches/owners-exclude.sql new file mode 100644 index 0000000000..6870c528b5 --- /dev/null +++ b/resources/sql/patches/owners-exclude.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_owners.owners_path + ADD excluded bool NOT NULL DEFAULT '0'; diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 98bf98ce96..bd1639b0bc 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -2326,7 +2326,7 @@ celerity_register_resource_map(array( ), 'owners-path-editor' => array( - 'uri' => '/res/e6c51eb6/rsrc/js/application/owners/OwnersPathEditor.js', + 'uri' => '/res/29b68354/rsrc/js/application/owners/OwnersPathEditor.js', 'type' => 'js', 'requires' => array( @@ -2335,12 +2335,13 @@ celerity_register_resource_map(array( 2 => 'path-typeahead', 3 => 'javelin-dom', 4 => 'javelin-util', + 5 => 'phabricator-prefab', ), 'disk' => '/rsrc/js/application/owners/OwnersPathEditor.js', ), 'owners-path-editor-css' => array( - 'uri' => '/res/9bc5332c/rsrc/css/application/owners/owners-path-editor.css', + 'uri' => '/res/4fcaabf6/rsrc/css/application/owners/owners-path-editor.css', 'type' => 'css', 'requires' => array( diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d9884c49a4..bbfbfd8a1d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -928,6 +928,7 @@ phutil_register_library_map(array( 'PhabricatorOwnersPackage' => 'applications/owners/storage/PhabricatorOwnersPackage.php', 'PhabricatorOwnersPackagePathValidator' => 'applications/repository/worker/commitchangeparser/PhabricatorOwnersPackagePathValidator.php', 'PhabricatorOwnersPackageQuery' => 'applications/owners/query/PhabricatorOwnersPackageQuery.php', + 'PhabricatorOwnersPackageTestCase' => 'applications/owners/storage/__tests__/PhabricatorOwnersPackageTestCase.php', 'PhabricatorOwnersPath' => 'applications/owners/storage/PhabricatorOwnersPath.php', 'PhabricatorPHID' => 'applications/phid/storage/PhabricatorPHID.php', 'PhabricatorPHIDConstants' => 'applications/phid/PhabricatorPHIDConstants.php', @@ -2144,6 +2145,7 @@ phutil_register_library_map(array( 1 => 'PhabricatorPolicyInterface', ), 'PhabricatorOwnersPackageQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'PhabricatorOwnersPackageTestCase' => 'PhabricatorTestCase', 'PhabricatorOwnersPath' => 'PhabricatorOwnersDAO', 'PhabricatorPHIDController' => 'PhabricatorController', 'PhabricatorPHIDLookupController' => 'PhabricatorPHIDController', diff --git a/src/applications/diffusion/controller/DiffusionLintController.php b/src/applications/diffusion/controller/DiffusionLintController.php index dc4471b970..467e1d6283 100644 --- a/src/applications/diffusion/controller/DiffusionLintController.php +++ b/src/applications/diffusion/controller/DiffusionLintController.php @@ -193,11 +193,16 @@ final class DiffusionLintController extends DiffusionController { foreach ($paths as $path) { $branch = idx($branches, $repositories[$path->getRepositoryPHID()]); if ($branch) { - $or[] = qsprintf( + $condition = qsprintf( $conn, '(branchID IN (%Ld) AND path LIKE %>)', array_keys($branch), $path->getPath()); + if ($path->getExcluded()) { + $where[] = 'NOT '.$condition; + } else { + $or[] = $condition; + } } } if (!$or) { diff --git a/src/applications/diffusion/view/DiffusionCommitChangeTableView.php b/src/applications/diffusion/view/DiffusionCommitChangeTableView.php index 1a1f66bc4c..c3ec644cd7 100644 --- a/src/applications/diffusion/view/DiffusionCommitChangeTableView.php +++ b/src/applications/diffusion/view/DiffusionCommitChangeTableView.php @@ -66,10 +66,14 @@ final class DiffusionCommitChangeTableView extends DiffusionView { $row_class = null; foreach ($this->ownersPaths as $owners_path) { + $excluded = $owners_path->getExcluded(); $owners_path = $owners_path->getPath(); if (strncmp('/'.$path, $owners_path, strlen($owners_path)) == 0) { + if ($excluded) { + $row_class = null; + break; + } $row_class = 'highlighted'; - break; } } $rowc[] = $row_class; diff --git a/src/applications/owners/controller/PhabricatorOwnersDetailController.php b/src/applications/owners/controller/PhabricatorOwnersDetailController.php index 0334958dc5..e11025ce9a 100644 --- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php +++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php @@ -100,7 +100,9 @@ final class PhabricatorOwnersDetailController 'href' => (string) $href, ), phutil_escape_html($path->getPath())); - $path_links[] = $repo_name.' '.$path_link; + $path_links[] = + ($path->getExcluded() ? '–' : '+').' '. + $repo_name.' '.$path_link; } $path_links = implode('
', $path_links); $rows[] = array( diff --git a/src/applications/owners/controller/PhabricatorOwnersEditController.php b/src/applications/owners/controller/PhabricatorOwnersEditController.php index 77f1a00bc5..9e953d6933 100644 --- a/src/applications/owners/controller/PhabricatorOwnersEditController.php +++ b/src/applications/owners/controller/PhabricatorOwnersEditController.php @@ -47,6 +47,7 @@ final class PhabricatorOwnersEditController $paths = $request->getArr('path'); $repos = $request->getArr('repo'); + $excludes = $request->getArr('exclude'); $path_refs = array(); for ($ii = 0; $ii < count($paths); $ii++) { @@ -56,6 +57,7 @@ final class PhabricatorOwnersEditController $path_refs[] = array( 'repositoryPHID' => $repos[$ii], 'path' => $paths[$ii], + 'excluded' => $excludes[$ii], ); } @@ -102,6 +104,7 @@ final class PhabricatorOwnersEditController $path_refs[] = array( 'repositoryPHID' => $path->getRepositoryPHID(), 'path' => $path->getPath(), + 'excluded' => $path->getExcluded(), ); } } diff --git a/src/applications/owners/controller/PhabricatorOwnersListController.php b/src/applications/owners/controller/PhabricatorOwnersListController.php index e9c13438df..f44e03ea44 100644 --- a/src/applications/owners/controller/PhabricatorOwnersListController.php +++ b/src/applications/owners/controller/PhabricatorOwnersListController.php @@ -34,6 +34,7 @@ final class PhabricatorOwnersListController $where = array('1 = 1'); $join = array(); + $having = ''; if ($request->getStr('name')) { $where[] = qsprintf( @@ -59,10 +60,14 @@ final class PhabricatorOwnersListController if ($request->getStr('path')) { $where[] = qsprintf( $conn_r, - 'path.path LIKE %~ OR %s LIKE CONCAT(path.path, %s)', + '(path.path LIKE %~ AND NOT path.excluded) OR + %s LIKE CONCAT(REPLACE(path.path, %s, %s), %s)', $request->getStr('path'), $request->getStr('path'), + '_', + '\_', '%'); + $having = 'HAVING MAX(path.excluded) = 0'; } } @@ -80,10 +85,11 @@ final class PhabricatorOwnersListController $data = queryfx_all( $conn_r, - 'SELECT p.* FROM %T p %Q WHERE %Q GROUP BY p.id', + 'SELECT p.* FROM %T p %Q WHERE %Q GROUP BY p.id %Q', $package->getTableName(), implode(' ', $join), - '('.implode(') AND (', $where).')'); + '('.implode(') AND (', $where).')', + $having); $packages = $package->loadAllFromArray($data); $header = 'Search Results'; @@ -254,6 +260,7 @@ final class PhabricatorOwnersListController 'action' => 'browse', )); $pkg_paths[$key] = + ($path->getExcluded() ? '–' : '+').' '. ''.phutil_escape_html($repo->getName()).' '. phutil_render_tag( 'a', diff --git a/src/applications/owners/mail/PackageMail.php b/src/applications/owners/mail/PackageMail.php index 2a9d57c258..5d4b45203f 100644 --- a/src/applications/owners/mail/PackageMail.php +++ b/src/applications/owners/mail/PackageMail.php @@ -46,8 +46,8 @@ abstract class PackageMail { $section[] = ' In repository '.$handles[$repository_phid]->getName(). ' - '. PhabricatorEnv::getProductionURI($handles[$repository_phid] ->getURI()); - foreach ($paths as $path => $ignored) { - $section[] = ' '.$path; + foreach ($paths as $path => $excluded) { + $section[] = ' '.($excluded ? 'Excluded' : 'Included').' '.$path; } return implode("\n", $section); @@ -70,8 +70,11 @@ abstract class PackageMail { } $this->mailTo = $mail_to; - $paths = $package->loadPaths(); - $this->paths = mgroup($paths, 'getRepositoryPHID', 'getPath'); + $this->paths = array(); + $repository_paths = mgroup($package->loadPaths(), 'getRepositoryPHID'); + foreach ($repository_paths as $repository_phid => $paths) { + $this->paths[$repository_phid] = mpull($paths, 'getExcluded', 'getPath'); + } $phids = array_merge( $this->mailTo, diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php index 20d68754a6..e5577ace7f 100644 --- a/src/applications/owners/storage/PhabricatorOwnersPackage.php +++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php @@ -113,15 +113,7 @@ final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO return array(); } - $fragments = array( - '/' => true, - ); - - foreach ($paths as $path) { - $fragments += self::splitPath($path); - } - - return self::loadPackagesForPaths($repository, array_keys($fragments)); + return self::loadPackagesForPaths($repository, $paths); } public static function loadOwningPackages($repository, $path) { @@ -129,14 +121,21 @@ final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO return array(); } - $fragments = self::splitPath($path); - return self::loadPackagesForPaths($repository, array_keys($fragments), 1); + return self::loadPackagesForPaths($repository, array($path), 1); } private static function loadPackagesForPaths( PhabricatorRepository $repository, array $paths, $limit = 0) { + + $fragments = array(); + foreach ($paths as $path) { + foreach (self::splitPath($path) as $fragment) { + $fragments[$fragment][$path] = true; + } + } + $package = new PhabricatorOwnersPackage(); $path = new PhabricatorOwnersPath(); $conn = $package->establishConnection('r'); @@ -151,28 +150,21 @@ final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO // branch. Break it apart so that it will fit within 'max_allowed_packet', // and then merge results in PHP. - $ids = array(); - foreach (array_chunk($paths, 128) as $chunk) { - $rows = queryfx_all( + $rows = array(); + foreach (array_chunk(array_keys($fragments), 128) as $chunk) { + $rows[] = queryfx_all( $conn, - 'SELECT pkg.id id, LENGTH(p.path) len + 'SELECT pkg.id, p.excluded, p.path FROM %T pkg JOIN %T p ON p.packageID = pkg.id WHERE p.path IN (%Ls) %Q', $package->getTableName(), $path->getTableName(), $chunk, $repository_clause); - - foreach ($rows as $row) { - $id = (int)$row['id']; - $len = (int)$row['len']; - if (isset($ids[$id])) { - $ids[$id] = max($len, $ids[$id]); - } else { - $ids[$id] = $len; - } - } } + $rows = array_mergev($rows); + + $ids = self::findLongestPathsPerPackage($rows, $fragments); if (!$ids) { return array(); @@ -190,6 +182,38 @@ final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO return $packages; } + 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 += $relevant_paths[$package_path['path']]; + unset($relevant_paths[$package_path['path']]); + } + } + + if ($remove) { + foreach ($relevant_paths as $fragment => $fragment_paths) { + $relevant_paths[$fragment] = array_diff_key($fragment_paths, $remove); + } + } + + $relevant_paths = array_filter($relevant_paths); + if ($relevant_paths) { + $ids[$id] = max(array_map('strlen', array_keys($relevant_paths))); + } + } + + return $ids; + } + public function save() { if ($this->getID()) { @@ -238,9 +262,15 @@ final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO $new_paths = igroup($this->unsavedPaths, 'repositoryPHID', 'path'); $cur_paths = $this->loadPaths(); foreach ($cur_paths as $key => $path) { - if (empty($new_paths[$path->getRepositoryPHID()][$path->getPath()])) { - $touched_repos[$path->getRepositoryPHID()] = true; - $remove_paths[$path->getRepositoryPHID()][$path->getPath()] = true; + $repository_phid = $path->getRepositoryPHID(); + $new_path = head(idx( + idx($new_paths, $repository_phid, array()), + $path->getPath(), + array())); + $excluded = $path->getExcluded(); + if (!$new_path || $new_path['excluded'] != $excluded) { + $touched_repos[$repository_phid] = true; + $remove_paths[$repository_phid][$path->getPath()] = $excluded; $path->delete(); unset($cur_paths[$key]); } @@ -255,7 +285,7 @@ final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO if (!$repository) { continue; } - foreach ($paths as $path => $ignored) { + foreach ($paths as $path => $dicts) { $path = ltrim($path, '/'); // build query to validate path $drequest = DiffusionRequest::newFromDictionary( @@ -286,11 +316,13 @@ final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO } if (empty($cur_paths[$repository_phid][$path]) && $valid) { $touched_repos[$repository_phid] = true; - $add_paths[$repository_phid][$path] = true; + $excluded = idx(reset($dicts), 'excluded', 0); + $add_paths[$repository_phid][$path] = $excluded; $obj = new PhabricatorOwnersPath(); $obj->setPackageID($this->getID()); $obj->setRepositoryPHID($repository_phid); $obj->setPath($path); + $obj->setExcluded($excluded); $obj->save(); } } @@ -340,12 +372,12 @@ final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO } private static function splitPath($path) { - $result = array(); + $result = array('/'); $trailing_slash = preg_match('@/$@', $path) ? '/' : ''; $path = trim($path, '/'); $parts = explode('/', $path); while (count($parts)) { - $result['/'.implode('/', $parts).$trailing_slash] = true; + $result[] = '/'.implode('/', $parts).$trailing_slash; $trailing_slash = '/'; array_pop($parts); } diff --git a/src/applications/owners/storage/PhabricatorOwnersPath.php b/src/applications/owners/storage/PhabricatorOwnersPath.php index 58319ae874..b241b6f0ec 100644 --- a/src/applications/owners/storage/PhabricatorOwnersPath.php +++ b/src/applications/owners/storage/PhabricatorOwnersPath.php @@ -5,6 +5,7 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO { protected $packageID; protected $repositoryPHID; protected $path; + protected $excluded; public function getConfiguration() { return array( diff --git a/src/applications/owners/storage/__tests__/PhabricatorOwnersPackageTestCase.php b/src/applications/owners/storage/__tests__/PhabricatorOwnersPackageTestCase.php new file mode 100644 index 0000000000..39b4dd959b --- /dev/null +++ b/src/applications/owners/storage/__tests__/PhabricatorOwnersPackageTestCase.php @@ -0,0 +1,34 @@ + 1, 'excluded' => 0, 'path' => 'src/'), + array('id' => 1, 'excluded' => 1, 'path' => 'src/releeph/'), + array('id' => 2, 'excluded' => 0, 'path' => 'src/releeph/'), + ); + + $paths = array( + 'src/' => array('src/a.php' => true, 'src/releeph/b.php' => true), + 'src/releeph/' => array('src/releeph/b.php' => true), + ); + $this->assertEqual( + array( + 1 => strlen('src/'), + 2 => strlen('src/releeph/'), + ), + PhabricatorOwnersPackage::findLongestPathsPerPackage($rows, $paths)); + + $paths = array( + 'src/' => array('src/releeph/b.php' => true), + 'src/releeph/' => array('src/releeph/b.php' => true), + ); + $this->assertEqual( + array( + 2 => strlen('src/releeph/'), + ), + PhabricatorOwnersPackage::findLongestPathsPerPackage($rows, $paths)); + } + +} diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index 6490f76b59..44df8a6061 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1040,6 +1040,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('pholio.sql'), ), + 'owners-exclude.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('owners-exclude.sql'), + ), ); } diff --git a/webroot/rsrc/css/application/owners/owners-path-editor.css b/webroot/rsrc/css/application/owners/owners-path-editor.css index 79e106b42c..31e5f6915b 100644 --- a/webroot/rsrc/css/application/owners/owners-path-editor.css +++ b/webroot/rsrc/css/application/owners/owners-path-editor.css @@ -10,12 +10,12 @@ padding: 2px 4px; } -.owners-path-editor-table select { +.owners-path-editor-table select.owners-repo { width: 150px; } .owners-path-editor-table input { - width: 550px; + width: 450px; } .owners-path-editor-table div.error-display { diff --git a/webroot/rsrc/js/application/owners/OwnersPathEditor.js b/webroot/rsrc/js/application/owners/OwnersPathEditor.js index 8db02cb016..d38797788a 100644 --- a/webroot/rsrc/js/application/owners/OwnersPathEditor.js +++ b/webroot/rsrc/js/application/owners/OwnersPathEditor.js @@ -4,6 +4,7 @@ * path-typeahead * javelin-dom * javelin-util + * phabricator-prefab * @provides owners-path-editor * @javelin */ @@ -95,7 +96,8 @@ JX.install('OwnersPathEditor', { this._lastRepositoryChoice; var options = this._buildRepositoryOptions(selected_repository); var attrs = { - name : "repo[" + this._count + "]" + name : "repo[" + this._count + "]", + className : 'owners-repo' }; var repo_select = JX.$N('select', attrs, options); @@ -132,8 +134,14 @@ JX.install('OwnersPathEditor', { var error_display_cell = JX.$N('td', {}, error_display); + var exclude = JX.Prefab.renderSelect( + {'0' : 'Include', '1' : 'Exclude'}, + path_ref.excluded, + {name : 'exclude[' + this._count + ']'}); + var exclude_cell = JX.$N('td', {}, exclude); + var row = this._rowManager.addRow( - [repo_cell, typeahead_cell, error_display_cell]); + [exclude_cell, repo_cell, typeahead_cell, error_display_cell]); new JX.PathTypeahead({ repositoryDefaultPaths : this._repositoryDefaultPaths,