Make "CommitData" wrap and persist a "CommitRef" record

Summary:
Ref T13552. Turn "CommitData" into an application-level layer on top of the repository-level "CommitRef" object.

For older commits which will not have a "CommitRef" record on disk, build a synthetic one at runtime. This could eventually be migrated.

Test Plan: Ran "bin/repository reparse --message", browsed Diffusion.

Maniphest Tasks: T13552

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

src/applications/diffusion/data/DiffusionCommitRef.php

@@ -50,10 +50,6 @@ final class DiffusionCommitRef extends Phobject {
       ->setHashes($hashes);
   }
 
-  public static function newFromConduitResult(array $result) {
-    return self::newFromDictionary($result);
-  }
-
   public function setHashes(array $hashes) {
     assert_instances_of($hashes, 'DiffusionCommitHash');
     $this->hashes = $hashes;

src/applications/repository/storage/PhabricatorRepositoryCommit.php

@@ -174,6 +174,11 @@ final class PhabricatorRepositoryCommit
     return $this;
   }
 
+  public function hasCommitData() {
+    return ($this->commitData !== self::ATTACHABLE) &&
+           ($this->commitData !== null);
+  }
+
   public function getCommitData() {
     return $this->assertAttached($this->commitData);
   }

src/applications/repository/storage/PhabricatorRepositoryCommitData.php

