Perform derived index updates in TransactionEditor

Summary: Ref T2222. We have two tables (one for hashes; one for paths) which were unevenly updated before. Now, update them consistently in the TransactionEditor.

Test Plan: Created a revision, saw it populate this information.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

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

src/applications/differential/editor/DifferentialRevisionEditor.php

@@ -18,75 +18,12 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
   private $auxiliaryFields = array();
   private $contentSource;
   private $isCreate;
-  private $aphrontRequestForEventDispatch;
-
-
-  public function setAphrontRequestForEventDispatch(AphrontRequest $request) {
-    $this->aphrontRequestForEventDispatch = $request;
-    return $this;
-  }
-
-  public function getAphrontRequestForEventDispatch() {
-    return $this->aphrontRequestForEventDispatch;
-  }
 
   public function __construct(DifferentialRevision $revision) {
     $this->revision = $revision;
     $this->isCreate = !($revision->getID());
   }
 
-  public static function newRevisionFromConduitWithDiff(
-    array $fields,
-    DifferentialDiff $diff,
-    PhabricatorUser $actor) {
-
-    $revision = DifferentialRevision::initializeNewRevision($actor);
-    $revision->setPHID($revision->generatePHID());
-
-    $editor = new DifferentialRevisionEditor($revision);
-    $editor->setActor($actor);
-    $editor->addDiff($diff, null);
-    $editor->copyFieldsFromConduit($fields);
-
-    $editor->save();
-
-    return $revision;
-  }
-
-  public function copyFieldsFromConduit(array $fields) {
-
-    $actor = $this->getActor();
-    $revision = $this->revision;
-    $revision->loadRelationships();
-
-    $all_fields = DifferentialFieldSelector::newSelector()
-      ->getFieldSpecifications();
-
-    $aux_fields = array();
-    foreach ($all_fields as $aux_field) {
-      $aux_field->setRevision($revision);
-      $aux_field->setDiff($this->diff);
-      $aux_field->setUser($actor);
-      if ($aux_field->shouldAppearOnCommitMessage()) {
-        $aux_fields[$aux_field->getCommitMessageKey()] = $aux_field;
-      }
-    }
-
-    foreach ($fields as $field => $value) {
-      if (empty($aux_fields[$field])) {
-        throw new Exception(
-          "Parsed commit message contains unrecognized field '{$field}'.");
-      }
-      $aux_fields[$field]->setValueFromParsedCommitMessage($value);
-    }
-
-    foreach ($aux_fields as $aux_field) {
-      $aux_field->validateField();
-    }
-
-    $this->setAuxiliaryFields($all_fields);
-  }
-
   public function setAuxiliaryFields(array $auxiliary_fields) {
     assert_instances_of($auxiliary_fields, 'DifferentialFieldSpecification');
     $this->auxiliaryFields = $auxiliary_fields;
@@ -399,9 +336,6 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
       $diff->setDescription(preg_replace('/\n.*/s', '', $this->getComments()));
       $diff->save();
 
-      $this->updateAffectedPathTable($revision, $diff, $changesets);
-      $this->updateRevisionHashTable($revision, $diff);
-
       // An updated diff should require review, as long as it's not closed
       // or accepted. The "accepted" status is "sticky" to encourage courtesy
       // re-diffs after someone accepts with minor changes/suggestions.
@@ -686,177 +620,7 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
     }
   }
 
