From 958b00c01016dd243c6f2d9411bf86c73d0aaeb7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 8 Mar 2011 14:29:02 -0800 Subject: [PATCH] Diffusion: encapsulate request in DiffusionRequest Summary: Put an indirection layer between controllers and URI management, adding branches to git repositories. Test Plan: Looked at browse, history browse, file browse views, bad branches, bad commits Reviewed By: jwilson Reviewers: aran, jwilson CC: jwilson, epriestley Differential Revision: 65 --- src/__phutil_library_map__.php | 3 + .../controller/base/DiffusionController.php | 10 -- .../diffusion/controller/base/__init__.php | 1 - .../browse/DiffusionBrowseController.php | 43 +++---- .../diffusion/controller/browse/__init__.php | 3 +- .../file/DiffusionBrowseFileController.php | 27 +---- .../browse/base/DiffusionBrowseQuery.php | 29 ++--- .../browse/git/DiffusionGitBrowseQuery.php | 12 +- .../base/DiffusionFileContentQuery.php | 29 ++--- .../git/DiffusionGitFileContentQuery.php | 10 +- .../query/filecontent/git/__init__.php | 2 - .../request/base/DiffusionRequest.php | 102 +++++++++++++++++ .../diffusion/request/base/__init__.php | 15 +++ .../request/git/DiffusionGitRequest.php | 106 ++++++++++++++++++ .../diffusion/request/git/__init__.php | 15 +++ .../browsetable/DiffusionBrowseTableView.php | 19 +--- 16 files changed, 293 insertions(+), 133 deletions(-) create mode 100644 src/applications/diffusion/request/base/DiffusionRequest.php create mode 100644 src/applications/diffusion/request/base/__init__.php create mode 100644 src/applications/diffusion/request/git/DiffusionGitRequest.php create mode 100644 src/applications/diffusion/request/git/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index c401455052..6573b1230c 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -151,8 +151,10 @@ phutil_register_library_map(array( 'DiffusionFileContentQuery' => 'applications/diffusion/query/filecontent/base', 'DiffusionGitBrowseQuery' => 'applications/diffusion/query/browse/git', 'DiffusionGitFileContentQuery' => 'applications/diffusion/query/filecontent/git', + 'DiffusionGitRequest' => 'applications/diffusion/request/git', 'DiffusionHomeController' => 'applications/diffusion/controller/home', 'DiffusionRepositoryPath' => 'applications/diffusion/data/repositorypath', + 'DiffusionRequest' => 'applications/diffusion/request/base', 'Javelin' => 'infrastructure/javelin/api', 'LiskDAO' => 'storage/lisk/dao', 'ManiphestController' => 'applications/maniphest/controller/base', @@ -437,6 +439,7 @@ phutil_register_library_map(array( 'DiffusionController' => 'PhabricatorController', 'DiffusionGitBrowseQuery' => 'DiffusionBrowseQuery', 'DiffusionGitFileContentQuery' => 'DiffusionFileContentQuery', + 'DiffusionGitRequest' => 'DiffusionRequest', 'DiffusionHomeController' => 'DiffusionController', 'ManiphestController' => 'PhabricatorController', 'ManiphestDAO' => 'PhabricatorLiskDAO', diff --git a/src/applications/diffusion/controller/base/DiffusionController.php b/src/applications/diffusion/controller/base/DiffusionController.php index c0d46bf25c..3e07b1230b 100644 --- a/src/applications/diffusion/controller/base/DiffusionController.php +++ b/src/applications/diffusion/controller/base/DiffusionController.php @@ -32,14 +32,4 @@ abstract class DiffusionController extends PhabricatorController { return $response->setContent($page->render()); } - protected function loadRepositoryByCallsign($callsign) { - $repository = id(new PhabricatorRepository())->loadOneWhere( - 'callsign = %s', - $callsign); - if (!$repository) { - throw new Exception("No such repository '{$callsign}'."); - } - return $repository; - } - } diff --git a/src/applications/diffusion/controller/base/__init__.php b/src/applications/diffusion/controller/base/__init__.php index 22786d371f..cf7b058332 100644 --- a/src/applications/diffusion/controller/base/__init__.php +++ b/src/applications/diffusion/controller/base/__init__.php @@ -8,7 +8,6 @@ phutil_require_module('phabricator', 'aphront/response/webpage'); phutil_require_module('phabricator', 'applications/base/controller/base'); -phutil_require_module('phabricator', 'applications/repository/storage/repository'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/diffusion/controller/browse/DiffusionBrowseController.php b/src/applications/diffusion/controller/browse/DiffusionBrowseController.php index d24cfa29c6..4111071053 100644 --- a/src/applications/diffusion/controller/browse/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/browse/DiffusionBrowseController.php @@ -18,35 +18,22 @@ class DiffusionBrowseController extends DiffusionController { - private $callsign; - private $path; - private $line; - private $commit; - - private $repository; - public function willProcessRequest(array $data) { - $this->callsign = $data['callsign']; - $this->path = rtrim($data['path'], '/'); - - $this->line = idx($data, 'line'); - $this->commit = idx($data, 'commit'); + $this->diffusionRequest = DiffusionRequest::newFromAphrontRequestDictionary( + $data); } public function processRequest() { - $repository = $this->loadRepositoryByCallsign($this->callsign); + $drequest = $this->diffusionRequest; - $browse_data = DiffusionBrowseQuery::newFromRepository( - $repository, - $this->path, - $this->commit); - $results = $browse_data->loadPaths(); + $browse_query = DiffusionBrowseQuery::newFromDiffusionRequest($drequest); + $results = $browse_query->loadPaths(); $content = array(); if (!$results) { - switch ($browse_data->getReasonForEmptyResultSet()) { + switch ($browse_query->getReasonForEmptyResultSet()) { case DiffusionBrowseQuery::REASON_IS_NONEXISTENT: $title = 'Path Does Not Exist'; // TODO: Under git, this error message should be more specific. It @@ -56,9 +43,9 @@ class DiffusionBrowseController extends DiffusionController { break; case DiffusionBrowseQuery::REASON_IS_DELETED: // TODO: Format all these commits into nice VCS-agnostic links. - $commit = $this->commit; - $deleted = $browse_data->getDeletedAtCommit(); - $existed = $browse_data->getExistedAtCommit(); + $commit = $drequest->getCommit(); + $deleted = $browse_query->getDeletedAtCommit(); + $existed = $browse_query->getExistedAtCommit(); $title = 'Path Was Deleted'; $body = "This path does not exist at {$commit}. It was deleted in ". @@ -67,9 +54,7 @@ class DiffusionBrowseController extends DiffusionController { break; case DiffusionBrowseQuery::REASON_IS_FILE: $controller = new DiffusionBrowseFileController($this->getRequest()); - $controller->setRepository($repository); - $controller->setPath($this->path); - $controller->setCommit($this->commit); + $controller->setDiffusionRequest($drequest); return $this->delegateToController($controller); break; default: @@ -85,13 +70,11 @@ class DiffusionBrowseController extends DiffusionController { } else { $browse_table = new DiffusionBrowseTableView(); - $browse_table->setRepository($repository); + $browse_table->setDiffusionRequest($drequest); $browse_table->setPaths($results); - $browse_table->setRoot($this->path); - $browse_table->setCommit($this->commit); $browse_panel = new AphrontPanelView(); - $browse_panel->setHeader($this->path); + $browse_panel->setHeader($drequest->getPath()); $browse_panel->appendChild($browse_table); $content[] = $browse_panel; @@ -105,7 +88,7 @@ class DiffusionBrowseController extends DiffusionController { return $this->buildStandardPageResponse( $content, array( - 'title' => basename($this->path), + 'title' => basename($drequest->getPath()), )); } diff --git a/src/applications/diffusion/controller/browse/__init__.php b/src/applications/diffusion/controller/browse/__init__.php index 1ee1f7f36b..39dd2990e2 100644 --- a/src/applications/diffusion/controller/browse/__init__.php +++ b/src/applications/diffusion/controller/browse/__init__.php @@ -9,11 +9,10 @@ phutil_require_module('phabricator', 'applications/diffusion/controller/base'); phutil_require_module('phabricator', 'applications/diffusion/controller/file'); phutil_require_module('phabricator', 'applications/diffusion/query/browse/base'); +phutil_require_module('phabricator', 'applications/diffusion/request/base'); phutil_require_module('phabricator', 'applications/diffusion/view/browsetable'); phutil_require_module('phabricator', 'view/form/error'); phutil_require_module('phabricator', 'view/layout/panel'); -phutil_require_module('phutil', 'utils'); - phutil_require_source('DiffusionBrowseController.php'); diff --git a/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php index 6a605b72c1..87f79b7803 100644 --- a/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php +++ b/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php @@ -18,33 +18,16 @@ class DiffusionBrowseFileController extends DiffusionController { - private $callsign; - private $path; - private $line; - private $commit; - private $view; + private $diffusionRequest; - private $repository; - - public function setRepository(DiffusionRepository $repository) { - $this->repository = $repository; - } - - public function setCommit($commit) { - $this->commit = $commit; - return $this; - } - - public function setPath($path) { - $this->path = $path; + public function setDiffusionRequest(DiffusionRequest $request) { + $this->diffusionRequest = $request; return $this; } public function processRequest() { - $file_query = DiffusionFileContentQuery::newFromRepository( - $this->repository, - $this->path, - $this->commit); + $file_query = DiffusionFileContentQuery::newFromDiffusionRequest( + $this->diffusionRequest); $file_content = $file_query->loadFileContent(); $corpus = phutil_render_tag( diff --git a/src/applications/diffusion/query/browse/base/DiffusionBrowseQuery.php b/src/applications/diffusion/query/browse/base/DiffusionBrowseQuery.php index 0a0c4ba567..6b11312b0f 100644 --- a/src/applications/diffusion/query/browse/base/DiffusionBrowseQuery.php +++ b/src/applications/diffusion/query/browse/base/DiffusionBrowseQuery.php @@ -18,9 +18,7 @@ abstract class DiffusionBrowseQuery { - private $repository; - private $path; - private $commit; + private $request; protected $reason; protected $existedAtCommit; @@ -29,15 +27,16 @@ abstract class DiffusionBrowseQuery { const REASON_IS_FILE = 'is-file'; const REASON_IS_DELETED = 'is-deleted'; const REASON_IS_NONEXISTENT = 'nonexistent'; + const REASON_BAD_COMMIT = 'bad-commit'; final private function __construct() { // } - final public static function newFromRepository( - PhabricatorRepository $repository, - $path = '/', - $commit = null) { + final public static function newFromDiffusionRequest( + DiffusionRequest $request) { + + $repository = $request->getRepository(); switch ($repository->getVersionControlSystem()) { case 'git': @@ -51,23 +50,13 @@ abstract class DiffusionBrowseQuery { PhutilSymbolLoader::loadClass($class); $query = new $class(); - $query->repository = $repository; - $query->path = $path; - $query->commit = $commit; + $query->request = $request; return $query; } - final protected function getRepository() { - return $this->repository; - } - - final protected function getPath() { - return $this->path; - } - - final protected function getCommit() { - return $this->commit; + final protected function getRequest() { + return $this->request; } final public function getReasonForEmptyResultSet() { diff --git a/src/applications/diffusion/query/browse/git/DiffusionGitBrowseQuery.php b/src/applications/diffusion/query/browse/git/DiffusionGitBrowseQuery.php index c68fe6bf55..92064cc416 100644 --- a/src/applications/diffusion/query/browse/git/DiffusionGitBrowseQuery.php +++ b/src/applications/diffusion/query/browse/git/DiffusionGitBrowseQuery.php @@ -19,12 +19,13 @@ final class DiffusionGitBrowseQuery extends DiffusionBrowseQuery { protected function executeQuery() { - $repository = $this->getRepository(); - $path = $this->getPath(); - $commit = nonempty($this->getCommit(), 'HEAD'); + $drequest = $this->getRequest(); + $repository = $drequest->getRepository(); + + $path = $drequest->getPath(); + $commit = $drequest->getCommit(); $local_path = $repository->getDetail('local-path'); - $git = PhabricatorEnv::getEnvConfig('git.path'); try { @@ -35,7 +36,8 @@ final class DiffusionGitBrowseQuery extends DiffusionBrowseQuery { $commit, $path); } catch (CommandException $e) { - if (preg_match('/^fatal: Not a valid object name/', $e->getStderr())) { + $stderr = $e->getStdErr(); + if (preg_match('/^fatal: Not a valid object name/', $stderr)) { // Grab two logs, since the first one is when the object was deleted. list($stdout) = execx( '(cd %s && %s log -n2 --format="%%H" %s -- %s)', diff --git a/src/applications/diffusion/query/filecontent/base/DiffusionFileContentQuery.php b/src/applications/diffusion/query/filecontent/base/DiffusionFileContentQuery.php index b2f41b94a3..efab915a49 100644 --- a/src/applications/diffusion/query/filecontent/base/DiffusionFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/base/DiffusionFileContentQuery.php @@ -18,18 +18,16 @@ abstract class DiffusionFileContentQuery { - private $repository; - private $path; - private $commit; + private $request; final private function __construct() { // } - final public static function newFromRepository( - PhabricatorRepository $repository, - $path = '/', - $commit = null) { + final public static function newFromDiffusionRequest( + DiffusionRequest $request) { + + $repository = $request->getRepository(); switch ($repository->getVersionControlSystem()) { case 'git': @@ -42,26 +40,15 @@ abstract class DiffusionFileContentQuery { PhutilSymbolLoader::loadClass($class); $query = new $class(); - $query->repository = $repository; - $query->path = $path; - $query->commit = $commit; + $query->request = $request; return $query; } - final protected function getRepository() { - return $this->repository; + final protected function getRequest() { + return $this->request; } - final protected function getPath() { - return $this->path; - } - - final protected function getCommit() { - return $this->commit; - } - - final public function loadFileContent() { return $this->executeQuery(); } diff --git a/src/applications/diffusion/query/filecontent/git/DiffusionGitFileContentQuery.php b/src/applications/diffusion/query/filecontent/git/DiffusionGitFileContentQuery.php index 38a635bb87..b1f5895694 100644 --- a/src/applications/diffusion/query/filecontent/git/DiffusionGitFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/git/DiffusionGitFileContentQuery.php @@ -19,12 +19,14 @@ final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery { protected function executeQuery() { - $repository = $this->getRepository(); - $path = $this->getPath(); - $commit = nonempty($this->getCommit(), 'HEAD'); + $drequest = $this->getRequest(); + + $repository = $drequest->getRepository(); + $path = $drequest->getPath(); + $commit = $drequest->getCommit(); $local_path = $repository->getDetail('local-path'); - $git = PhabricatorEnv::getEnvConfig('git.path'); + $git = $drequest->getPathToGitBinary(); list($corpus) = execx( '(cd %s && %s cat-file blob %s:%s)', diff --git a/src/applications/diffusion/query/filecontent/git/__init__.php b/src/applications/diffusion/query/filecontent/git/__init__.php index 03cf4d9473..364b4c0c9e 100644 --- a/src/applications/diffusion/query/filecontent/git/__init__.php +++ b/src/applications/diffusion/query/filecontent/git/__init__.php @@ -8,10 +8,8 @@ phutil_require_module('phabricator', 'applications/diffusion/data/filecontent'); phutil_require_module('phabricator', 'applications/diffusion/query/filecontent/base'); -phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phutil', 'future/exec'); -phutil_require_module('phutil', 'utils'); phutil_require_source('DiffusionGitFileContentQuery.php'); diff --git a/src/applications/diffusion/request/base/DiffusionRequest.php b/src/applications/diffusion/request/base/DiffusionRequest.php new file mode 100644 index 0000000000..de84896e53 --- /dev/null +++ b/src/applications/diffusion/request/base/DiffusionRequest.php @@ -0,0 +1,102 @@ + + } + + final public static function newFromAphrontRequestDictionary(array $data) { + + $vcs = null; + $repository = null; + $callsign = idx($data, 'callsign'); + if ($callsign) { + $repository = id(new PhabricatorRepository())->loadOneWhere( + 'callsign = %s', + $callsign); + if (!$repository) { + throw new Exception("No such repository '{$callsign}'."); + } + $vcs = $repository->getVersionControlSystem(); + } + + switch ($vcs) { + case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: + $class = 'DiffusionGitRequest'; + break; + default: + $class = 'DiffusionRequest'; + break; + } + + $object = new $class(); + + $object->callsign = $callsign; + $object->repository = $repository; + $object->line = idx($data, 'line'); + $object->commit = idx($data, 'commit'); + $object->path = idx($data, 'path'); + + $object->initializeFromAphrontRequestDictionary(); + + return $object; + } + + protected function initializeFromAphrontRequestDictionary() { + + } + + protected function parsePath($path) { + $this->path = $path; + } + + public function getRepository() { + return $this->repository; + } + + public function getCallsign() { + return $this->callsign; + } + + public function getPath() { + return $this->path; + } + + public function getLine() { + return $this->line; + } + + public function getCommit() { + return $this->commit; + } + + public function getBranch() { + return $this->branch; + } + +} diff --git a/src/applications/diffusion/request/base/__init__.php b/src/applications/diffusion/request/base/__init__.php new file mode 100644 index 0000000000..cdb31cde66 --- /dev/null +++ b/src/applications/diffusion/request/base/__init__.php @@ -0,0 +1,15 @@ +path; + $parts = explode('/', $path); + + $branch = array_shift($parts); + $this->branch = $this->decodeBranchName($branch); + + $this->path = implode('/', $parts); + + if ($this->repository) { + $local_path = $this->repository->getDetail('local-path'); + $git = $this->getPathToGitBinary(); + + // TODO: This is not terribly efficient and does not produce terribly + // good error messages, but it seems better to put error handling code + // here than to try to do it in every query. + + $branch = $this->getBranch(); + execx( + '(cd %s && %s rev-parse --verify %s)', + $local_path, + $git, + $branch); + + if ($this->commit) { + execx( + '(cd %s && %s rev-parse --verify %s)', + $local_path, + $git, + $this->commit); + list($contains) = execx( + '(cd %s && %s branch --contains %s)', + $local_path, + $git, + $this->commit); + $contains = array_filter(explode("\n", $contains)); + $found = false; + foreach ($contains as $containing_branch) { + $containing_branch = trim($containing_branch, "* \n"); + if ($containing_branch == $branch) { + $found = true; + break; + } + } + if (!$found) { + throw new Exception( + "Commit does not exist on this branch!"); + } + } + } + + + } + + public function getPathToGitBinary() { + return PhabricatorEnv::getEnvConfig('git.path'); + } + + public function getBranch() { + if ($this->branch) { + return $this->branch; + } + if ($this->repository) { + return $this->repository->getDetail('default-branch', 'master'); + } + throw new Exception("Unable to determine branch!"); + } + + public function getCommit() { + if ($this->commit) { + return $this->commit; + } + return $this->getBranch(); + } + + private function decodeBranchName($branch) { + return str_replace(':', '/', $branch); + } + + private function encodeBranchName($branch) { + return str_replace('/', ':', $branch); + } + +} diff --git a/src/applications/diffusion/request/git/__init__.php b/src/applications/diffusion/request/git/__init__.php new file mode 100644 index 0000000000..c9a2ab51e0 --- /dev/null +++ b/src/applications/diffusion/request/git/__init__.php @@ -0,0 +1,15 @@ +repository = $repository; + public function setDiffusionRequest(DiffusionRequest $request) { + $this->request = $request; return $this; } @@ -33,16 +30,6 @@ final class DiffusionBrowseTableView extends AphrontView { return $this; } - public function setRoot($root) { - $this->root = $root; - return $this; - } - - public function setCommit($commit) { - $this->commit = $commit; - return $this; - } - public function render() { $rows = array(); foreach ($this->paths as $path) {