Index: src/__phutil_library_map__.php =================================================================== --- src/__phutil_library_map__.php +++ src/__phutil_library_map__.php @@ -508,6 +508,7 @@ '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 @@ 'DiffusionLastModifiedController' => 'DiffusionController', 'DiffusionLintController' => 'DiffusionController', 'DiffusionLintDetailsController' => 'DiffusionController', + 'DiffusionLowLevelCommitFieldsQuery' => 'DiffusionLowLevelQuery', 'DiffusionLowLevelCommitQuery' => 'DiffusionLowLevelQuery', 'DiffusionLowLevelGitRefQuery' => 'DiffusionLowLevelQuery', 'DiffusionLowLevelMercurialBranchesQuery' => 'DiffusionLowLevelQuery', Index: src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php =================================================================== --- /dev/null +++ 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); + } + +} Index: src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php =================================================================== --- src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -43,16 +43,11 @@ $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 @@ 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 @@ 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) {