Move task and revision closure to the "publishing" step of the commit import pipeline

Summary:
Ref T13552. Now that these steps can build their own "CommitRef" object from storage on the "CommitData" object, move them from the "Message" step to the "Publishing" step.

This should resolve the root issue in T13552, where a commit moved from a non-permanent branch to a permanent branch does not publish closures properly.

Test Plan: Used "bin/repository reparse --publish ..." to republish changes.

Maniphest Tasks: T13552

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

src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php

@@ -30,11 +30,6 @@ final class PhabricatorRepositoryCommitPublishWorker
     PhabricatorRepositoryCommit $commit) {
     $viewer = PhabricatorUser::getOmnipotentUser();
 
-    $publisher = $repository->newPublisher();
-    if (!$publisher->shouldPublishCommit($commit)) {
-      return;
-    }
-
     $commit_phid = $commit->getPHID();
 
     // Reload the commit to get the commit data, identities, and any
@@ -53,6 +48,36 @@ final class PhabricatorRepositoryCommitPublishWorker
           $commit_phid));
     }
 
+    $publisher = $repository->newPublisher();
+    $should_publish = $publisher->shouldPublishCommit($commit);
+
+    if (!$should_publish) {
+      $hold_reasons = $publisher->getCommitHoldReasons($commit);
+    } else {
+      $hold_reasons = array();
+    }
+
+    $data = $commit->getCommitData();
+    if ($data->getCommitDetail('holdReasons') !== $hold_reasons) {
+      $data->setCommitDetail('holdReasons', $hold_reasons);
+      $data->save();
+    }
+
+    if (!$should_publish) {
+      return;
+    }
+
+    $this->applyTransactions($viewer, $repository, $commit);
+
+    $this->closeRevisions($viewer, $commit);
+    $this->closeTasks($viewer, $commit);
+  }
+
+  private function applyTransactions(
+    PhabricatorUser $actor,
+    PhabricatorRepository $repository,
+    PhabricatorRepositoryCommit $commit) {
+
     $xactions = array(
       $this->newAuditTransactions($commit),
       $this->newPublishTransactions($commit),
@@ -63,7 +88,7 @@ final class PhabricatorRepositoryCommitPublishWorker
     $content_source = $this->newContentSource();
 
     $revision = DiffusionCommitRevisionQuery::loadRevisionForCommit(
-      $viewer,
+      $actor,
       $commit);
 
     // Prevent the commit from generating a mention of the associated
@@ -75,7 +100,7 @@ final class PhabricatorRepositoryCommitPublishWorker
     }
 
     $editor = $commit->getApplicationTransactionEditor()
-      ->setActor($viewer)
+      ->setActor($actor)
       ->setActingAsPHID($acting_phid)
       ->setContinueOnNoEffect(true)
       ->setContinueOnMissingFields(true)
@@ -391,4 +416,129 @@ final class PhabricatorRepositoryCommitPublishWorker
 
     return $file->loadFileData();
   }
+
+  private function closeRevisions(
+    PhabricatorUser $actor,
+    PhabricatorRepositoryCommit $commit) {
+
+    $differential = 'PhabricatorDifferentialApplication';
+    if (!PhabricatorApplication::isClassInstalled($differential)) {
+      return;
+    }
+
+    $repository = $commit->getRepository();
+    $data = $commit->getCommitData();
+    $ref = $data->getCommitRef();
+
+    $field_query = id(new DiffusionLowLevelCommitFieldsQuery())
+      ->setRepository($repository)
+      ->withCommitRef($ref);
+
+    $field_values = $field_query->execute();
+
+    $revision_id = idx($field_values, 'revisionID');
+    if (!$revision_id) {
+      return;
+    }
+
+    $revision = id(new DifferentialRevisionQuery())
+      ->setViewer($actor)
+      ->withIDs(array($revision_id))
+      ->executeOne();
+    if (!$revision) {
+      return;
+    }
+
+    // NOTE: This is very old code from when revisions had a single reviewer.
+    // It still powers the "Reviewer (Deprecated)" field in Herald, but should
+    // be removed.
+    if (!empty($field_values['reviewedByPHIDs'])) {
+      $data->setCommitDetail(
+        'reviewerPHID',
+        head($field_values['reviewedByPHIDs']));
+    }
+
+    $match_data = $field_query->getRevisionMatchData();
+
+    $data->setCommitDetail('differential.revisionID', $revision_id);
+    $data->setCommitDetail('revisionMatchData', $match_data);
+
+    $data->save();
+
+    $properties = array(
+      'revisionMatchData' => $match_data,
+    );
+    $this->queueObjectUpdate($commit, $revision, $properties);
+  }
+
+  private function closeTasks(
+    PhabricatorUser $actor,
+    PhabricatorRepositoryCommit $commit) {
+
+    $maniphest = 'PhabricatorManiphestApplication';
+    if (!PhabricatorApplication::isClassInstalled($maniphest)) {
+      return;
+    }
+
+    $data = $commit->getCommitData();
+
+    $prefixes = ManiphestTaskStatus::getStatusPrefixMap();
+    $suffixes = ManiphestTaskStatus::getStatusSuffixMap();
+    $message = $data->getCommitMessage();
+
+    $matches = id(new ManiphestCustomFieldStatusParser())
+      ->parseCorpus($message);
+
+    $task_map = array();
+    foreach ($matches as $match) {
+      $prefix = phutil_utf8_strtolower($match['prefix']);
+      $suffix = phutil_utf8_strtolower($match['suffix']);
+
+      $status = idx($suffixes, $suffix);
+      if (!$status) {
+        $status = idx($prefixes, $prefix);
+      }
+
+      foreach ($match['monograms'] as $task_monogram) {
+        $task_id = (int)trim($task_monogram, 'tT');
+        $task_map[$task_id] = $status;
+      }
+    }
+
+    if (!$task_map) {
+      return;
+    }
+
+    $tasks = id(new ManiphestTaskQuery())
+      ->setViewer($actor)
+      ->withIDs(array_keys($task_map))
+      ->execute();
+    foreach ($tasks as $task_id => $task) {
+      $status = $task_map[$task_id];
+
+      $properties = array(
+        'status' => $status,
+      );
+
+      $this->queueObjectUpdate($commit, $task, $properties);
+    }
+  }
+
+  private function queueObjectUpdate(
+    PhabricatorRepositoryCommit $commit,
+    $object,
+    array $properties) {
+
+    $this->queueTask(
+      'DiffusionUpdateObjectAfterCommitWorker',
+      array(
+        'commitPHID' => $commit->getPHID(),
+        'objectPHID' => $object->getPHID(),
+        'properties' => $properties,
+      ),
+      array(
+        'priority' => PhabricatorWorker::PRIORITY_DEFAULT,
+      ));
+  }
+
 }

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

