Improve DifferentialRevisionQuery and add the ability to query by arcanist project

Summary:
  - We currently post-filter by branches, but should do this in SQL. See T799.
  - We currently identify branch-name-matches as being in the working copy even if they belong to a different project (e.g., two different projects with commits on the branch "master"). See T1100.
  - Denormalize branch and project information into DifferentialRevision.
  - Expose project information in the API.

Test Plan: Ran conduit API queries with branches and arc project IDs, got reasonable results.

Reviewers: btrahan, vrana, jungejason

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1100, T799

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

Author: epriestley

resources/sql/patches/130.denormalrevisionquery.sql

@@ -0,0 +1,5 @@
+ALTER TABLE phabricator_differential.differential_revision
+  ADD branchName VARCHAR(255) COLLATE utf8_general_ci;
+
+ALTER TABLE phabricator_differential.differential_revision
+  ADD arcanistProjectPHID VARCHAR(64) COLLATE utf8_bin;

resources/sql/patches/131.migraterevisionquery.php

@@ -0,0 +1,44 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+$table = new DifferentialRevision();
+$conn_w = $table->establishConnection('w');
+
+$revisions = id(new DifferentialRevision())->loadAll();
+
+echo "Migrating ".count($revisions)." revisions";
+foreach ($revisions as $revision) {
+  echo ".";
+
+  $diff = $revision->loadActiveDiff();
+  if (!$diff) {
+    continue;
+  }
+
+  $branch_name = $diff->getBranch();
+  $arc_project_phid = $diff->getArcanistProjectPHID();
+
+  queryfx(
+    $conn_w,
+    'UPDATE %T SET branchName = %s, arcanistProjectPHID = %s WHERE id = %d',
+    $table->getTableName(),
+    $branch_name,
+    $arc_project_phid,
+    $revision->getID());
+}
+echo "\nDone.\n";

src/applications/conduit/method/differential/query/ConduitAPI_differential_query_Method.php

@@ -62,6 +62,7 @@ public function defineParamTypes() {
       'subscribers'       => 'optional list<phid>',
       'responsibleUsers'  => 'optional list<phid>',
       'branches'          => 'optional list<string>',
+      'arcanistProjects'  => 'optional list<string>',
     );
   }
 
