Build an "affected path" index when attaching diffs to revisions

Summary: See T262. This creates the index on the Differential side which we need in order to execute this query efficiently on the Diffusion side.

Also renames "DiffusionGitPathIDQuery" to "DiffusionPathIDQuery", this query object has nothing to do with git.

Test Plan: Attached top-level and sub-level diffs to revisions and verified they populated the table with sensible data.

Reviewers: bmaurer, aravindn, fmoo, jungejason, nh, tuomaspelkonen, aran

CC:

Differential Revision: 931

resources/sql/patches/074.affectedpath.sql

@@ -0,0 +1,8 @@
+CREATE TABLE phabricator_differential.differential_affectedpath (
+  repositoryID INT UNSIGNED NOT NULL,
+  pathID INT UNSIGNED NOT NULL,
+  epoch INT UNSIGNED NOT NULL,
+  KEY (repositoryID, pathID, epoch),
+  revisionID INT UNSIGNED NOT NULL,
+  KEY (revisionID)
+) ENGINE=InnoDB;

src/__phutil_library_map__.php

@@ -146,6 +146,7 @@ phutil_register_library_map(array(
     'DatabaseConfigurationProvider' => 'applications/base/storage/configuration',
     'DifferentialAction' => 'applications/differential/constants/action',
     'DifferentialAddCommentView' => 'applications/differential/view/addcomment',
+    'DifferentialAffectedPath' => 'applications/differential/storage/affectedpath',
     'DifferentialApplyPatchFieldSpecification' => 'applications/differential/field/specification/applypatch',
     'DifferentialArcanistProjectFieldSpecification' => 'applications/differential/field/specification/arcanistproject',
     'DifferentialAuthorFieldSpecification' => 'applications/differential/field/specification/author',
@@ -250,7 +251,6 @@ phutil_register_library_map(array(
     'DiffusionGitFileContentQuery' => 'applications/diffusion/query/filecontent/git',
     'DiffusionGitHistoryQuery' => 'applications/diffusion/query/history/git',
     'DiffusionGitLastModifiedQuery' => 'applications/diffusion/query/lastmodified/git',
-    'DiffusionGitPathIDQuery' => 'applications/diffusion/query/pathid/base',
     'DiffusionGitRequest' => 'applications/diffusion/request/git',
     'DiffusionHistoryController' => 'applications/diffusion/controller/history',
     'DiffusionHistoryQuery' => 'applications/diffusion/query/history/base',
@@ -261,6 +261,7 @@ phutil_register_library_map(array(
     'DiffusionPathChange' => 'applications/diffusion/data/pathchange',
     'DiffusionPathChangeQuery' => 'applications/diffusion/query/pathchange/base',
     'DiffusionPathCompleteController' => 'applications/diffusion/controller/pathcomplete',
+    'DiffusionPathIDQuery' => 'applications/diffusion/query/pathid/base',
     'DiffusionPathValidateController' => 'applications/diffusion/controller/pathvalidate',
     'DiffusionRepositoryController' => 'applications/diffusion/controller/repository',
     'DiffusionRepositoryPath' => 'applications/diffusion/data/repositorypath',
@@ -831,6 +832,7 @@ phutil_register_library_map(array(
     'DarkConsoleServicesPlugin' => 'DarkConsolePlugin',
     'DarkConsoleXHProfPlugin' => 'DarkConsolePlugin',
     'DifferentialAddCommentView' => 'AphrontView',
+    'DifferentialAffectedPath' => 'DifferentialDAO',
     'DifferentialApplyPatchFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialArcanistProjectFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialAuthorFieldSpecification' => 'DifferentialFieldSpecification',

src/aphront/console/core/__init__.php

@@ -9,7 +9,6 @@
 phutil_require_module('phabricator', 'infrastructure/javelin/markup');
 
 phutil_require_module('phutil', 'markup');
-phutil_require_module('phutil', 'symbols');
 phutil_require_module('phutil', 'utils');
 
 

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

@@ -364,6 +364,8 @@ class DifferentialRevisionEditor {
       $diff->setDescription(substr($this->getComments(), 0, 80));
       $diff->save();
 
+      $this->updateAffectedPathTable($revision, $diff, $changesets);
+
       // An updated diff should require review, as long as it's not committed
       // or accepted. The "accepted" status is "sticky" to encourage courtesy
       // re-diffs after someone accepts with minor changes/suggestions.
@@ -719,5 +721,94 @@ class DifferentialRevisionEditor {
     }
   }
 
+  /**
+   * Updated 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) {
+
+    $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();
+    }
+
+    $path_map = id(new DiffusionPathIDQuery($paths))->loadPathIDs($paths);
+
+    $table = new DifferentialAffectedPath();
+    $conn_w = $table->establishConnection('w');
+
+    $sql = array();
+    foreach ($paths as $path) {
+      $path_id = idx($path_map, $path);
+      if (!$path_id) {
+        // Don't bother creating these, it probably means we're either adding
+        // a file (in which case having this row is irrelevant since Diffusion
+        // won't be querying for it) or something is misconfigured (in which
+        // case we'd just be writing garbage).
+        continue;
+      }
+      $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));
+    }
+  }
+
 }
 

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

@@ -11,9 +11,11 @@ phutil_require_module('phabricator', 'applications/differential/constants/revisi
 phutil_require_module('phabricator', 'applications/differential/field/selector/base');
 phutil_require_module('phabricator', 'applications/differential/mail/ccwelcome');
 phutil_require_module('phabricator', 'applications/differential/mail/newdiff');
+phutil_require_module('phabricator', 'applications/differential/storage/affectedpath');
 phutil_require_module('phabricator', 'applications/differential/storage/auxiliaryfield');
 phutil_require_module('phabricator', 'applications/differential/storage/comment');
 phutil_require_module('phabricator', 'applications/differential/storage/revision');
+phutil_require_module('phabricator', 'applications/diffusion/query/pathid/base');
 phutil_require_module('phabricator', 'applications/feed/constants/story');
 phutil_require_module('phabricator', 'applications/feed/publisher');
 phutil_require_module('phabricator', 'applications/herald/adapter/differential');
@@ -26,6 +28,7 @@ phutil_require_module('phabricator', 'infrastructure/env');
 phutil_require_module('phabricator', 'storage/qsprintf');
 phutil_require_module('phabricator', 'storage/queryfx');
 
+phutil_require_module('phutil', 'parser/uri');
 phutil_require_module('phutil', 'utils');
 
 

src/applications/differential/storage/affectedpath/DifferentialAffectedPath.php

@@ -0,0 +1,37 @@
+<?php
+
+/*
+ * Copyright 2011 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.
+ */
+
+/**
+ * Denormalized index table which stores relationships between revisions in
+ * Differential and paths in Diffusion.
+ *
+ * @group differential
+ */
+final class DifferentialAffectedPath extends DifferentialDAO {
+
+  protected $repositoryID;
+  protected $pathID;
+  protected $epoch;
+  protected $revisionID;
+
+  public function getConfiguration() {
+    return array(
+      self::CONFIG_TIMESTAMPS => false,
+    ) + parent::getConfiguration();
+  }
+}

src/applications/differential/storage/affectedpath/__init__.php

@@ -0,0 +1,12 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/differential/storage/base');
+
+
+phutil_require_source('DifferentialAffectedPath.php');

src/applications/differential/storage/diff/DifferentialDiff.php

@@ -75,6 +75,15 @@ class DifferentialDiff extends DifferentialDAO {
       $this->getID());
   }
 
+  public function loadArcanistProject() {
+    if (!$this->getArcanistProjectPHID()) {
+      return null;
+    }
+    return id(new PhabricatorRepositoryArcanistProject())->loadOneWhere(
+      'phid = %s',
+      $this->getArcanistProjectPHID());
+  }
+
   public function save() {
 // TODO: sort out transactions
 //    $this->openTransaction();

src/applications/differential/storage/diff/__init__.php

@@ -9,6 +9,7 @@
 phutil_require_module('phabricator', 'applications/differential/storage/base');
 phutil_require_module('phabricator', 'applications/differential/storage/changeset');
 phutil_require_module('phabricator', 'applications/differential/storage/hunk');
+phutil_require_module('phabricator', 'applications/repository/storage/arcanistproject');
 
 phutil_require_module('phutil', 'utils');
 

src/applications/diffusion/query/browse/svn/DiffusionSvnBrowseQuery.php

@@ -37,7 +37,7 @@ final class DiffusionSvnBrowseQuery extends DiffusionBrowseQuery {
     $conn_r = $repository->establishConnection('r');
 
     $parent_path = dirname($path);
-    $path_query = new DiffusionGitPathIDQuery(
+    $path_query = new DiffusionPathIDQuery(
       array(
         $path,
         $parent_path,
@@ -162,7 +162,7 @@ final class DiffusionSvnBrowseQuery extends DiffusionBrowseQuery {
       $commit_data = mpull($commit_data, null, 'getCommitID');
     }
 
-    $path_normal = DiffusionGitPathIDQuery::normalizePath($path);
+    $path_normal = DiffusionPathIDQuery::normalizePath($path);
 
     $results = array();
     foreach ($browse as $file) {

src/applications/diffusion/query/pathid/base/DiffusionPathIDQuery.php

@@ -16,7 +16,7 @@
  * limitations under the License.
  */
 
-final class DiffusionGitPathIDQuery {
+final class DiffusionPathIDQuery {
 
   public function __construct(array $paths) {
     $this->paths = $paths;

src/applications/diffusion/query/pathid/base/__init__.php

@@ -12,4 +12,4 @@ phutil_require_module('phabricator', 'storage/queryfx');
 phutil_require_module('phutil', 'utils');
 
 
-phutil_require_source('DiffusionGitPathIDQuery.php');
+phutil_require_source('DiffusionPathIDQuery.php');

src/applications/repository/storage/arcanistproject/PhabricatorRepositoryArcanistProject.php

@@ -34,4 +34,11 @@ class PhabricatorRepositoryArcanistProject
     return PhabricatorPHID::generateNewPHID('APRJ');
   }
 
+  public function loadRepository() {
+    if (!$this->getRepositoryID()) {
+      return null;
+    }
+    return id(new PhabricatorRepository())->load($this->getRepositoryID());
+  }
+
 }

src/applications/repository/storage/arcanistproject/__init__.php

@@ -8,6 +8,9 @@
 
 phutil_require_module('phabricator', 'applications/phid/storage/phid');
 phutil_require_module('phabricator', 'applications/repository/storage/base');
+phutil_require_module('phabricator', 'applications/repository/storage/repository');
+
+phutil_require_module('phutil', 'utils');
 
 
 phutil_require_source('PhabricatorRepositoryArcanistProject.php');