Convert Owners paths to application transactions

Summary:
Ref T8320. Fixes T8317. Fixes T2831. Fixes T8073. Fixes T7127.

There was a bug with this line:

  for ($ii = 0; $ii < count($paths); $ii++) {

...because the array may be sparse if there have been deletes, so `count($paths)` might be 3, but the real keys could be `1`, `5` and `6`. I think this was the primary issue behind T7127.

The old Editor did a lot of work to try to validate paths. When a path failed to validate, it silently discarded it. This was silly and pointless: it's incredibly bad UX; and it's totally fine if users saves "invalid" paths. This was likely the cause of T8317, and probably the cause of T8073.

T2831 I'm less sure about, but I can't reproduce it and I rewrote all the logic so I suspect it's gone.

This also records and shows edits, so if stuff does keep happening it should be more clear what's going on.

I removed some adjacent stuff:

  - I removed the ability to delete packages. I'll add "disable" in a future diff, plus `bin/remove destroy`, like other objects. Getting rid of this now let me get rid of all the mail stuff.
  - I removed "path validation" where packages would try to automatically update in response to commits. This doesn't necessarily make sense in Git/Mercurial, is sketchy, could easily have been the source of T2831, and seems generally complicated and not very valuable. We could maybe restore it some day, but I'd like to get Owners stable before trying to do crazy stuff like that.

Test Plan: {F437687}

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8317, T8073, T7127, T2831, T8320

Differential Revision: https://secure.phabricator.com/D13032

resources/celerity/map.php

@@ -7,7 +7,7 @@
  */
 return array(
   'names' => array(
-    'core.pkg.css' => '4e7df908',
+    'core.pkg.css' => '97a49e3e',
     'core.pkg.js' => '328799d0',
     'darkconsole.pkg.js' => 'e7393ebb',
     'differential.pkg.css' => '30602b8c',
@@ -26,7 +26,7 @@ return array(
     'rsrc/css/aphront/pager-view.css' => '2e3539af',
     'rsrc/css/aphront/panel-view.css' => '8427b78d',
     'rsrc/css/aphront/phabricator-nav-view.css' => '7aeaf435',
-    'rsrc/css/aphront/table-view.css' => '59e2c0f8',
+    'rsrc/css/aphront/table-view.css' => '0b4cd283',
     'rsrc/css/aphront/tokenizer.css' => '86a13f7f',
     'rsrc/css/aphront/tooltip.css' => '7672b60f',
     'rsrc/css/aphront/two-column.css' => '16ab3ad2',
@@ -489,7 +489,7 @@ return array(
     'aphront-multi-column-view-css' => 'fd18389d',
     'aphront-pager-view-css' => '2e3539af',
     'aphront-panel-view-css' => '8427b78d',
-    'aphront-table-view-css' => '59e2c0f8',
+    'aphront-table-view-css' => '0b4cd283',
     'aphront-tokenizer-control-css' => '86a13f7f',
     'aphront-tooltip-css' => '7672b60f',
     'aphront-two-column-view-css' => '16ab3ad2',

src/__phutil_library_map__.php

@@ -1230,10 +1230,6 @@ phutil_register_library_map(array(
     'PHUITypeaheadExample' => 'applications/uiexample/examples/PHUITypeaheadExample.php',
     'PHUIWorkboardView' => 'view/phui/PHUIWorkboardView.php',
     'PHUIWorkpanelView' => 'view/phui/PHUIWorkpanelView.php',
-    'PackageCreateMail' => 'applications/owners/mail/PackageCreateMail.php',
-    'PackageDeleteMail' => 'applications/owners/mail/PackageDeleteMail.php',
-    'PackageMail' => 'applications/owners/mail/PackageMail.php',
-    'PackageModifyMail' => 'applications/owners/mail/PackageModifyMail.php',
     'PassphraseAbstractKey' => 'applications/passphrase/keys/PassphraseAbstractKey.php',
     'PassphraseConduitAPIMethod' => 'applications/passphrase/conduit/PassphraseConduitAPIMethod.php',
     'PassphraseController' => 'applications/passphrase/controller/PassphraseController.php',
@@ -2167,16 +2163,13 @@ phutil_register_library_map(array(
     'PhabricatorOwnersConfigOptions' => 'applications/owners/config/PhabricatorOwnersConfigOptions.php',
     'PhabricatorOwnersController' => 'applications/owners/controller/PhabricatorOwnersController.php',
     'PhabricatorOwnersDAO' => 'applications/owners/storage/PhabricatorOwnersDAO.php',
-    'PhabricatorOwnersDeleteController' => 'applications/owners/controller/PhabricatorOwnersDeleteController.php',
     'PhabricatorOwnersDetailController' => 'applications/owners/controller/PhabricatorOwnersDetailController.php',
     'PhabricatorOwnersEditController' => 'applications/owners/controller/PhabricatorOwnersEditController.php',
     'PhabricatorOwnersListController' => 'applications/owners/controller/PhabricatorOwnersListController.php',
     'PhabricatorOwnersOwner' => 'applications/owners/storage/PhabricatorOwnersOwner.php',
     'PhabricatorOwnersPackage' => 'applications/owners/storage/PhabricatorOwnersPackage.php',
     'PhabricatorOwnersPackageDatasource' => 'applications/owners/typeahead/PhabricatorOwnersPackageDatasource.php',
-    'PhabricatorOwnersPackageEditor' => 'applications/owners/editor/PhabricatorOwnersPackageEditor.php',
     'PhabricatorOwnersPackagePHIDType' => 'applications/owners/phid/PhabricatorOwnersPackagePHIDType.php',
-    'PhabricatorOwnersPackagePathValidator' => 'applications/repository/worker/commitchangeparser/PhabricatorOwnersPackagePathValidator.php',
     'PhabricatorOwnersPackageQuery' => 'applications/owners/query/PhabricatorOwnersPackageQuery.php',
     'PhabricatorOwnersPackageSearchEngine' => 'applications/owners/query/PhabricatorOwnersPackageSearchEngine.php',
     'PhabricatorOwnersPackageTestCase' => 'applications/owners/storage/__tests__/PhabricatorOwnersPackageTestCase.php',
@@ -4565,10 +4558,6 @@ phutil_register_library_map(array(
     'PHUITypeaheadExample' => 'PhabricatorUIExample',
     'PHUIWorkboardView' => 'AphrontTagView',
     'PHUIWorkpanelView' => 'AphrontTagView',
-    'PackageCreateMail' => 'PackageMail',
-    'PackageDeleteMail' => 'PackageMail',
-    'PackageMail' => 'PhabricatorMail',
-    'PackageModifyMail' => 'PackageMail',
     'PassphraseAbstractKey' => 'Phobject',
     'PassphraseConduitAPIMethod' => 'ConduitAPIMethod',
     'PassphraseController' => 'PhabricatorController',
@@ -5576,7 +5565,6 @@ phutil_register_library_map(array(
     'PhabricatorOwnersConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorOwnersController' => 'PhabricatorController',
     'PhabricatorOwnersDAO' => 'PhabricatorLiskDAO',
-    'PhabricatorOwnersDeleteController' => 'PhabricatorOwnersController',
     'PhabricatorOwnersDetailController' => 'PhabricatorOwnersController',
     'PhabricatorOwnersEditController' => 'PhabricatorOwnersController',
     'PhabricatorOwnersListController' => 'PhabricatorOwnersController',
@@ -5587,7 +5575,6 @@ phutil_register_library_map(array(
       'PhabricatorApplicationTransactionInterface',
     ),
     'PhabricatorOwnersPackageDatasource' => 'PhabricatorTypeaheadDatasource',
-    'PhabricatorOwnersPackageEditor' => 'PhabricatorEditor',
     'PhabricatorOwnersPackagePHIDType' => 'PhabricatorPHIDType',
     'PhabricatorOwnersPackageQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorOwnersPackageSearchEngine' => 'PhabricatorApplicationSearchEngine',

src/applications/owners/application/PhabricatorOwnersApplication.php

@@ -46,7 +46,6 @@ final class PhabricatorOwnersApplication extends PhabricatorApplication {
         'edit/(?P<id>[1-9]\d*)/' => 'PhabricatorOwnersEditController',
         'new/' => 'PhabricatorOwnersEditController',
         'package/(?P<id>[1-9]\d*)/' => 'PhabricatorOwnersDetailController',
-        'delete/(?P<id>[1-9]\d*)/' => 'PhabricatorOwnersDeleteController',
         'paths/(?P<id>[1-9]\d*)/' => 'PhabricatorOwnersPathsController',
       ),
     );

src/applications/owners/controller/PhabricatorOwnersDeleteController.php

@@ -1,44 +0,0 @@
-<?php
-
-final class PhabricatorOwnersDeleteController
-  extends PhabricatorOwnersController {
-
-  private $id;
-
-  public function willProcessRequest(array $data) {
-    $this->id = $data['id'];
-  }
-
-  public function processRequest() {
-    $request = $this->getRequest();
-    $user = $request->getUser();
-
-    $package = id(new PhabricatorOwnersPackage())->load($this->id);
-    if (!$package) {
-      return new Aphront404Response();
-    }
-
-    if ($request->isDialogFormPost()) {
-      id(new PhabricatorOwnersPackageEditor())
-        ->setActor($user)
-        ->setPackage($package)
-        ->delete();
-      return id(new AphrontRedirectResponse())->setURI('/owners/');
-    }
-
-    $text = pht(
-      'Are you sure you want to delete the "%s" package? This '.
-      'operation can not be undone.',
-      $package->getName());
-    $dialog = id(new AphrontDialogView())
-      ->setUser($user)
-      ->setTitle(pht('Really delete this package?'))
-      ->appendChild(phutil_tag('p', array(), $text))
-      ->addSubmitButton(pht('Delete'))
-      ->addCancelButton('/owners/package/'.$package->getID().'/')
-      ->setSubmitURI($request->getRequestURI());
-
-    return id(new AphrontDialogResponse())->setDialog($dialog);
-  }
-
-}

src/applications/owners/controller/PhabricatorOwnersDetailController.php

@@ -195,7 +195,6 @@ final class PhabricatorOwnersDetailController
     $id = $package->getID();
     $edit_uri = $this->getApplicationURI("/edit/{$id}/");
     $paths_uri = $this->getApplicationURI("/paths/{$id}/");
-    $delete_uri = $this->getApplicationURI("/delete/{$id}/");
 
     $view = id(new PhabricatorActionListView())
       ->setUser($viewer)
@@ -213,14 +212,7 @@ final class PhabricatorOwnersDetailController
           ->setIcon('fa-folder-open')
           ->setDisabled(!$can_edit)
           ->setWorkflow(!$can_edit)
-          ->setHref($paths_uri))
+          ->setHref($paths_uri));
-      ->addAction(
-        id(new PhabricatorActionView())
-          ->setName(pht('Delete Package'))
-          ->setIcon('fa-times')
-          ->setDisabled(!$can_edit)
-          ->setWorkflow(true)
-          ->setHref($delete_uri));
 
     return $view;
   }

src/applications/owners/controller/PhabricatorOwnersPathsController.php

@@ -26,39 +26,48 @@ final class PhabricatorOwnersPathsController
       $excludes = $request->getArr('exclude');
 
       $path_refs = array();
-      for ($ii = 0; $ii < count($paths); $ii++) {
+      foreach ($paths as $key => $path) {
-        if (empty($paths[$ii]) || empty($repos[$ii])) {
+        if (!isset($repos[$key])) {
-          continue;
+          throw new Exception(
+            pht(
+              'No repository PHID for path "%s"!',
+              $key));
         }
+
+        if (!isset($excludes[$key])) {
+          throw new Exception(
+            pht(
+              'No exclusion value for path "%s"!',
+              $key));
+        }
+
         $path_refs[] = array(
-          'repositoryPHID' => $repos[$ii],
+          'repositoryPHID' => $repos[$key],
-          'path' => $paths[$ii],
+          'path' => $path,
-          'excluded' => $excludes[$ii],
+          'excluded' => (int)$excludes[$key],
         );
       }
 
-      $package->attachUnsavedOwners(array());
+      $type_paths = PhabricatorOwnersPackageTransaction::TYPE_PATHS;
-      $package->attachUnsavedPaths($path_refs);
-      $package->attachOldAuditingEnabled($package->getAuditingEnabled());
-      $package->attachOldPrimaryOwnerPHID($package->getPrimaryOwnerPHID());
 
-      id(new PhabricatorOwnersPackageEditor())
+      $xactions = array();
+      $xactions[] = id(new PhabricatorOwnersPackageTransaction())
+        ->setTransactionType($type_paths)
+        ->setNewValue($path_refs);
+
+      $editor = id(new PhabricatorOwnersPackageTransactionEditor())
         ->setActor($viewer)
-        ->setPackage($package)
+        ->setContentSourceFromRequest($request)
-        ->save();
+        ->setContinueOnNoEffect(true)
+        ->setContinueOnMissingFields(true);
+
+      $editor->applyTransactions($package, $xactions);
 
       return id(new AphrontRedirectResponse())
         ->setURI('/owners/package/'.$package->getID().'/');
     } else {
       $paths = $package->loadPaths();
-      $path_refs = array();
+      $path_refs = mpull($paths, 'getRef');
-      foreach ($paths as $path) {
-        $path_refs[] = array(
-          'repositoryPHID' => $path->getRepositoryPHID(),
-          'path' => $path->getPath(),
-          'excluded' => $path->getExcluded(),
-        );
-      }
     }
 
     $repos = id(new PhabricatorRepositoryQuery())

src/applications/owners/editor/PhabricatorOwnersPackageEditor.php

@@ -1,198 +0,0 @@
-<?php
-
-final class PhabricatorOwnersPackageEditor extends PhabricatorEditor {
-
-  private $package;
-
-  public function setPackage(PhabricatorOwnersPackage $package) {
-    $this->package = $package;
-    return $this;
-  }
-
-  public function getPackage() {
-    return $this->package;
-  }
-
-  public function save() {
-    $actor = $this->getActor();
-    $package = $this->getPackage();
-    $package->attachActorPHID($actor->getPHID());
-
-    if ($package->getID()) {
-      $is_new = false;
-    } else {
-      $is_new = true;
-    }
-
-    $package->openTransaction();
-
-      $ret = $package->save();
-
-      $add_owners = array();
-      $remove_owners = array();
-      $all_owners = array();
-      if ($package->getUnsavedOwners()) {
-        $new_owners = array_fill_keys($package->getUnsavedOwners(), true);
-        $cur_owners = array();
-        foreach ($package->loadOwners() as $owner) {
-          if (empty($new_owners[$owner->getUserPHID()])) {
-            $remove_owners[$owner->getUserPHID()] = true;
-            $owner->delete();
-            continue;
-          }
-          $cur_owners[$owner->getUserPHID()] = true;
-        }
-
-        $add_owners = array_diff_key($new_owners, $cur_owners);
-        $all_owners = array_merge(
-          array($package->getPrimaryOwnerPHID() => true),
-          $new_owners,
-          $remove_owners);
-        foreach ($add_owners as $phid => $ignored) {
-          $owner = new PhabricatorOwnersOwner();
-          $owner->setPackageID($package->getID());
-          $owner->setUserPHID($phid);
-          $owner->save();
-        }
-        $package->attachUnsavedOwners(array());
-      }
-
-      $add_paths = array();
-      $remove_paths = array();
-      $touched_repos = array();
-      if ($package->getUnsavedPaths()) {
-        $new_paths = igroup(
-          $package->getUnsavedPaths(),
-          'repositoryPHID',
-          'path');
-        $cur_paths = $package->loadPaths();
-        foreach ($cur_paths as $key => $path) {
-          $repository_phid = $path->getRepositoryPHID();
-          $new_path = head(idx(
-            idx($new_paths, $repository_phid, array()),
-            $path->getPath(),
-            array()));
-          $excluded = $path->getExcluded();
-          if ($new_path === false ||
-              idx($new_path, 'excluded') != $excluded) {
-            $touched_repos[$repository_phid] = true;
-            $remove_paths[$repository_phid][$path->getPath()] = $excluded;
-            $path->delete();
-            unset($cur_paths[$key]);
-          }
-        }
-
-        $cur_paths = mgroup($cur_paths, 'getRepositoryPHID', 'getPath');
-        $repositories = id(new PhabricatorRepositoryQuery())
-          ->setViewer($actor)
-          ->withPHIDs(array_keys($cur_paths))
-          ->execute();
-        $repositories = mpull($repositories, null, 'getPHID');
-        foreach ($new_paths as $repository_phid => $paths) {
-          $repository = idx($repositories, $repository_phid);
-          if (!$repository) {
-            continue;
-          }
-          foreach ($paths as $path => $dicts) {
-            $path = ltrim($path, '/');
-            // build query to validate path
-            $drequest = DiffusionRequest::newFromDictionary(
-              array(
-                'user' => $actor,
-                'repository'  => $repository,
-                'path'        => $path,
-              ));
-            $results = DiffusionBrowseResultSet::newFromConduit(
-              DiffusionQuery::callConduitWithDiffusionRequest(
-                $actor,
-                $drequest,
-                'diffusion.browsequery',
-                array(
-                  'commit' => $drequest->getCommit(),
-                  'path' => $path,
-                  'needValidityOnly' => true,
-                )));
-            $valid = $results->isValidResults();
-            $is_directory = true;
-            if (!$valid) {
-              switch ($results->getReasonForEmptyResultSet()) {
-                case DiffusionBrowseResultSet::REASON_IS_FILE:
-                  $valid = true;
-                  $is_directory = false;
-                  break;
-                case DiffusionBrowseResultSet::REASON_IS_EMPTY:
-                  $valid = true;
-                  break;
-              }
-            }
-            if ($is_directory && substr($path, -1) != '/') {
-              $path .= '/';
-            }
-            if (substr($path, 0, 1) != '/') {
-              $path = '/'.$path;
-            }
-            if (empty($cur_paths[$repository_phid][$path]) && $valid) {
-              $touched_repos[$repository_phid] = true;
-              $excluded = idx(reset($dicts), 'excluded', 0);
-              $add_paths[$repository_phid][$path] = $excluded;
-              $obj = new PhabricatorOwnersPath();
-              $obj->setPackageID($package->getID());
-              $obj->setRepositoryPHID($repository_phid);
-              $obj->setPath($path);
-              $obj->setExcluded($excluded);
-              $obj->save();
-            }
-          }
-        }
-        $package->attachUnsavedPaths(array());
-      }
-
-    $package->saveTransaction();
-
-    if ($is_new) {
-      $mail = new PackageCreateMail($package);
-    } else {
-      $mail = new PackageModifyMail(
-        $package,
-        array_keys($add_owners),
-        array_keys($remove_owners),
-        array_keys($all_owners),
-        array_keys($touched_repos),
-        $add_paths,
-        $remove_paths);
-    }
-    $mail->setActor($actor);
-    $mail->send();
-
-    return $ret;
-  }
-
-  public function delete() {
-    $actor = $this->getActor();
-    $package = $this->getPackage();
-    $package->attachActorPHID($actor->getPHID());
-
-    $mails = id(new PackageDeleteMail($package))
-      ->setActor($actor)
-      ->prepareMails();
-
-    $package->openTransaction();
-
-      foreach ($package->loadOwners() as $owner) {
-        $owner->delete();
-      }
-      foreach ($package->loadPaths() as $path) {
-        $path->delete();
-      }
-      $ret = $package->delete();
-
-    $package->saveTransaction();
-
-    foreach ($mails as $mail) {
-      $mail->saveAndSend();
-    }
-
-    return $ret;
-  }
-
-}

src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php

@@ -19,6 +19,7 @@ final class PhabricatorOwnersPackageTransactionEditor
     $types[] = PhabricatorOwnersPackageTransaction::TYPE_OWNERS;
     $types[] = PhabricatorOwnersPackageTransaction::TYPE_AUDITING;
     $types[] = PhabricatorOwnersPackageTransaction::TYPE_DESCRIPTION;
+    $types[] = PhabricatorOwnersPackageTransaction::TYPE_PATHS;
 
     return $types;
   }
@@ -41,6 +42,10 @@ final class PhabricatorOwnersPackageTransactionEditor
         return (int)$object->getAuditingEnabled();
       case PhabricatorOwnersPackageTransaction::TYPE_DESCRIPTION:
         return $object->getDescription();
+      case PhabricatorOwnersPackageTransaction::TYPE_PATHS:
+        // TODO: needPaths() this on the query
+        $paths = $object->loadPaths();
+        return mpull($paths, 'getRef');
     }
   }
 
@@ -52,6 +57,7 @@ final class PhabricatorOwnersPackageTransactionEditor
       case PhabricatorOwnersPackageTransaction::TYPE_NAME:
       case PhabricatorOwnersPackageTransaction::TYPE_PRIMARY:
       case PhabricatorOwnersPackageTransaction::TYPE_DESCRIPTION:
+      case PhabricatorOwnersPackageTransaction::TYPE_PATHS:
         return $xaction->getNewValue();
       case PhabricatorOwnersPackageTransaction::TYPE_AUDITING:
         return (int)$xaction->getNewValue();
@@ -63,6 +69,24 @@ final class PhabricatorOwnersPackageTransactionEditor
     }
   }
 
+  protected function transactionHasEffect(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    switch ($xaction->getTransactionType()) {
+      case PhabricatorOwnersPackageTransaction::TYPE_PATHS:
+        $old = $xaction->getOldValue();
+        $new = $xaction->getNewValue();
+
+        $diffs = PhabricatorOwnersPath::getTransactionValueChanges($old, $new);
+        list($rem, $add) = $diffs;
+
+        return ($rem || $add);
+    }
+
+    return parent::transactionHasEffect($object, $xaction);
+  }
+
   protected function applyCustomInternalTransaction(
     PhabricatorLiskDAO $object,
     PhabricatorApplicationTransaction $xaction) {
@@ -81,6 +105,7 @@ final class PhabricatorOwnersPackageTransactionEditor
         $object->setAuditingEnabled($xaction->getNewValue());
         return;
       case PhabricatorOwnersPackageTransaction::TYPE_OWNERS:
+      case PhabricatorOwnersPackageTransaction::TYPE_PATHS:
         return;
     }
 
@@ -122,6 +147,31 @@ final class PhabricatorOwnersPackageTransactionEditor
         }
 
         // TODO: Attach owners here
+        return;
+      case PhabricatorOwnersPackageTransaction::TYPE_PATHS:
+        $old = $xaction->getOldValue();
+        $new = $xaction->getNewValue();
+
+        // TODO: needPaths this
+        $paths = $object->loadPaths();
+
+        $diffs = PhabricatorOwnersPath::getTransactionValueChanges($old, $new);
+        list($rem, $add) = $diffs;
+
+        $set = PhabricatorOwnersPath::getSetFromTransactionValue($rem);
+        foreach ($paths as $path) {
+          $ref = $path->getRef();
+          if (PhabricatorOwnersPath::isRefInSet($ref, $set)) {
+            $path->delete();
+          }
+        }
+
+        foreach ($add as $ref) {
+          $path = PhabricatorOwnersPath::newFromRef($ref)
+            ->setPackageID($object->getID())
+            ->save();
+        }
+
         return;
     }
 

src/applications/owners/mail/PackageCreateMail.php

@@ -1,12 +0,0 @@
-<?php
-
-final class PackageCreateMail extends PackageMail {
-
-  protected function isNewThread() {
-    return true;
-  }
-
-  protected function getVerb() {
-    return pht('Created');
-  }
-}

src/applications/owners/mail/PackageDeleteMail.php

@@ -1,13 +0,0 @@
-<?php
-
-final class PackageDeleteMail extends PackageMail {
-
-  protected function getVerb() {
-    return pht('Deleted');
-  }
-
-  protected function isNewThread() {
-    return false;
-  }
-
-}

src/applications/owners/mail/PackageMail.php

@@ -1,212 +0,0 @@
-<?php
-
-abstract class PackageMail extends PhabricatorMail {
-
-  protected $package;
-  protected $handles;
-  protected $owners;
-  protected $paths;
-  protected $mailTo;
-
-  public function __construct(PhabricatorOwnersPackage $package) {
-    $this->package = $package;
-  }
-
-  abstract protected function getVerb();
-
-  abstract protected function isNewThread();
-
-  final protected function getPackage() {
-    return $this->package;
-  }
-
-  final protected function getHandles() {
-    return $this->handles;
-  }
-
-  final protected function getOwners() {
-    return $this->owners;
-  }
-
-  final protected function getPaths() {
-    return $this->paths;
-  }
-
-  final protected function getMailTo() {
-    return $this->mailTo;
-  }
-
-  final protected function renderPackageTitle() {
-    return $this->getPackage()->getName();
-  }
-
-  final protected function renderRepoSubSection($repository_phid, $paths) {
-    $handles = $this->getHandles();
-    $section = array();
-    $section[] = '  '.
-      pht('In repository %s', $handles[$repository_phid]->getName()).
-      ' - '.PhabricatorEnv::getProductionURI($handles[$repository_phid]
-      ->getURI());
-    foreach ($paths as $path => $excluded) {
-      $section[] = '    '.
-        ($excluded ? pht('Excluded') : pht('Included')).' '.$path;
-    }
-
-    return implode("\n", $section);
-  }
-
-  protected function needSend() {
-    return true;
-  }
-
-  protected function loadData() {
-    $package = $this->getPackage();
-    $owners = $package->loadOwners();
-    $this->owners = $owners;
-
-    $owner_phids = mpull($owners, 'getUserPHID');
-    $primary_owner_phid = $package->getPrimaryOwnerPHID();
-    $mail_to = $owner_phids;
-    if (!in_array($primary_owner_phid, $owner_phids)) {
-      $mail_to[] = $primary_owner_phid;
-    }
-    $this->mailTo = $mail_to;
-
-    $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,
-      array($package->getActorPHID()),
-      array_keys($this->paths));
-    $this->handles = id(new PhabricatorHandleQuery())
-      ->setViewer($this->getActor())
-      ->withPHIDs($phids)
-      ->execute();
-  }
-
-  final protected function renderSummarySection() {
-    $package = $this->getPackage();
-    $handles = $this->getHandles();
-    $section = array();
-    $section[] = $handles[$package->getActorPHID()]->getName().' '.
-      strtolower($this->getVerb()).' '.$this->renderPackageTitle().'.';
-    $section[] = '';
-
-    $section[] = pht('PACKAGE DETAIL');
-    $section[] = '  '.PhabricatorEnv::getProductionURI(
-      '/owners/package/'.$package->getID().'/');
-
-    return implode("\n", $section);
-  }
-
-  protected function renderDescriptionSection() {
-    return pht('PACKAGE DESCRIPTION')."\n  ".
-      $this->getPackage()->getDescription();
-  }
-
-  protected function renderPrimaryOwnerSection() {
-    $handles = $this->getHandles();
-    return pht('PRIMARY OWNER')."\n  ".
-      $handles[$this->getPackage()->getPrimaryOwnerPHID()]->getName();
-  }
-
-  protected function renderOwnersSection() {
-    $handles = $this->getHandles();
-    $owners = $this->getOwners();
-    if (!$owners) {
-      return null;
-    }
-
-    $owners = mpull($owners, 'getUserPHID');
-    $owners = array_select_keys($handles, $owners);
-    $owners = mpull($owners, 'getName');
-    return pht('OWNERS')."\n  ".implode(', ', $owners);
-  }
-
-  protected function renderAuditingEnabledSection() {
-    return pht('AUDITING ENABLED STATUS')."\n  ".
-      ($this->getPackage()->getAuditingEnabled()
-        ? pht('Enabled')
-        : pht('Disabled'));
-  }
-
-  protected function renderPathsSection() {
-    $section = array();
-    $section[] = pht('PATHS');
-    foreach ($this->paths as $repository_phid => $paths) {
-      $section[] = $this->renderRepoSubSection($repository_phid, $paths);
-    }
-
-    return implode("\n", $section);
-  }
-
-  final protected function renderBody() {
-    $body = array();
-    $body[] = $this->renderSummarySection();
-    $body[] = $this->renderDescriptionSection();
-    $body[] = $this->renderPrimaryOwnerSection();
-    $body[] = $this->renderOwnersSection();
-    $body[] = $this->renderAuditingEnabledSection();
-    $body[] = $this->renderPathsSection();
-    $body = array_filter($body);
-    return implode("\n\n", $body)."\n";
-  }
-
-  final public function send() {
-    $mails = $this->prepareMails();
-
-    foreach ($mails as $mail) {
-      $mail->saveAndSend();
-    }
-  }
-
-  final public function prepareMails() {
-    if (!$this->needSend()) {
-      return array();
-    }
-
-    $this->loadData();
-
-    $package = $this->getPackage();
-    $prefix = PhabricatorEnv::getEnvConfig('metamta.package.subject-prefix');
-    $verb = $this->getVerb();
-    $threading = $this->getMailThreading();
-    list($thread_id, $thread_topic) = $threading;
-
-    $template = id(new PhabricatorMetaMTAMail())
-      ->setSubject($this->renderPackageTitle())
-      ->setSubjectPrefix($prefix)
-      ->setVarySubjectPrefix("[{$verb}]")
-      ->setFrom($package->getActorPHID())
-      ->setThreadID($thread_id, $this->isNewThread())
-      ->addHeader('Thread-Topic', $thread_topic)
-      ->setRelatedPHID($package->getPHID())
-      ->setIsBulk(true)
-      ->setBody($this->renderBody());
-
-    $reply_handler = $this->newReplyHandler();
-    $mails = $reply_handler->multiplexMail(
-      $template,
-      array_select_keys($this->getHandles(), $this->getMailTo()),
-      array());
-    return $mails;
-  }
-
-  private function getMailThreading() {
-    return array(
-      'package-'.$this->getPackage()->getPHID(),
-      'Package '.$this->getPackage()->getOriginalName(),
-    );
-  }
-
-  private function newReplyHandler() {
-    $reply_handler = new OwnersPackageReplyHandler();
-    $reply_handler->setMailReceiver($this->getPackage());
-    return $reply_handler;
-  }
-
-}

src/applications/owners/mail/PackageModifyMail.php

@@ -1,160 +0,0 @@
-<?php
-
-final class PackageModifyMail extends PackageMail {
-
-  protected $addOwners;
-  protected $removeOwners;
-  protected $allOwners;
-  protected $touchedRepos;
-  protected $addPaths;
-  protected $removePaths;
-
-  public function __construct(
-    PhabricatorOwnersPackage $package,
-    $add_owners,
-    $remove_owners,
-    $all_owners,
-    $touched_repos,
-    $add_paths,
-    $remove_paths) {
-
-    $this->package = $package;
-
-    $this->addOwners = $add_owners;
-    $this->removeOwners = $remove_owners;
-    $this->allOwners = $all_owners;
-    $this->touchedRepos = $touched_repos;
-    $this->addPaths = $add_paths;
-    $this->removePaths = $remove_paths;
-  }
-
-  protected function getVerb() {
-    return pht('Modified');
-  }
-
-  protected function isNewThread() {
-    return false;
-  }
-
-  protected function needSend() {
-    $package = $this->getPackage();
-    if ($package->getOldPrimaryOwnerPHID() !== $package->getPrimaryOwnerPHID()
-        || $package->getOldAuditingEnabled() != $package->getAuditingEnabled()
-        || $this->addOwners
-        || $this->removeOwners
-        || $this->addPaths
-        || $this->removePaths) {
-      return true;
-    } else {
-      return false;
-    }
-  }
-
-  protected function loadData() {
-    $this->mailTo = $this->allOwners;
-
-    $phids = array_merge(
-      $this->allOwners,
-      $this->touchedRepos,
-      array(
-        $this->getPackage()->getActorPHID(),
-      ));
-    $this->handles = id(new PhabricatorHandleQuery())
-      ->setViewer($this->getActor())
-      ->withPHIDs($phids)
-      ->execute();
-  }
-
-  protected function renderDescriptionSection() {
-    return null;
-  }
-
-  protected function renderPrimaryOwnerSection() {
-    $package = $this->getPackage();
-    $handles = $this->getHandles();
-
-    $old_primary_owner_phid = $package->getOldPrimaryOwnerPHID();
-    $primary_owner_phid = $package->getPrimaryOwnerPHID();
-    if ($old_primary_owner_phid == $primary_owner_phid) {
-      return null;
-    }
-
-    $section = array();
-    $section[] = pht('PRIMARY OWNER CHANGE');
-    $section[] = '  '.pht('Old owner:').' '.
-      $handles[$old_primary_owner_phid]->getName();
-    $section[] = '  '.pht('New owner:').' '.
-      $handles[$primary_owner_phid]->getName();
-
-    return implode("\n", $section);
-  }
-
-  protected function renderOwnersSection() {
-    $section = array();
-    $add_owners = $this->addOwners;
-    $remove_owners = $this->removeOwners;
-    $handles = $this->getHandles();
-
-    if ($add_owners) {
-      $add_owners = array_select_keys($handles, $add_owners);
-      $add_owners = mpull($add_owners, 'getName');
-      $section[] = pht('ADDED OWNERS');
-      $section[] = '  '.implode(', ', $add_owners);
-    }
-
-    if ($remove_owners) {
-      if ($add_owners) {
-        $section[] = '';
-      }
-      $remove_owners = array_select_keys($handles, $remove_owners);
-      $remove_owners = mpull($remove_owners, 'getName');
-      $section[] = pht('REMOVED OWNERS');
-      $section[] = '  '.implode(', ', $remove_owners);
-    }
-
-    if ($section) {
-      return implode("\n", $section);
-    } else {
-      return null;
-    }
-  }
-
-  protected function renderAuditingEnabledSection() {
-    $package = $this->getPackage();
-    $old_auditing_enabled = $package->getOldAuditingEnabled();
-    $auditing_enabled = $package->getAuditingEnabled();
-    if ($old_auditing_enabled == $auditing_enabled) {
-      return null;
-    }
-
-    $section = array();
-    $section[] = pht('AUDITING ENABLED STATUS CHANGE');
-    $section[] = '  '.pht('Old value:').' '.
-      ($old_auditing_enabled ? pht('Enabled') : pht('Disabled'));
-    $section[] = '  '.pht('New value:').' '.
-      ($auditing_enabled ? pht('Enabled') : pht('Disabled'));
-    return implode("\n", $section);
-  }
-
-  protected function renderPathsSection() {
-    $section = array();
-    if ($this->addPaths) {
-      $section[] = pht('ADDED PATHS');
-      foreach ($this->addPaths as $repository_phid => $paths) {
-        $section[] = $this->renderRepoSubSection($repository_phid, $paths);
-      }
-    }
-
-    if ($this->removePaths) {
-      if ($this->addPaths) {
-        $section[] = '';
-      }
-      $section[] = pht('REMOVED PATHS');
-      foreach ($this->removePaths as $repository_phid => $paths) {
-        $section[] = $this->renderRepoSubSection($repository_phid, $paths);
-      }
-    }
-    return implode("\n", $section);
-  }
-
-}

src/applications/owners/storage/PhabricatorOwnersPackage.php

@@ -13,12 +13,6 @@ final class PhabricatorOwnersPackage
   protected $primaryOwnerPHID;
   protected $mailKey;
 
-  private $unsavedOwners = self::ATTACHABLE;
-  private $unsavedPaths = self::ATTACHABLE;
-  private $actorPHID;
-  private $oldPrimaryOwnerPHID;
-  private $oldAuditingEnabled;
-
   public static function initializeNewPackage(PhabricatorUser $actor) {
     return id(new PhabricatorOwnersPackage())
       ->setAuditingEnabled(0)
@@ -83,51 +77,6 @@ final class PhabricatorOwnersPackage
     return parent::save();
   }
 
-  public function attachUnsavedOwners(array $owners) {
-    $this->unsavedOwners = $owners;
-    return $this;
-  }
-
-  public function getUnsavedOwners() {
-    return $this->assertAttached($this->unsavedOwners);
-  }
-
-  public function attachUnsavedPaths(array $paths) {
-    $this->unsavedPaths = $paths;
-    return $this;
-  }
-
-  public function getUnsavedPaths() {
-    return $this->assertAttached($this->unsavedPaths);
-  }
-
-  public function attachActorPHID($actor_phid) {
-    $this->actorPHID = $actor_phid;
-    return $this;
-  }
-
-  public function getActorPHID() {
-    return $this->actorPHID;
-  }
-
-  public function attachOldPrimaryOwnerPHID($old_primary) {
-    $this->oldPrimaryOwnerPHID = $old_primary;
-    return $this;
-  }
-
-  public function getOldPrimaryOwnerPHID() {
-    return $this->oldPrimaryOwnerPHID;
-  }
-
-  public function attachOldAuditingEnabled($auditing_enabled) {
-    $this->oldAuditingEnabled = $auditing_enabled;
-    return $this;
-  }
-
-  public function getOldAuditingEnabled() {
-    return $this->oldAuditingEnabled;
-  }
-
   public function setName($name) {
     $this->name = $name;
     if (!$this->getID()) {

src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php

@@ -8,6 +8,7 @@ final class PhabricatorOwnersPackageTransaction
   const TYPE_OWNERS = 'owners.owners';
   const TYPE_AUDITING = 'owners.auditing';
   const TYPE_DESCRIPTION = 'owners.description';
+  const TYPE_PATHS = 'owners.paths';
 
   public function getApplicationName() {
     return 'owners';
@@ -120,6 +121,11 @@ final class PhabricatorOwnersPackageTransaction
         return pht(
           '%s updated the description for this package.',
           $this->renderHandleLink($author_phid));
+      case self::TYPE_PATHS:
+        // TODO: Flesh this out.
+        return pht(
+          '%s updated paths for this package.',
+          $this->renderHandleLink($author_phid));
     }
 
     return parent::getTitle();
@@ -129,6 +135,8 @@ final class PhabricatorOwnersPackageTransaction
     switch ($this->getTransactionType()) {
       case self::TYPE_DESCRIPTION:
         return ($this->getOldValue() !== null);
+      case self::TYPE_PATHS:
+        return true;
     }
 
     return parent::hasChangeDetails();
@@ -144,6 +152,57 @@ final class PhabricatorOwnersPackageTransaction
           $viewer,
           $old,
           $new);
+      case self::TYPE_PATHS:
+        $old = $this->getOldValue();
+        $new = $this->getNewValue();
+
+        $diffs = PhabricatorOwnersPath::getTransactionValueChanges($old, $new);
+        list($rem, $add) = $diffs;
+
+        $rows = array();
+        foreach ($rem as $ref) {
+          $rows[] = array(
+            'class' => 'diff-removed',
+            'change' => '-',
+          ) + $ref;
+        }
+
+        foreach ($add as $ref) {
+          $rows[] = array(
+            'class' => 'diff-added',
+            'change' => '+',
+          ) + $ref;
+        }
+
+        $rowc = array();
+        foreach ($rows as $key => $row) {
+          $rowc[] = $row['class'];
+          $rows[$key] = array(
+            $row['change'],
+            $row['excluded'] ? pht('Exclude') : pht('Include'),
+            $viewer->renderHandle($row['repositoryPHID']),
+            $row['path'],
+          );
+        }
+
+        $table = id(new AphrontTableView($rows))
+          ->setRowClasses($rowc)
+          ->setHeaders(
+            array(
+              null,
+              pht('Type'),
+              pht('Repository'),
+              pht('Path'),
+            ))
+          ->setColumnClasses(
+            array(
+              null,
+              null,
+              null,
+              'wide',
+            ));
+
+        return $table;
     }
 
     return parent::renderChangeDetails($viewer);

src/applications/owners/storage/PhabricatorOwnersPath.php

@@ -22,4 +22,52 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO {
     ) + parent::getConfiguration();
   }
 
+
+  public static function newFromRef(array $ref) {
+    $path = new PhabricatorOwnersPath();
+    $path->repositoryPHID = $ref['repositoryPHID'];
+    $path->path = $ref['path'];
+    $path->excluded = $ref['excluded'];
+    return $path;
+  }
+
+  public function getRef() {
+    return array(
+      'repositoryPHID' => $this->getRepositoryPHID(),
+      'path' => $this->getPath(),
+      'excluded' => (int)$this->getExcluded(),
+    );
+  }
+
+  public static function getTransactionValueChanges(array $old, array $new) {
+    return array(
+      self::getTransactionValueDiff($old, $new),
+      self::getTransactionValueDiff($new, $old),
+    );
+  }
+
+  private static function getTransactionValueDiff(array $u, array $v) {
+    $set = self::getSetFromTransactionValue($v);
+
+    foreach ($u as $key => $ref) {
+      if (self::isRefInSet($ref, $set)) {
+        unset($u[$key]);
+      }
+    }
+
+    return $u;
+  }
+
+  public static function getSetFromTransactionValue(array $v) {
+    $set = array();
+    foreach ($v as $ref) {
+      $set[$ref['repositoryPHID']][$ref['path']][$ref['excluded']] = true;
+    }
+    return $set;
+  }
+
+  public static function isRefInSet(array $ref, array $set) {
+    return isset($set[$ref['repositoryPHID']][$ref['path']][$ref['excluded']]);
+  }
+
 }

src/applications/repository/worker/commitchangeparser/PhabricatorOwnersPackagePathValidator.php

@@ -1,108 +0,0 @@
-<?php
-
-final class PhabricatorOwnersPackagePathValidator {
-
-  /*
-   * If a file/directory was moved the paths in owners package become stale.
-   * This method updates the stale paths in the owners packages to their new
-   * paths.
-   */
-  public static function updateOwnersPackagePaths(
-    PhabricatorRepositoryCommit $commit,
-    PhabricatorUser $actor) {
-
-    $repository = id(new PhabricatorRepositoryQuery())
-      ->setViewer($actor)
-      ->withIDs(array($commit->getRepositoryID()))
-      ->executeOne();
-    if (!$repository) {
-      return;
-    }
-    $changes = self::loadDiffusionChangesForCommit(
-      $repository,
-      $commit,
-      $actor);
-
-    if (!$changes) {
-      return;
-    }
-
-    $move_map = array();
-    foreach ($changes as $change) {
-      if ($change->getChangeType() == DifferentialChangeType::TYPE_MOVE_HERE) {
-        $from_path = '/'.$change->getTargetPath();
-        $to_path = '/'.$change->getPath();
-        if ($change->getFileType() == DifferentialChangeType::FILE_DIRECTORY) {
-          $to_path = $to_path.'/';
-          $from_path = $from_path.'/';
-        }
-        $move_map[$from_path] = $to_path;
-      }
-    }
-
-    if ($move_map) {
-      self::updateAffectedPackages($repository, $move_map);
-    }
-  }
-
-  private static function updateAffectedPackages($repository, array $move_map) {
-    $paths = array_keys($move_map);
-    if ($paths) {
-      $packages = PhabricatorOwnersPackage::loadAffectedPackages($repository,
-        $paths);
-      foreach ($packages as $package) {
-        self::updatePackagePaths($package, $move_map);
-      }
-    }
-  }
-
-  private static function updatePackagePaths($package, array $move_map) {
-    $paths = array_keys($move_map);
-    $pkg_paths = $package->loadPaths();
-    $new_paths = array();
-    foreach ($pkg_paths as $pkg_path) {
-      $path_changed = false;
-
-      foreach ($paths as $old_path) {
-        if (strncmp($pkg_path->getPath(), $old_path, strlen($old_path)) === 0) {
-          $new_paths[] = array (
-            'packageID' => $package->getID(),
-            'repositoryPHID' => $pkg_path->getRepositoryPHID(),
-            'path' => str_replace($pkg_path->getPath(), $old_path,
-                                    $move_map[$old_path]),
-          );
-          $path_changed = true;
-        }
-      }
-
-      if (!$path_changed) {
-        $new_paths[] = array (
-          'packageID' => $package->getID(),
-          'repositoryPHID' => $pkg_path->getRepositoryPHID(),
-          'path' => $pkg_path->getPath(),
-        );
-      }
-    }
-
-    if ($new_paths) {
-      $package->attachOldPrimaryOwnerPHID($package->getPrimaryOwnerPHID());
-      $package->attachUnsavedPaths($new_paths);
-      $package->save(); // save the changes and notify the owners.
-    }
-  }
-
-  private static function loadDiffusionChangesForCommit(
-    PhabricatorRepository $repository,
-    PhabricatorRepositoryCommit $commit,
-    PhabricatorUser $actor) {
-    $data = array(
-      'user' => $actor,
-      'repository' => $repository,
-      'commit' => $commit->getCommitIdentifier(),
-    );
-    $drequest = DiffusionRequest::newFromDictionary($data);
-    $change_query =
-      DiffusionPathChangeQuery::newFromDiffusionRequest($drequest);
-    return $change_query->loadChanges();
-  }
-}

src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php

@@ -96,9 +96,6 @@ abstract class PhabricatorRepositoryCommitChangeParserWorker
     id(new PhabricatorSearchIndexer())
       ->queueDocumentForIndexing($commit->getPHID());
 
-    PhabricatorOwnersPackagePathValidator::updateOwnersPackagePaths(
-      $commit,
-      PhabricatorUser::getOmnipotentUser());
     if ($this->shouldQueueFollowupTasks()) {
       $this->queueTask(
         'PhabricatorRepositoryCommitOwnersWorker',

webroot/rsrc/css/aphront/table-view.css

@@ -211,6 +211,16 @@ span.single-display-line-content {
   background: #fcf2bb;
 }
 
+.aphront-table-view tr.diff-removed,
+.aphront-table-view tr.alt-diff-removed {
+  background: {$lightred}
+}
+
+.aphront-table-view tr.diff-added,
+.aphront-table-view tr.alt-diff-added {
+  background: {$lightgreen}
+}
+
 .aphront-table-view tr.no-data td {
   padding: 12px;
   text-align: center;