From 0e3cb3b393ccf284c533e3af6a36c48fcb9b82f0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 21 Jul 2013 10:57:07 -0700 Subject: [PATCH] Use Application PHIDs for commits Summary: Ref T2716. Ref T2715. Move CMIT to use Application PHIDs. Nothing too special here, but I consolidated some code into DiffusionCommitQuery. Depends on D6514. Test Plan: Browsed Diffusion. Browsed Differential/Maniphest with linked commits. Used jump nav; used `phid.lookup` and `phid.query`. Used remarkup for Git and SVN repos. Grepped for PHID_TYPE_CMIT. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T2715, T2716 Differential Revision: https://secure.phabricator.com/D6515 --- src/__phutil_library_map__.php | 2 + .../diffusion/query/DiffusionCommitQuery.php | 68 ++++++++++++++- .../remarkup/DiffusionRemarkupRule.php | 59 ++----------- .../phid/PhabricatorObjectHandle.php | 1 - .../phid/PhabricatorPHIDConstants.php | 1 - .../handle/PhabricatorObjectHandleData.php | 40 --------- .../phid/storage/PhabricatorPHID.php | 24 +----- .../PhabricatorRepositoryPHIDTypeCommit.php | 86 +++++++++++++++++++ .../storage/PhabricatorRepositoryCommit.php | 9 +- .../PhabricatorSearchAttachController.php | 4 +- .../PhabricatorSearchController.php | 2 +- .../PhabricatorSearchAbstractDocument.php | 2 +- .../edges/constants/PhabricatorEdgeConfig.php | 1 - 13 files changed, 166 insertions(+), 133 deletions(-) create mode 100644 src/applications/repository/phid/PhabricatorRepositoryPHIDTypeCommit.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 909849d3ba..719a17bf77 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1451,6 +1451,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryManagementWorkflow' => 'applications/repository/management/PhabricatorRepositoryManagementWorkflow.php', 'PhabricatorRepositoryMercurialCommitChangeParserWorker' => 'applications/repository/worker/commitchangeparser/PhabricatorRepositoryMercurialCommitChangeParserWorker.php', 'PhabricatorRepositoryMercurialCommitMessageParserWorker' => 'applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php', + 'PhabricatorRepositoryPHIDTypeCommit' => 'applications/repository/phid/PhabricatorRepositoryPHIDTypeCommit.php', 'PhabricatorRepositoryPullEngine' => 'applications/repository/engine/PhabricatorRepositoryPullEngine.php', 'PhabricatorRepositoryPullLocalDaemon' => 'applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php', 'PhabricatorRepositoryQuery' => 'applications/repository/query/PhabricatorRepositoryQuery.php', @@ -3445,6 +3446,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryManagementWorkflow' => 'PhutilArgumentWorkflow', 'PhabricatorRepositoryMercurialCommitChangeParserWorker' => 'PhabricatorRepositoryCommitChangeParserWorker', 'PhabricatorRepositoryMercurialCommitMessageParserWorker' => 'PhabricatorRepositoryCommitMessageParserWorker', + 'PhabricatorRepositoryPHIDTypeCommit' => 'PhabricatorPHIDType', 'PhabricatorRepositoryPullEngine' => 'PhabricatorRepositoryEngine', 'PhabricatorRepositoryPullLocalDaemon' => 'PhabricatorDaemon', 'PhabricatorRepositoryQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php index cb5ae37c41..36e5a711ea 100644 --- a/src/applications/diffusion/query/DiffusionCommitQuery.php +++ b/src/applications/diffusion/query/DiffusionCommitQuery.php @@ -7,6 +7,7 @@ final class DiffusionCommitQuery private $identifiers; private $phids; private $defaultRepository; + private $identifierMap; /** * Load commits by partial or full identifiers, e.g. "rXab82393", "rX1234", @@ -43,7 +44,19 @@ final class DiffusionCommitQuery return $this; } + public function getIdentifierMap() { + if ($this->identifierMap === null) { + throw new Exception( + "You must execute() the query before accessing the identifier map."); + } + return $this->identifierMap; + } + protected function loadPage() { + if ($this->identifierMap === null) { + $this->identifierMap = array(); + } + $table = new PhabricatorRepositoryCommit(); $conn_r = $table->establishConnection('r'); @@ -59,10 +72,6 @@ final class DiffusionCommitQuery } public function willFilterPage(array $commits) { - if (!$commits) { - return array(); - } - $repository_ids = mpull($commits, 'getRepositoryID', 'getRepositoryID'); $repos = id(new PhabricatorRepositoryQuery()) ->setViewer($this->getViewer()) @@ -78,6 +87,47 @@ final class DiffusionCommitQuery } } + if ($this->identifiers !== null) { + $ids = array_fuse($this->identifiers); + $min_qualified = PhabricatorRepository::MINIMUM_QUALIFIED_HASH; + + $result = array(); + foreach ($commits as $commit) { + $prefix = 'r'.$commit->getRepository()->getCallsign(); + $suffix = $commit->getCommitIdentifier(); + + if ($commit->getRepository()->isSVN()) { + if (isset($ids[$prefix.$suffix])) { + $result[$prefix.$suffix][] = $commit; + } + } else { + // This awkward contruction is so we can link the commits up in O(N) + // time instead of O(N^2). + for ($ii = $min_qualified; $ii <= strlen($suffix); $ii++) { + $part = substr($suffix, 0, $ii); + if (isset($ids[$prefix.$part])) { + $result[$prefix.$part][] = $commit; + } + if (isset($ids[$part])) { + $result[$part][] = $commit; + } + } + } + } + + foreach ($result as $identifier => $matching_commits) { + if (count($matching_commits) == 1) { + $result[$identifier] = head($matching_commits); + } else { + // This reference is ambiguous (it matches more than one commit) so + // don't link it + unset($result[$identifier]); + } + } + + $this->identifierMap += $result; + } + return $commits; } @@ -190,4 +240,14 @@ final class DiffusionCommitQuery return $this->formatWhereClause($where); } + public function didFilterResults(array $filtered) { + if ($this->identifierMap) { + foreach ($this->identifierMap as $name => $commit) { + if (isset($filtered[$commit->getPHID()])) { + unset($this->identifierMap[$name]); + } + } + } + } + } diff --git a/src/applications/diffusion/remarkup/DiffusionRemarkupRule.php b/src/applications/diffusion/remarkup/DiffusionRemarkupRule.php index eea5ad44b5..52fd88568e 100644 --- a/src/applications/diffusion/remarkup/DiffusionRemarkupRule.php +++ b/src/applications/diffusion/remarkup/DiffusionRemarkupRule.php @@ -12,68 +12,19 @@ final class DiffusionRemarkupRule } protected function getObjectIDPattern() { - $min_unqualified = PhabricatorRepository::MINIMUM_UNQUALIFIED_HASH; - $min_qualified = PhabricatorRepository::MINIMUM_QUALIFIED_HASH; - - return - 'r[A-Z]+[1-9]\d*'. - '|'. - 'r[A-Z]+[a-f0-9]{'.$min_qualified.',40}'. - '|'. - '[a-f0-9]{'.$min_unqualified.',40}'; + return PhabricatorRepositoryPHIDTypeCommit::getCommitObjectNamePattern(); } protected function loadObjects(array $ids) { $viewer = $this->getEngine()->getConfig('viewer'); - $min_qualified = PhabricatorRepository::MINIMUM_QUALIFIED_HASH; - $commits = id(new DiffusionCommitQuery()) + $query = id(new DiffusionCommitQuery()) ->setViewer($viewer) - ->withIdentifiers($ids) - ->execute(); + ->withIdentifiers($ids); - if (!$commits) { - return array(); - } + $query->execute(); - $ids = array_fuse($ids); - - $result = array(); - foreach ($commits as $commit) { - $prefix = 'r'.$commit->getRepository()->getCallsign(); - $suffix = $commit->getCommitIdentifier(); - - if ($commit->getRepository()->isSVN()) { - if (isset($ids[$prefix.$suffix])) { - $result[$prefix.$suffix][] = $commit; - } - } else { - // This awkward contruction is so we can link the commits up in O(N) - // time instead of O(N^2). - for ($ii = $min_qualified; $ii <= strlen($suffix); $ii++) { - $part = substr($suffix, 0, $ii); - if (isset($ids[$prefix.$part])) { - $result[$prefix.$part][] = $commit; - } - if (isset($ids[$part])) { - $result[$part][] = $commit; - } - } - } - } - - foreach ($result as $identifier => $commits) { - if (count($commits) == 1) { - $result[$identifier] = head($commits); - } else { - // This reference is ambiguous -- it matches more than one commit -- so - // don't link it. We could potentially improve this, but it's a bit - // tricky since the superclass expects a single object. - unset($result[$identifier]); - } - } - - return $result; + return $query->getIdentifierMap(); } } diff --git a/src/applications/phid/PhabricatorObjectHandle.php b/src/applications/phid/PhabricatorObjectHandle.php index dcd067d4d8..de72591451 100644 --- a/src/applications/phid/PhabricatorObjectHandle.php +++ b/src/applications/phid/PhabricatorObjectHandle.php @@ -110,7 +110,6 @@ final class PhabricatorObjectHandle static $map = array( PhabricatorPHIDConstants::PHID_TYPE_USER => 'User', PhabricatorPHIDConstants::PHID_TYPE_TASK => 'Task', - PhabricatorPHIDConstants::PHID_TYPE_CMIT => 'Commit', PhabricatorPHIDConstants::PHID_TYPE_WIKI => 'Phriction Document', PhabricatorPHIDConstants::PHID_TYPE_MCRO => 'Image Macro', PhabricatorPHIDConstants::PHID_TYPE_MOCK => 'Pholio Mock', diff --git a/src/applications/phid/PhabricatorPHIDConstants.php b/src/applications/phid/PhabricatorPHIDConstants.php index 3d23e4b3b2..04c1777bf5 100644 --- a/src/applications/phid/PhabricatorPHIDConstants.php +++ b/src/applications/phid/PhabricatorPHIDConstants.php @@ -9,7 +9,6 @@ final class PhabricatorPHIDConstants { const PHID_TYPE_UNKNOWN = '????'; const PHID_TYPE_MAGIC = '!!!!'; const PHID_TYPE_REPO = 'REPO'; - const PHID_TYPE_CMIT = 'CMIT'; const PHID_TYPE_OPKG = 'OPKG'; const PHID_TYPE_PSTE = 'PSTE'; const PHID_TYPE_STRY = 'STRY'; diff --git a/src/applications/phid/handle/PhabricatorObjectHandleData.php b/src/applications/phid/handle/PhabricatorObjectHandleData.php index c169addced..d17878a886 100644 --- a/src/applications/phid/handle/PhabricatorObjectHandleData.php +++ b/src/applications/phid/handle/PhabricatorObjectHandleData.php @@ -56,13 +56,6 @@ final class PhabricatorObjectHandleData { $phids); return mpull($users, null, 'getPHID'); - case PhabricatorPHIDConstants::PHID_TYPE_CMIT: - $commits = id(new DiffusionCommitQuery()) - ->setViewer($this->viewer) - ->withPHIDs($phids) - ->execute(); - return mpull($commits, null, 'getPHID'); - case PhabricatorPHIDConstants::PHID_TYPE_TASK: // TODO: Update this to ManiphestTaskQuery, //especially// after we have // policy-awareness @@ -325,39 +318,6 @@ final class PhabricatorObjectHandleData { } break; - case PhabricatorPHIDConstants::PHID_TYPE_CMIT: - foreach ($phids as $phid) { - $handle = new PhabricatorObjectHandle(); - $handle->setPHID($phid); - $handle->setType($type); - - if (empty($objects[$phid])) { - $handle->setName('Unknown Commit'); - } else { - $repository = $objects[$phid]->getRepository(); - $commit = $objects[$phid]; - $callsign = $repository->getCallsign(); - $commit_identifier = $commit->getCommitIdentifier(); - - $name = $repository->formatCommitName($commit_identifier); - $handle->setName($name); - - $summary = $commit->getSummary(); - if (strlen($summary)) { - $handle->setFullName($name.': '.$summary); - } else { - $handle->setFullName($name); - } - - $handle->setURI('/r'.$callsign.$commit_identifier); - $handle->setTimestamp($commit->getEpoch()); - $handle->setComplete(true); - } - - $handles[$phid] = $handle; - } - break; - case PhabricatorPHIDConstants::PHID_TYPE_TASK: foreach ($phids as $phid) { $handle = new PhabricatorObjectHandle(); diff --git a/src/applications/phid/storage/PhabricatorPHID.php b/src/applications/phid/storage/PhabricatorPHID.php index 765d2a17a2..0266d0b1f5 100644 --- a/src/applications/phid/storage/PhabricatorPHID.php +++ b/src/applications/phid/storage/PhabricatorPHID.php @@ -43,28 +43,8 @@ final class PhabricatorPHID { // It's already a PHID! Yay. return $name; } - if (preg_match('/^r([A-Z]+)(\S*)$/', $name, $match)) { - $repository = id(new PhabricatorRepository()) - ->loadOneWhere('callsign = %s', $match[1]); - if ($match[2] == '') { - $object = $repository; - } else if ($repository) { - $object = id(new PhabricatorRepositoryCommit())->loadOneWhere( - 'repositoryID = %d AND commitIdentifier = %s', - $repository->getID(), - $match[2]); - if (!$object) { - try { - $object = id(new PhabricatorRepositoryCommit())->loadOneWhere( - 'repositoryID = %d AND commitIdentifier LIKE %>', - $repository->getID(), - $match[2]); - } catch (AphrontQueryCountException $ex) { - // Ambiguous; return nothing. - } - } - } - } else if (preg_match('/^t(\d+)$/i', $name, $match)) { + + if (preg_match('/^t(\d+)$/i', $name, $match)) { $object = id(new ManiphestTask())->load($match[1]); } else if (preg_match('/^m(\d+)$/i', $name, $match)) { $objects = id(new PholioMockQuery()) diff --git a/src/applications/repository/phid/PhabricatorRepositoryPHIDTypeCommit.php b/src/applications/repository/phid/PhabricatorRepositoryPHIDTypeCommit.php new file mode 100644 index 0000000000..72ed350e22 --- /dev/null +++ b/src/applications/repository/phid/PhabricatorRepositoryPHIDTypeCommit.php @@ -0,0 +1,86 @@ +setViewer($query->getViewer()) + ->withPHIDs($phids) + ->execute(); + } + + public function loadHandles( + PhabricatorHandleQuery $query, + array $handles, + array $objects) { + + foreach ($handles as $phid => $handle) { + $commit = $objects[$phid]; + $repository = $commit->getRepository(); + $callsign = $repository->getCallsign(); + $commit_identifier = $commit->getCommitIdentifier(); + + $name = $repository->formatCommitName($commit_identifier); + $summary = $commit->getSummary(); + if (strlen($summary)) { + $full_name = $name.': '.$summary; + } else { + $full_name = $name; + } + + $handle->setName($name); + $handle->setFullName($full_name); + $handle->setURI('/r'.$callsign.$commit_identifier); + $handle->setTimestamp($commit->getEpoch()); + } + } + + public static function getCommitObjectNamePattern() { + $min_unqualified = PhabricatorRepository::MINIMUM_UNQUALIFIED_HASH; + $min_qualified = PhabricatorRepository::MINIMUM_QUALIFIED_HASH; + + return + 'r[A-Z]+[1-9]\d*'. + '|'. + 'r[A-Z]+[a-f0-9]{'.$min_qualified.',40}'. + '|'. + '[a-f0-9]{'.$min_unqualified.',40}'; + } + + + public function canLoadNamedObject($name) { + $pattern = self::getCommitObjectNamePattern(); + return preg_match('(^'.$pattern.'$)', $name); + } + + public function loadNamedObjects( + PhabricatorObjectQuery $query, + array $names) { + + $query = id(new DiffusionCommitQuery()) + ->setViewer($query->getViewer()) + ->withIdentifiers($names); + + $query->execute(); + + return $query->getIdentifierMap(); + } + +} diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index e33806a97e..33504d237a 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -17,7 +17,7 @@ final class PhabricatorRepositoryCommit private $commitData; private $audits; private $isUnparsed; - private $repository; + private $repository = self::ATTACHABLE; public function attachRepository(PhabricatorRepository $repository) { $this->repository = $repository; @@ -25,10 +25,7 @@ final class PhabricatorRepositoryCommit } public function getRepository() { - if ($this->repository === null) { - throw new Exception("Call attachRepository() before getRepository()!"); - } - return $this->repository; + return $this->assertAttached($this->repository); } public function setIsUnparsed($is_unparsed) { @@ -49,7 +46,7 @@ final class PhabricatorRepositoryCommit public function generatePHID() { return PhabricatorPHID::generateNewPHID( - PhabricatorPHIDConstants::PHID_TYPE_CMIT); + PhabricatorRepositoryPHIDTypeCommit::TYPECONST); } public function loadCommitData() { diff --git a/src/applications/search/controller/PhabricatorSearchAttachController.php b/src/applications/search/controller/PhabricatorSearchAttachController.php index 4d6d0b1b38..c620c4a8c1 100644 --- a/src/applications/search/controller/PhabricatorSearchAttachController.php +++ b/src/applications/search/controller/PhabricatorSearchAttachController.php @@ -206,7 +206,7 @@ final class PhabricatorSearchAttachController $noun = 'Tasks'; $selected = 'assigned'; break; - case PhabricatorPHIDConstants::PHID_TYPE_CMIT: + case PhabricatorRepositoryPHIDTypeCommit::TYPECONST: $noun = 'Commits'; $selected = 'created'; break; @@ -269,7 +269,7 @@ final class PhabricatorSearchAttachController } private function getEdgeType($src_type, $dst_type) { - $t_cmit = PhabricatorPHIDConstants::PHID_TYPE_CMIT; + $t_cmit = PhabricatorRepositoryPHIDTypeCommit::TYPECONST; $t_task = PhabricatorPHIDConstants::PHID_TYPE_TASK; $t_drev = DifferentialPHIDTypeRevision::TYPECONST; $t_mock = PhabricatorPHIDConstants::PHID_TYPE_MOCK; diff --git a/src/applications/search/controller/PhabricatorSearchController.php b/src/applications/search/controller/PhabricatorSearchController.php index 7dffddabaf..5373c62cd3 100644 --- a/src/applications/search/controller/PhabricatorSearchController.php +++ b/src/applications/search/controller/PhabricatorSearchController.php @@ -63,7 +63,7 @@ final class PhabricatorSearchController case PhabricatorSearchScope::SCOPE_COMMITS: $query->setParameter( 'type', - PhabricatorPHIDConstants::PHID_TYPE_CMIT); + PhabricatorRepositoryPHIDTypeCommit::TYPECONST); break; default: break; diff --git a/src/applications/search/index/PhabricatorSearchAbstractDocument.php b/src/applications/search/index/PhabricatorSearchAbstractDocument.php index c6bdc906da..b4baec4860 100644 --- a/src/applications/search/index/PhabricatorSearchAbstractDocument.php +++ b/src/applications/search/index/PhabricatorSearchAbstractDocument.php @@ -16,7 +16,7 @@ final class PhabricatorSearchAbstractDocument { public static function getSupportedTypes() { return array( DifferentialPHIDTypeRevision::TYPECONST => 'Differential Revisions', - PhabricatorPHIDConstants::PHID_TYPE_CMIT => 'Repository Commits', + PhabricatorRepositoryPHIDTypeCommit::TYPECONST => 'Repository Commits', PhabricatorPHIDConstants::PHID_TYPE_TASK => 'Maniphest Tasks', PhabricatorPHIDConstants::PHID_TYPE_WIKI => 'Phriction Documents', PhabricatorPHIDConstants::PHID_TYPE_USER => 'Phabricator Users', diff --git a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php index 8b09f49648..0612c90c4c 100644 --- a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php +++ b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php @@ -155,7 +155,6 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { static $class_map = array( PhabricatorPHIDConstants::PHID_TYPE_TASK => 'ManiphestTask', - PhabricatorPHIDConstants::PHID_TYPE_CMIT => 'PhabricatorRepository', PhabricatorPHIDConstants::PHID_TYPE_FILE => 'PhabricatorFile', PhabricatorPHIDConstants::PHID_TYPE_USER => 'PhabricatorUser', PhabricatorPHIDConstants::PHID_TYPE_PROJ => 'PhabricatorProject',