-  /**
-   * Update the table which links Differential revisions to paths they affect,
-   * so Diffusion can efficiently find pending revisions for a given file.
-   */
-  private function updateAffectedPathTable(
-    DifferentialRevision $revision,
-    DifferentialDiff $diff,
-    array $changesets) {
-    assert_instances_of($changesets, 'DifferentialChangeset');
 
-    $project = $diff->loadArcanistProject();
-    if (!$project) {
-      // Probably an old revision from before projects.
-      return;
-    }
-
-    $repository = $project->loadRepository();
-    if (!$repository) {
-      // Probably no project <-> repository link, or the repository where the
-      // project lives is untracked.
-      return;
-    }
-
-    $path_prefix = null;
-
-    $local_root = $diff->getSourceControlPath();
-    if ($local_root) {
-      // We're in a working copy which supports subdirectory checkouts (e.g.,
-      // SVN) so we need to figure out what prefix we should add to each path
-      // (e.g., trunk/projects/example/) to get the absolute path from the
-      // root of the repository. DVCS systems like Git and Mercurial are not
-      // affected.
-
-      // Normalize both paths and check if the repository root is a prefix of
-      // the local root. If so, throw it away. Note that this correctly handles
-      // the case where the remote path is "/".
-      $local_root = id(new PhutilURI($local_root))->getPath();
-      $local_root = rtrim($local_root, '/');
-
-      $repo_root = id(new PhutilURI($repository->getRemoteURI()))->getPath();
-      $repo_root = rtrim($repo_root, '/');
-
-      if (!strncmp($repo_root, $local_root, strlen($repo_root))) {
-        $path_prefix = substr($local_root, strlen($repo_root));
-      }
-    }
-
-    $paths = array();
-    foreach ($changesets as $changeset) {
-      $paths[] = $path_prefix.'/'.$changeset->getFilename();
-    }
-
-    // Mark this as also touching all parent paths, so you can see all pending
-    // changes to any file within a directory.
-    $all_paths = array();
-    foreach ($paths as $local) {
-      foreach (DiffusionPathIDQuery::expandPathToRoot($local) as $path) {
-        $all_paths[$path] = true;
-      }
-    }
-    $all_paths = array_keys($all_paths);
-
-    $path_ids =
-      PhabricatorRepositoryCommitChangeParserWorker::lookupOrCreatePaths(
-        $all_paths);
-
-    $table = new DifferentialAffectedPath();
-    $conn_w = $table->establishConnection('w');
-
-    $sql = array();
-    foreach ($path_ids as $path_id) {
-      $sql[] = qsprintf(
-        $conn_w,
-        '(%d, %d, %d, %d)',
-        $repository->getID(),
-        $path_id,
-        time(),
-        $revision->getID());
-    }
-
-    queryfx(
-      $conn_w,
-      'DELETE FROM %T WHERE revisionID = %d',
-      $table->getTableName(),
-      $revision->getID());
-    foreach (array_chunk($sql, 256) as $chunk) {
-      queryfx(
-        $conn_w,
-        'INSERT INTO %T (repositoryID, pathID, epoch, revisionID) VALUES %Q',
-        $table->getTableName(),
-        implode(', ', $chunk));
-    }
-  }
-
-
-  /**
-   * Update the table connecting revisions to DVCS local hashes, so we can
-   * identify revisions by commit/tree hashes.
-   */
-  private function updateRevisionHashTable(
-    DifferentialRevision $revision,
-    DifferentialDiff $diff) {
-
-    $vcs = $diff->getSourceControlSystem();
-    if ($vcs == DifferentialRevisionControlSystem::SVN) {
-      // Subversion has no local commit or tree hash information, so we don't
-      // have to do anything.
-      return;
-    }
-
-    $property = id(new DifferentialDiffProperty())->loadOneWhere(
-      'diffID = %d AND name = %s',
-      $diff->getID(),
-      'local:commits');
-    if (!$property) {
-      return;
-    }
-
-    $hashes = array();
-
-    $data = $property->getData();
-    switch ($vcs) {
-      case DifferentialRevisionControlSystem::GIT:
-        foreach ($data as $commit) {
-          $hashes[] = array(
-            ArcanistDifferentialRevisionHash::HASH_GIT_COMMIT,
-            $commit['commit'],
-          );
-          $hashes[] = array(
-            ArcanistDifferentialRevisionHash::HASH_GIT_TREE,
-            $commit['tree'],
-          );
-        }
-        break;
-      case DifferentialRevisionControlSystem::MERCURIAL:
-        foreach ($data as $commit) {
-          $hashes[] = array(
-            ArcanistDifferentialRevisionHash::HASH_MERCURIAL_COMMIT,
-            $commit['rev'],
-          );
-        }
-        break;
-    }
-
-    $conn_w = $revision->establishConnection('w');
-
-    $sql = array();
-    foreach ($hashes as $info) {
-      list($type, $hash) = $info;
-      $sql[] = qsprintf(
-        $conn_w,
-        '(%d, %s, %s)',
-        $revision->getID(),
-        $type,
-        $hash);
-    }
-
-    queryfx(
-      $conn_w,
-      'DELETE FROM %T WHERE revisionID = %d',
-      ArcanistDifferentialRevisionHash::TABLE_NAME,
-      $revision->getID());
-
-    if ($sql) {
-      queryfx(
-        $conn_w,
-        'INSERT INTO %T (revisionID, type, hash) VALUES %Q',
-        ArcanistDifferentialRevisionHash::TABLE_NAME,
-        implode(', ', $sql));
-    }
-  }
 
   /**
    * Try to move a revision to "accepted". We look for:

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -276,7 +276,6 @@ final class DifferentialTransactionEditor
         $maniphest = 'PhabricatorApplicationManiphest';
         if (PhabricatorApplication::isClassInstalled($maniphest)) {
           $diff = $this->loadDiff($xaction->getNewValue());
-          if ($diff) {
           $branch = $diff->getBranch();
 
           // No "$", to allow for branches like T123_demo.
@@ -299,7 +298,6 @@ final class DifferentialTransactionEditor
             }
           }
         }
-        }
         break;
 
       case PhabricatorTransactions::TYPE_COMMENT:
@@ -506,6 +504,20 @@ final class DifferentialTransactionEditor
     PhabricatorLiskDAO $object,
     array $xactions) {
 
+    foreach ($xactions as $xaction) {
+      switch ($xaction->getTransactionType()) {
+        case DifferentialTransaction::TYPE_UPDATE:
+          $diff = $this->loadDiff($xaction->getNewValue(), true);
+
+          // Update these denormalized index tables when we attach a new
+          // diff to a revision.
+
+          $this->updateRevisionHashTable($object, $diff);
+          $this->updateAffectedPathTable($object, $diff);
+          break;
+      }
+    }
+
     $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED;
     $status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
     $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
@@ -1167,11 +1179,16 @@ final class DifferentialTransactionEditor
     return implode("\n", $result);
   }
 
-  private function loadDiff($phid) {
+  private function loadDiff($phid, $need_changesets = false) {
-    return id(new DifferentialDiffQuery())
+    $query = id(new DifferentialDiffQuery())
       ->withPHIDs(array($phid))
-      ->setViewer($this->getActor())
+      ->setViewer($this->getActor());
-      ->executeOne();
+
+    if ($need_changesets) {
+      $query->needChangesets(true);
+    }
+
+    return $query->executeOne();
   }
 
 /* -(  Herald Integration  )------------------------------------------------- */
