From 6b8da8c34794710dedf608d9a048ed152c967fe7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 18 May 2011 07:44:53 -0700 Subject: [PATCH] Fix Diffusion line number links in Git Summary: When you click a line number link in Git from a branch tip, it takes you to "...;origin/master$..." which (a) doesn't work and (b) doesn't permanently reference the line. Link to the "stable commit name" instead. Also fix a few other bugs/warnings/layout things. Test Plan: Clicked line number links in Git and SVN repositories, browsed around stuff, checked error log. Reviewed By: jungejason Reviewers: jungejason, tuomaspelkonen, aran CC: aran, epriestley, jungejason Differential Revision: 303 --- .../file/DiffusionBrowseFileController.php | 25 +++++---- .../browse/git/DiffusionGitBrowseQuery.php | 52 +++++++++++-------- .../request/base/DiffusionRequest.php | 14 +++++ .../request/git/DiffusionGitRequest.php | 10 +++- .../request/svn/DiffusionSvnRequest.php | 32 ++++++++---- .../diffusion/view/base/DiffusionView.php | 2 +- .../diffusion/diffusion-source.css | 8 +++ 7 files changed, 100 insertions(+), 43 deletions(-) diff --git a/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php index 6ced8bf77b..84e572b186 100644 --- a/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php +++ b/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php @@ -54,17 +54,20 @@ class DiffusionBrowseFileController extends DiffusionController { } $select .= ''; + require_celerity_resource('diffusion-source-css'); + $view_select_panel = new AphrontPanelView(); $view_select_form = phutil_render_tag( 'form', array( 'action' => $request->getRequestURI(), 'method' => 'get', - 'style' => 'display: inline', + 'class' => 'diffusion-browse-type-form', ), $select. - ''); + ''); $view_select_panel->appendChild($view_select_form); + $view_select_panel->appendChild('
'); // Build the content of the file. $corpus = $this->buildCorpus($selected); @@ -102,7 +105,7 @@ class DiffusionBrowseFileController extends DiffusionController { */ public function getImageType($path) { $ext = pathinfo($path); - $ext = $ext['extension']; + $ext = idx($ext, 'extension'); return idx($this->imageTypes, $ext); } @@ -173,7 +176,6 @@ class DiffusionBrowseFileController extends DiffusionController { case 'blame': default: require_celerity_resource('syntax-highlighting-css'); - require_celerity_resource('diffusion-source-css'); list($text_list, $rev_list, $blame_dict) = $file_query->getBlameData(); @@ -215,10 +217,14 @@ class DiffusionBrowseFileController extends DiffusionController { $rows = array(); $n = 1; - $epoch_list = ipull($blame_dict, 'epoch'); - $max = max($epoch_list); - $min = min($epoch_list); - $range = $max - $min + 1; + if ($blame_dict) { + $epoch_list = ipull($blame_dict, 'epoch'); + $max = max($epoch_list); + $min = min($epoch_list); + $range = $max - $min + 1; + } else { + $range = 1; + } foreach ($text_list as $k => $line) { if ($needs_blame) { @@ -284,11 +290,12 @@ class DiffusionBrowseFileController extends DiffusionController { // Create the row display. $uri_path = $drequest->getUriPath(); - $uri_rev = $drequest->getCommit(); + $uri_rev = $drequest->getStableCommitName(); $l = phutil_render_tag( 'a', array( + 'class' => 'diffusion-line-link', 'href' => $uri_path.';'.$uri_rev.'$'.$n, ), $n); diff --git a/src/applications/diffusion/query/browse/git/DiffusionGitBrowseQuery.php b/src/applications/diffusion/query/browse/git/DiffusionGitBrowseQuery.php index a044b022b8..02c25fee5c 100644 --- a/src/applications/diffusion/query/browse/git/DiffusionGitBrowseQuery.php +++ b/src/applications/diffusion/query/browse/git/DiffusionGitBrowseQuery.php @@ -27,34 +27,40 @@ final class DiffusionGitBrowseQuery extends DiffusionBrowseQuery { $local_path = $repository->getDetail('local-path'); - try { - list($stdout) = execx( - "(cd %s && git cat-file -t %s:%s)", - $local_path, - $commit, - $path); - } catch (CommandException $e) { - $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. + if ($path == '') { + // Fast path to improve the performance of the repository view; we know + // the root is always a tree at any commit and always exists. + $stdout = 'tree'; + } else { + try { list($stdout) = execx( - '(cd %s && git log -n2 --format="%%H" %s -- %s)', + "(cd %s && git cat-file -t %s:%s)", $local_path, $commit, $path); - $stdout = trim($stdout); - if ($stdout) { - $commits = explode("\n", $stdout); - $this->reason = self::REASON_IS_DELETED; - $this->deletedAtCommit = idx($commits, 0); - $this->existedAtCommit = idx($commits, 1); - return array(); - } + } catch (CommandException $e) { + $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 && git log -n2 --format="%%H" %s -- %s)', + $local_path, + $commit, + $path); + $stdout = trim($stdout); + if ($stdout) { + $commits = explode("\n", $stdout); + $this->reason = self::REASON_IS_DELETED; + $this->deletedAtCommit = idx($commits, 0); + $this->existedAtCommit = idx($commits, 1); + return array(); + } - $this->reason = self::REASON_IS_NONEXISTENT; - return array(); - } else { - throw $e; + $this->reason = self::REASON_IS_NONEXISTENT; + return array(); + } else { + throw $e; + } } } diff --git a/src/applications/diffusion/request/base/DiffusionRequest.php b/src/applications/diffusion/request/base/DiffusionRequest.php index 4498d136c1..d28a875050 100644 --- a/src/applications/diffusion/request/base/DiffusionRequest.php +++ b/src/applications/diffusion/request/base/DiffusionRequest.php @@ -27,6 +27,7 @@ class DiffusionRequest { protected $repository; protected $repositoryCommit; protected $repositoryCommitData; + protected $stableCommitName; final private function __construct() { // @@ -136,6 +137,19 @@ 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 getStableCommitName() { + return $this->stableCommitName; + } + final public function getRawCommit() { return $this->commit; } diff --git a/src/applications/diffusion/request/git/DiffusionGitRequest.php b/src/applications/diffusion/request/git/DiffusionGitRequest.php index 46d32d6655..4206daf0ca 100644 --- a/src/applications/diffusion/request/git/DiffusionGitRequest.php +++ b/src/applications/diffusion/request/git/DiffusionGitRequest.php @@ -52,7 +52,7 @@ class DiffusionGitRequest extends DiffusionRequest { // message to indicate whether they've typed in some bogus branch and/or // followed a bad link, or misconfigured the default branch in the // Repository tool. - execx( + list($this->stableCommitName) = execx( '(cd %s && git rev-parse --verify %s)', $local_path, $branch); @@ -67,6 +67,10 @@ class DiffusionGitRequest extends DiffusionRequest { // sha1s. $this->commit = trim($commit); + // If we have a commit, overwrite the branch commit with the more + // specific commit. + $this->stableCommitName = $this->commit; + /* TODO: Unclear if this is actually a good idea or not; it breaks commit views @@ -119,6 +123,10 @@ class DiffusionGitRequest extends DiffusionRequest { return $this->getBranch(); } + public function getStableCommitName() { + return substr($this->stableCommitName, 0, 16); + } + public function getBranchURIComponent($branch) { return $this->encodeBranchName($branch).'/'; } diff --git a/src/applications/diffusion/request/svn/DiffusionSvnRequest.php b/src/applications/diffusion/request/svn/DiffusionSvnRequest.php index dea97cb9f4..577e36c64e 100644 --- a/src/applications/diffusion/request/svn/DiffusionSvnRequest.php +++ b/src/applications/diffusion/request/svn/DiffusionSvnRequest.php @@ -18,8 +18,6 @@ class DiffusionSvnRequest extends DiffusionRequest { - private $loadedCommit; - protected function initializeFromAphrontRequestDictionary(array $data) { parent::initializeFromAphrontRequestDictionary($data); if (!strncmp($this->path, ':', 1)) { @@ -28,18 +26,34 @@ class DiffusionSvnRequest extends DiffusionRequest { } } + public function getStableCommitName() { + if ($this->commit) { + return $this->commit; + } + + if ($this->stableCommitName === null) { + $commit = id(new PhabricatorRepositoryCommit()) + ->loadOneWhere( + 'repositoryID = %d ORDER BY epoch DESC LIMIT 1', + $this->getRepository()->getID()); + if ($commit) { + $this->stableCommitName = $commit->getCommitIdentifier(); + } else { + // For new repositories, we may not have parsed any commits yet. Call + // the stable commit "1" and avoid fataling. + $this->stableCommitName = 1; + } + } + + return $this->stableCommitName; + } + public function getCommit() { if ($this->commit) { return $this->commit; } - if (!$this->loadedCommit) { - $this->loadedCommit = id(new PhabricatorRepositoryCommit())->loadOneWhere( - 'repositoryID = %d ORDER BY epoch DESC LIMIT 1', - $this->getRepository()->getID())->getCommitIdentifier(); - } - - return $this->loadedCommit; + return $this->getStableCommitName(); } } diff --git a/src/applications/diffusion/view/base/DiffusionView.php b/src/applications/diffusion/view/base/DiffusionView.php index 3cb5c8bc35..05a2101d49 100644 --- a/src/applications/diffusion/view/base/DiffusionView.php +++ b/src/applications/diffusion/view/base/DiffusionView.php @@ -124,7 +124,7 @@ abstract class DiffusionView extends AphrontView { switch ($repository->getVersionControlSystem()) { case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: - $commit_name = substr($commit, 0, 7); + $commit_name = substr($commit, 0, 16); break; default: $commit_name = $commit; diff --git a/webroot/rsrc/css/application/diffusion/diffusion-source.css b/webroot/rsrc/css/application/diffusion/diffusion-source.css index b1d0682fcc..da03a9f467 100644 --- a/webroot/rsrc/css/application/diffusion/diffusion-source.css +++ b/webroot/rsrc/css/application/diffusion/diffusion-source.css @@ -37,3 +37,11 @@ overflow: hidden; } +.diffusion-line-link { + /* Give the user a larger click target. */ + display: block; +} + +.diffusion-browse-type-form { + float: right; +}