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

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_owners.owners_path
+  ADD excluded bool NOT NULL DEFAULT '0';

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(

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',

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) {

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;

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('<br />', $path_links);
     $rows[] = array(

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(),
         );
       }
     }

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() ? '–' : '+').' '.
             '<strong>'.phutil_escape_html($repo->getName()).'</strong> '.
             phutil_render_tag(
               'a',

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) {
+    foreach ($paths as $path => $excluded) {
-      $section[] = '    '.$path;
+      $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 = array();
-    $this->paths = mgroup($paths, 'getRepositoryPHID', 'getPath');
+    $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,

src/applications/owners/storage/PhabricatorOwnersPackage.php

@@ -113,15 +113,7 @@ final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO
       return array();
     }
 
-    $fragments = array(
+    return self::loadPackagesForPaths($repository, $paths);
-      '/' => true,
-    );
-
-    foreach ($paths as $path) {
-      $fragments += self::splitPath($path);
-    }
-
-    return self::loadPackagesForPaths($repository, array_keys($fragments));
  }
 
  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($path), 1);
-    return self::loadPackagesForPaths($repository, array_keys($fragments), 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();
+    $rows = array();
-    foreach (array_chunk($paths, 128) as $chunk) {
+    foreach (array_chunk(array_keys($fragments), 128) as $chunk) {
-      $rows = queryfx_all(
+      $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()])) {
+        $repository_phid = $path->getRepositoryPHID();
-          $touched_repos[$path->getRepositoryPHID()] = true;
+        $new_path = head(idx(
-          $remove_paths[$path->getRepositoryPHID()][$path->getPath()] = true;
+          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);
     }

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(

src/applications/owners/storage/__tests__/PhabricatorOwnersPackageTestCase.php

@@ -0,0 +1,34 @@
+<?php
+
+final class PhabricatorOwnersPackageTestCase extends PhabricatorTestCase {
+
+  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/'),
+    );
+
+    $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));
+  }
+
+}

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

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 {

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,