@@ -1300,4 +1317,178 @@ final class DifferentialTransactionEditor
     return $xactions;
   }
 
+  /**
+   * Update the table which links Differential revisions to paths they affect,
+   * so Diffusion can efficiently find pending revisions for a given file.
+   */
+  private function updateAffectedPathTable(
+    DifferentialRevision $revision,
+    DifferentialDiff $diff) {
+
+    $changesets = $diff->getChangesets();
+
+    // TODO: This all needs to be modernized.
+
+    $project = $diff->loadArcanistProject();
+    if (!$project) {
+      // Probably an old revision from before projects.
+      return;
+    }
+
+    $repository = $project->loadRepository();
+    if (!$repository) {
+      // Probably no project <-> repository link, or the repository where the
+      // project lives is untracked.
+      return;
+    }
+
+    $path_prefix = null;
+
+    $local_root = $diff->getSourceControlPath();
+    if ($local_root) {
+      // We're in a working copy which supports subdirectory checkouts (e.g.,
+      // SVN) so we need to figure out what prefix we should add to each path
+      // (e.g., trunk/projects/example/) to get the absolute path from the
+      // root of the repository. DVCS systems like Git and Mercurial are not
+      // affected.
+
+      // Normalize both paths and check if the repository root is a prefix of
+      // the local root. If so, throw it away. Note that this correctly handles
+      // the case where the remote path is "/".
+      $local_root = id(new PhutilURI($local_root))->getPath();
+      $local_root = rtrim($local_root, '/');
+
+      $repo_root = id(new PhutilURI($repository->getRemoteURI()))->getPath();
+      $repo_root = rtrim($repo_root, '/');
+
+      if (!strncmp($repo_root, $local_root, strlen($repo_root))) {
+        $path_prefix = substr($local_root, strlen($repo_root));
+      }
+    }
+
+    $paths = array();
+    foreach ($changesets as $changeset) {
+      $paths[] = $path_prefix.'/'.$changeset->getFilename();
+    }
+
+    // Mark this as also touching all parent paths, so you can see all pending
+    // changes to any file within a directory.
+    $all_paths = array();
+    foreach ($paths as $local) {
+      foreach (DiffusionPathIDQuery::expandPathToRoot($local) as $path) {
+        $all_paths[$path] = true;
+      }
+    }
+    $all_paths = array_keys($all_paths);
+
+    $path_ids =
+      PhabricatorRepositoryCommitChangeParserWorker::lookupOrCreatePaths(
+        $all_paths);
+
+    $table = new DifferentialAffectedPath();
+    $conn_w = $table->establishConnection('w');
+
+    $sql = array();
+    foreach ($path_ids as $path_id) {
+      $sql[] = qsprintf(
+        $conn_w,
+        '(%d, %d, %d, %d)',
+        $repository->getID(),
+        $path_id,
+        time(),
+        $revision->getID());
+    }
+
+    queryfx(
+      $conn_w,
+      'DELETE FROM %T WHERE revisionID = %d',
+      $table->getTableName(),
+      $revision->getID());
+    foreach (array_chunk($sql, 256) as $chunk) {
+      queryfx(
+        $conn_w,
+        'INSERT INTO %T (repositoryID, pathID, epoch, revisionID) VALUES %Q',
+        $table->getTableName(),
+        implode(', ', $chunk));
+    }
+  }
+
+
+  /**
+   * Update the table connecting revisions to DVCS local hashes, so we can
+   * identify revisions by commit/tree hashes.
+   */
+  private function updateRevisionHashTable(
+    DifferentialRevision $revision,
+    DifferentialDiff $diff) {
+
+    $vcs = $diff->getSourceControlSystem();
+    if ($vcs == DifferentialRevisionControlSystem::SVN) {
+      // Subversion has no local commit or tree hash information, so we don't
+      // have to do anything.
+      return;
+    }
+
+    $property = id(new DifferentialDiffProperty())->loadOneWhere(
+      'diffID = %d AND name = %s',
+      $diff->getID(),
+      'local:commits');
+    if (!$property) {
+      return;
+    }
+
+    $hashes = array();
+
+    $data = $property->getData();
+    switch ($vcs) {
+      case DifferentialRevisionControlSystem::GIT:
+        foreach ($data as $commit) {
+          $hashes[] = array(
+            ArcanistDifferentialRevisionHash::HASH_GIT_COMMIT,
+            $commit['commit'],
+          );
+          $hashes[] = array(
+            ArcanistDifferentialRevisionHash::HASH_GIT_TREE,
+            $commit['tree'],
+          );
+        }
+        break;
+      case DifferentialRevisionControlSystem::MERCURIAL:
+        foreach ($data as $commit) {
+          $hashes[] = array(
+            ArcanistDifferentialRevisionHash::HASH_MERCURIAL_COMMIT,
+            $commit['rev'],
+          );
+        }
+        break;
+    }
+
+    $conn_w = $revision->establishConnection('w');
+
+    $sql = array();
+    foreach ($hashes as $info) {
+      list($type, $hash) = $info;
+      $sql[] = qsprintf(
+        $conn_w,
+        '(%d, %s, %s)',
+        $revision->getID(),
+        $type,
+        $hash);
+    }
+
+    queryfx(
+      $conn_w,
+      'DELETE FROM %T WHERE revisionID = %d',
+      ArcanistDifferentialRevisionHash::TABLE_NAME,
+      $revision->getID());
+
+    if ($sql) {
+      queryfx(
+        $conn_w,
+        'INSERT INTO %T (revisionID, type, hash) VALUES %Q',
+        ArcanistDifferentialRevisionHash::TABLE_NAME,
+        implode(', ', $sql));
+    }
+  }
+
 }