From 64cd4f969d2a71b8463ee56659b40257b20c93cc Mon Sep 17 00:00:00 2001 From: jungejason Date: Thu, 31 Mar 2011 18:28:24 -0700 Subject: [PATCH] Add color to diffusion blame and improve plain view Summary: query the database to get the epoch info for the commits, then calculate the color depending on the epoch (the newer the commit, the dark its color). Also improved the plain blame view for git, as the git-blame doesn't produce a good display by default. Now we format the output it from the data we fetches from the database. Test Plan: verify both git and svn browsing page work for 'plain', 'plainblame', 'highlighted' and 'highlightedblame' view. Reviewed By: epriestley Reviewers: epriestley CC: epriestley, jungejason Differential Revision: 93 --- .../file/DiffusionBrowseFileController.php | 135 +++++++++++------- .../diffusion/controller/file/__init__.php | 2 +- .../base/DiffusionFileContentQuery.php | 44 +++++- .../query/filecontent/base/__init__.php | 2 + .../git/DiffusionGitFileContentQuery.php | 40 ++---- .../svn/DiffusionSvnFileContentQuery.php | 35 ++--- 6 files changed, 142 insertions(+), 116 deletions(-) diff --git a/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php index a6f5ca6322..f2ac3271ae 100644 --- a/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php +++ b/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php @@ -82,35 +82,18 @@ class DiffusionBrowseFileController extends DiffusionController { } - public static function renderRevision( - DiffusionRequest $drequest, - $revision) { - - $callsign = $drequest->getCallsign(); - - $name = 'r'.$callsign.$revision; - return phutil_render_tag( - 'a', - array( - 'href' => '/'.$name, - ), - $name - ); - } - - private function buildCorpus($selected) { - $blame = ($selected == 'blame' || $selected == 'plainblame'); + $needs_blame = ($selected == 'blame' || $selected == 'plainblame'); $file_query = DiffusionFileContentQuery::newFromDiffusionRequest( $this->diffusionRequest); - $file_query->setNeedsBlame($blame); + $file_query->setNeedsBlame($needs_blame); + $file_query->loadFileContent(); // TODO: image // TODO: blame of blame. switch ($selected) { case 'plain': - case 'plainblame': $style = "margin: 1em 2em; width: 90%; height: 80em; font-family: monospace"; $corpus = phutil_render_tag( @@ -119,25 +102,49 @@ class DiffusionBrowseFileController extends DiffusionController { 'style' => $style, ), phutil_escape_html($file_query->getRawData())); + break; + case 'plainblame': + $style = + "margin: 1em 2em; width: 90%; height: 80em; font-family: monospace"; + list($text_list, $rev_list, $blame_dict) = + $file_query->getBlameData(); + + $rows = array(); + foreach ($text_list as $k => $line) { + $rev = $rev_list[$k]; + $author = $blame_dict[$rev]['author']; + $rows[] = + sprintf("%-10s %-15s %s", substr($rev, 0, 7), $author, $line); + } + + $corpus = phutil_render_tag( + 'textarea', + array( + 'style' => $style, + ), + phutil_escape_html(implode("\n", $rows))); + + break; + case 'highlighted': case 'blame': default: require_celerity_resource('syntax-highlighting-css'); require_celerity_resource('diffusion-source-css'); - list($data, $blamedata, $revs) = $file_query->getTokenizedData(); + list($text_list, $rev_list, $blame_dict) = $file_query->getBlameData(); $drequest = $this->getDiffusionRequest(); $path = $drequest->getPath(); $highlightEngine = new PhutilDefaultSyntaxHighlighterEngine(); - $data = $highlightEngine->highlightSource($path, $data); - $data = explode("\n", rtrim($data)); + $text_list = explode("\n", $highlightEngine->highlightSource($path, + implode("\n", $text_list))); - $rows = $this->buildDisplayRows($data, $blame, $blamedata, $drequest, - $revs); + $rows = $this->buildDisplayRows($text_list, $rev_list, $blame_dict, + $needs_blame, $drequest); $corpus_table = phutil_render_tag( 'table', @@ -159,49 +166,51 @@ class DiffusionBrowseFileController extends DiffusionController { } - private static function buildDisplayRows($data, $blame, $blamedata, $drequest, - $revs) { - $last = null; + private static function buildDisplayRows($text_list, $rev_list, $blame_dict, + $needs_blame, DiffusionRequest $drequest) { + $last_rev = null; $color = null; $rows = array(); $n = 1; - foreach ($data as $k => $line) { - if ($blame) { - if ($last == $blamedata[$k][0]) { - $blameinfo = + + $epoch_list = ipull($blame_dict, 'epoch'); + $max = max($epoch_list); + $min = min($epoch_list); + $range = $max - $min + 1; + + foreach ($text_list as $k => $line) { + if ($needs_blame) { + // If the line's rev is same as the line above, show empty content + // with same color; otherwise generate blame info. The newer a change + // is, the darker the color. + $rev = $rev_list[$k]; + if ($last_rev == $rev) { + $blame_info = ''. ''; } else { - switch ($drequest->getRepository()->getVersionControlSystem()) { - case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: - // TODO: better color for git. - $color = '#dddddd'; - break; - case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: - $color = sprintf( - '#%02xee%02x', - $revs[$blamedata[$k][0]], - $revs[$blamedata[$k][0]]); - break; - default: - throw new Exception('repository type not supported'); - } - $revision_link = self::renderRevision( - $drequest, - $blamedata[$k][0]); - $author_link = $blamedata[$k][1]; - $blameinfo = + $color_number = (int)(0xEE - + 0xEE * ($blame_dict[$rev]['epoch'] - $min) / $range); + $color = sprintf('#%02xee%02x', $color_number, $color_number); + + $revision_link = self::renderRevision( + $drequest, + substr($rev, 0, 7)); + + $author_link = $blame_dict[$rev]['author']; + $blame_info = ''.$revision_link.''. ''.$author_link.''; - $last = $blamedata[$k][0]; + $last_rev = $rev; } } else { - $blameinfo = null; + $blame_info = null; } + // Highlight the line of interest if needed. if ($n == $drequest->getLine()) { $tr = ''; $targ = ''; @@ -212,6 +221,7 @@ class DiffusionBrowseFileController extends DiffusionController { $targ = null; } + // Create the row display. $uri_path = $drequest->getUriPath(); $uri_rev = $drequest->getCommit(); @@ -222,10 +232,27 @@ class DiffusionBrowseFileController extends DiffusionController { ), $n); - $rows[] = $tr.$blameinfo.''.$l.''.$targ.$line.''; + $rows[] = $tr.$blame_info.''.$l.''.$targ.$line.''; ++$n; } return $rows; } + + + private static function renderRevision(DiffusionRequest $drequest, + $revision) { + + $callsign = $drequest->getCallsign(); + + $name = 'r'.$callsign.$revision; + return phutil_render_tag( + 'a', + array( + 'href' => '/'.$name, + ), + $name + ); + } + } diff --git a/src/applications/diffusion/controller/file/__init__.php b/src/applications/diffusion/controller/file/__init__.php index 7b85e05b1f..529bf8288a 100644 --- a/src/applications/diffusion/controller/file/__init__.php +++ b/src/applications/diffusion/controller/file/__init__.php @@ -8,13 +8,13 @@ phutil_require_module('phabricator', 'applications/diffusion/controller/base'); phutil_require_module('phabricator', 'applications/diffusion/query/filecontent/base'); -phutil_require_module('phabricator', 'applications/repository/constants/repositorytype'); phutil_require_module('phabricator', 'infrastructure/celerity/api'); phutil_require_module('phabricator', 'infrastructure/javelin/api'); phutil_require_module('phabricator', 'view/layout/panel'); phutil_require_module('phutil', 'markup'); phutil_require_module('phutil', 'markup/syntax/engine/default'); +phutil_require_module('phutil', 'utils'); phutil_require_source('DiffusionBrowseFileController.php'); diff --git a/src/applications/diffusion/query/filecontent/base/DiffusionFileContentQuery.php b/src/applications/diffusion/query/filecontent/base/DiffusionFileContentQuery.php index a832e6cc77..42be58a645 100644 --- a/src/applications/diffusion/query/filecontent/base/DiffusionFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/base/DiffusionFileContentQuery.php @@ -20,6 +20,7 @@ abstract class DiffusionFileContentQuery { private $request; private $needsBlame; + private $fileContent; final private function __construct() { // @@ -54,20 +55,53 @@ abstract class DiffusionFileContentQuery { } final public function loadFileContent() { - return $this->executeQuery(); + $this->fileContent = $this->executeQuery(); } abstract protected function executeQuery(); final public function getRawData() { - return $this->loadFileContent()->getCorpus(); + return $this->fileContent->getCorpus(); } - final public function getTokenizedData() { - return $this->tokenizeData($this->getRawData()); + final public function getBlameData() { + $raw_data = $this->getRawData(); + + $text_list = array(); + $rev_list = array(); + $blame_dict = array(); + + if (!$this->getNeedsBlame()) { + $text_list = explode("\n", rtrim($raw_data)); + } else { + foreach (explode("\n", rtrim($raw_data)) as $k => $line) { + list($rev_id, $author, $text) = $this->tokenizeLine($line); + + $text_list[$k] = $text; + $rev_list[$k] = $rev_id; + + if (!isset($blame_dict[$rev_id]) && + !isset($blame_dict[$rev_id]['author'] )) { + $blame_dict[$rev_id]['author'] = $author; + } + } + + $repository = $this->getRequest()->getRepository(); + $commits = id(new PhabricatorRepositoryCommit())->loadAllWhere( + 'repositoryID = %d AND commitIdentifier IN (%Ls)', $repository->getID(), + array_unique($rev_list)); + + foreach ($commits as $commit) { + $commitIdentifier = $commit->getCommitIdentifier(); + $epoch = $commit->getEpoch(); + $blame_dict[$commitIdentifier]['epoch'] = $epoch; + } + } + + return array($text_list, $rev_list, $blame_dict); } - abstract protected function tokenizeData($data); + abstract protected function tokenizeLine($line); public function setNeedsBlame($needs_blame) { diff --git a/src/applications/diffusion/query/filecontent/base/__init__.php b/src/applications/diffusion/query/filecontent/base/__init__.php index f964d49824..7ad4326f05 100644 --- a/src/applications/diffusion/query/filecontent/base/__init__.php +++ b/src/applications/diffusion/query/filecontent/base/__init__.php @@ -7,8 +7,10 @@ phutil_require_module('phabricator', 'applications/repository/constants/repositorytype'); +phutil_require_module('phabricator', 'applications/repository/storage/commit'); phutil_require_module('phutil', 'symbols'); +phutil_require_module('phutil', 'utils'); phutil_require_source('DiffusionFileContentQuery.php'); diff --git a/src/applications/diffusion/query/filecontent/git/DiffusionGitFileContentQuery.php b/src/applications/diffusion/query/filecontent/git/DiffusionGitFileContentQuery.php index 0465f6ccf0..9176fbfebb 100644 --- a/src/applications/diffusion/query/filecontent/git/DiffusionGitFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/git/DiffusionGitFileContentQuery.php @@ -28,7 +28,7 @@ final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery { $local_path = $repository->getDetail('local-path'); if ($this->getNeedsBlame()) { list($corpus) = execx( - '(cd %s && git --no-pager blame -c --date short %s -- %s)', + '(cd %s && git --no-pager blame -c -l --date short %s -- %s)', $local_path, $commit, $path); @@ -46,35 +46,17 @@ final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery { return $file_content; } - protected function tokenizeData($data) { + + protected function tokenizeLine($line) { $m = array(); - $blamedata = array(); - $revs = array(); - - if ($this->getNeedsBlame()) { - $data = explode("\n", rtrim($data)); - foreach ($data as $k => $line) { - // sample line: - // d1b4fcdd ( hzhao 2009-05-01 202)function print(); - preg_match('/^\s*?(\S+?)\s*\(\s*(\S+)\s+\S+\s+\d+\)(.*)?$/', - $line, $m); - $data[$k] = idx($m, 3); - $blamedata[$k] = array($m[1], $m[2]); - - $revs[$m[1]] = true; - } - $data = implode("\n", $data); - - krsort($revs); - $ii = 0; - $len = count($revs); - foreach ($revs as $rev => $ignored) { - $revs[$rev] = (int)(0xEE * ($ii / $len)); - ++$ii; - } - } - - return array($data, $blamedata, $revs); + // sample line: + // d1b4fcdd2a7c8c0f8cbdd01ca839d992135424dc + // ( hzhao 2009-05-01 202)function print(); + preg_match('/^\s*?(\S+?)\s*\(\s*(\S+)\s+\S+\s+\d+\)(.*)?$/', $line, $m); + $rev_id = $m[1]; + $author = $m[2]; + $text = idx($m, 3); + return array($rev_id, $author, $text); } } diff --git a/src/applications/diffusion/query/filecontent/svn/DiffusionSvnFileContentQuery.php b/src/applications/diffusion/query/filecontent/svn/DiffusionSvnFileContentQuery.php index 27f1b05326..64e7f9f085 100644 --- a/src/applications/diffusion/query/filecontent/svn/DiffusionSvnFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/svn/DiffusionSvnFileContentQuery.php @@ -56,35 +56,16 @@ final class DiffusionSvnFileContentQuery extends DiffusionFileContentQuery { return $file_content; } - protected function tokenizeData($data) - { + protected function tokenizeLine($line) { + // sample line: + // 347498 yliu function print(); $m = array(); - $blamedata = array(); - $revs = array(); + preg_match('/^\s*(\d+)\s+(\S+)(?: (.*))?$/', $line, $m); + $rev_id = $m[1]; + $author = $m[2]; + $text = idx($m, 3); - if ($this->getNeedsBlame()) { - $data = explode("\n", rtrim($data)); - foreach ($data as $k => $line) { - // sample line: - // 347498 yliu function print(); - preg_match('/^\s*(\d+)\s+(\S+)(?: (.*))?$/', $line, $m); - $data[$k] = idx($m, 3); - $blamedata[$k] = array($m[1], $m[2]); - - $revs[$m[1]] = true; - } - $data = implode("\n", $data); - - krsort($revs); - $ii = 0; - $len = count($revs); - foreach ($revs as $rev => $ignored) { - $revs[$rev] = (int)(0xEE * ($ii / $len)); - ++$ii; - } - } - - return array($data, $blamedata, $revs); + return array($rev_id, $author, $text); } }