From b848ab87b3d5b303e444743730cc76051bfbae86 Mon Sep 17 00:00:00 2001 From: epriestley <git@epriestley.com> Date: Mon, 11 Jan 2016 02:49:04 -0800 Subject: [PATCH] Introduce "local names" for commits Summary: Ref T4245. Full commit display names (like `rPaaaa`) are going to be obnoxious soon in some cases (e.g., `rPaaaa` becomes `R123:aaaa`, which is much uglier) so reduce how often we show the repository in cases where it isn't really necessary to include it. Test Plan: - Saw no more `rX` on repository list view for Git/Mercurial (still present for Subversion). - Saw no more `rX` on various repository detail views, except when referencing other commits (e.g., mentions). - Grepped for removed `getShortName()`. Reviewers: chad Reviewed By: chad Maniphest Tasks: T4245 Differential Revision: https://secure.phabricator.com/D14990 --- .../controller/DiffusionBrowseController.php | 2 +- .../diffusion/view/DiffusionView.php | 2 +- .../PhabricatorRepositorySearchEngine.php | 14 ++++++++++---- .../storage/PhabricatorRepository.php | 19 +++++++++++++++---- .../storage/PhabricatorRepositoryCommit.php | 15 +++++++++++++-- 5 files changed, 40 insertions(+), 12 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index eef95bb1ee..0f0a583af8 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -1761,7 +1761,7 @@ final class DiffusionBrowseController extends DiffusionController { 'size' => 600, ), ), - $commit->getShortName()); + $commit->getLocalName()); $links[$identifier] = $commit_link; } diff --git a/src/applications/diffusion/view/DiffusionView.php b/src/applications/diffusion/view/DiffusionView.php index b540dc3a4e..74de704dc3 100644 --- a/src/applications/diffusion/view/DiffusionView.php +++ b/src/applications/diffusion/view/DiffusionView.php @@ -128,7 +128,7 @@ abstract class DiffusionView extends AphrontView { $commit, $summary = '') { - $commit_name = $repository->formatCommitName($commit); + $commit_name = $repository->formatCommitName($commit, $local = true); if (strlen($summary)) { $commit_name .= ': '.$summary; diff --git a/src/applications/repository/query/PhabricatorRepositorySearchEngine.php b/src/applications/repository/query/PhabricatorRepositorySearchEngine.php index ad3b61c8df..83ec6c92e2 100644 --- a/src/applications/repository/query/PhabricatorRepositorySearchEngine.php +++ b/src/applications/repository/query/PhabricatorRepositorySearchEngine.php @@ -160,10 +160,16 @@ final class PhabricatorRepositorySearchEngine $commit = $repository->getMostRecentCommit(); if ($commit) { - $commit_link = DiffusionView::linkCommit( - $repository, - $commit->getCommitIdentifier(), - $commit->getSummary()); + $commit_link = phutil_tag( + 'a', + array( + 'href' => $commit->getURI(), + ), + pht( + '%s: %s', + $commit->getLocalName(), + $commit->getSummary())); + $item->setSubhead($commit_link); $item->setEpoch($commit->getEpoch()); } diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index ee0e15c879..28e74ca5af 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -924,7 +924,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO return $this->isBranchInFilter($branch, 'branch-filter'); } - public function formatCommitName($commit_identifier) { + public function formatCommitName($commit_identifier, $local = false) { $vcs = $this->getVersionControlSystem(); $type_git = PhabricatorRepositoryType::REPOSITORY_TYPE_GIT; @@ -933,12 +933,23 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $is_git = ($vcs == $type_git); $is_hg = ($vcs == $type_hg); if ($is_git || $is_hg) { - $short_identifier = substr($commit_identifier, 0, 12); + $name = substr($commit_identifier, 0, 12); + $need_scope = false; } else { - $short_identifier = $commit_identifier; + $name = $commit_identifier; + $need_scope = true; } - return 'r'.$this->getCallsign().$short_identifier; + if (!$local) { + $need_scope = true; + } + + if ($need_scope) { + $scope = 'r'.$this->getCallsign(); + $name = $scope.$name; + } + + return $name; } public function isImporting() { diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index d7a2e111e2..fc67d8b7b3 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -266,9 +266,20 @@ final class PhabricatorRepositoryCommit return $repository->formatCommitName($identifier); } - public function getShortName() { + /** + * Return a local display name for use in the context of the containing + * repository. + * + * In Git and Mercurial, this returns only a short hash, like "abcdef012345". + * See @{method:getDisplayName} for a short name that always includes + * repository context. + * + * @return string Short human-readable name for use inside a repository. + */ + public function getLocalName() { + $repository = $this->getRepository(); $identifier = $this->getCommitIdentifier(); - return substr($identifier, 0, 9); + return $repository->formatCommitName($identifier, $local = true); } public function renderAuthorLink($handles) {