@@ -6,6 +6,7 @@ final class PhabricatorRepositoryCommitData extends PhabricatorRepositoryDAO {
   protected $authorName    = '';
   protected $commitMessage = '';
   protected $commitDetails = array();
+  private $commitRef;
 
   protected function getConfiguration() {
     return array(
@@ -93,8 +94,14 @@ final class PhabricatorRepositoryCommitData extends PhabricatorRepositoryDAO {
   }
 
   public function getAuthorString() {
-    $author = phutil_string_cast($this->authorName);
+    $ref = $this->getCommitRef();
 
+    $author = $ref->getAuthor();
+    if (strlen($author)) {
+      return $author;
+    }
+
+    $author = phutil_string_cast($this->authorName);
     if (strlen($author)) {
       return $author;
     }
@@ -103,15 +110,15 @@ final class PhabricatorRepositoryCommitData extends PhabricatorRepositoryDAO {
   }
 
   public function getAuthorDisplayName() {
-    return $this->getCommitDetailString('authorName');
+    return $this->getCommitRef()->getAuthorName();
   }
 
   public function getAuthorEmail() {
-    return $this->getCommitDetailString('authorEmail');
+    return $this->getCommitRef()->getAuthorEmail();
   }
 
   public function getAuthorEpoch() {
-    $epoch = $this->getCommitDetail('authorEpoch');
+    $epoch = $this->getCommitRef()->getAuthorEpoch();
 
     if ($epoch) {
       return (int)$epoch;
@@ -121,15 +128,22 @@ final class PhabricatorRepositoryCommitData extends PhabricatorRepositoryDAO {
   }
 
   public function getCommitterString() {
+    $ref = $this->getCommitRef();
+
+    $committer = $ref->getCommitter();
+    if (strlen($committer)) {
+      return $committer;
+    }
+
     return $this->getCommitDetailString('committer');
   }
 
   public function getCommitterDisplayName() {
-    return $this->getCommitDetailString('committerName');
+    return $this->getCommitRef()->getCommitterName();
   }
 
   public function getCommitterEmail() {
-    return $this->getCommitDetailString('committerEmail');
+    return $this->getCommitRef()->getCommitterEmail();
   }
 
   private function getCommitDetailString($key) {
@@ -143,4 +157,35 @@ final class PhabricatorRepositoryCommitData extends PhabricatorRepositoryDAO {
     return null;
   }
 
+  public function setCommitRef(DiffusionCommitRef $ref) {
+    $this->setCommitDetail('ref', $ref->newDictionary());
+    $this->commitRef = null;
+
+    return $this;
+  }
+
+  public function getCommitRef() {
+    if ($this->commitRef === null) {
+      $map = $this->getCommitDetail('ref', array());
+
+      if (!is_array($map)) {
+        $map = array();
+      }
+
+      $map = $map + array(
+        'authorName' => $this->getCommitDetailString('authorName'),
+        'authorEmail' => $this->getCommitDetailString('authorEmail'),
+        'authorEpoch' => $this->getCommitDetailString('authorEpoch'),
+        'committerName' => $this->getCommitDetailString('committerName'),
+        'committerEmail' => $this->getCommitDetailString('committerEmail'),
+        'message' => $this->getCommitMessage(),
+      );
+
+      $ref = DiffusionCommitRef::newFromDictionary($map);
+      $this->commitRef = $ref;
+    }
+
+    return $this->commitRef;
+  }
+
 }

src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php

@@ -125,6 +125,26 @@ abstract class PhabricatorRepositoryCommitParserWorker
     return array($link, $suffix);
   }
 
+  final protected function loadCommitData(PhabricatorRepositoryCommit $commit) {
+    if ($commit->hasCommitData()) {
+      return $commit->getCommitData();
+    }
+
+    $commit_id = $commit->getID();
+
+    $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere(
+      'commitID = %d',
+      $commit_id);
+    if (!$data) {
+      $data = id(new PhabricatorRepositoryCommitData())
+        ->setCommitID($commit_id);
+    }
+
+    $commit->attachCommitData($data);
+
+    return $data;
+  }
+
   final public function getViewer() {
     return PhabricatorUser::getOmnipotentUser();
   }

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

@@ -18,7 +18,10 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
 
       $ref = $commit->newCommitRef($viewer);
 
-      $this->updateCommitData($ref);
+      $data = $this->loadCommitData($commit);
+      $data->setCommitRef($ref);
+
+      $this->updateCommitData($commit, $data);
     }
 
     if ($this->shouldQueueFollowupTasks()) {
@@ -38,14 +41,17 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
     }
   }
 
-  final protected function updateCommitData(DiffusionCommitRef $ref) {
+  final protected function updateCommitData(
-    $commit = $this->commit;
+    PhabricatorRepositoryCommit $commit,
+    PhabricatorRepositoryCommitData $data) {
+
+    $ref = $data->getCommitRef();
+    $viewer = $this->getViewer();
+
     $author = $ref->getAuthor();
     $committer = $ref->getCommitter();
     $has_committer = (bool)strlen($committer);
 
-    $viewer = PhabricatorUser::getOmnipotentUser();
-
     $identity_engine = id(new DiffusionRepositoryIdentityEngine())
       ->setViewer($viewer)
       ->setSourcePHID($commit->getPHID());
@@ -66,13 +72,6 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
       $committer_identity = null;
     }
 
-    $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere(
-      'commitID = %d',
-      $commit->getID());
-    if (!$data) {
-      $data = new PhabricatorRepositoryCommitData();
-    }
-    $data->setCommitID($commit->getID());
     $data->setAuthorName(id(new PhutilUTF8StringTruncator())
       ->setMaximumBytes(255)
       ->truncateString((string)$author));
@@ -122,7 +121,9 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
     $commit->setAuthorIdentityPHID($author_identity->getPHID());
 
     $commit->setSummary($data->getSummary());
+
     $commit->save();
+    $data->save();
 
     // If we're publishing this commit, we're going to queue tasks to update
     // referenced objects (like tasks and revisions). Otherwise, record some
@@ -130,15 +131,14 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
 
     $publisher = $repository->newPublisher();
     if ($publisher->shouldPublishCommit($commit)) {
-      $actor = PhabricatorUser::getOmnipotentUser();
+      $this->closeRevisions($viewer, $commit);
-      $this->closeRevisions($actor, $ref, $commit, $data);
+      $this->closeTasks($viewer, $commit);
-      $this->closeTasks($actor, $ref, $commit, $data);
     } else {
       $hold_reasons = $publisher->getCommitHoldReasons($commit);
-      $data->setCommitDetail('holdReasons', $hold_reasons);
-    }
 
+      $data->setCommitDetail('holdReasons', $hold_reasons);
       $data->save();
+    }
 
     $commit->writeImportStatusFlag(
       PhabricatorRepositoryCommit::IMPORTED_MESSAGE);
@@ -146,9 +146,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
 
   private function closeRevisions(
     PhabricatorUser $actor,
-    DiffusionCommitRef $ref,
+    PhabricatorRepositoryCommit $commit) {
-    PhabricatorRepositoryCommit $commit,
-    PhabricatorRepositoryCommitData $data) {
 
     $differential = 'PhabricatorDifferentialApplication';
     if (!PhabricatorApplication::isClassInstalled($differential)) {
@@ -156,6 +154,8 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
     }
 
     $repository = $commit->getRepository();
+    $data = $commit->getCommitData();
+    $ref = $data->getCommitRef();
 
     $field_query = id(new DiffusionLowLevelCommitFieldsQuery())
       ->setRepository($repository)
@@ -198,15 +198,15 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
 
   private function closeTasks(
     PhabricatorUser $actor,
-    DiffusionCommitRef $ref,
+    PhabricatorRepositoryCommit $commit) {
-    PhabricatorRepositoryCommit $commit,
-    PhabricatorRepositoryCommitData $data) {
 
     $maniphest = 'PhabricatorManiphestApplication';
     if (!PhabricatorApplication::isClassInstalled($maniphest)) {
       return;
     }
 
+    $data = $commit->getCommitData();
+
     $prefixes = ManiphestTaskStatus::getStatusPrefixMap();
     $suffixes = ManiphestTaskStatus::getStatusSuffixMap();
     $message = $data->getCommitMessage();