Differential - modernize "Local Commits" table

Summary: ...also link to commits we know about in "Local Commits" and "Revision Update History" tables. Fixes T4585.

Test Plan: made a repo. made a diff (foo) and committed it (bar). made a new diff that was comprised of two local commits. noted links to (bar) in various commit hashes as expected

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin, chad

Maniphest Tasks: T4585

Differential Revision: https://secure.phabricator.com/D8679

resources/celerity/map.php

@@ -10,7 +10,7 @@ return array(
     'core.pkg.css' => 'fb144113',
     'core.pkg.js' => 'd3fecc57',
     'darkconsole.pkg.js' => 'ca8671ce',
-    'differential.pkg.css' => 'cb97e095',
+    'differential.pkg.css' => '3ad9692c',
     'differential.pkg.js' => '11a5b750',
     'diffusion.pkg.css' => '3783278d',
     'diffusion.pkg.js' => '5b4010f4',
@@ -57,7 +57,6 @@ return array(
     'rsrc/css/application/differential/add-comment.css' => 'c478bcaa',
     'rsrc/css/application/differential/changeset-view.css' => 'e710a360',
     'rsrc/css/application/differential/core.css' => '7ac3cabc',
-    'rsrc/css/application/differential/local-commits-view.css' => '19649019',
     'rsrc/css/application/differential/results-table.css' => '239924f9',
     'rsrc/css/application/differential/revision-comment.css' => '48186045',
     'rsrc/css/application/differential/revision-history.css' => '0e8eb855',
@@ -509,7 +508,6 @@ return array(
     'differential-changeset-view-css' => 'e710a360',
     'differential-core-view-css' => '7ac3cabc',
     'differential-inline-comment-editor' => 'f2441746',
-    'differential-local-commits-view-css' => '19649019',
     'differential-results-table-css' => '239924f9',
     'differential-revision-add-comment-css' => 'c478bcaa',
     'differential-revision-comment-css' => '48186045',
@@ -2121,8 +2119,7 @@ return array(
       7 => 'differential-revision-add-comment-css',
       8 => 'phabricator-object-selector-css',
       9 => 'phabricator-content-source-view-css',
-      10 => 'differential-local-commits-view-css',
+      10 => 'inline-comment-summary-css',
-      11 => 'inline-comment-summary-css',
     ),
     'differential.pkg.js' =>
     array(

resources/celerity/packages.php

@@ -126,7 +126,6 @@ return array(
     'differential-revision-add-comment-css',
     'phabricator-object-selector-css',
     'phabricator-content-source-view-css',
-    'differential-local-commits-view-css',
     'inline-comment-summary-css',
   ),
   'differential.pkg.js' => array(

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -186,6 +186,18 @@ final class DifferentialRevisionViewController extends DifferentialController {
       }
     }
 
+    $commit_hashes = mpull($diffs, 'getSourceControlBaseRevision');
+    $local_commits = idx($props, 'local:commits', array());
+    foreach ($local_commits as $local_commit) {
+      $commit_hashes[] = idx($local_commit, 'tree');
+      $commit_hashes[] = idx($local_commit, 'local');
+    }
+    $commit_hashes = array_unique(array_filter($commit_hashes));
+    $commits_for_links = id(new DiffusionCommitQuery())
+      ->setViewer($user)
+      ->withIdentifiers($commit_hashes)
+      ->execute();
+    $commits_for_links = mpull($commits_for_links, null, 'getCommitIdentifier');
 
     $revision_detail = id(new DifferentialRevisionDetailView())
       ->setUser($user)
@@ -267,16 +279,18 @@ final class DifferentialRevisionViewController extends DifferentialController {
     $changeset_view->setSymbolIndexes($symbol_indexes);
     $changeset_view->setTitle('Diff '.$target->getID());
 
-    $diff_history = new DifferentialRevisionUpdateHistoryView();
+    $diff_history = id(new DifferentialRevisionUpdateHistoryView())
-    $diff_history->setDiffs($diffs);
+      ->setUser($user)
-    $diff_history->setSelectedVersusDiffID($diff_vs);
+      ->setDiffs($diffs)
-    $diff_history->setSelectedDiffID($target->getID());
+      ->setSelectedVersusDiffID($diff_vs)
-    $diff_history->setSelectedWhitespace($whitespace);
+      ->setSelectedDiffID($target->getID())
-    $diff_history->setUser($user);
+      ->setSelectedWhitespace($whitespace)
+      ->setCommitsForLinks($commits_for_links);
 
-    $local_view = new DifferentialLocalCommitsView();
+    $local_view = id(new DifferentialLocalCommitsView())
-    $local_view->setUser($user);
+      ->setUser($user)
-    $local_view->setLocalCommits(idx($props, 'local:commits'));
+      ->setLocalCommits(idx($props, 'local:commits'))
+      ->setCommitsForLinks($commits_for_links);
 
     if ($repository) {
       $other_revisions = $this->loadOtherRevisions(

src/applications/differential/view/DifferentialLocalCommitsView.php

@@ -3,12 +3,19 @@
 final class DifferentialLocalCommitsView extends AphrontView {
 
   private $localCommits;
+  private $commitsForLinks = array();
 
   public function setLocalCommits($local_commits) {
     $this->localCommits = $local_commits;
     return $this;
   }
 
+  public function setCommitsForLinks(array $commits) {
+    assert_instances_of($commits, 'PhabricatorRepositoryCommit');
+    $this->commitsForLinks = $commits;
+    return $this;
+  }
+
   public function render() {
     $user = $this->user;
     if (!$user) {
@@ -20,8 +27,6 @@ final class DifferentialLocalCommitsView extends AphrontView {
       return null;
     }
 
-    $this->requireResource('differential-local-commits-view-css');
-
     $has_tree = false;
     $has_local = false;
 
@@ -35,36 +40,23 @@ final class DifferentialLocalCommitsView extends AphrontView {
     }
 
     $rows = array();
-    $highlight = true;
     foreach ($local as $commit) {
-      if ($highlight) {
-        $class = 'alt';
-        $highlight = false;
-      } else {
-        $class = '';
-        $highlight = true;
-      }
-
-
       $row = array();
       if (idx($commit, 'commit')) {
-        $commit_hash = self::formatCommit($commit['commit']);
+        $commit_link = $this->buildCommitLink($commit['commit']);
       } else if (isset($commit['rev'])) {
-        $commit_hash = self::formatCommit($commit['rev']);
+        $commit_link = $this->buildCommitLink($commit['rev']);
       } else {
-        $commit_hash = null;
+        $commit_link = null;
       }
-      $row[] = phutil_tag('td', array(), $commit_hash);
+      $row[] = $commit_link;
 
       if ($has_tree) {
-        $tree = idx($commit, 'tree');
+        $row[] = $this->buildCommitLink($commit['tree']);
-        $tree = self::formatCommit($tree);
-        $row[] = phutil_tag('td', array(), $tree);
       }
 
       if ($has_local) {
-        $local_rev = idx($commit, 'local', null);
+        $row[] = $this->buildCommitLink($commit['local']);
-        $row[] = phutil_tag('td', array(), $local_rev);
       }
 
       $parents = idx($commit, 'parents', array());
@@ -72,15 +64,15 @@ final class DifferentialLocalCommitsView extends AphrontView {
         if (is_array($parent)) {
           $parent = idx($parent, 'rev');
         }
-        $parents[$k] = self::formatCommit($parent);
+        $parents[$k] = $this->buildCommitLink($parent);
       }
       $parents = phutil_implode_html(phutil_tag('br'), $parents);
-      $row[] = phutil_tag('td', array(), $parents);
+      $row[] = $parents;
 
       $author = nonempty(
         idx($commit, 'user'),
         idx($commit, 'author'));
-      $row[] = phutil_tag('td', array(), $author);
+      $row[] = $author;
 
       $message = idx($commit, 'message');
 
@@ -94,12 +86,7 @@ final class DifferentialLocalCommitsView extends AphrontView {
         $view->setMore(phutil_escape_html_newlines($message));
       }
 
-      $row[] = phutil_tag(
+      $row[] = $view->render();
-        'td',
-        array(
-          'class' => 'summary',
-        ),
-        $view->render());
 
       $date = nonempty(
         idx($commit, 'date'),
@@ -107,39 +94,60 @@ final class DifferentialLocalCommitsView extends AphrontView {
       if ($date) {
         $date = phabricator_datetime($date, $user);
       }
-      $row[] = phutil_tag('td', array(), $date);
+      $row[] = $date;
 
-      $rows[] = phutil_tag('tr', array('class' => $class), $row);
+      $rows[] = $row;
     }
 
-
+    $column_classes = array('');
-    $headers = array();
-    $headers[] = phutil_tag('th', array(), pht('Commit'));
     if ($has_tree) {
-      $headers[] = phutil_tag('th', array(), pht('Tree'));
+      $column_classes[] = '';
     }
     if ($has_local) {
-      $headers[] = phutil_tag('th', array(), pht('Local'));
+      $column_classes[] = '';
     }
-    $headers[] = phutil_tag('th', array(), pht('Parents'));
+    $column_classes[] = '';
-    $headers[] = phutil_tag('th', array(), pht('Author'));
+    $column_classes[] = '';
-    $headers[] = phutil_tag('th', array(), pht('Summary'));
+    $column_classes[] = 'wide';
-    $headers[] = phutil_tag('th', array(), pht('Date'));
+    $column_classes[] = 'date';
-
+    $table = id(new AphrontTableView($rows))
-    $headers = phutil_tag('tr', array(), $headers);
+      ->setColumnClasses($column_classes);
-
+    $headers = array();
-    $content = phutil_tag_div('differential-panel', phutil_tag(
+    $headers[] = pht('Commit');
-      'table',
+    if ($has_tree) {
-      array('class' => 'differential-local-commits-table'),
+      $headers[] = pht('Tree');
-      array($headers, phutil_implode_html("\n", $rows))));
+    }
+    if ($has_local) {
+      $headers[] = pht('Local');
+    }
+    $headers[] = pht('Parents');
+    $headers[] = pht('Author');
+    $headers[] = pht('Summary');
+    $headers[] = pht('Date');
+    $table->setHeaders($headers);
 
     return id(new PHUIObjectBoxView())
       ->setHeaderText(pht('Local Commits'))
-      ->appendChild($content);
+      ->appendChild($table);
   }
 
   private static function formatCommit($commit) {
     return substr($commit, 0, 12);
   }
 
+  private function buildCommitLink($hash) {
+    $commit_for_link = idx($this->commitsForLinks, $hash);
+    $commit_hash = self::formatCommit($hash);
+    if ($commit_for_link) {
+      $link = phutil_tag(
+        'a',
+        array(
+          'href' => $commit_for_link->getURI()),
+        $commit_hash);
+    } else {
+      $link = $commit_hash;
+    }
+    return $link;
+  }
+
 }

src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php

@@ -6,6 +6,7 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView {
   private $selectedVersusDiffID;
   private $selectedDiffID;
   private $selectedWhitespace;
+  private $commitsForLinks = array();
 
   public function setDiffs(array $diffs) {
     assert_instances_of($diffs, 'DifferentialDiff');
@@ -28,6 +29,12 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView {
     return $this;
   }
 
+  public function setCommitsForLinks(array $commits) {
+    assert_instances_of($commits, 'PhabricatorRepositoryCommit');
+    $this->commitsForLinks = $commits;
+    return $this;
+  }
+
   public function render() {
 
     $this->requireResource('differential-core-view-css');
@@ -378,20 +385,38 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView {
       case 'git':
         $base = $diff->getSourceControlBaseRevision();
         if (strpos($base, '@') === false) {
-          return substr($base, 0, 7);
+          $label = substr($base, 0, 7);
         } else {
           // The diff is from git-svn
           $base = explode('@', $base);
           $base = last($base);
-          return $base;
+          $label = $base;
         }
+        break;
       case 'svn':
         $base = $diff->getSourceControlBaseRevision();
         $base = explode('@', $base);
         $base = last($base);
-        return $base;
+        $label = $base;
+        break;
       default:
-        return null;
+        $label = null;
+        break;
     }
+    $link = null;
+    if ($label) {
+      $commit_for_link = idx(
+        $this->commitsForLinks,
+        $diff->getSourceControlBaseRevision());
+      if ($commit_for_link) {
+        $link = phutil_tag(
+          'a',
+          array('href' => $commit_for_link->getURI()),
+          $label);
+      } else {
+        $link = $label;
+      }
+    }
+    return $link;
   }
 }

src/applications/repository/phid/PhabricatorRepositoryPHIDTypeCommit.php

@@ -32,7 +32,6 @@ final class PhabricatorRepositoryPHIDTypeCommit extends PhabricatorPHIDType {
     foreach ($handles as $phid => $handle) {
       $commit = $objects[$phid];
       $repository = $commit->getRepository();
-      $callsign = $repository->getCallsign();
       $commit_identifier = $commit->getCommitIdentifier();
 
       $name = $repository->formatCommitName($commit_identifier);
@@ -45,7 +44,7 @@ final class PhabricatorRepositoryPHIDTypeCommit extends PhabricatorPHIDType {
 
       $handle->setName($name);
       $handle->setFullName($full_name);
-      $handle->setURI('/r'.$callsign.$commit_identifier);
+      $handle->setURI($commit->getURI());
       $handle->setTimestamp($commit->getEpoch());
     }
   }

src/applications/repository/storage/PhabricatorRepositoryCommit.php

@@ -130,6 +130,13 @@ final class PhabricatorRepositoryCommit
     return $this->getEpoch();
   }
 
+  public function getURI() {
+    $repository = $this->getRepository();
+    $callsign = $repository->getCallsign();
+    $commit_identifier = $this->getCommitIdentifier();
+    return '/r'.$callsign.$commit_identifier;
+  }
+
   /**
    * Synchronize a commit's overall audit status with the individual audit
    * triggers.

webroot/rsrc/css/application/differential/local-commits-view.css

@@ -1,29 +0,0 @@
-/**
- * @provides differential-local-commits-view-css
- */
-
-.differential-local-commits-table {
-  width: 100%;
-  border-collapse: separate;
-  border-spacing: 1px 2px;
-}
-
-.differential-local-commits-table th {
-  color: {$darkbluetext};
-  padding: 4px 6px;
-}
-
-.differential-local-commits-table tr.alt td {
-  background: {$lightgreybackground};
-}
-
-.differential-local-commits-table td {
-  padding: 4px 6px;
-  white-space: nowrap;
-  vertical-align: top;
-}
-
-.differential-local-commits-table td.summary {
-  white-space: normal;
-  width: 100%;
-}