@@ -89,6 +90,7 @@ protected function execute(ConduitAPIRequest $request) {
     $subscribers        = $request->getValue('subscribers');
     $responsible_users  = $request->getValue('responsibleUsers');
     $branches           = $request->getValue('branches');
+    $arc_projects       = $request->getValue('arcanistProjects');
 
     $query = new DifferentialRevisionQuery();
     if ($authors) {
@@ -151,6 +153,19 @@ protected function execute(ConduitAPIRequest $request) {
     if ($branches) {
       $query->withBranches($branches);
     }
+    if ($arc_projects) {
+      // This is sort of special-cased, but don't make arc do an extra round
+      // trip.
+      $projects = id(new PhabricatorRepositoryArcanistProject())
+        ->loadAllWhere(
+          'name in (%Ls)',
+          $arc_projects);
+      if (!$projects) {
+        return array();
+      }
+
+      $query->withArcanistProjectPHIDs(mpull($projects, 'getPHID'));
+    }
 
     $query->needRelationships(true);
     $query->needCommitPHIDs(true);

src/applications/conduit/method/differential/query/__init__.php

@@ -12,7 +12,10 @@
 phutil_require_module('phabricator', 'applications/conduit/method/base');
 phutil_require_module('phabricator', 'applications/conduit/protocol/exception');
 phutil_require_module('phabricator', 'applications/differential/query/revision');
+phutil_require_module('phabricator', 'applications/repository/storage/arcanistproject');
 phutil_require_module('phabricator', 'infrastructure/env');
 
+phutil_require_module('phutil', 'utils');
+
 
 phutil_require_source('ConduitAPI_differential_query_Method.php');

src/applications/differential/editor/revision/DifferentialRevisionEditor.php

@@ -206,7 +206,13 @@ public function save() {
 
     $revision->openTransaction();
 
+      if ($diff) {
+        $revision->setBranchName($diff->getBranch());
+        $revision->setArcanistProjectPHID($diff->getArcanistProjectPHID());
+      }
+
       $revision->save();
+
       if ($diff) {
         $diff->setRevisionID($revision->getID());
         $diff->save();

src/applications/differential/query/revision/DifferentialRevisionQuery.php

@@ -51,6 +51,7 @@ final class DifferentialRevisionQuery {
   private $subscribers = array();
   private $responsibles = array();
   private $branches = array();
+  private $arcanistProjectPHIDs = array();
 
   private $order            = 'order-modified';
   const ORDER_MODIFIED      = 'order-modified';
@@ -244,6 +245,20 @@ public function withSubscribers(array $subscriber_phids) {
   }
 
 
+  /**
+   * Filter results to only return revisions with a given set of arcanist
+   * projects.
+   *
+   * @param array List of project PHIDs.
+   * @return this
+   * @task config
+   */
+  public function withArcanistProjectPHIDs(array $arc_project_phids) {
+    $this->arcanistProjectPHIDs = $arc_project_phids;
+    return $this;
+  }
+
+
   /**
    * Set result ordering. Provide a class constant, such as
    * ##DifferentialRevisionQuery::ORDER_CREATED##.
@@ -368,45 +383,17 @@ public function execute() {
         $this->loadCommitPHIDs($conn_r, $revisions);
       }
 
-      $need_active = $this->needActiveDiffs ||
-                     $this->branches;
-
+      $need_active = $this->needActiveDiffs;
       $need_ids = $need_active ||
                   $this->needDiffIDs;
 
-
       if ($need_ids) {
         $this->loadDiffIDs($conn_r, $revisions);
       }
 
       if ($need_active) {
         $this->loadActiveDiffs($conn_r, $revisions);
       }
-
-      if ($this->branches) {
-
-        // TODO: We could filter this in SQL instead and might get better
-        // performance in some cases.
-
-        $branch_map = array_fill_keys($this->branches, true);
-        foreach ($revisions as $key => $revision) {
-          $diff = $revision->getActiveDiff();
-          if (!$diff) {
-            unset($revisions[$key]);
-            continue;
-          }
-
-          // TODO: Old arc uploaded the wrong branch name for Mercurial (i.e.,
-          // with a trailing "\n"). Once the arc version gets bumped, do a
-          // migration and remove this.
-          $branch = trim($diff->getBranch());
-
-          if (!$diff || empty($branch_map[$branch])) {
-            unset($revisions[$key]);
-            continue;
-          }
-        }
-      }
     }
 
     return $revisions;
@@ -431,7 +418,9 @@ private function shouldUseResponsibleFastPath() {
         !$this->authors &&
         !$this->revIDs &&
         !$this->commitHashes &&
-        !$this->phids) {
+        !$this->phids &&
+        !$this->branches &&
+        !$this->arcanistProjectPHIDs) {
       return true;
     }
     return false;
@@ -656,6 +645,20 @@ private function buildWhereClause($conn_r) {
         $this->responsibles);
     }
 
+    if ($this->branches) {
+      $where[] = qsprintf(
+        $conn_r,
+        'r.branchName in (%Ls)',
+        $this->branches);
+    }
+
+    if ($this->arcanistProjectPHIDs) {
+      $where[] = qsprintf(
+        $conn_r,
+        'r.arcanistProjectPHID in (%Ls)',
+        $this->arcanistProjectPHIDs);
+    }
+
     switch ($this->status) {
       case self::STATUS_ANY:
         break;

src/applications/differential/storage/revision/DifferentialRevision.php

@@ -35,12 +35,15 @@ final class DifferentialRevision extends DifferentialDAO {
   protected $unsubscribed = array();
 
   protected $mailKey;
+  protected $branchName;
+  protected $arcanistProjectPHID;
 
   private $relationships;
   private $commits;
   private $activeDiff = false;
   private $diffIDs;
 
+
   const RELATIONSHIP_TABLE    = 'differential_relationship';
   const TABLE_COMMIT          = 'differential_commit';