From d7c4edab28a4e1b166e9608309b9251483aea115 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 20 Dec 2013 12:38:44 -0800 Subject: [PATCH] Move commit message/metadata field query to a separate class Summary: Ref T4195. I need to query commit metadata to figure out which revision a commit is associated with. Move this out of the MessageParser so the code can be called from the HookEngine. Test Plan: Used `reparse.php` to reparse a variety of SVN, Mercurial and Git commits. Used `var_dump()` to verify sensible fields were returned. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4195 Differential Revision: https://secure.phabricator.com/D7805 --- src/__phutil_library_map__.php | 2 + .../DiffusionLowLevelCommitFieldsQuery.php | 86 +++++++++++++++++ ...torRepositoryCommitMessageParserWorker.php | 95 ++----------------- 3 files changed, 94 insertions(+), 89 deletions(-) create mode 100644 src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 04a4abd246..ff75e15f6c 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -508,6 +508,7 @@ phutil_register_library_map(array( 'DiffusionLintController' => 'applications/diffusion/controller/DiffusionLintController.php', 'DiffusionLintDetailsController' => 'applications/diffusion/controller/DiffusionLintDetailsController.php', 'DiffusionLintSaveRunner' => 'applications/diffusion/DiffusionLintSaveRunner.php', + 'DiffusionLowLevelCommitFieldsQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php', 'DiffusionLowLevelCommitQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelCommitQuery.php', 'DiffusionLowLevelGitRefQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelGitRefQuery.php', 'DiffusionLowLevelMercurialBranchesQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialBranchesQuery.php', @@ -2894,6 +2895,7 @@ phutil_register_library_map(array( 'DiffusionLastModifiedController' => 'DiffusionController', 'DiffusionLintController' => 'DiffusionController', 'DiffusionLintDetailsController' => 'DiffusionController', + 'DiffusionLowLevelCommitFieldsQuery' => 'DiffusionLowLevelQuery', 'DiffusionLowLevelCommitQuery' => 'DiffusionLowLevelQuery', 'DiffusionLowLevelGitRefQuery' => 'DiffusionLowLevelQuery', 'DiffusionLowLevelMercurialBranchesQuery' => 'DiffusionLowLevelQuery', diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php new file mode 100644 index 0000000000..64d541f151 --- /dev/null +++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php @@ -0,0 +1,86 @@ +ref = $ref; + return $this; + } + + public function executeQuery() { + $ref = $this->ref; + $message = $ref->getMessage(); + $hashes = $ref->getHashes(); + + $params = array( + 'corpus' => $message, + 'partial' => true, + ); + + $result = id(new ConduitCall('differential.parsecommitmessage', $params)) + ->setUser(PhabricatorUser::getOmnipotentUser()) + ->execute(); + $fields = $result['fields']; + + // If there is no "Differential Revision:" field in the message, try to + // identify the revision by doing a hash lookup. + + $revision_id = idx($fields, 'revisionID'); + if (!$revision_id && $hashes) { + $hash_list = array(); + foreach ($hashes as $hash) { + $hash_list[] = array($hash->getHashType(), $hash->getHashValue()); + } + $revisions = id(new DifferentialRevisionQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withCommitHashes($hash_list) + ->execute(); + + if (!empty($revisions)) { + $revision = $this->pickBestRevision($revisions); + $fields['revisionID'] = $revision->getID(); + } + } + + return $fields; + } + + + /** + * When querying for revisions by hash, more than one revision may be found. + * This function identifies the "best" revision from such a set. Typically, + * there is only one revision found. Otherwise, we try to pick an accepted + * revision first, followed by an open revision, and otherwise we go with a + * closed or abandoned revision as a last resort. + */ + private function pickBestRevision(array $revisions) { + assert_instances_of($revisions, 'DifferentialRevision'); + + // If we have more than one revision of a given status, choose the most + // recently updated one. + $revisions = msort($revisions, 'getDateModified'); + $revisions = array_reverse($revisions); + + // Try to find an accepted revision first. + $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; + foreach ($revisions as $revision) { + if ($revision->getStatus() == $status_accepted) { + return $revision; + } + } + + // Try to find an open revision. + foreach ($revisions as $revision) { + if (!$revision->isClosed()) { + return $revision; + } + } + + // Settle for whatever's left. + return head($revisions); + } + +} diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php index 4ec5d75b79..51de4ee37b 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -43,16 +43,11 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $author_phid); } - $call = new ConduitCall( - 'differential.parsecommitmessage', - array( - 'corpus' => $message, - 'partial' => true, - )); - $call->setUser(PhabricatorUser::getOmnipotentUser()); - $result = $call->execute(); - - $field_values = $result['fields']; + $field_values = id(new DiffusionLowLevelCommitFieldsQuery()) + ->setRepository($repository) + ->withCommitRef($ref) + ->execute(); + $revision_id = idx($field_values, 'revisionID'); if (!empty($field_values['reviewedByPHIDs'])) { $data->setCommitDetail( @@ -60,26 +55,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker reset($field_values['reviewedByPHIDs'])); } - $revision_id = idx($field_values, 'revisionID'); - if (!$revision_id && $hashes) { - $hash_list = array(); - foreach ($hashes as $hash) { - $hash_list[] = array($hash->getHashType(), $hash->getHashValue()); - } - $revisions = id(new DifferentialRevisionQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withCommitHashes($hash_list) - ->execute(); - - if (!empty($revisions)) { - $revision = $this->identifyBestRevision($revisions); - $revision_id = $revision->getID(); - } - } - - $data->setCommitDetail( - 'differential.revisionID', - $revision_id); + $data->setCommitDetail('differential.revisionID', $revision_id); if ($author_phid != $commit->getAuthorPHID()) { $commit->setAuthorPHID($author_phid); @@ -386,65 +362,6 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker return null; } - /** - * When querying for revisions by hash, more than one revision may be found. - * This function identifies the "best" revision from such a set. Typically, - * there is only one revision found. Otherwise, we try to pick an accepted - * revision first, followed by an open revision, and otherwise we go with a - * closed or abandoned revision as a last resort. - */ - private function identifyBestRevision(array $revisions) { - assert_instances_of($revisions, 'DifferentialRevision'); - // get the simplest, common case out of the way - if (count($revisions) == 1) { - return reset($revisions); - } - - $first_choice = array(); - $second_choice = array(); - $third_choice = array(); - foreach ($revisions as $revision) { - switch ($revision->getStatus()) { - // "Accepted" revisions -- ostensibly what we're looking for! - case ArcanistDifferentialRevisionStatus::ACCEPTED: - $first_choice[] = $revision; - break; - // "Open" revisions - case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: - case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: - $second_choice[] = $revision; - break; - // default is a wtf? here - default: - case ArcanistDifferentialRevisionStatus::ABANDONED: - case ArcanistDifferentialRevisionStatus::CLOSED: - $third_choice[] = $revision; - break; - } - } - - // go down the ladder like a bro at last call - if (!empty($first_choice)) { - return $this->identifyMostRecentRevision($first_choice); - } - if (!empty($second_choice)) { - return $this->identifyMostRecentRevision($second_choice); - } - if (!empty($third_choice)) { - return $this->identifyMostRecentRevision($third_choice); - } - } - - /** - * Given a set of revisions, returns the revision with the latest - * updated time. This is ostensibly the most recent revision. - */ - private function identifyMostRecentRevision(array $revisions) { - assert_instances_of($revisions, 'DifferentialRevision'); - $revisions = msort($revisions, 'getDateModified'); - return end($revisions); - } - private function resolveUserPHID( PhabricatorRepositoryCommit $commit, $user_name) {