From 824f9346227938bf59889ebd446fa8baf3369e3d Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Tue, 21 May 2013 16:22:49 -0700 Subject: [PATCH] Diffusion - move commit parents query to conduit Summary: Ref T2784. Relatively complicated one as this bad boy is used in a repository daemon. While testing, I noticed bugs in the expandshortname query stuff. Those variables are private to the parent class so they need some setX love. Also, was unable to find links to the "before" stuff, but made them by hand by looking at some of these T2784 diffs, browsing a file at a specific revision, then hacking the "before" variable to be some known commit that also touched the file. This produced sensical results. On the process of doing that I upgraded a query to use the proper policy query. Test Plan: In git, mercurial, svn, verified on a commit page the "parents" showed up correctly. played around with ?before parameter on specific file browse page, with commits known to have interesting history and stuff looked good Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin Maniphest Tasks: T2784 Differential Revision: https://secure.phabricator.com/D5988 --- src/__phutil_library_map__.php | 2 ++ ...PI_diffusion_commitparentsquery_Method.php | 31 +++++++++++++++++++ .../DiffusionBrowseFileController.php | 12 +++++-- .../controller/DiffusionCommitController.php | 7 +++-- .../query/DiffusionRenameHistoryQuery.php | 15 ++++++--- .../DiffusionExpandShortNameQuery.php | 9 ++++++ .../DiffusionGitExpandShortNameQuery.php | 10 +++--- ...DiffusionMercurialExpandShortNameQuery.php | 2 +- 8 files changed, 72 insertions(+), 16 deletions(-) create mode 100644 src/applications/diffusion/conduit/ConduitAPI_diffusion_commitparentsquery_Method.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 859bcf7f6c..d3efbcca59 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -148,6 +148,7 @@ phutil_register_library_map(array( 'ConduitAPI_diffusion_branchquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_branchquery_Method.php', 'ConduitAPI_diffusion_browsequery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_browsequery_Method.php', 'ConduitAPI_diffusion_commitbranchesquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_commitbranchesquery_Method.php', + 'ConduitAPI_diffusion_commitparentsquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_commitparentsquery_Method.php', 'ConduitAPI_diffusion_diffquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_diffquery_Method.php', 'ConduitAPI_diffusion_existsquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_existsquery_Method.php', 'ConduitAPI_diffusion_expandshortcommitquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_expandshortcommitquery_Method.php', @@ -1957,6 +1958,7 @@ phutil_register_library_map(array( 'ConduitAPI_diffusion_branchquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method', 'ConduitAPI_diffusion_browsequery_Method' => 'ConduitAPI_diffusion_abstractquery_Method', 'ConduitAPI_diffusion_commitbranchesquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method', + 'ConduitAPI_diffusion_commitparentsquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method', 'ConduitAPI_diffusion_diffquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method', 'ConduitAPI_diffusion_existsquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method', 'ConduitAPI_diffusion_expandshortcommitquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method', diff --git a/src/applications/diffusion/conduit/ConduitAPI_diffusion_commitparentsquery_Method.php b/src/applications/diffusion/conduit/ConduitAPI_diffusion_commitparentsquery_Method.php new file mode 100644 index 0000000000..f0315e46f2 --- /dev/null +++ b/src/applications/diffusion/conduit/ConduitAPI_diffusion_commitparentsquery_Method.php @@ -0,0 +1,31 @@ + 'required string', + ); + } + + protected function getResult(ConduitAPIRequest $request) { + $drequest = $this->getDiffusionRequest(); + + $query = DiffusionCommitParentsQuery::newFromDiffusionRequest($drequest); + $parents = $query->loadParents(); + return $parents; + } +} diff --git a/src/applications/diffusion/controller/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/DiffusionBrowseFileController.php index c177f4918f..28f6015fa8 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseFileController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseFileController.php @@ -866,6 +866,7 @@ final class DiffusionBrowseFileController extends DiffusionController { $rename_query = new DiffusionRenameHistoryQuery(); $rename_query->setRequest($drequest); $rename_query->setOldCommit($grandparent->getCommitIdentifier()); + $rename_query->setViewer($request->getUser()); $old_filename = $rename_query->loadOldFilename(); $was_created = $rename_query->getWasCreated(); } @@ -954,16 +955,21 @@ final class DiffusionBrowseFileController extends DiffusionController { private function loadParentRevisionOf($commit) { $drequest = $this->getDiffusionRequest(); + $user = $this->getRequest()->getUser(); $before_req = DiffusionRequest::newFromDictionary( array( - 'user' => $this->getRequest()->getUser(), + 'user' => $user, 'repository' => $drequest->getRepository(), 'commit' => $commit, )); - $query = DiffusionCommitParentsQuery::newFromDiffusionRequest($before_req); - $parents = $query->loadParents(); + $parents = DiffusionQuery::callConduitWithDiffusionRequest( + $user, + $before_req, + 'diffusion.commitparentsquery', + array( + 'commit' => $commit)); return head($parents); } diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 5190e07221..ab781afa3e 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -74,8 +74,9 @@ final class DiffusionCommitController extends DiffusionController { require_celerity_resource('diffusion-commit-view-css'); require_celerity_resource('phabricator-remarkup-css'); - $parent_query = DiffusionCommitParentsQuery::newFromDiffusionRequest( - $drequest); + $parents = $this->callConduitWithDiffusionRequest( + 'diffusion.commitparentsquery', + array('commit' => $drequest->getCommit())); $headsup_view = id(new PhabricatorHeaderView()) ->setHeader(nonempty($commit->getSummary(), pht('Commit Detail'))); @@ -85,7 +86,7 @@ final class DiffusionCommitController extends DiffusionController { $commit_properties = $this->loadCommitProperties( $commit, $commit_data, - $parent_query->loadParents()); + $parents); $property_list = id(new PhabricatorPropertyListView()) ->setHasKeyboardShortcuts(true) ->setUser($user) diff --git a/src/applications/diffusion/query/DiffusionRenameHistoryQuery.php b/src/applications/diffusion/query/DiffusionRenameHistoryQuery.php index 8145d73db8..62d293319e 100644 --- a/src/applications/diffusion/query/DiffusionRenameHistoryQuery.php +++ b/src/applications/diffusion/query/DiffusionRenameHistoryQuery.php @@ -5,6 +5,12 @@ final class DiffusionRenameHistoryQuery { private $oldCommit; private $wasCreated; private $request; + private $viewer; + + public function setViewer(PhabricatorUser $viewer) { + $this->viewer = $viewer; + return $this; + } public function getWasCreated() { return $this->wasCreated; @@ -72,10 +78,11 @@ final class DiffusionRenameHistoryQuery { } private function loadCommitId($commit_identifier) { - $commit = id(new PhabricatorRepositoryCommit())->loadOneWhere( - 'repositoryID = %d AND commitIdentifier = %s', - $this->request->getRepository()->getID(), - $commit_identifier); + $commit = id(new DiffusionCommitQuery()) + ->setViewer($this->viewer) + ->withIdentifiers(array($commit_identifier)) + ->withDefaultRepository($this->request->getRepository()) + ->executeOne(); return $commit->getID(); } diff --git a/src/applications/diffusion/query/expandshortname/DiffusionExpandShortNameQuery.php b/src/applications/diffusion/query/expandshortname/DiffusionExpandShortNameQuery.php index 1413d6450f..d8034b57d6 100644 --- a/src/applications/diffusion/query/expandshortname/DiffusionExpandShortNameQuery.php +++ b/src/applications/diffusion/query/expandshortname/DiffusionExpandShortNameQuery.php @@ -22,6 +22,15 @@ abstract class DiffusionExpandShortNameQuery extends DiffusionQuery { return $this->repository; } + protected function setCommitType($type) { + $this->commitType = $type; + return $this; + } + protected function setTagContent($content) { + $this->tagContent = $content; + return $this; + } + final public static function newFromRepository( PhabricatorRepository $repository) { diff --git a/src/applications/diffusion/query/expandshortname/DiffusionGitExpandShortNameQuery.php b/src/applications/diffusion/query/expandshortname/DiffusionGitExpandShortNameQuery.php index 96694e613f..f000be2f6e 100644 --- a/src/applications/diffusion/query/expandshortname/DiffusionGitExpandShortNameQuery.php +++ b/src/applications/diffusion/query/expandshortname/DiffusionGitExpandShortNameQuery.php @@ -16,12 +16,12 @@ extends DiffusionExpandShortNameQuery { if ($type == 'missing') { throw new DiffusionExpandCommitQueryException( DiffusionExpandCommitQueryException::CODE_MISSING, - "Bad commit '{$this->commit}'."); + "Bad commit '{$commit}'."); } switch ($type) { case 'tag': - $this->commitType = 'tag'; + $this->setCommitType('tag'); $matches = null; $ok = preg_match( @@ -35,18 +35,18 @@ extends DiffusionExpandShortNameQuery { } $hash = $matches[1]; - $this->tagContent = trim($matches[2]); + $this->setTagContent(trim($matches[2])); break; case 'commit': break; default: throw new DiffusionExpandCommitQueryException( DiffusionExpandCommitQueryException::CODE_INVALID, - "The reference '{$this->commit}' does not name a valid ". + "The reference '{$commit}' does not name a valid ". 'commit or a tag in this repository.'); break; } - $this->commit = $hash; + $this->setCommit($hash); } } diff --git a/src/applications/diffusion/query/expandshortname/DiffusionMercurialExpandShortNameQuery.php b/src/applications/diffusion/query/expandshortname/DiffusionMercurialExpandShortNameQuery.php index 6d1d5ed404..7448cf9118 100644 --- a/src/applications/diffusion/query/expandshortname/DiffusionMercurialExpandShortNameQuery.php +++ b/src/applications/diffusion/query/expandshortname/DiffusionMercurialExpandShortNameQuery.php @@ -17,7 +17,7 @@ extends DiffusionExpandShortNameQuery { // TODO: Show "multiple matching commits" if count is larger than 1. For // now, pick the first one. - $this->commit = head($full_hash); + $this->setCommit(head($full_hash)); } }