From a39c5904423548f22e166bd37532c66ee2829d4d Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 12 Aug 2020 13:07:44 -0700 Subject: [PATCH] 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 --- ...abricatorRepositoryCommitPublishWorker.php | 164 +++++++++++++++++- ...torRepositoryCommitMessageParserWorker.php | 137 --------------- 2 files changed, 157 insertions(+), 144 deletions(-) diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php index 3a555e7075..846eb7987a 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php +++ b/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, + )); + } + } diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php index 1ae1084743..ea277a84c9 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/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, - )); - } - }