From 347252fda85a597d3c8de9cb5161aa33869dfb44 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 13 May 2014 13:52:48 -0700 Subject: [PATCH] Improve clarity of commit and symbol handling in DiffusionRequest Summary: Ref T2683. Currently, DiffusionRequest has four different "commitey" things: - `commit` - `rawCommit` - `symbolicCommit` - `stableCommit` Of these, only two are actually distinct, useful values: `symbolicCommit` (which holds the value the request originally contained, if one existed) and `stableCommit` (which resolves that value, or the value implied by its omission, into a stable, permanent commit identifier). - `rawCommit` is equivalent to `symbolicCommit` and can be simply removed. - `commit` has some sketchy magic around it that needs to be pulled out before it can be jettisoned. Test Plan: Viewed SVN, Git, and Mercurial repositories. Viewed brwose/history/change/tag/branch/etc views. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T2683 Differential Revision: https://secure.phabricator.com/D9098 --- .../ConduitAPI_diffusion_tagsquery_Method.php | 2 +- .../controller/DiffusionBrowseController.php | 2 +- .../controller/DiffusionChangeController.php | 4 +- .../controller/DiffusionCommitController.php | 2 +- .../controller/DiffusionController.php | 10 +- .../controller/DiffusionHistoryController.php | 2 +- .../controller/DiffusionTagListController.php | 7 +- .../diffusion/request/DiffusionGitRequest.php | 19 +-- .../request/DiffusionMercurialRequest.php | 26 +--- .../diffusion/request/DiffusionRequest.php | 137 +++++++++++------- .../diffusion/request/DiffusionSvnRequest.php | 15 +- 11 files changed, 112 insertions(+), 114 deletions(-) diff --git a/src/applications/diffusion/conduit/ConduitAPI_diffusion_tagsquery_Method.php b/src/applications/diffusion/conduit/ConduitAPI_diffusion_tagsquery_Method.php index 474a26b55f..aeed037aec 100644 --- a/src/applications/diffusion/conduit/ConduitAPI_diffusion_tagsquery_Method.php +++ b/src/applications/diffusion/conduit/ConduitAPI_diffusion_tagsquery_Method.php @@ -24,7 +24,7 @@ final class ConduitAPI_diffusion_tagsquery_Method protected function getGitResult(ConduitAPIRequest $request) { $drequest = $this->getDiffusionRequest(); $repository = $drequest->getRepository(); - $commit = $drequest->getRawCommit(); + $commit = $drequest->getSymbolicCommit(); $commit_filter = null; if ($commit) { diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index 64cf276be6..8f89aaa7cb 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -96,7 +96,7 @@ abstract class DiffusionBrowseController extends DiffusionController { ->setHref($history_uri) ->setIcon('fa-list')); - $behind_head = $drequest->getRawCommit(); + $behind_head = $drequest->getSymbolicCommit(); $head_uri = $drequest->generateURI( array( 'commit' => '', diff --git a/src/applications/diffusion/controller/DiffusionChangeController.php b/src/applications/diffusion/controller/DiffusionChangeController.php index c3b5dc22b5..01de42dead 100644 --- a/src/applications/diffusion/controller/DiffusionChangeController.php +++ b/src/applications/diffusion/controller/DiffusionChangeController.php @@ -31,7 +31,7 @@ final class DiffusionChangeController extends DiffusionController { $repository = $drequest->getRepository(); $callsign = $repository->getCallsign(); - $commit = $drequest->getRawCommit(); + $commit = $drequest->getSymbolicCommit(); $changesets = array( 0 => $changeset, ); @@ -53,7 +53,7 @@ final class DiffusionChangeController extends DiffusionController { ); $right_uri = $drequest->generateURI($raw_params); - $raw_params['params']['before'] = $drequest->getRawCommit(); + $raw_params['params']['before'] = $drequest->getStableCommit(); $left_uri = $drequest->generateURI($raw_params); $changeset_view->setRawFileURIs($left_uri, $right_uri); diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 8dd9e898b4..425c6f0d20 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -21,6 +21,7 @@ final class DiffusionCommitController extends DiffusionController { public function processRequest() { $drequest = $this->getDiffusionRequest(); + $request = $this->getRequest(); $user = $request->getUser(); @@ -32,7 +33,6 @@ final class DiffusionCommitController extends DiffusionController { $callsign = $repository->getCallsign(); $content = array(); - $commit = id(new DiffusionCommitQuery()) ->setViewer($request->getUser()) ->withRepository($repository) diff --git a/src/applications/diffusion/controller/DiffusionController.php b/src/applications/diffusion/controller/DiffusionController.php index ab80ff3c3a..2a76156a5d 100644 --- a/src/applications/diffusion/controller/DiffusionController.php +++ b/src/applications/diffusion/controller/DiffusionController.php @@ -98,17 +98,17 @@ abstract class DiffusionController extends PhabricatorController { ))); $crumb_list[] = $crumb; - $raw_commit = $drequest->getRawCommit(); + $stable_commit = $drequest->getStableCommit(); if ($spec['tags']) { $crumb = new PhabricatorCrumbView(); if ($spec['commit']) { $crumb->setName( - pht("Tags for %s", 'r'.$callsign.$raw_commit)); + pht("Tags for %s", 'r'.$callsign.$stable_commit)); $crumb->setHref($drequest->generateURI( array( 'action' => 'commit', - 'commit' => $raw_commit, + 'commit' => $drequest->getStableCommit(), ))); } else { $crumb->setName(pht('Tags')); @@ -126,8 +126,8 @@ abstract class DiffusionController extends PhabricatorController { if ($spec['commit']) { $crumb = id(new PhabricatorCrumbView()) - ->setName("r{$callsign}{$raw_commit}") - ->setHref("r{$callsign}{$raw_commit}"); + ->setName("r{$callsign}{$stable_commit}") + ->setHref("r{$callsign}{$stable_commit}"); $crumb_list[] = $crumb; return $crumb_list; } diff --git a/src/applications/diffusion/controller/DiffusionHistoryController.php b/src/applications/diffusion/controller/DiffusionHistoryController.php index 9010dc9f02..91e4479acf 100644 --- a/src/applications/diffusion/controller/DiffusionHistoryController.php +++ b/src/applications/diffusion/controller/DiffusionHistoryController.php @@ -152,7 +152,7 @@ final class DiffusionHistoryController extends DiffusionController { ->setUser($viewer) ->setActionList($actions); - $stable_commit = $drequest->getStableCommit(); + $stable_commit = $drequest->getStableCommitName(); $callsign = $drequest->getRepository()->getCallsign(); $view->addProperty( diff --git a/src/applications/diffusion/controller/DiffusionTagListController.php b/src/applications/diffusion/controller/DiffusionTagListController.php index b6e0710630..aee4c0d1ad 100644 --- a/src/applications/diffusion/controller/DiffusionTagListController.php +++ b/src/applications/diffusion/controller/DiffusionTagListController.php @@ -20,9 +20,10 @@ final class DiffusionTagListController extends DiffusionController { $params = array( 'limit' => $pager->getPageSize() + 1, 'offset' => $pager->getOffset()); - if ($drequest->getRawCommit()) { + + if ($drequest->getSymbolicCommit()) { $is_commit = true; - $params['commit'] = $drequest->getRawCommit(); + $params['commit'] = $drequest->getSymbolicCommit(); } else { $is_commit = false; } @@ -76,7 +77,7 @@ final class DiffusionTagListController extends DiffusionController { $crumbs = $this->buildCrumbs( array( 'tags' => true, - 'commit' => $drequest->getRawCommit(), + 'commit' => $drequest->getSymbolicCommit(), )); return $this->buildApplicationPage( diff --git a/src/applications/diffusion/request/DiffusionGitRequest.php b/src/applications/diffusion/request/DiffusionGitRequest.php index 584ab3acf3..f42f7a57bb 100644 --- a/src/applications/diffusion/request/DiffusionGitRequest.php +++ b/src/applications/diffusion/request/DiffusionGitRequest.php @@ -1,20 +1,13 @@ commit) { - return; - } - - $this->expandCommitName(); + protected function isStableCommit($symbol) { + return preg_match('/^[a-f0-9]{40}\z/', $symbol); } public function getBranch() { @@ -27,14 +20,6 @@ final class DiffusionGitRequest extends DiffusionRequest { throw new Exception("Unable to determine branch!"); } - public function getCommit() { - if ($this->commit) { - return $this->commit; - } - - return $this->getResolvableBranchName($this->getBranch()); - } - protected function getResolvableBranchName($branch) { if ($this->repository->isWorkingCopyBare()) { return $branch; diff --git a/src/applications/diffusion/request/DiffusionMercurialRequest.php b/src/applications/diffusion/request/DiffusionMercurialRequest.php index b82c237479..b76d0597d6 100644 --- a/src/applications/diffusion/request/DiffusionMercurialRequest.php +++ b/src/applications/diffusion/request/DiffusionMercurialRequest.php @@ -1,27 +1,14 @@ commit) { - return; - } - - if (strlen($this->commit) == 40) { - return; - } - - $this->expandCommitName(); - } + protected function isStableCommit($symbol) { + return preg_match('/^[a-f0-9]{40}\z/', $symbol); + } public function getBranch() { if ($this->branch) { @@ -35,11 +22,4 @@ final class DiffusionMercurialRequest extends DiffusionRequest { throw new Exception("Unable to determine branch!"); } - public function getCommit() { - if ($this->commit) { - return $this->commit; - } - return $this->getBranch(); - } - } diff --git a/src/applications/diffusion/request/DiffusionRequest.php b/src/applications/diffusion/request/DiffusionRequest.php index dd20d7380e..65b717716c 100644 --- a/src/applications/diffusion/request/DiffusionRequest.php +++ b/src/applications/diffusion/request/DiffusionRequest.php @@ -29,7 +29,11 @@ abstract class DiffusionRequest { private $user; abstract protected function getSupportsBranches(); - abstract protected function didInitialize(); + abstract protected function isStableCommit($symbol); + + protected function didInitialize() { + return null; + } /* -( Creating Requests )-------------------------------------------------- */ @@ -130,6 +134,7 @@ abstract class DiffusionRequest { ->setViewer($viewer) ->withCallsigns(array($callsign)) ->executeOne(); + if (!$repository) { throw new Exception("No such repository '{$callsign}'."); } @@ -179,11 +184,10 @@ abstract class DiffusionRequest { */ final private function initializeFromDictionary(array $data) { $this->path = idx($data, 'path'); - $this->symbolicCommit = idx($data, 'commit'); - $this->commit = idx($data, 'commit'); $this->line = idx($data, 'line'); $this->initFromConduit = idx($data, 'initFromConduit', true); + $this->symbolicCommit = idx($data, 'commit'); if ($this->getSupportsBranches()) { $this->branch = idx($data, 'branch'); } @@ -234,17 +238,85 @@ abstract class DiffusionRequest { } public function getCommit() { - return $this->commit; + + // TODO: Probably remove all of this. + + // Required for sketchy sins that `diffusion.diffquery` commits. + if ($this->commit) { + return $this->commit; + } + + if ($this->getSymbolicCommit() !== null) { + return $this->getSymbolicCommit(); + } + + return $this->getStableCommit(); } + /** + * Get the symbolic commit associated with this request. + * + * A symbolic commit may be a commit hash, an abbreviated commit hash, a + * branch name, a tag name, or an expression like "HEAD^^^". The symbolic + * commit may also be absent. + * + * This method always returns the symbol present in the original request, + * in unmodified form. + * + * See also @{method:getStableCommit}. + * + * @return string|null Symbolic commit, if one was present in the request. + */ public function getSymbolicCommit() { return $this->symbolicCommit; } + + /** + * Get the ref type (`commit` or `tag`) of the location associated with this + * request. + * + * If a symbolic commit is present in the request, this method identifies + * the type of the symbol. Otherwise, it identifies the type of symbol of + * the location the request is implicitly associated with. This will probably + * always be `commit`. + * + * @return string Symbolic commit type (`commit` or `tag`). + */ public function getSymbolicType() { + if ($this->symbolicType === null) { + // As a side effect, this resolves the symbolic type. + $this->getStableCommit(); + } return $this->symbolicType; } + + /** + * Retrieve the stable, permanent commit name identifying the repository + * location associated with this request. + * + * This returns a non-symbolic identifier for the current commit: in Git and + * Mercurial, a 40-character SHA1; in SVN, a revision number. + * + * See also @{method:getSymbolicCommit}. + * + * @return string Stable commit name, like a git hash or SVN revision. Not + * a symbolic commit reference. + */ + public function getStableCommit() { + if (!$this->stableCommit) { + if ($this->isStableCommit($this->symbolicCommit)) { + $this->stableCommit = $this->symbolicCommit; + $this->symbolicType = 'commit'; + } else { + $this->queryStableCommit(); + } + } + return $this->stableCommit; + } + + public function getBranch() { return $this->branch; } @@ -308,26 +380,6 @@ abstract class DiffusionRequest { return $this->repositoryCommitData; } - /** - * Retrieve a stable, permanent commit name. This returns a non-symbolic - * identifier for the current commit: e.g., a specific commit hash in git - * (NOT a symbolic name like "origin/master") or a specific revision number - * in SVN (NOT a symbolic name like "HEAD"). - * - * @return string Stable commit name, like a git hash or SVN revision. Not - * a symbolic commit reference. - */ - public function getStableCommit() { - if (!$this->stableCommit) { - $this->queryStableCommit(); - } - return $this->stableCommit; - } - - final public function getRawCommit() { - return $this->commit; - } - public function setCommit($commit) { $this->commit = $commit; return $this; @@ -347,7 +399,7 @@ abstract class DiffusionRequest { */ public function generateURI(array $params) { if (empty($params['stable'])) { - $default_commit = $this->getRawCommit(); + $default_commit = $this->getSymbolicCommit(); } else { $default_commit = $this->getStableCommit(); } @@ -620,30 +672,15 @@ abstract class DiffusionRequest { "Guide' in the documentation for help setting up repositories."); } - final protected function expandCommitName() { - $results = $this->resolveRefs(array($this->commit)); - $matches = idx($results, $this->commit, array()); - if (count($results) !== 1) { - throw new Exception( - pht('Ref "%s" is ambiguous or does not exist.', $this->commit)); - } - - $match = head($matches); - - $this->commit = $match['identifier']; - $this->symbolicType = $match['type']; - } - private function queryStableCommit() { - if ($this->commit) { - $this->stableCommit = $this->commit; - return $this->stableCommit; - } - - if ($this->getSupportsBranches()) { - $ref = $this->getResolvableBranchName($this->getBranch()); + if ($this->symbolicCommit) { + $ref = $this->symbolicCommit; } else { - $ref = 'HEAD'; + if ($this->getSupportsBranches()) { + $ref = $this->getResolvableBranchName($this->getBranch()); + } else { + $ref = 'HEAD'; + } } $results = $this->resolveRefs(array($ref)); @@ -655,8 +692,10 @@ abstract class DiffusionRequest { ->setRef($ref); } - $this->stableCommit = idx(head($matches), 'identifier'); - return $this->stableCommit; + $match = head($matches); + + $this->stableCommit = $match['identifier']; + $this->symbolicType = $match['type']; } protected function getResolvableBranchName($branch) { diff --git a/src/applications/diffusion/request/DiffusionSvnRequest.php b/src/applications/diffusion/request/DiffusionSvnRequest.php index 85020c6c63..10d75d60bd 100644 --- a/src/applications/diffusion/request/DiffusionSvnRequest.php +++ b/src/applications/diffusion/request/DiffusionSvnRequest.php @@ -1,14 +1,15 @@ path === null) { $subpath = $this->repository->getDetail('svn-subpath'); @@ -22,12 +23,4 @@ final class DiffusionSvnRequest extends DiffusionRequest { return 'svn'; } - public function getCommit() { - if ($this->commit) { - return $this->commit; - } - - return $this->getStableCommit(); - } - }