Use Application PHIDs for commits

Summary: Ref T2716. Ref T2715. Move CMIT to use Application PHIDs. Nothing too special here, but I consolidated some code into DiffusionCommitQuery. Depends on D6514.

Test Plan: Browsed Diffusion. Browsed Differential/Maniphest with linked commits. Used jump nav; used `phid.lookup` and `phid.query`. Used remarkup for Git and SVN repos. Grepped for PHID_TYPE_CMIT.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2715, T2716

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

src/__phutil_library_map__.php

@@ -1451,6 +1451,7 @@ phutil_register_library_map(array(
     'PhabricatorRepositoryManagementWorkflow' => 'applications/repository/management/PhabricatorRepositoryManagementWorkflow.php',
     'PhabricatorRepositoryMercurialCommitChangeParserWorker' => 'applications/repository/worker/commitchangeparser/PhabricatorRepositoryMercurialCommitChangeParserWorker.php',
     'PhabricatorRepositoryMercurialCommitMessageParserWorker' => 'applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php',
+    'PhabricatorRepositoryPHIDTypeCommit' => 'applications/repository/phid/PhabricatorRepositoryPHIDTypeCommit.php',
     'PhabricatorRepositoryPullEngine' => 'applications/repository/engine/PhabricatorRepositoryPullEngine.php',
     'PhabricatorRepositoryPullLocalDaemon' => 'applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php',
     'PhabricatorRepositoryQuery' => 'applications/repository/query/PhabricatorRepositoryQuery.php',
@@ -3445,6 +3446,7 @@ phutil_register_library_map(array(
     'PhabricatorRepositoryManagementWorkflow' => 'PhutilArgumentWorkflow',
     'PhabricatorRepositoryMercurialCommitChangeParserWorker' => 'PhabricatorRepositoryCommitChangeParserWorker',
     'PhabricatorRepositoryMercurialCommitMessageParserWorker' => 'PhabricatorRepositoryCommitMessageParserWorker',
+    'PhabricatorRepositoryPHIDTypeCommit' => 'PhabricatorPHIDType',
     'PhabricatorRepositoryPullEngine' => 'PhabricatorRepositoryEngine',
     'PhabricatorRepositoryPullLocalDaemon' => 'PhabricatorDaemon',
     'PhabricatorRepositoryQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',

src/applications/diffusion/query/DiffusionCommitQuery.php

@@ -7,6 +7,7 @@ final class DiffusionCommitQuery
   private $identifiers;
   private $phids;
   private $defaultRepository;
+  private $identifierMap;
 
   /**
    * Load commits by partial or full identifiers, e.g. "rXab82393", "rX1234",
@@ -43,7 +44,19 @@ final class DiffusionCommitQuery
     return $this;
   }
 
+  public function getIdentifierMap() {
+    if ($this->identifierMap === null) {
+      throw new Exception(
+        "You must execute() the query before accessing the identifier map.");
+    }
+    return $this->identifierMap;
+  }
+
   protected function loadPage() {
+    if ($this->identifierMap === null) {
+      $this->identifierMap = array();
+    }
+
     $table = new PhabricatorRepositoryCommit();
     $conn_r = $table->establishConnection('r');
 
@@ -59,10 +72,6 @@ final class DiffusionCommitQuery
   }
 
   public function willFilterPage(array $commits) {
-    if (!$commits) {
-      return array();
-    }
-
     $repository_ids = mpull($commits, 'getRepositoryID', 'getRepositoryID');
     $repos = id(new PhabricatorRepositoryQuery())
       ->setViewer($this->getViewer())
@@ -78,6 +87,47 @@ final class DiffusionCommitQuery
       }
     }
 
+    if ($this->identifiers !== null) {
+      $ids = array_fuse($this->identifiers);
+      $min_qualified = PhabricatorRepository::MINIMUM_QUALIFIED_HASH;
+
+      $result = array();
+      foreach ($commits as $commit) {
+        $prefix = 'r'.$commit->getRepository()->getCallsign();
+        $suffix = $commit->getCommitIdentifier();
+
+        if ($commit->getRepository()->isSVN()) {
+          if (isset($ids[$prefix.$suffix])) {
+            $result[$prefix.$suffix][] = $commit;
+          }
+        } else {
+          // This awkward contruction is so we can link the commits up in O(N)
+          // time instead of O(N^2).
+          for ($ii = $min_qualified; $ii <= strlen($suffix); $ii++) {
+            $part = substr($suffix, 0, $ii);
+            if (isset($ids[$prefix.$part])) {
+              $result[$prefix.$part][] = $commit;
+            }
+            if (isset($ids[$part])) {
+              $result[$part][] = $commit;
+            }
+          }
+        }
+      }
+
+      foreach ($result as $identifier => $matching_commits) {
+        if (count($matching_commits) == 1) {
+          $result[$identifier] = head($matching_commits);
+        } else {
+          // This reference is ambiguous (it matches more than one commit) so
+          // don't link it
+          unset($result[$identifier]);
+        }
+      }
+
+      $this->identifierMap += $result;
+    }
+
     return $commits;
   }
 
@@ -190,4 +240,14 @@ final class DiffusionCommitQuery
     return $this->formatWhereClause($where);
   }
 
+  public function didFilterResults(array $filtered) {
+    if ($this->identifierMap) {
+      foreach ($this->identifierMap as $name => $commit) {
+        if (isset($filtered[$commit->getPHID()])) {
+          unset($this->identifierMap[$name]);
+        }
+      }
+    }
+  }
+
 }

src/applications/diffusion/remarkup/DiffusionRemarkupRule.php

@@ -12,68 +12,19 @@ final class DiffusionRemarkupRule
   }
 
   protected function getObjectIDPattern() {
-    $min_unqualified = PhabricatorRepository::MINIMUM_UNQUALIFIED_HASH;
+    return PhabricatorRepositoryPHIDTypeCommit::getCommitObjectNamePattern();
-    $min_qualified   = PhabricatorRepository::MINIMUM_QUALIFIED_HASH;
-
-    return
-      'r[A-Z]+[1-9]\d*'.
-      '|'.
-      'r[A-Z]+[a-f0-9]{'.$min_qualified.',40}'.
-      '|'.
-      '[a-f0-9]{'.$min_unqualified.',40}';
   }
 
   protected function loadObjects(array $ids) {
     $viewer = $this->getEngine()->getConfig('viewer');
-    $min_qualified = PhabricatorRepository::MINIMUM_QUALIFIED_HASH;
 
-    $commits = id(new DiffusionCommitQuery())
+    $query = id(new DiffusionCommitQuery())
       ->setViewer($viewer)
-      ->withIdentifiers($ids)
+      ->withIdentifiers($ids);
-      ->execute();
 
-    if (!$commits) {
+    $query->execute();
-      return array();
-    }
 
-    $ids = array_fuse($ids);
+    return $query->getIdentifierMap();
-
-    $result = array();
-    foreach ($commits as $commit) {
-      $prefix = 'r'.$commit->getRepository()->getCallsign();
-      $suffix = $commit->getCommitIdentifier();
-
-      if ($commit->getRepository()->isSVN()) {
-        if (isset($ids[$prefix.$suffix])) {
-          $result[$prefix.$suffix][] = $commit;
-        }
-      } else {
-        // This awkward contruction is so we can link the commits up in O(N)
-        // time instead of O(N^2).
-        for ($ii = $min_qualified; $ii <= strlen($suffix); $ii++) {
-          $part = substr($suffix, 0, $ii);
-          if (isset($ids[$prefix.$part])) {
-            $result[$prefix.$part][] = $commit;
-          }
-          if (isset($ids[$part])) {
-            $result[$part][] = $commit;
-          }
-        }
-      }
-    }
-
-    foreach ($result as $identifier => $commits) {
-      if (count($commits) == 1) {
-        $result[$identifier] = head($commits);
-      } else {
-        // This reference is ambiguous -- it matches more than one commit -- so
-        // don't link it. We could potentially improve this, but it's a bit
-        // tricky since the superclass expects a single object.
-        unset($result[$identifier]);
-      }
-    }
-
-    return $result;
   }
 
 }

src/applications/phid/PhabricatorObjectHandle.php

@@ -110,7 +110,6 @@ final class PhabricatorObjectHandle
     static $map = array(
       PhabricatorPHIDConstants::PHID_TYPE_USER => 'User',
       PhabricatorPHIDConstants::PHID_TYPE_TASK => 'Task',
-      PhabricatorPHIDConstants::PHID_TYPE_CMIT => 'Commit',
       PhabricatorPHIDConstants::PHID_TYPE_WIKI => 'Phriction Document',
       PhabricatorPHIDConstants::PHID_TYPE_MCRO => 'Image Macro',
       PhabricatorPHIDConstants::PHID_TYPE_MOCK => 'Pholio Mock',

src/applications/phid/PhabricatorPHIDConstants.php

@@ -9,7 +9,6 @@ final class PhabricatorPHIDConstants {
   const PHID_TYPE_UNKNOWN = '????';
   const PHID_TYPE_MAGIC   = '!!!!';
   const PHID_TYPE_REPO    = 'REPO';
-  const PHID_TYPE_CMIT    = 'CMIT';
   const PHID_TYPE_OPKG    = 'OPKG';
   const PHID_TYPE_PSTE    = 'PSTE';
   const PHID_TYPE_STRY    = 'STRY';

src/applications/phid/handle/PhabricatorObjectHandleData.php

@@ -56,13 +56,6 @@ final class PhabricatorObjectHandleData {
           $phids);
         return mpull($users, null, 'getPHID');
 
-      case PhabricatorPHIDConstants::PHID_TYPE_CMIT:
-        $commits = id(new DiffusionCommitQuery())
-          ->setViewer($this->viewer)
-          ->withPHIDs($phids)
-          ->execute();
-        return mpull($commits, null, 'getPHID');
-
       case PhabricatorPHIDConstants::PHID_TYPE_TASK:
         // TODO: Update this to ManiphestTaskQuery, //especially// after we have
         // policy-awareness
@@ -325,39 +318,6 @@ final class PhabricatorObjectHandleData {
           }
           break;
 
-        case PhabricatorPHIDConstants::PHID_TYPE_CMIT:
-          foreach ($phids as $phid) {
-            $handle = new PhabricatorObjectHandle();
-            $handle->setPHID($phid);
-            $handle->setType($type);
-
-            if (empty($objects[$phid])) {
-              $handle->setName('Unknown Commit');
-            } else {
-              $repository = $objects[$phid]->getRepository();
-              $commit = $objects[$phid];
-              $callsign = $repository->getCallsign();
-              $commit_identifier = $commit->getCommitIdentifier();
-
-              $name = $repository->formatCommitName($commit_identifier);
-              $handle->setName($name);
-
-              $summary = $commit->getSummary();
-              if (strlen($summary)) {
-                $handle->setFullName($name.': '.$summary);
-              } else {
-                $handle->setFullName($name);
-              }
-
-              $handle->setURI('/r'.$callsign.$commit_identifier);
-              $handle->setTimestamp($commit->getEpoch());
-              $handle->setComplete(true);
-            }
-
-            $handles[$phid] = $handle;
-          }
-          break;
-
         case PhabricatorPHIDConstants::PHID_TYPE_TASK:
           foreach ($phids as $phid) {
             $handle = new PhabricatorObjectHandle();

src/applications/phid/storage/PhabricatorPHID.php

@@ -43,28 +43,8 @@ final class PhabricatorPHID {
       // It's already a PHID! Yay.
       return $name;
     }
-    if (preg_match('/^r([A-Z]+)(\S*)$/', $name, $match)) {
+
-      $repository = id(new PhabricatorRepository())
+    if (preg_match('/^t(\d+)$/i', $name, $match)) {
-        ->loadOneWhere('callsign = %s', $match[1]);
-      if ($match[2] == '') {
-        $object = $repository;
-      } else if ($repository) {
-        $object = id(new PhabricatorRepositoryCommit())->loadOneWhere(
-          'repositoryID = %d AND commitIdentifier = %s',
-          $repository->getID(),
-          $match[2]);
-        if (!$object) {
-          try {
-            $object = id(new PhabricatorRepositoryCommit())->loadOneWhere(
-              'repositoryID = %d AND commitIdentifier LIKE %>',
-              $repository->getID(),
-              $match[2]);
-          } catch (AphrontQueryCountException $ex) {
-            // Ambiguous; return nothing.
-          }
-        }
-      }
-    } else if (preg_match('/^t(\d+)$/i', $name, $match)) {
       $object = id(new ManiphestTask())->load($match[1]);
     } else if (preg_match('/^m(\d+)$/i', $name, $match)) {
       $objects = id(new PholioMockQuery())

src/applications/repository/phid/PhabricatorRepositoryPHIDTypeCommit.php

@@ -0,0 +1,86 @@
+<?php
+
+final class PhabricatorRepositoryPHIDTypeCommit extends PhabricatorPHIDType {
+
+  const TYPECONST = 'CMIT';
+
+  public function getTypeConstant() {
+    return self::TYPECONST;
+  }
+
+  public function getTypeName() {
+    return pht('Commit');
+  }
+
+  public function newObject() {
+    return new PhabricatorRepositoryCommit();
+  }
+
+  public function loadObjects(
+    PhabricatorObjectQuery $query,
+    array $phids) {
+
+    return id(new DiffusionCommitQuery())
+      ->setViewer($query->getViewer())
+      ->withPHIDs($phids)
+      ->execute();
+  }
+
+  public function loadHandles(
+    PhabricatorHandleQuery $query,
+    array $handles,
+    array $objects) {
+
+    foreach ($handles as $phid => $handle) {
+      $commit = $objects[$phid];
+      $repository = $commit->getRepository();
+      $callsign = $repository->getCallsign();
+      $commit_identifier = $commit->getCommitIdentifier();
+
+      $name = $repository->formatCommitName($commit_identifier);
+      $summary = $commit->getSummary();
+      if (strlen($summary)) {
+        $full_name = $name.': '.$summary;
+      } else {
+        $full_name = $name;
+      }
+
+      $handle->setName($name);
+      $handle->setFullName($full_name);
+      $handle->setURI('/r'.$callsign.$commit_identifier);
+      $handle->setTimestamp($commit->getEpoch());
+    }
+  }
+
+  public static function getCommitObjectNamePattern() {
+    $min_unqualified = PhabricatorRepository::MINIMUM_UNQUALIFIED_HASH;
+    $min_qualified   = PhabricatorRepository::MINIMUM_QUALIFIED_HASH;
+
+    return
+      'r[A-Z]+[1-9]\d*'.
+      '|'.
+      'r[A-Z]+[a-f0-9]{'.$min_qualified.',40}'.
+      '|'.
+      '[a-f0-9]{'.$min_unqualified.',40}';
+  }
+
+
+  public function canLoadNamedObject($name) {
+    $pattern = self::getCommitObjectNamePattern();
+    return preg_match('(^'.$pattern.'$)', $name);
+  }
+
+  public function loadNamedObjects(
+    PhabricatorObjectQuery $query,
+    array $names) {
+
+    $query = id(new DiffusionCommitQuery())
+      ->setViewer($query->getViewer())
+      ->withIdentifiers($names);
+
+    $query->execute();
+
+    return $query->getIdentifierMap();
+  }
+
+}

src/applications/repository/storage/PhabricatorRepositoryCommit.php

@@ -17,7 +17,7 @@ final class PhabricatorRepositoryCommit
   private $commitData;
   private $audits;
   private $isUnparsed;
-  private $repository;
+  private $repository = self::ATTACHABLE;
 
   public function attachRepository(PhabricatorRepository $repository) {
     $this->repository = $repository;
@@ -25,10 +25,7 @@ final class PhabricatorRepositoryCommit
   }
 
   public function getRepository() {
-    if ($this->repository === null) {
+    return $this->assertAttached($this->repository);
-      throw new Exception("Call attachRepository() before getRepository()!");
-    }
-    return $this->repository;
   }
 
   public function setIsUnparsed($is_unparsed) {
@@ -49,7 +46,7 @@ final class PhabricatorRepositoryCommit
 
   public function generatePHID() {
     return PhabricatorPHID::generateNewPHID(
-      PhabricatorPHIDConstants::PHID_TYPE_CMIT);
+      PhabricatorRepositoryPHIDTypeCommit::TYPECONST);
   }
 
   public function loadCommitData() {

src/applications/search/controller/PhabricatorSearchAttachController.php

@@ -206,7 +206,7 @@ final class PhabricatorSearchAttachController
         $noun = 'Tasks';
         $selected = 'assigned';
         break;
-      case PhabricatorPHIDConstants::PHID_TYPE_CMIT:
+      case PhabricatorRepositoryPHIDTypeCommit::TYPECONST:
         $noun = 'Commits';
         $selected = 'created';
         break;
@@ -269,7 +269,7 @@ final class PhabricatorSearchAttachController
   }
 
   private function getEdgeType($src_type, $dst_type) {
-    $t_cmit = PhabricatorPHIDConstants::PHID_TYPE_CMIT;
+    $t_cmit = PhabricatorRepositoryPHIDTypeCommit::TYPECONST;
     $t_task = PhabricatorPHIDConstants::PHID_TYPE_TASK;
     $t_drev = DifferentialPHIDTypeRevision::TYPECONST;
     $t_mock = PhabricatorPHIDConstants::PHID_TYPE_MOCK;

src/applications/search/controller/PhabricatorSearchController.php

@@ -63,7 +63,7 @@ final class PhabricatorSearchController
               case PhabricatorSearchScope::SCOPE_COMMITS:
                 $query->setParameter(
                   'type',
-                  PhabricatorPHIDConstants::PHID_TYPE_CMIT);
+                  PhabricatorRepositoryPHIDTypeCommit::TYPECONST);
                 break;
               default:
                 break;

src/applications/search/index/PhabricatorSearchAbstractDocument.php

@@ -16,7 +16,7 @@ final class PhabricatorSearchAbstractDocument {
   public static function getSupportedTypes() {
     return array(
       DifferentialPHIDTypeRevision::TYPECONST => 'Differential Revisions',
-      PhabricatorPHIDConstants::PHID_TYPE_CMIT => 'Repository Commits',
+      PhabricatorRepositoryPHIDTypeCommit::TYPECONST => 'Repository Commits',
       PhabricatorPHIDConstants::PHID_TYPE_TASK => 'Maniphest Tasks',
       PhabricatorPHIDConstants::PHID_TYPE_WIKI => 'Phriction Documents',
       PhabricatorPHIDConstants::PHID_TYPE_USER => 'Phabricator Users',

src/infrastructure/edges/constants/PhabricatorEdgeConfig.php

@@ -155,7 +155,6 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants {
 
     static $class_map = array(
       PhabricatorPHIDConstants::PHID_TYPE_TASK  => 'ManiphestTask',
-      PhabricatorPHIDConstants::PHID_TYPE_CMIT  => 'PhabricatorRepository',
       PhabricatorPHIDConstants::PHID_TYPE_FILE  => 'PhabricatorFile',
       PhabricatorPHIDConstants::PHID_TYPE_USER  => 'PhabricatorUser',
       PhabricatorPHIDConstants::PHID_TYPE_PROJ  => 'PhabricatorProject',