From 925c9a71e7eac24b76a6d836ed4c35a7fb4cea33 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 15 Mar 2021 09:53:41 -0700
Subject: [PATCH] Support a "withPaths()" API in DifferentialRevisionQuery, and
 use it on the revision view

Summary: Ref T13639. Move away from "withPath(..., ...)" to "withPaths(...)".

Test Plan: {F8539323}

Maniphest Tasks: T13639

Differential Revision: https://secure.phabricator.com/D21619
---
 .../DifferentialRevisionViewController.php    | 24 +++----
 .../query/DifferentialRevisionQuery.php       | 67 +++++++++++++++++++
 2 files changed, 78 insertions(+), 13 deletions(-)

diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php
index e37fa2b529..13ab8a8954 100644
--- a/src/applications/differential/controller/DifferentialRevisionViewController.php
+++ b/src/applications/differential/controller/DifferentialRevisionViewController.php
@@ -986,6 +986,8 @@ final class DifferentialRevisionViewController
     PhabricatorRepository $repository) {
     assert_instances_of($changesets, 'DifferentialChangeset');
 
+    $viewer = $this->getViewer();
+
     $paths = array();
     foreach ($changesets as $changeset) {
       $paths[] = $changeset->getAbsoluteRepositoryPath(
@@ -997,34 +999,30 @@ final class DifferentialRevisionViewController
       return array();
     }
 
-    $path_map = id(new DiffusionPathIDQuery($paths))->loadPathIDs();
-
-    if (!$path_map) {
-      return array();
-    }
-
     $recent = (PhabricatorTime::getNow() - phutil_units('30 days in seconds'));
 
     $query = id(new DifferentialRevisionQuery())
-      ->setViewer($this->getRequest()->getUser())
+      ->setViewer($viewer)
       ->withIsOpen(true)
       ->withUpdatedEpochBetween($recent, null)
       ->setOrder(DifferentialRevisionQuery::ORDER_MODIFIED)
       ->setLimit(10)
       ->needFlags(true)
       ->needDrafts(true)
-      ->needReviewers(true);
-
-    foreach ($path_map as $path => $path_id) {
-      $query->withPath($repository->getID(), $path_id);
-    }
+      ->needReviewers(true)
+      ->withRepositoryPHIDs(
+        array(
+          $repository->getPHID(),
+        ))
+      ->withPaths($paths);
 
     $results = $query->execute();
 
     // Strip out *this* revision.
     foreach ($results as $key => $result) {
       if ($result->getID() == $this->revisionID) {
-        unset($results[$key]);
+       unset($results[$key]);
+       break;
       }
     }
 
diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php
index 0f6d5d944b..c77e99d8c2 100644
--- a/src/applications/differential/query/DifferentialRevisionQuery.php
+++ b/src/applications/differential/query/DifferentialRevisionQuery.php
@@ -27,6 +27,7 @@ final class DifferentialRevisionQuery
   private $createdEpochMin;
   private $createdEpochMax;
   private $noReviewers;
+  private $paths;
 
   const ORDER_MODIFIED      = 'order-modified';
   const ORDER_CREATED       = 'order-created';
@@ -62,6 +63,18 @@ final class DifferentialRevisionQuery
     return $this;
   }
 
+  /**
+   * Find revisions affecting one or more items in a list of paths.
+   *
+   * @param list<string> List of file paths.
+   * @return this
+   * @task config
+   */
+  public function withPaths(array $paths) {
+    $this->paths = $paths;
+    return $this;
+  }
+
   /**
    * Filter results to revisions authored by one of the given PHIDs. Calling
    * this function will clear anything set by previous calls to
@@ -576,6 +589,14 @@ final class DifferentialRevisionQuery
         $path_table->getTableName());
     }
 
+    if ($this->paths) {
+      $path_table = new DifferentialAffectedPath();
+      $joins[] = qsprintf(
+        $conn,
+        'JOIN %R paths ON paths.revisionID = r.id',
+        $path_table);
+    }
+
     if ($this->commitHashes) {
       $joins[] = qsprintf(
         $conn,
@@ -635,6 +656,7 @@ final class DifferentialRevisionQuery
    * @task internal
    */
   protected function buildWhereClause(AphrontDatabaseConnection $conn) {
+    $viewer = $this->getViewer();
     $where = array();
 
     if ($this->pathIDs) {
@@ -651,6 +673,45 @@ final class DifferentialRevisionQuery
       $where[] = $path_clauses;
     }
 
+    if ($this->paths !== null) {
+      $paths = $this->paths;
+
+      $path_map = id(new DiffusionPathIDQuery($paths))
+        ->loadPathIDs();
+
+      if (!$path_map) {
+        // If none of the paths have entries in the PathID table, we can not
+        // possibly find any revisions affecting them.
+        throw new PhabricatorEmptyQueryException();
+      }
+
+      $where[] = qsprintf(
+        $conn,
+        'paths.pathID IN (%Ld)',
+        array_fuse($path_map));
+
+      // If we have repository PHIDs, additionally constrain this query to
+      // try to help MySQL execute it efficiently.
+      if ($this->repositoryPHIDs !== null) {
+        $repositories = id(new PhabricatorRepositoryQuery())
+          ->setViewer($viewer)
+          ->setParentQuery($this)
+          ->withPHIDs($this->repositoryPHIDs)
+          ->execute();
+
+        if (!$repositories) {
+          throw new PhabricatorEmptyQueryException();
+        }
+
+        $repository_ids = mpull($repositories, 'getID');
+
+        $where[] = qsprintf(
+          $conn,
+          'paths.repositoryID IN (%Ld)',
+          $repository_ids);
+      }
+    }
+
     if ($this->authors) {
       $where[] = qsprintf(
         $conn,
@@ -782,6 +843,12 @@ final class DifferentialRevisionQuery
       return true;
     }
 
+    if ($this->paths) {
+      // (If we have exactly one repository and exactly one path, we don't
+      // technically need to group, but it's simpler to always group.)
+      return true;
+    }
+
     if (count($this->ccs) > 1) {
       return true;
     }