Move commit hash querying to DiffusionLowLevelCommitQuery

Summary: Ref T4195. I need this for the Herald pre-commit rules, and it generally simplifies things.

Test Plan: Used `reparse.php` plus `var_dump()` to inspect refs in Git, Mercurial and SVN repos. They all looked correct and reparsed correctly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

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

src/__phutil_library_map__.php

@@ -478,6 +478,7 @@ phutil_register_library_map(array(
     'DiffusionCommitChangeTableView' => 'applications/diffusion/view/DiffusionCommitChangeTableView.php',
     'DiffusionCommitController' => 'applications/diffusion/controller/DiffusionCommitController.php',
     'DiffusionCommitEditController' => 'applications/diffusion/controller/DiffusionCommitEditController.php',
+    'DiffusionCommitHash' => 'applications/diffusion/data/DiffusionCommitHash.php',
     'DiffusionCommitHookEngine' => 'applications/diffusion/engine/DiffusionCommitHookEngine.php',
     'DiffusionCommitHookRejectException' => 'applications/diffusion/exception/DiffusionCommitHookRejectException.php',
     'DiffusionCommitParentsQuery' => 'applications/diffusion/query/parents/DiffusionCommitParentsQuery.php',
@@ -2866,6 +2867,7 @@ phutil_register_library_map(array(
     'DiffusionCommitChangeTableView' => 'DiffusionView',
     'DiffusionCommitController' => 'DiffusionController',
     'DiffusionCommitEditController' => 'DiffusionController',
+    'DiffusionCommitHash' => 'Phobject',
     'DiffusionCommitHookEngine' => 'Phobject',
     'DiffusionCommitHookRejectException' => 'Exception',
     'DiffusionCommitParentsQuery' => 'DiffusionQuery',

src/applications/diffusion/data/DiffusionCommitHash.php

@@ -0,0 +1,26 @@
+<?php
+
+final class DiffusionCommitHash extends Phobject {
+
+  private $hashType;
+  private $hashValue;
+
+  public function setHashValue($hash_value) {
+    $this->hashValue = $hash_value;
+    return $this;
+  }
+
+  public function getHashValue() {
+    return $this->hashValue;
+  }
+
+  public function setHashType($hash_type) {
+    $this->hashType = $hash_type;
+    return $this;
+  }
+
+  public function getHashType() {
+    return $this->hashType;
+  }
+
+}

src/applications/diffusion/data/DiffusionCommitRef.php

@@ -7,6 +7,16 @@ final class DiffusionCommitRef extends Phobject {
   private $authorEmail;
   private $committerName;
   private $committerEmail;
+  private $hashes = array();
+
+  public function setHashes(array $hashes) {
+    $this->hashes = $hashes;
+    return $this;
+  }
+
+  public function getHashes() {
+    return $this->hashes;
+  }
 
   public function setCommitterEmail($committer_email) {
     $this->committerEmail = $committer_email;

src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitQuery.php

@@ -51,7 +51,9 @@ final class DiffusionLowLevelCommitQuery
     list($info) = $repository->execxLocalCommand(
       'log -n 1 --encoding=%s --format=%s %s --',
       'UTF-8',
-      implode('%x00', array('%e', '%cn', '%ce', '%an', '%ae', '%s%n%n%b')),
+      implode(
+        '%x00',
+        array('%e', '%cn', '%ce', '%an', '%ae', '%T', '%s%n%n%b')),
       $this->identifier);
 
     $parts = explode("\0", $info);
@@ -67,12 +69,22 @@ final class DiffusionLowLevelCommitQuery
       }
     }
 
+    $hashes = array(
+      id(new DiffusionCommitHash())
+        ->setHashType(ArcanistDifferentialRevisionHash::HASH_GIT_COMMIT)
+        ->setHashValue($this->identifier),
+      id(new DiffusionCommitHash())
+        ->setHashType(ArcanistDifferentialRevisionHash::HASH_GIT_TREE)
+        ->setHashValue($parts[4]),
+    );
+
     return id(new DiffusionCommitRef())
       ->setCommitterName($parts[0])
       ->setCommitterEmail($parts[1])
       ->setAuthorName($parts[2])
       ->setAuthorEmail($parts[3])
-      ->setMessage($parts[4]);
+      ->setHashes($hashes)
+      ->setMessage($parts[5]);
   }
 
   private function loadMercurialCommitRef() {
@@ -90,10 +102,17 @@ final class DiffusionLowLevelCommitQuery
 
     list($author_name, $author_email) = $this->splitUserIdentifier($author);
 
+    $hashes = array(
+      id(new DiffusionCommitHash())
+        ->setHashType(ArcanistDifferentialRevisionHash::HASH_MERCURIAL_COMMIT)
+        ->setHashValue($this->identifier),
+    );
+
     return id(new DiffusionCommitRef())
       ->setAuthorName($author_name)
       ->setAuthorEmail($author_email)
-      ->setMessage($message);
+      ->setMessage($message)
+      ->setHashes($hashes);
   }
 
   private function loadSubversionCommitRef() {
@@ -114,10 +133,14 @@ final class DiffusionLowLevelCommitQuery
 
     list($author_name, $author_email) = $this->splitUserIdentifier($author);
 
+    // No hashes in Subversion.
+    $hashes = array();
+
     return id(new DiffusionCommitRef())
       ->setAuthorName($author_name)
       ->setAuthorEmail($author_email)
-      ->setMessage($message);
+      ->setMessage($message)
+      ->setHashes($hashes);
   }
 
   private function splitUserIdentifier($user) {

src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php

@@ -3,14 +3,12 @@
 abstract class PhabricatorRepositoryCommitMessageParserWorker
   extends PhabricatorRepositoryCommitParserWorker {
 
-  abstract protected function getCommitHashes(
+  final protected function updateCommitData(DiffusionCommitRef $ref) {
-    PhabricatorRepository $repository,
-    PhabricatorRepositoryCommit $commit);
-
-  final protected function updateCommitData($author, $message,
-    $committer = null) {
-
     $commit = $this->commit;
+    $author = $ref->getAuthor();
+    $message = $ref->getMessage();
+    $committer = $ref->getCommitter();
+    $hashes = $ref->getHashes();
 
     $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere(
       'commitID = %d',
@@ -26,7 +24,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
 
     $data->setCommitMessage($message);
 
-    if ($committer) {
+    if (strlen($committer)) {
       $data->setCommitDetail('committer', $committer);
       $data->setCommitDetail(
         'committerPHID',
@@ -63,14 +61,14 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
     }
 
     $revision_id = idx($field_values, 'revisionID');
-    if (!$revision_id) {
+    if (!$revision_id && $hashes) {
-      $hashes = $this->getCommitHashes(
+      $hash_list = array();
-        $repository,
+      foreach ($hashes as $hash) {
-        $commit);
+        $hash_list[] = array($hash->getHashType(), $hash->getHashValue());
-      if ($hashes) {
+      }
       $revisions = id(new DifferentialRevisionQuery())
         ->setViewer(PhabricatorUser::getOmnipotentUser())
-          ->withCommitHashes($hashes)
+        ->withCommitHashes($hash_list)
         ->execute();
 
       if (!empty($revisions)) {
@@ -78,7 +76,6 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
         $revision_id = $revision->getID();
       }
     }
-    }
 
     $data->setCommitDetail(
       'differential.revisionID',

src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php

@@ -12,15 +12,7 @@ final class PhabricatorRepositoryGitCommitMessageParserWorker
       ->withIdentifier($commit->getCommitIdentifier())
       ->execute();
 
-    $committer = $ref->getCommitter();
+    $this->updateCommitData($ref);
-    $author = $ref->getAuthor();
-    $message = $ref->getMessage();
-
-    if ($committer == $author) {
-      $committer = null;
-    }
-
-    $this->updateCommitData($author, $message, $committer);
 
     if ($this->shouldQueueFollowupTasks()) {
       PhabricatorWorker::scheduleTask(
@@ -31,24 +23,4 @@ final class PhabricatorRepositoryGitCommitMessageParserWorker
     }
   }
 
-  protected function getCommitHashes(
-    PhabricatorRepository $repository,
-    PhabricatorRepositoryCommit $commit) {
-
-    list($stdout) = $repository->execxLocalCommand(
-      'log -n 1 --format=%s %s --',
-      '%T',
-      $commit->getCommitIdentifier());
-
-    $commit_hash = $commit->getCommitIdentifier();
-    $tree_hash = trim($stdout);
-
-    return array(
-      array(ArcanistDifferentialRevisionHash::HASH_GIT_COMMIT,
-            $commit_hash),
-      array(ArcanistDifferentialRevisionHash::HASH_GIT_TREE,
-            $tree_hash),
-    );
-  }
-
 }

src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php

@@ -12,10 +12,7 @@ final class PhabricatorRepositoryMercurialCommitMessageParserWorker
       ->withIdentifier($commit->getCommitIdentifier())
       ->execute();
 
-    $author = $ref->getAuthor();
+    $this->updateCommitData($ref);
-    $message = $ref->getMessage();
-
-    $this->updateCommitData($author, $message);
 
     if ($this->shouldQueueFollowupTasks()) {
       PhabricatorWorker::scheduleTask(
@@ -26,16 +23,4 @@ final class PhabricatorRepositoryMercurialCommitMessageParserWorker
     }
   }
 
-  protected function getCommitHashes(
-    PhabricatorRepository $repository,
-    PhabricatorRepositoryCommit $commit) {
-
-    $commit_hash = $commit->getCommitIdentifier();
-
-    return array(
-      array(ArcanistDifferentialRevisionHash::HASH_MERCURIAL_COMMIT,
-            $commit_hash),
-    );
-  }
-
 }

src/applications/repository/worker/commitmessageparser/PhabricatorRepositorySvnCommitMessageParserWorker.php

@@ -12,10 +12,7 @@ final class PhabricatorRepositorySvnCommitMessageParserWorker
       ->withIdentifier($commit->getCommitIdentifier())
       ->execute();
 
-    $author = $ref->getAuthor();
+    $this->updateCommitData($ref);
-    $message = $ref->getMessage();
-
-    $this->updateCommitData($author, $message);
 
     if ($this->shouldQueueFollowupTasks()) {
       PhabricatorWorker::scheduleTask(
@@ -26,10 +23,4 @@ final class PhabricatorRepositorySvnCommitMessageParserWorker
     }
   }
 
-  protected function getCommitHashes(
-    PhabricatorRepository $repository,
-    PhabricatorRepositoryCommit $commit) {
-    return array();
-  }
-
 }