@@ -125,145 +125,8 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
     $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
-    // details about why we are not publishing it yet.
-
-    $publisher = $repository->newPublisher();
-    if ($publisher->shouldPublishCommit($commit)) {
-      $this->closeRevisions($viewer, $commit);
-      $this->closeTasks($viewer, $commit);
-    } else {
-      $hold_reasons = $publisher->getCommitHoldReasons($commit);
-
-      $data->setCommitDetail('holdReasons', $hold_reasons);
-      $data->save();
-    }
-
     $commit->writeImportStatusFlag(
       PhabricatorRepositoryCommit::IMPORTED_MESSAGE);
   }
 
-  private function closeRevisions(
-    PhabricatorUser $actor,
-    PhabricatorRepositoryCommit $commit) {
-
-    $differential = 'PhabricatorDifferentialApplication';
-    if (!PhabricatorApplication::isClassInstalled($differential)) {
-      return;
-    }
-
-    $repository = $commit->getRepository();
-    $data = $commit->getCommitData();
-    $ref = $data->getCommitRef();
-
-    $field_query = id(new DiffusionLowLevelCommitFieldsQuery())
-      ->setRepository($repository)
-      ->withCommitRef($ref);
-
-    $field_values = $field_query->execute();
-
-    $revision_id = idx($field_values, 'revisionID');
-    if (!$revision_id) {
-      return;
-    }
-
-    $revision = id(new DifferentialRevisionQuery())
-      ->setViewer($actor)
-      ->withIDs(array($revision_id))
-      ->executeOne();
-    if (!$revision) {
-      return;
-    }
-
-    // NOTE: This is very old code from when revisions had a single reviewer.
-    // It still powers the "Reviewer (Deprecated)" field in Herald, but should
-    // be removed.
-    if (!empty($field_values['reviewedByPHIDs'])) {
-      $data->setCommitDetail(
-        'reviewerPHID',
-        head($field_values['reviewedByPHIDs']));
-    }
-
-    $match_data = $field_query->getRevisionMatchData();
-
-    $data->setCommitDetail('differential.revisionID', $revision_id);
-    $data->setCommitDetail('revisionMatchData', $match_data);
-
-    $properties = array(
-      'revisionMatchData' => $match_data,
-    );
-    $this->queueObjectUpdate($commit, $revision, $properties);
-  }
-
-  private function closeTasks(
-    PhabricatorUser $actor,
-    PhabricatorRepositoryCommit $commit) {
-
-    $maniphest = 'PhabricatorManiphestApplication';
-    if (!PhabricatorApplication::isClassInstalled($maniphest)) {
-      return;
-    }
-
-    $data = $commit->getCommitData();
-
-    $prefixes = ManiphestTaskStatus::getStatusPrefixMap();
-    $suffixes = ManiphestTaskStatus::getStatusSuffixMap();
-    $message = $data->getCommitMessage();
-
-    $matches = id(new ManiphestCustomFieldStatusParser())
-      ->parseCorpus($message);
-
-    $task_map = array();
-    foreach ($matches as $match) {
-      $prefix = phutil_utf8_strtolower($match['prefix']);
-      $suffix = phutil_utf8_strtolower($match['suffix']);
-
-      $status = idx($suffixes, $suffix);
-      if (!$status) {
-        $status = idx($prefixes, $prefix);
-      }
-
-      foreach ($match['monograms'] as $task_monogram) {
-        $task_id = (int)trim($task_monogram, 'tT');
-        $task_map[$task_id] = $status;
-      }
-    }
-
-    if (!$task_map) {
-      return;
-    }
-
-    $tasks = id(new ManiphestTaskQuery())
-      ->setViewer($actor)
-      ->withIDs(array_keys($task_map))
-      ->execute();
-    foreach ($tasks as $task_id => $task) {
-      $status = $task_map[$task_id];
-
-      $properties = array(
-        'status' => $status,
-      );
-
-      $this->queueObjectUpdate($commit, $task, $properties);
-    }
-  }
-
-  private function queueObjectUpdate(
-    PhabricatorRepositoryCommit $commit,
-    $object,
-    array $properties) {
-
-    $this->queueTask(
-      'DiffusionUpdateObjectAfterCommitWorker',
-      array(
-        'commitPHID' => $commit->getPHID(),
-        'objectPHID' => $object->getPHID(),
-        'properties' => $properties,
-      ),
-      array(
-        'priority' => PhabricatorWorker::PRIORITY_DEFAULT,
-      ));
-  }
-
 }