From fe78944c9df749199dd5ef64886c46633f1030a5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 27 Feb 2013 08:04:54 -0800 Subject: [PATCH] Prepare Diffusion for hovercards Summary: Move Diffusion to be hovercard-ready, and expand our ability to resolve commit references. - Link unqualified hashes of 7 characters or more which match a commit. - Link qualified hashes of 5 characters or more which match a commit. - Support `{...}` syntax. Test Plan: {F33896} Reviewers: chad, vrana Reviewed By: vrana CC: aran Differential Revision: https://secure.phabricator.com/D5121 --- resources/sql/patches/20130226.commitkey.sql | 5 + src/__phutil_library_map__.php | 12 +- .../PhabricatorApplicationDiffusion.php | 6 + .../diffusion/query/DiffusionCommitQuery.php | 140 ++++++++++++++++++ .../remarkup/DiffusionRemarkupRule.php | 74 ++++++++- .../storage/PhabricatorRepository.php | 10 ++ .../storage/PhabricatorRepositoryCommit.php | 35 ++++- .../markup/PhabricatorMarkupEngine.php | 5 +- .../rule/PhabricatorRemarkupRuleObject.php | 10 +- .../PhabricatorRemarkupRuleObjectName.php | 51 ------- .../patch/PhabricatorBuiltinPatchList.php | 4 + 11 files changed, 283 insertions(+), 69 deletions(-) create mode 100644 resources/sql/patches/20130226.commitkey.sql create mode 100644 src/applications/diffusion/query/DiffusionCommitQuery.php delete mode 100644 src/infrastructure/markup/rule/PhabricatorRemarkupRuleObjectName.php diff --git a/resources/sql/patches/20130226.commitkey.sql b/resources/sql/patches/20130226.commitkey.sql new file mode 100644 index 0000000000..795dc39ae4 --- /dev/null +++ b/resources/sql/patches/20130226.commitkey.sql @@ -0,0 +1,5 @@ +ALTER TABLE {$NAMESPACE}_repository.repository_commit + DROP KEY `repositoryID`; + +ALTER TABLE {$NAMESPACE}_repository.repository_commit + ADD UNIQUE KEY `key_commit_identity` (commitIdentifier, repositoryID); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a97804d2df..e00cbb07ba 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -374,6 +374,7 @@ phutil_register_library_map(array( 'DiffusionCommitController' => 'applications/diffusion/controller/DiffusionCommitController.php', 'DiffusionCommitEditController' => 'applications/diffusion/controller/DiffusionCommitEditController.php', 'DiffusionCommitParentsQuery' => 'applications/diffusion/query/parents/DiffusionCommitParentsQuery.php', + 'DiffusionCommitQuery' => 'applications/diffusion/query/DiffusionCommitQuery.php', 'DiffusionCommitTagsController' => 'applications/diffusion/controller/DiffusionCommitTagsController.php', 'DiffusionCommitTagsQuery' => 'applications/diffusion/query/committags/DiffusionCommitTagsQuery.php', 'DiffusionContainsQuery' => 'applications/diffusion/query/contains/DiffusionContainsQuery.php', @@ -1186,7 +1187,6 @@ phutil_register_library_map(array( 'PhabricatorRemarkupRuleMeme' => 'applications/macro/remarkup/PhabricatorRemarkupRuleMeme.php', 'PhabricatorRemarkupRuleMention' => 'applications/people/remarkup/PhabricatorRemarkupRuleMention.php', 'PhabricatorRemarkupRuleObject' => 'infrastructure/markup/rule/PhabricatorRemarkupRuleObject.php', - 'PhabricatorRemarkupRuleObjectName' => 'infrastructure/markup/rule/PhabricatorRemarkupRuleObjectName.php', 'PhabricatorRemarkupRuleYoutube' => 'infrastructure/markup/rule/PhabricatorRemarkupRuleYoutube.php', 'PhabricatorRepository' => 'applications/repository/storage/PhabricatorRepository.php', 'PhabricatorRepositoryArcanistProject' => 'applications/repository/storage/PhabricatorRepositoryArcanistProject.php', @@ -1898,6 +1898,7 @@ phutil_register_library_map(array( 'DiffusionCommitController' => 'DiffusionController', 'DiffusionCommitEditController' => 'DiffusionController', 'DiffusionCommitParentsQuery' => 'DiffusionQuery', + 'DiffusionCommitQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'DiffusionCommitTagsController' => 'DiffusionController', 'DiffusionCommitTagsQuery' => 'DiffusionQuery', 'DiffusionContainsQuery' => 'DiffusionQuery', @@ -1953,7 +1954,7 @@ phutil_register_library_map(array( 'DiffusionPathValidateController' => 'DiffusionController', 'DiffusionPeopleMenuEventListener' => 'PhutilEventListener', 'DiffusionRawDiffQuery' => 'DiffusionQuery', - 'DiffusionRemarkupRule' => 'PhabricatorRemarkupRuleObjectName', + 'DiffusionRemarkupRule' => 'PhabricatorRemarkupRuleObject', 'DiffusionRepositoryController' => 'DiffusionController', 'DiffusionSetupException' => 'AphrontUsageException', 'DiffusionSvnBrowseQuery' => 'DiffusionBrowseQuery', @@ -2661,7 +2662,6 @@ phutil_register_library_map(array( 'PhabricatorRemarkupRuleMeme' => 'PhutilRemarkupRule', 'PhabricatorRemarkupRuleMention' => 'PhutilRemarkupRule', 'PhabricatorRemarkupRuleObject' => 'PhutilRemarkupRule', - 'PhabricatorRemarkupRuleObjectName' => 'PhutilRemarkupRule', 'PhabricatorRemarkupRuleYoutube' => 'PhutilRemarkupRule', 'PhabricatorRepository' => array( @@ -2673,7 +2673,11 @@ phutil_register_library_map(array( 'PhabricatorRepositoryArcanistProjectEditController' => 'PhabricatorRepositoryController', 'PhabricatorRepositoryAuditRequest' => 'PhabricatorRepositoryDAO', 'PhabricatorRepositoryBranch' => 'PhabricatorRepositoryDAO', - 'PhabricatorRepositoryCommit' => 'PhabricatorRepositoryDAO', + 'PhabricatorRepositoryCommit' => + array( + 0 => 'PhabricatorRepositoryDAO', + 1 => 'PhabricatorPolicyInterface', + ), 'PhabricatorRepositoryCommitChangeParserWorker' => 'PhabricatorRepositoryCommitParserWorker', 'PhabricatorRepositoryCommitData' => 'PhabricatorRepositoryDAO', 'PhabricatorRepositoryCommitHeraldWorker' => 'PhabricatorRepositoryCommitParserWorker', diff --git a/src/applications/diffusion/application/PhabricatorApplicationDiffusion.php b/src/applications/diffusion/application/PhabricatorApplicationDiffusion.php index 588c179d15..8db6c43d6a 100644 --- a/src/applications/diffusion/application/PhabricatorApplicationDiffusion.php +++ b/src/applications/diffusion/application/PhabricatorApplicationDiffusion.php @@ -30,6 +30,12 @@ final class PhabricatorApplicationDiffusion extends PhabricatorApplication { ); } + public function getRemarkupRules() { + return array( + new DiffusionRemarkupRule(), + ); + } + public function getRoutes() { return array( '/r(?P[A-Z]+)(?P[a-z0-9]+)' diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php new file mode 100644 index 0000000000..a7f40496ee --- /dev/null +++ b/src/applications/diffusion/query/DiffusionCommitQuery.php @@ -0,0 +1,140 @@ +identifiers = $identifiers; + return $this; + } + + public function loadPage() { + $table = new PhabricatorRepositoryCommit(); + $conn_r = $table->establishConnection('r'); + + $data = queryfx_all( + $conn_r, + 'SELECT * FROM %T %Q %Q %Q', + $table->getTableName(), + $this->buildWhereClause($conn_r), + $this->buildOrderClause($conn_r), + $this->buildLimitClause($conn_r)); + + return $table->loadAllFromArray($data); + } + + public function willFilterPage(array $commits) { + if (!$commits) { + return array(); + } + + $repository_ids = mpull($commits, 'getRepositoryID', 'getRepositoryID'); + $repos = id(new PhabricatorRepositoryQuery()) + ->setViewer($this->getViewer()) + ->withIDs($repository_ids) + ->execute(); + + foreach ($commits as $key => $commit) { + $repo = idx($repos, $commit->getRepositoryID()); + if ($repo) { + $commit->attachRepository($repo); + } else { + unset($commits[$key]); + } + } + + return $commits; + } + + private function buildWhereClause(AphrontDatabaseConnection $conn_r) { + $where = array(); + + if ($this->identifiers) { + $min_unqualified = PhabricatorRepository::MINIMUM_UNQUALIFIED_HASH; + $min_qualified = PhabricatorRepository::MINIMUM_QUALIFIED_HASH; + + $refs = array(); + $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); + + if ($repo === null) { + if (strlen($identifier) < $min_unqualified) { + continue; + } + $bare[] = $identifier; + } else { + $refs[] = array( + 'callsign' => $repo, + 'identifier' => $identifier, + ); + } + } + + $sql = array(); + + foreach ($bare as $identifier) { + $sql[] = qsprintf( + $conn_r, + '(commitIdentifier LIKE %> AND LENGTH(commitIdentifier) = 40)', + $identifier); + } + + if ($refs) { + $callsigns = ipull($refs, 'callsign'); + $repos = id(new PhabricatorRepositoryQuery()) + ->setViewer($this->getViewer()) + ->withCallsigns($callsigns) + ->execute(); + $repos = mpull($repos, null, 'getCallsign'); + + foreach ($refs as $key => $ref) { + $repo = idx($repos, $ref['callsign']); + if (!$repo) { + continue; + } + + if ($repo->isSVN()) { + $sql[] = qsprintf( + $conn_r, + '(repositoryID = %d AND commitIdentifier = %d)', + $repo->getID(), + $ref['identifier']); + } else { + if (strlen($ref['identifier']) < $min_qualified) { + continue; + } + $sql[] = qsprintf( + $conn_r, + '(repositoryID = %d AND commitIdentifier LIKE %>)', + $repo->getID(), + $ref['identifier']); + } + } + } + + if ($sql) { + $where[] = '('.implode(' OR ', $sql).')'; + } else { + // If we discarded all possible identifiers (e.g., they all referenced + // bogus repositories or were all too short), make sure the query finds + // nothing. + $where[] = qsprintf($conn_r, '1 = 0'); + } + } + + return $this->formatWhereClause($where); + } + +} diff --git a/src/applications/diffusion/remarkup/DiffusionRemarkupRule.php b/src/applications/diffusion/remarkup/DiffusionRemarkupRule.php index 058bb30bc0..8e3d4da871 100644 --- a/src/applications/diffusion/remarkup/DiffusionRemarkupRule.php +++ b/src/applications/diffusion/remarkup/DiffusionRemarkupRule.php @@ -1,17 +1,79 @@ getEngine()->getConfig('viewer'); + $min_qualified = PhabricatorRepository::MINIMUM_QUALIFIED_HASH; + + if (!$viewer) { + return array(); + } + + $commits = id(new DiffusionCommitQuery()) + ->setViewer($viewer) + ->withIdentifiers($ids) + ->execute(); + + if (!$commits) { + return array(); + } + + $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; } } diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 1b2a772178..b6644a453a 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -6,6 +6,16 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO implements PhabricatorPolicyInterface { + /** + * Shortest hash we'll recognize in raw "a829f32" form. + */ + const MINIMUM_UNQUALIFIED_HASH = 7; + + /** + * Shortest hash we'll recognize in qualified "rXab7ef2f8" form. + */ + const MINIMUM_QUALIFIED_HASH = 5; + const TABLE_PATH = 'repository_path'; const TABLE_PATHCHANGE = 'repository_pathchange'; const TABLE_FILESYSTEM = 'repository_filesystem'; diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index 2b24e835d6..b35df5596f 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -1,6 +1,8 @@ repository = $repository; + return $this; + } + + public function getRepository() { + if ($this->repository === null) { + throw new Exception("Call attachRepository() before getRepository()!"); + } + return $this->repository; + } public function setIsUnparsed($is_unparsed) { $this->isUnparsed = $is_unparsed; @@ -138,4 +153,22 @@ final class PhabricatorRepositoryCommit extends PhabricatorRepositoryDAO { return $this->setAuditStatus($status); } + + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + public function getCapabilities() { + return array( + PhabricatorPolicyCapability::CAN_VIEW, + ); + } + + public function getPolicy($capability) { + return $this->getRepository()->getPolicy($capability); + } + + public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + return $this->getRepository()->hasAutomaticCapability($capability, $viewer); + } + } diff --git a/src/infrastructure/markup/PhabricatorMarkupEngine.php b/src/infrastructure/markup/PhabricatorMarkupEngine.php index d95b4b308d..e7db410e22 100644 --- a/src/infrastructure/markup/PhabricatorMarkupEngine.php +++ b/src/infrastructure/markup/PhabricatorMarkupEngine.php @@ -41,7 +41,7 @@ final class PhabricatorMarkupEngine { private $objects = array(); private $viewer; - private $version = 4; + private $version = 5; /* -( Markup Pipeline )---------------------------------------------------- */ @@ -401,9 +401,6 @@ final class PhabricatorMarkupEngine { $rules[] = new PhrictionRemarkupRule(); $rules[] = new PhabricatorRemarkupRuleEmbedFile(); - - $rules[] = new DiffusionRemarkupRule(); - $rules[] = new PhabricatorCountdownRemarkupRule(); $applications = PhabricatorApplication::getAllInstalledApplications(); diff --git a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleObject.php b/src/infrastructure/markup/rule/PhabricatorRemarkupRuleObject.php index 74dc9ec151..38084b7dac 100644 --- a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleObject.php +++ b/src/infrastructure/markup/rule/PhabricatorRemarkupRuleObject.php @@ -36,9 +36,9 @@ abstract class PhabricatorRemarkupRuleObject return $result; } - protected function renderObjectRef($object, $handle, $anchor) { + protected function renderObjectRef($object, $handle, $anchor, $id) { $href = $handle->getURI(); - $text = $this->getObjectNamePrefix().$object->getID(); + $text = $this->getObjectNamePrefix().$id; if ($anchor) { $matches = null; if (preg_match('@^#(?:comment-)?(\d{1,7})$@', $anchor, $matches)) { @@ -172,7 +172,11 @@ abstract class PhabricatorRemarkupRuleObject $object = $objects[$spec['id']]; switch ($spec['type']) { case 'ref': - $view = $this->renderObjectRef($object, $handle, $spec['anchor']); + $view = $this->renderObjectRef( + $object, + $handle, + $spec['anchor'], + $spec['id']); break; case 'embed': $view = $this->renderObjectEmbed($object, $handle, $spec['options']); diff --git a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleObjectName.php b/src/infrastructure/markup/rule/PhabricatorRemarkupRuleObjectName.php deleted file mode 100644 index b81c37cc2f..0000000000 --- a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleObjectName.php +++ /dev/null @@ -1,51 +0,0 @@ -getObjectNamePrefix(); - $id = $this->getObjectIDPattern(); - return preg_replace_callback( - "@\b({$prefix})({$id})(?:#([-\w\d]+))?\b@", - array($this, 'markupObjectNameLink'), - $text); - } - - public function markupObjectNameLink($matches) { - list(, $prefix, $id) = $matches; - - if (isset($matches[3])) { - $href = $matches[3]; - $text = $matches[3]; - if (preg_match('@^(?:comment-)?(\d{1,7})$@', $href, $matches)) { - // Maximum length is 7 because 12345678 could be a file hash. - $href = "comment-{$matches[1]}"; - $text = $matches[1]; - } - $href = "/{$prefix}{$id}#{$href}"; - $text = "{$prefix}{$id}#{$text}"; - } else { - $href = "/{$prefix}{$id}"; - $text = "{$prefix}{$id}"; - } - - return $this->getEngine()->storeText( - phutil_tag( - 'a', - array( - 'href' => $href, - ), - $text)); - } - -} diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index 16c31ea6ac..858ce30ad0 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1153,6 +1153,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('20130222.dropchannel.sql'), ), + '20130226.commitkey.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('20130226.commitkey.sql'), + ), ); }