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

src/applications/diffusion/controller/file/DiffusionBrowseFileController.php

@@ -54,17 +54,20 @@ class DiffusionBrowseFileController extends DiffusionController {
     }
     $select .= '</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.
-      '<button>view</button>');
+      '<button>View</button>');
     $view_select_panel->appendChild($view_select_form);
+    $view_select_panel->appendChild('<div style="clear: both;"></div>');
 
     // 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');
+    if ($blame_dict) {
-    $max = max($epoch_list);
+      $epoch_list = ipull($blame_dict, 'epoch');
-    $min = min($epoch_list);
+      $max = max($epoch_list);
-    $range = $max - $min + 1;
+      $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);

src/applications/diffusion/query/browse/git/DiffusionGitBrowseQuery.php

@@ -27,34 +27,40 @@ final class DiffusionGitBrowseQuery extends DiffusionBrowseQuery {
 
     $local_path = $repository->getDetail('local-path');
 
-    try {
+    if ($path == '') {
-      list($stdout) = execx(
+      // Fast path to improve the performance of the repository view; we know
-        "(cd %s && git cat-file -t %s:%s)",
+      // the root is always a tree at any commit and always exists.
-        $local_path,
+      $stdout = 'tree';
-        $commit,
+    } else {
-        $path);
+      try {
-    } 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)',
+          "(cd %s && git cat-file -t %s:%s)",
           $local_path,
           $commit,
           $path);
-        $stdout = trim($stdout);
+      } catch (CommandException $e) {
-        if ($stdout) {
+        $stderr = $e->getStdErr();
-          $commits = explode("\n", $stdout);
+        if (preg_match('/^fatal: Not a valid object name/', $stderr)) {
-          $this->reason = self::REASON_IS_DELETED;
+          // Grab two logs, since the first one is when the object was deleted.
-          $this->deletedAtCommit = idx($commits, 0);
+          list($stdout) = execx(
-          $this->existedAtCommit = idx($commits, 1);
+            '(cd %s && git log -n2 --format="%%H" %s -- %s)',
-          return array();
+            $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;
+          $this->reason = self::REASON_IS_NONEXISTENT;
-        return array();
+          return array();
-      } else {
+        } else {
-        throw $e;
+          throw $e;
+        }
       }
     }
 

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() {
     // <private>
@@ -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;
   }

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).'/';
   }

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) {
+    return $this->getStableCommitName();
-      $this->loadedCommit = id(new PhabricatorRepositoryCommit())->loadOneWhere(
-        'repositoryID = %d ORDER BY epoch DESC LIMIT 1',
-        $this->getRepository()->getID())->getCommitIdentifier();
-    }
-
-    return $this->loadedCommit;
   }
 
 }

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;

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;
+}