From f33e2de092869f206d54037c7fd090429ec48c8d Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Thu, 1 Jan 2015 08:07:26 -0800 Subject: [PATCH] make repo callsigns optional Summary: Ref T4245 Make repo callsigns optional This is far from done and still very ugly. I'm just submitting it to check if i'm solving this in the right places. Right now there's three places with duplicate code and building the identifierMap in the CommitQuery is very ugly. If we only want to support this in the user frontend then i could hack it into the Markup rule itself and not touch the CommitQuery. Even uglier but more limited in scope... Generally this approach will need a lot of "check this first and then try the other" in a few places. I could move the Repository queries into a specialised PhabricatorRepositoryQuery method (withCallsignOrID) but i'm not sure about that. Test Plan: - phid.lookup works with R1 and rTEST (which is the same repo) - R1 and rTEST euqally work in remarkup (tested in comments). - Reviewed the following syntax also all works: rTEST rTESTd773137a7cb9 rTEST:d773137a7cb9 R1 R1:d773137a7cb9 d773137a7cb9 {rTEST} {rTESTd773137a7cb9} {rTEST:d773137a7cb9} {R1} {R1:d773137a7cb9} {d773137a7cb9} Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin, epriestley Maniphest Tasks: T4245 Differential Revision: https://secure.phabricator.com/D11050 --- src/__phutil_library_map__.php | 2 + .../PhabricatorDiffusionApplication.php | 3 +- .../diffusion/query/DiffusionCommitQuery.php | 60 +++++++----- .../remarkup/DiffusionCommitRemarkupRule.php | 1 - .../DiffusionRepositoryByIDRemarkupRule.php | 29 ++++++ .../DiffusionRepositoryRemarkupRule.php | 12 ++- .../DiffusionCommitRemarkupRuleTestCase.php | 73 ++++++++++++++ .../PhabricatorRepositoryCommitPHIDType.php | 4 +- ...habricatorRepositoryRepositoryPHIDType.php | 26 ++--- .../query/PhabricatorRepositoryQuery.php | 98 ++++++++++++++++++- 10 files changed, 262 insertions(+), 46 deletions(-) create mode 100644 src/applications/diffusion/remarkup/DiffusionRepositoryByIDRemarkupRule.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index c50eedb361..2583effa70 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -552,6 +552,7 @@ phutil_register_library_map(array( 'DiffusionRefNotFoundException' => 'applications/diffusion/exception/DiffusionRefNotFoundException.php', 'DiffusionRefsQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionRefsQueryConduitAPIMethod.php', 'DiffusionRenameHistoryQuery' => 'applications/diffusion/query/DiffusionRenameHistoryQuery.php', + 'DiffusionRepositoryByIDRemarkupRule' => 'applications/diffusion/remarkup/DiffusionRepositoryByIDRemarkupRule.php', 'DiffusionRepositoryController' => 'applications/diffusion/controller/DiffusionRepositoryController.php', 'DiffusionRepositoryCreateController' => 'applications/diffusion/controller/DiffusionRepositoryCreateController.php', 'DiffusionRepositoryDatasource' => 'applications/diffusion/typeahead/DiffusionRepositoryDatasource.php', @@ -3595,6 +3596,7 @@ phutil_register_library_map(array( 'DiffusionReadmeView' => 'DiffusionView', 'DiffusionRefNotFoundException' => 'Exception', 'DiffusionRefsQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', + 'DiffusionRepositoryByIDRemarkupRule' => 'PhabricatorObjectRemarkupRule', 'DiffusionRepositoryController' => 'DiffusionController', 'DiffusionRepositoryCreateController' => 'DiffusionRepositoryEditController', 'DiffusionRepositoryDatasource' => 'PhabricatorTypeaheadDatasource', diff --git a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php index 42460f5bf0..f41fa18d36 100644 --- a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php +++ b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php @@ -40,8 +40,9 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication { public function getRemarkupRules() { return array( - new DiffusionRepositoryRemarkupRule(), new DiffusionCommitRemarkupRule(), + new DiffusionRepositoryRemarkupRule(), + new DiffusionRepositoryByIDRemarkupRule(), ); } diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php index 38d7e8bfff..d63487c021 100644 --- a/src/applications/diffusion/query/DiffusionCommitQuery.php +++ b/src/applications/diffusion/query/DiffusionCommitQuery.php @@ -183,6 +183,9 @@ final class DiffusionCommitQuery ->withIDs($repository_ids) ->execute(); + $min_qualified = PhabricatorRepository::MINIMUM_QUALIFIED_HASH; + $result = array(); + foreach ($commits as $key => $commit) { $repo = idx($repos, $commit->getRepositoryID()); if ($repo) { @@ -190,36 +193,40 @@ final class DiffusionCommitQuery } else { unset($commits[$key]); } - } - 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(); + // Build the identifierMap + if ($this->identifiers !== null) { + $ids = array_fuse($this->identifiers); + $prefixes = array( + 'r'.$commit->getRepository()->getCallsign(), + 'r'.$commit->getRepository()->getCallsign().':', + 'R'.$commit->getRepository()->getID().':', + '', // No prefix is valid too and will only match the commitIdentifier + ); $suffix = $commit->getCommitIdentifier(); if ($commit->getRepository()->isSVN()) { - if (isset($ids[$prefix.$suffix])) { - $result[$prefix.$suffix][] = $commit; + foreach ($prefixes as $prefix) { + if (isset($ids[$prefix.$suffix])) { + $result[$prefix.$suffix][] = $commit; + } } } else { // This awkward construction 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 ($prefixes as $prefix) { + if (isset($ids[$prefix.$part])) { + $result[$prefix.$part][] = $commit; + } } } } } + } + if ($result) { foreach ($result as $identifier => $matching_commits) { if (count($matching_commits) == 1) { $result[$identifier] = head($matching_commits); @@ -229,7 +236,6 @@ final class DiffusionCommitQuery unset($result[$identifier]); } } - $this->identifierMap += $result; } @@ -327,9 +333,10 @@ final class DiffusionCommitQuery $bare = array(); foreach ($this->identifiers as $identifier) { $matches = null; - preg_match('/^(?:r([A-Z]+))?(.*)$/', $identifier, $matches); - $repo = nonempty($matches[1], null); - $identifier = nonempty($matches[2], null); + preg_match('/^(?:[rR]([A-Z]+:?|[0-9]+:))?(.*)$/', + $identifier, $matches); + $repo = nonempty(rtrim($matches[1], ':'), null); + $commit_identifier = nonempty($matches[2], null); if ($repo === null) { if ($this->defaultRepository) { @@ -338,14 +345,14 @@ final class DiffusionCommitQuery } if ($repo === null) { - if (strlen($identifier) < $min_unqualified) { + if (strlen($commit_identifier) < $min_unqualified) { continue; } - $bare[] = $identifier; + $bare[] = $commit_identifier; } else { $refs[] = array( 'callsign' => $repo, - 'identifier' => $identifier, + 'identifier' => $commit_identifier, ); } } @@ -362,14 +369,17 @@ final class DiffusionCommitQuery if ($refs) { $callsigns = ipull($refs, 'callsign'); + $repos = id(new PhabricatorRepositoryQuery()) ->setViewer($this->getViewer()) - ->withCallsigns($callsigns) - ->execute(); - $repos = mpull($repos, null, 'getCallsign'); + ->withIdentifiers($callsigns); + $repos->execute(); + + $repos = $repos->getIdentifierMap(); foreach ($refs as $key => $ref) { $repo = idx($repos, $ref['callsign']); + if (!$repo) { continue; } diff --git a/src/applications/diffusion/remarkup/DiffusionCommitRemarkupRule.php b/src/applications/diffusion/remarkup/DiffusionCommitRemarkupRule.php index a40060adcb..2722e9d819 100644 --- a/src/applications/diffusion/remarkup/DiffusionCommitRemarkupRule.php +++ b/src/applications/diffusion/remarkup/DiffusionCommitRemarkupRule.php @@ -22,7 +22,6 @@ final class DiffusionCommitRemarkupRule extends PhabricatorObjectRemarkupRule { ->withIdentifiers($ids); $query->execute(); - return $query->getIdentifierMap(); } diff --git a/src/applications/diffusion/remarkup/DiffusionRepositoryByIDRemarkupRule.php b/src/applications/diffusion/remarkup/DiffusionRepositoryByIDRemarkupRule.php new file mode 100644 index 0000000000..c5b76fd3a3 --- /dev/null +++ b/src/applications/diffusion/remarkup/DiffusionRepositoryByIDRemarkupRule.php @@ -0,0 +1,29 @@ +getEngine()->getConfig('viewer'); + + $repos = id(new PhabricatorRepositoryQuery()) + ->setViewer($viewer) + ->withIdentifiers($ids); + + $repos->execute(); + return $repos->getIdentifierMap(); + } + +} diff --git a/src/applications/diffusion/remarkup/DiffusionRepositoryRemarkupRule.php b/src/applications/diffusion/remarkup/DiffusionRepositoryRemarkupRule.php index 77652d4059..c845b64a46 100644 --- a/src/applications/diffusion/remarkup/DiffusionRepositoryRemarkupRule.php +++ b/src/applications/diffusion/remarkup/DiffusionRepositoryRemarkupRule.php @@ -11,15 +11,19 @@ final class DiffusionRepositoryRemarkupRule return '[A-Z]+'; } + public function getPriority() { + return 460.0; + } + protected function loadObjects(array $ids) { $viewer = $this->getEngine()->getConfig('viewer'); - $repositories = id(new PhabricatorRepositoryQuery()) + $repos = id(new PhabricatorRepositoryQuery()) ->setViewer($viewer) - ->withCallsigns($ids) - ->execute(); + ->withIdentifiers($ids); - return mpull($repositories, null, 'getCallsign'); + $repos->execute(); + return $repos->getIdentifierMap(); } } diff --git a/src/applications/diffusion/remarkup/__tests__/DiffusionCommitRemarkupRuleTestCase.php b/src/applications/diffusion/remarkup/__tests__/DiffusionCommitRemarkupRuleTestCase.php index b8476791be..450727bbe1 100644 --- a/src/applications/diffusion/remarkup/__tests__/DiffusionCommitRemarkupRuleTestCase.php +++ b/src/applications/diffusion/remarkup/__tests__/DiffusionCommitRemarkupRuleTestCase.php @@ -48,6 +48,79 @@ final class DiffusionCommitRemarkupRuleTestCase extends PhabricatorTestCase { ), ), ), + '{rP:1234 key=value}' => array( + 'embed' => array( + array( + 'offset' => 1, + 'id' => 'rP:1234', + 'tail' => ' key=value', + ), + ), + 'ref' => array( + array( + 'offset' => 1, + 'id' => 'rP:1234', + ), + ), + ), + '{R123:1234 key=value}' => array( + 'embed' => array( + array( + 'offset' => 1, + 'id' => 'R123:1234', + 'tail' => ' key=value', + ), + ), + 'ref' => array( + array( + 'offset' => 1, + 'id' => 'R123:1234', + ), + ), + ), + '{rP:12f3f6d3a9ef9c7731051815846810cb3c4cd248}' => array( + 'embed' => array( + array( + 'offset' => 1, + 'id' => 'rP:12f3f6d3a9ef9c7731051815846810cb3c4cd248', + ), + ), + 'ref' => array( + array( + 'offset' => 1, + 'id' => 'rP:12f3f6d3a9ef9c7731051815846810cb3c4cd248', + ), + ), + ), + '{R123:12f3f6d3a9ef9c7731051815846810cb3c4cd248}' => array( + 'embed' => array( + array( + 'offset' => 1, + 'id' => 'R123:12f3f6d3a9ef9c7731051815846810cb3c4cd248', + ), + ), + 'ref' => array( + array( + 'offset' => 1, + 'id' => 'R123:12f3f6d3a9ef9c7731051815846810cb3c4cd248', + ), + ), + ), + '{R123:12f3f6d3a9ef9c7731051815846810cb3c4cd248, key=value}' => array( + 'embed' => array( + array( + 'offset' => 1, + 'id' => 'R123:12f3f6d3a9ef9c7731051815846810cb3c4cd248', + 'tail' => ', key=value', + ), + ), + 'ref' => array( + array( + 'offset' => 1, + 'id' => 'R123:12f3f6d3a9ef9c7731051815846810cb3c4cd248', + ), + ), + ), ); foreach ($cases as $input => $expect) { diff --git a/src/applications/repository/phid/PhabricatorRepositoryCommitPHIDType.php b/src/applications/repository/phid/PhabricatorRepositoryCommitPHIDType.php index 75beddeef8..f49b63f102 100644 --- a/src/applications/repository/phid/PhabricatorRepositoryCommitPHIDType.php +++ b/src/applications/repository/phid/PhabricatorRepositoryCommitPHIDType.php @@ -54,9 +54,9 @@ final class PhabricatorRepositoryCommitPHIDType extends PhabricatorPHIDType { $min_qualified = PhabricatorRepository::MINIMUM_QUALIFIED_HASH; return - 'r[A-Z]+[1-9]\d*'. + '(?:r[A-Z]+:?|R[0-9]+:)[1-9]\d*'. '|'. - 'r[A-Z]+[a-f0-9]{'.$min_qualified.',40}'. + '(?:r[A-Z]+:?|R[0-9]+:)[a-f0-9]{'.$min_qualified.',40}'. '|'. '[a-f0-9]{'.$min_unqualified.',40}'; } diff --git a/src/applications/repository/phid/PhabricatorRepositoryRepositoryPHIDType.php b/src/applications/repository/phid/PhabricatorRepositoryRepositoryPHIDType.php index a98eb02f09..61da0cea0d 100644 --- a/src/applications/repository/phid/PhabricatorRepositoryRepositoryPHIDType.php +++ b/src/applications/repository/phid/PhabricatorRepositoryRepositoryPHIDType.php @@ -44,33 +44,35 @@ final class PhabricatorRepositoryRepositoryPHIDType } public function canLoadNamedObject($name) { - return preg_match('/^r[A-Z]+$/', $name); + return preg_match('/^r[A-Z]+|R[0-9]+$/', $name); } public function loadNamedObjects( PhabricatorObjectQuery $query, array $names) { + $results = array(); $id_map = array(); - foreach ($names as $name) { + foreach ($names as $key => $name) { $id = substr($name, 1); $id_map[$id][] = $name; + $names[$key] = substr($name, 1); } - $objects = id(new PhabricatorRepositoryQuery()) + $query = id(new PhabricatorRepositoryQuery()) ->setViewer($query->getViewer()) - ->withCallsigns(array_keys($id_map)) - ->execute(); + ->withIdentifiers($names); - $results = array(); - foreach ($objects as $object) { - $callsign = $object->getCallsign(); - foreach (idx($id_map, $callsign, array()) as $name) { - $results[$name] = $object; + if ($query->execute()) { + $objects = $query->getIdentifierMap(); + foreach ($objects as $key => $object) { + foreach (idx($id_map, $key, array()) as $name) + $results[$name] = $object; } + return $results; + } else { + return array(); } - - return $results; } } diff --git a/src/applications/repository/query/PhabricatorRepositoryQuery.php b/src/applications/repository/query/PhabricatorRepositoryQuery.php index d56540dc65..88d71e9972 100644 --- a/src/applications/repository/query/PhabricatorRepositoryQuery.php +++ b/src/applications/repository/query/PhabricatorRepositoryQuery.php @@ -12,6 +12,12 @@ final class PhabricatorRepositoryQuery private $remoteURIs; private $anyProjectPHIDs; + private $numericIdentifiers; + private $callsignIdentifiers; + private $phidIdentifiers; + + private $identifierMap; + const STATUS_OPEN = 'status-open'; const STATUS_CLOSED = 'status-closed'; const STATUS_ALL = 'status-all'; @@ -47,6 +53,27 @@ final class PhabricatorRepositoryQuery return $this; } + public function withIdentifiers(array $identifiers) { + $ids = array(); $callsigns = array(); $phids = array(); + foreach ($identifiers as $identifier) { + if (ctype_digit($identifier)) { + $ids[$identifier] = $identifier; + } else { + $repository_type = PhabricatorRepositoryRepositoryPHIDType::TYPECONST; + if (phid_get_type($identifier) === $repository_type) { + $phids[$identifier] = $identifier; + } else { + $callsigns[$identifier] = $identifier; + } + } + } + + $this->numericIdentifiers = $ids; + $this->callsignIdentifiers = $callsigns; + $this->phidIdentifiers = $phids; + return $this; + } + public function withStatus($status) { $this->status = $status; return $this; @@ -102,10 +129,22 @@ final class PhabricatorRepositoryQuery 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() { $table = new PhabricatorRepository(); $conn_r = $table->establishConnection('r'); + if ($this->identifierMap === null) { + $this->identifierMap = array(); + } + $data = queryfx_all( $conn_r, 'SELECT * FROM %T r %Q %Q %Q %Q', @@ -202,6 +241,35 @@ final class PhabricatorRepositoryQuery } } + // Build the identifierMap + if ($this->numericIdentifiers) { + foreach ($this->numericIdentifiers as $id) { + if (isset($repositories[$id])) { + $this->identifierMap[$id] = $repositories[$id]; + } + } + } + + if ($this->callsignIdentifiers) { + $repository_callsigns = mpull($repositories, null, 'getCallsign'); + + foreach ($this->callsignIdentifiers as $callsign) { + if (isset($repository_callsigns[$callsign])) { + $this->identifierMap[$callsign] = $repository_callsigns[$callsign]; + } + } + } + + if ($this->phidIdentifiers) { + $repository_phids = mpull($repositories, null, 'getPHID'); + + foreach ($this->phidIdentifiers as $phid) { + if (isset($repository_phids[$phid])) { + $this->identifierMap[$phid] = $repository_phids[$phid]; + } + } + } + return $repositories; } @@ -387,6 +455,35 @@ final class PhabricatorRepositoryQuery $this->callsigns); } + if ($this->numericIdentifiers || + $this->callsignIdentifiers || + $this->phidIdentifiers) { + $identifier_clause = array(); + + if ($this->numericIdentifiers) { + $identifier_clause[] = qsprintf( + $conn_r, + 'r.id IN (%Ld)', + $this->numericIdentifiers); + } + + if ($this->callsignIdentifiers) { + $identifier_clause[] = qsprintf( + $conn_r, + 'r.callsign IN (%Ls)', + $this->callsignIdentifiers); + } + + if ($this->phidIdentifiers) { + $identifier_clause[] = qsprintf( + $conn_r, + 'r.phid IN (%Ls)', + $this->phidIdentifiers); + } + + $where = array('('.implode(' OR ', $identifier_clause).')'); + } + if ($this->types) { $where[] = qsprintf( $conn_r, @@ -420,7 +517,6 @@ final class PhabricatorRepositoryQuery return $this->formatWhereClause($where); } - public function getQueryApplicationClass() { return 'PhabricatorDiffusionApplication'; }