From 23332241b20e26e802cc53239c3b10d1c664c219 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 20 Dec 2013 12:38:15 -0800 Subject: [PATCH] Move commit hash querying to DiffusionLowLevelCommitQuery Summary: Ref T4195. I need this for the Herald pre-commit rules, and it generally simplifies things. Test Plan: Used `reparse.php` plus `var_dump()` to inspect refs in Git, Mercurial and SVN repos. They all looked correct and reparsed correctly. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4195 Differential Revision: https://secure.phabricator.com/D7804 --- src/__phutil_library_map__.php | 2 + .../diffusion/data/DiffusionCommitHash.php | 26 +++++++++++++ .../diffusion/data/DiffusionCommitRef.php | 10 +++++ .../lowlevel/DiffusionLowLevelCommitQuery.php | 31 +++++++++++++-- ...torRepositoryCommitMessageParserWorker.php | 39 +++++++++---------- ...RepositoryGitCommitMessageParserWorker.php | 30 +------------- ...toryMercurialCommitMessageParserWorker.php | 17 +------- ...RepositorySvnCommitMessageParserWorker.php | 11 +----- 8 files changed, 86 insertions(+), 80 deletions(-) create mode 100644 src/applications/diffusion/data/DiffusionCommitHash.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index fb20f4ac65..04a4abd246 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -478,6 +478,7 @@ phutil_register_library_map(array( 'DiffusionCommitChangeTableView' => 'applications/diffusion/view/DiffusionCommitChangeTableView.php', 'DiffusionCommitController' => 'applications/diffusion/controller/DiffusionCommitController.php', 'DiffusionCommitEditController' => 'applications/diffusion/controller/DiffusionCommitEditController.php', + 'DiffusionCommitHash' => 'applications/diffusion/data/DiffusionCommitHash.php', 'DiffusionCommitHookEngine' => 'applications/diffusion/engine/DiffusionCommitHookEngine.php', 'DiffusionCommitHookRejectException' => 'applications/diffusion/exception/DiffusionCommitHookRejectException.php', 'DiffusionCommitParentsQuery' => 'applications/diffusion/query/parents/DiffusionCommitParentsQuery.php', @@ -2866,6 +2867,7 @@ phutil_register_library_map(array( 'DiffusionCommitChangeTableView' => 'DiffusionView', 'DiffusionCommitController' => 'DiffusionController', 'DiffusionCommitEditController' => 'DiffusionController', + 'DiffusionCommitHash' => 'Phobject', 'DiffusionCommitHookEngine' => 'Phobject', 'DiffusionCommitHookRejectException' => 'Exception', 'DiffusionCommitParentsQuery' => 'DiffusionQuery', diff --git a/src/applications/diffusion/data/DiffusionCommitHash.php b/src/applications/diffusion/data/DiffusionCommitHash.php new file mode 100644 index 0000000000..8605a99a34 --- /dev/null +++ b/src/applications/diffusion/data/DiffusionCommitHash.php @@ -0,0 +1,26 @@ +hashValue = $hash_value; + return $this; + } + + public function getHashValue() { + return $this->hashValue; + } + + public function setHashType($hash_type) { + $this->hashType = $hash_type; + return $this; + } + + public function getHashType() { + return $this->hashType; + } + +} diff --git a/src/applications/diffusion/data/DiffusionCommitRef.php b/src/applications/diffusion/data/DiffusionCommitRef.php index a13175c51b..5088eef00e 100644 --- a/src/applications/diffusion/data/DiffusionCommitRef.php +++ b/src/applications/diffusion/data/DiffusionCommitRef.php @@ -7,6 +7,16 @@ final class DiffusionCommitRef extends Phobject { private $authorEmail; private $committerName; private $committerEmail; + private $hashes = array(); + + public function setHashes(array $hashes) { + $this->hashes = $hashes; + return $this; + } + + public function getHashes() { + return $this->hashes; + } public function setCommitterEmail($committer_email) { $this->committerEmail = $committer_email; diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitQuery.php index dc5f3db137..54f42ef7d7 100644 --- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitQuery.php +++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitQuery.php @@ -51,7 +51,9 @@ final class DiffusionLowLevelCommitQuery list($info) = $repository->execxLocalCommand( 'log -n 1 --encoding=%s --format=%s %s --', 'UTF-8', - implode('%x00', array('%e', '%cn', '%ce', '%an', '%ae', '%s%n%n%b')), + implode( + '%x00', + array('%e', '%cn', '%ce', '%an', '%ae', '%T', '%s%n%n%b')), $this->identifier); $parts = explode("\0", $info); @@ -67,12 +69,22 @@ final class DiffusionLowLevelCommitQuery } } + $hashes = array( + id(new DiffusionCommitHash()) + ->setHashType(ArcanistDifferentialRevisionHash::HASH_GIT_COMMIT) + ->setHashValue($this->identifier), + id(new DiffusionCommitHash()) + ->setHashType(ArcanistDifferentialRevisionHash::HASH_GIT_TREE) + ->setHashValue($parts[4]), + ); + return id(new DiffusionCommitRef()) ->setCommitterName($parts[0]) ->setCommitterEmail($parts[1]) ->setAuthorName($parts[2]) ->setAuthorEmail($parts[3]) - ->setMessage($parts[4]); + ->setHashes($hashes) + ->setMessage($parts[5]); } private function loadMercurialCommitRef() { @@ -90,10 +102,17 @@ final class DiffusionLowLevelCommitQuery list($author_name, $author_email) = $this->splitUserIdentifier($author); + $hashes = array( + id(new DiffusionCommitHash()) + ->setHashType(ArcanistDifferentialRevisionHash::HASH_MERCURIAL_COMMIT) + ->setHashValue($this->identifier), + ); + return id(new DiffusionCommitRef()) ->setAuthorName($author_name) ->setAuthorEmail($author_email) - ->setMessage($message); + ->setMessage($message) + ->setHashes($hashes); } private function loadSubversionCommitRef() { @@ -114,10 +133,14 @@ final class DiffusionLowLevelCommitQuery list($author_name, $author_email) = $this->splitUserIdentifier($author); + // No hashes in Subversion. + $hashes = array(); + return id(new DiffusionCommitRef()) ->setAuthorName($author_name) ->setAuthorEmail($author_email) - ->setMessage($message); + ->setMessage($message) + ->setHashes($hashes); } private function splitUserIdentifier($user) { diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php index 0822b7c3a2..4ec5d75b79 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -3,14 +3,12 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker extends PhabricatorRepositoryCommitParserWorker { - abstract protected function getCommitHashes( - PhabricatorRepository $repository, - PhabricatorRepositoryCommit $commit); - - final protected function updateCommitData($author, $message, - $committer = null) { - + final protected function updateCommitData(DiffusionCommitRef $ref) { $commit = $this->commit; + $author = $ref->getAuthor(); + $message = $ref->getMessage(); + $committer = $ref->getCommitter(); + $hashes = $ref->getHashes(); $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere( 'commitID = %d', @@ -26,7 +24,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $data->setCommitMessage($message); - if ($committer) { + if (strlen($committer)) { $data->setCommitDetail('committer', $committer); $data->setCommitDetail( 'committerPHID', @@ -63,20 +61,19 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker } $revision_id = idx($field_values, 'revisionID'); - if (!$revision_id) { - $hashes = $this->getCommitHashes( - $repository, - $commit); - if ($hashes) { - $revisions = id(new DifferentialRevisionQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withCommitHashes($hashes) - ->execute(); + 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(); - } + if (!empty($revisions)) { + $revision = $this->identifyBestRevision($revisions); + $revision_id = $revision->getID(); } } diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php index b10a835ad1..62c7572678 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php @@ -12,15 +12,7 @@ final class PhabricatorRepositoryGitCommitMessageParserWorker ->withIdentifier($commit->getCommitIdentifier()) ->execute(); - $committer = $ref->getCommitter(); - $author = $ref->getAuthor(); - $message = $ref->getMessage(); - - if ($committer == $author) { - $committer = null; - } - - $this->updateCommitData($author, $message, $committer); + $this->updateCommitData($ref); if ($this->shouldQueueFollowupTasks()) { PhabricatorWorker::scheduleTask( @@ -31,24 +23,4 @@ final class PhabricatorRepositoryGitCommitMessageParserWorker } } - protected function getCommitHashes( - PhabricatorRepository $repository, - PhabricatorRepositoryCommit $commit) { - - list($stdout) = $repository->execxLocalCommand( - 'log -n 1 --format=%s %s --', - '%T', - $commit->getCommitIdentifier()); - - $commit_hash = $commit->getCommitIdentifier(); - $tree_hash = trim($stdout); - - return array( - array(ArcanistDifferentialRevisionHash::HASH_GIT_COMMIT, - $commit_hash), - array(ArcanistDifferentialRevisionHash::HASH_GIT_TREE, - $tree_hash), - ); - } - } diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php index 6084b62bd9..dedf14db82 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php @@ -12,10 +12,7 @@ final class PhabricatorRepositoryMercurialCommitMessageParserWorker ->withIdentifier($commit->getCommitIdentifier()) ->execute(); - $author = $ref->getAuthor(); - $message = $ref->getMessage(); - - $this->updateCommitData($author, $message); + $this->updateCommitData($ref); if ($this->shouldQueueFollowupTasks()) { PhabricatorWorker::scheduleTask( @@ -26,16 +23,4 @@ final class PhabricatorRepositoryMercurialCommitMessageParserWorker } } - protected function getCommitHashes( - PhabricatorRepository $repository, - PhabricatorRepositoryCommit $commit) { - - $commit_hash = $commit->getCommitIdentifier(); - - return array( - array(ArcanistDifferentialRevisionHash::HASH_MERCURIAL_COMMIT, - $commit_hash), - ); - } - } diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositorySvnCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositorySvnCommitMessageParserWorker.php index 7aabcd4346..d15613850d 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositorySvnCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositorySvnCommitMessageParserWorker.php @@ -12,10 +12,7 @@ final class PhabricatorRepositorySvnCommitMessageParserWorker ->withIdentifier($commit->getCommitIdentifier()) ->execute(); - $author = $ref->getAuthor(); - $message = $ref->getMessage(); - - $this->updateCommitData($author, $message); + $this->updateCommitData($ref); if ($this->shouldQueueFollowupTasks()) { PhabricatorWorker::scheduleTask( @@ -26,10 +23,4 @@ final class PhabricatorRepositorySvnCommitMessageParserWorker } } - protected function getCommitHashes( - PhabricatorRepository $repository, - PhabricatorRepositoryCommit $commit) { - return array(); - } - }