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

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,
+        $rows = $this->buildDisplayRows($text_list, $rev_list, $blame_dict,
-          $revs);
+          $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,
+  private static function buildDisplayRows($text_list, $rev_list, $blame_dict,
-    $revs) {
+    $needs_blame, DiffusionRequest $drequest) {
-    $last = null;
+    $last_rev = null;
     $color = null;
     $rows = array();
     $n = 1;
-    foreach ($data as $k => $line) {
+
-      if ($blame) {
+    $epoch_list = ipull($blame_dict, 'epoch');
-        if ($last == $blamedata[$k][0]) {
+    $max = max($epoch_list);
-          $blameinfo =
+    $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 =
             '<th style="background: '.$color.'; width: 9em;"></th>'.
             '<th style="background: '.$color.'"></th>';
         } 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];
+          $color_number = (int)(0xEE -
-          $blameinfo =
+            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 =
             '<th style="background: '.$color.
               '; width: 9em;">'.$revision_link.'</th>'.
             '<th style="background: '.$color.
               '; font-weight: normal; color: #333;">'.$author_link.'</th>';
-          $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 = '<tr style="background: #ffff00;">';
         $targ = '<a id="scroll_target"></a>';
@@ -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.'<th>'.$l.'</th><td>'.$targ.$line.'</td></tr>';
+      $rows[] = $tr.$blame_info.'<th>'.$l.'</th><td>'.$targ.$line.'</td></tr>';
       ++$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
+    );
+  }
+
 }

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');

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() {
     // <private>
@@ -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() {
+  final public function getBlameData() {
-    return $this->tokenizeData($this->getRawData());
+    $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)
   {

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');

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();
+    // sample line:
-    $revs = array();
+    // d1b4fcdd2a7c8c0f8cbdd01ca839d992135424dc
-
+    //                       (     hzhao   2009-05-01  202)function print();
-    if ($this->getNeedsBlame()) {
+    preg_match('/^\s*?(\S+?)\s*\(\s*(\S+)\s+\S+\s+\d+\)(.*)?$/', $line, $m);
-      $data = explode("\n", rtrim($data));
+    $rev_id = $m[1];
-      foreach ($data as $k => $line) {
+    $author = $m[2];
-        // sample line:
+    $text = idx($m, 3);
-        // d1b4fcdd        (     hzhao   2009-05-01  202)function print();
+    return array($rev_id, $author, $text);
-        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);
   }
 
 }

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();
+    preg_match('/^\s*(\d+)\s+(\S+)(?: (.*))?$/', $line, $m);
-    $revs = array();
+    $rev_id = $m[1];
+    $author = $m[2];
+    $text = idx($m, 3);
 
-    if ($this->getNeedsBlame()) {
+    return array($rev_id, $author, $text);
-      $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);
   }
 
 }