Improve displaying of very large commits

Summary:
Behave like Differential.

Also save one path ID query.

Test Plan: Displayed very large commit, clicked in ToC, clicked on Load.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

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

Author: vrana

src/__celerity_resource_map__.php

@@ -1138,7 +1138,7 @@
   ),
   'javelin-behavior-differential-populate' =>
   array(
-    'uri' => '/res/cf4691f5/rsrc/js/application/differential/behavior-populate.js',
+    'uri' => '/res/781dd9a5/rsrc/js/application/differential/behavior-populate.js',
     'type' => 'js',
     'requires' =>
     array(
@@ -2885,7 +2885,7 @@
       'uri' => '/res/pkg/96bc37d6/differential.pkg.css',
       'type' => 'css',
     ),
-    'df1fdda8' =>
+    '6617163c' =>
     array(
       'name' => 'differential.pkg.js',
       'symbols' =>
@@ -2909,7 +2909,7 @@
         16 => 'javelin-behavior-differential-dropdown-menus',
         17 => 'javelin-behavior-buoyant',
       ),
-      'uri' => '/res/pkg/df1fdda8/differential.pkg.js',
+      'uri' => '/res/pkg/6617163c/differential.pkg.js',
       'type' => 'js',
     ),
     'c8ce2d88' =>
@@ -3017,7 +3017,7 @@
     'aphront-typeahead-control-css' => '8b65e80d',
     'differential-changeset-view-css' => '96bc37d6',
     'differential-core-view-css' => '96bc37d6',
-    'differential-inline-comment-editor' => 'df1fdda8',
+    'differential-inline-comment-editor' => '6617163c',
     'differential-local-commits-view-css' => '96bc37d6',
     'differential-results-table-css' => '96bc37d6',
     'differential-revision-add-comment-css' => '96bc37d6',
@@ -3030,21 +3030,21 @@
     'inline-comment-summary-css' => '96bc37d6',
     'javelin-behavior' => '6fb20113',
     'javelin-behavior-aphront-basic-tokenizer' => '97f65640',
-    'javelin-behavior-aphront-drag-and-drop' => 'df1fdda8',
-    'javelin-behavior-aphront-drag-and-drop-textarea' => 'df1fdda8',
+    'javelin-behavior-aphront-drag-and-drop' => '6617163c',
+    'javelin-behavior-aphront-drag-and-drop-textarea' => '6617163c',
     'javelin-behavior-aphront-form-disable-on-submit' => '971b021e',
     'javelin-behavior-audit-preview' => '5e68be89',
-    'javelin-behavior-buoyant' => 'df1fdda8',
-    'javelin-behavior-differential-accept-with-errors' => 'df1fdda8',
-    'javelin-behavior-differential-add-reviewers-and-ccs' => 'df1fdda8',
-    'javelin-behavior-differential-comment-jump' => 'df1fdda8',
-    'javelin-behavior-differential-diff-radios' => 'df1fdda8',
-    'javelin-behavior-differential-dropdown-menus' => 'df1fdda8',
-    'javelin-behavior-differential-edit-inline-comments' => 'df1fdda8',
-    'javelin-behavior-differential-feedback-preview' => 'df1fdda8',
-    'javelin-behavior-differential-keyboard-navigation' => 'df1fdda8',
-    'javelin-behavior-differential-populate' => 'df1fdda8',
-    'javelin-behavior-differential-show-more' => 'df1fdda8',
+    'javelin-behavior-buoyant' => '6617163c',
+    'javelin-behavior-differential-accept-with-errors' => '6617163c',
+    'javelin-behavior-differential-add-reviewers-and-ccs' => '6617163c',
+    'javelin-behavior-differential-comment-jump' => '6617163c',
+    'javelin-behavior-differential-diff-radios' => '6617163c',
+    'javelin-behavior-differential-dropdown-menus' => '6617163c',
+    'javelin-behavior-differential-edit-inline-comments' => '6617163c',
+    'javelin-behavior-differential-feedback-preview' => '6617163c',
+    'javelin-behavior-differential-keyboard-navigation' => '6617163c',
+    'javelin-behavior-differential-populate' => '6617163c',
+    'javelin-behavior-differential-show-more' => '6617163c',
     'javelin-behavior-diffusion-commit-graph' => '5e68be89',
     'javelin-behavior-diffusion-pull-lastmodified' => '5e68be89',
     'javelin-behavior-maniphest-batch-selector' => '7707de41',
@@ -3054,12 +3054,12 @@
     'javelin-behavior-maniphest-transaction-preview' => '7707de41',
     'javelin-behavior-phabricator-autofocus' => '971b021e',
     'javelin-behavior-phabricator-keyboard-shortcuts' => '971b021e',
-    'javelin-behavior-phabricator-object-selector' => 'df1fdda8',
+    'javelin-behavior-phabricator-object-selector' => '6617163c',
     'javelin-behavior-phabricator-oncopy' => '971b021e',
     'javelin-behavior-phabricator-tooltips' => '971b021e',
     'javelin-behavior-phabricator-watch-anchor' => '971b021e',
     'javelin-behavior-refresh-csrf' => '971b021e',
-    'javelin-behavior-repository-crossreference' => 'df1fdda8',
+    'javelin-behavior-repository-crossreference' => '6617163c',
     'javelin-behavior-workflow' => '971b021e',
     'javelin-dom' => '6fb20113',
     'javelin-event' => '6fb20113',
@@ -3085,7 +3085,7 @@
     'phabricator-core-buttons-css' => '8b65e80d',
     'phabricator-core-css' => '8b65e80d',
     'phabricator-directory-css' => '8b65e80d',
-    'phabricator-drag-and-drop-file-upload' => 'df1fdda8',
+    'phabricator-drag-and-drop-file-upload' => '6617163c',
     'phabricator-dropdown-menu' => '971b021e',
     'phabricator-flag-css' => '8b65e80d',
     'phabricator-jump-nav' => '8b65e80d',
@@ -3097,7 +3097,7 @@
     'phabricator-prefab' => '971b021e',
     'phabricator-project-tag-css' => '7839ae2d',
     'phabricator-remarkup-css' => '8b65e80d',
-    'phabricator-shaped-request' => 'df1fdda8',
+    'phabricator-shaped-request' => '6617163c',
     'phabricator-standard-page-view' => '8b65e80d',
     'phabricator-tooltip' => '971b021e',
     'phabricator-transaction-view-css' => '8b65e80d',

src/applications/diffusion/controller/DiffusionCommitController.php

@@ -114,12 +114,6 @@ public function processRequest() {
 
     $content[] = $this->buildMergesTable($commit);
 
-    $original_changes_count = count($changes);
-    if ($request->getStr('show_all') !== 'true' &&
-        $original_changes_count > self::CHANGES_LIMIT) {
-      $changes = array_slice($changes, 0, self::CHANGES_LIMIT);
-    }
-
     $owners_paths = array();
     if ($this->highlightedAudits) {
       $packages = id(new PhabricatorOwnersPackage())->loadAllWhere(
@@ -180,7 +174,7 @@ public function processRequest() {
       $change_panel->setHeader("Changes (".number_format($count).")");
       $change_panel->setID('differential-review-toc');
 
-      if ($count !== $original_changes_count) {
+      if ($count > self::CHANGES_LIMIT) {
         $show_all_button = phutil_render_tag(
           'a',
           array(
@@ -190,10 +184,9 @@ public function processRequest() {
           phutil_escape_html('Show All Changes'));
         $warning_view = id(new AphrontErrorView())
           ->setSeverity(AphrontErrorView::SEVERITY_WARNING)
-          ->setTitle(sprintf(
-                       "Showing only the first %d changes out of %s!",
-                       self::CHANGES_LIMIT,
-                       number_format($original_changes_count)));
+          ->setTitle('Very Large Commit')
+          ->appendChild(
+            "<p>This commit is very large. Load each file individually.</p>");
 
         $change_panel->appendChild($warning_view);
         $change_panel->addButton($show_all_button);
@@ -240,16 +233,31 @@ public function processRequest() {
       // DifferentialChangeset. Make the objects ephemeral to make sure we don't
       // accidentally save them, and then set their ID to the appropriate ID for
       // this application (the path IDs).
-      $pquery = new DiffusionPathIDQuery(mpull($changesets, 'getFilename'));
-      $path_ids = $pquery->loadPathIDs();
+      $path_ids = array_flip(mpull($changes, 'getPath'));
       foreach ($changesets as $changeset) {
         $changeset->makeEphemeral();
         $changeset->setID($path_ids[$changeset->getFilename()]);
       }
 
+      if ($count <= self::CHANGES_LIMIT) {
+        $visible_changesets = $changesets;
+      } else {
+        $visible_changesets = array();
+        $inlines = id(new PhabricatorAuditInlineComment())->loadAllWhere(
+          'commitPHID = %s AND (auditCommentID IS NOT NULL OR authorPHID = %s)',
+          $commit->getPHID(),
+          $user->getPHID());
+        $path_ids = mpull($inlines, null, 'getPathID');
+        foreach ($changesets as $key => $changeset) {
+          if (array_key_exists($changeset->getID(), $path_ids)) {
+            $visible_changesets[$key] = $changeset;
+          }
+        }
+      }
+
       $change_list = new DifferentialChangesetListView();
       $change_list->setChangesets($changesets);
-      $change_list->setVisibleChangesets($changesets);
+      $change_list->setVisibleChangesets($visible_changesets);
       $change_list->setRenderingReferences($references);
       $change_list->setRenderURI('/diffusion/'.$callsign.'/diff/');
       $change_list->setRepository($repository);
@@ -266,6 +274,12 @@ public function processRequest() {
       $change_list->setInlineCommentControllerURI(
         '/diffusion/inline/edit/'.phutil_escape_uri($commit->getPHID()).'/');
 
+      $change_references = array();
+      foreach ($changesets as $key => $changeset) {
+        $change_references[$changeset->getID()] = $references[$key];
+      }
+      $change_table->setRenderingReferences($change_references);
+
       // TODO: This is pretty awkward, unify the CSS between Diffusion and
       // Differential better.
       require_celerity_resource('differential-core-view-css');

src/applications/diffusion/query/pathchange/DiffusionPathChangeQuery.php

@@ -80,7 +80,8 @@ protected function executeQuery() {
       $change->setTargetPath(ltrim($raw_change['targetPathName'], '/'));
       $change->setTargetCommitIdentifier($raw_change['targetCommitIdentifier']);
 
-      $changes[] = $change;
+      $id = $raw_change['pathID'];
+      $changes[$id] = $change;
     }
 
     // Deduce the away paths by examining all the changes.

src/applications/diffusion/view/DiffusionCommitChangeTableView.php

@@ -20,6 +20,7 @@ final class DiffusionCommitChangeTableView extends DiffusionView {
 
   private $pathChanges;
   private $ownersPaths = array();
+  private $renderingReferences;
 
   public function setPathChanges(array $path_changes) {
     assert_instances_of($path_changes, 'DiffusionPathChange');
@@ -33,6 +34,11 @@ public function setOwnersPaths(array $owners_paths) {
     return $this;
   }
 
+  public function setRenderingReferences(array $value) {
+    $this->renderingReferences = $value;
+    return $this;
+  }
+
   public function render() {
     $rows = array();
     $rowc = array();
@@ -41,17 +47,22 @@ public function render() {
 
     // TODO: Copy Away and Move Away are rendered junkily still.
 
-    foreach ($this->pathChanges as $change) {
+    foreach ($this->pathChanges as $id => $change) {
       $path = $change->getPath();
       $hash = substr(md5($path), 0, 8);
       if ($change->getFileType() == DifferentialChangeType::FILE_DIRECTORY) {
         $path .= '/';
       }
 
-      $path_column = phutil_render_tag(
+      $path_column = javelin_render_tag(
         'a',
         array(
           'href' => '#'.$hash,
+          'meta' => array(
+            'id' => 'diff-'.$hash,
+            'ref' => $this->renderingReferences[$id],
+          ),
+          'sigil' => 'differential-load',
         ),
         phutil_escape_html($path));
 

webroot/rsrc/js/application/differential/behavior-populate.js

@@ -42,16 +42,24 @@ JX.behavior('differential-populate', function(config) {
     'differential-load',
     function(e) {
       var meta = e.getNodeData('differential-load');
-      JX.DOM.setContent(
-        JX.$(meta.id),
-        JX.$H('<div class="differential-loading">Loading...</div>'));
-      var data = {
-        ref : meta.ref,
-        whitespace : config.whitespace
-      };
-      new JX.Workflow(config.uri, data)
-        .setHandler(JX.bind(null, onresponse, meta.id))
-        .start();
+      var diff;
+      try {
+        diff = JX.$(meta.id);
+      } catch (e) {
+        // Already loaded.
+      }
+      if (diff) {
+        JX.DOM.setContent(
+          diff,
+          JX.$H('<div class="differential-loading">Loading...</div>'));
+        var data = {
+          ref : meta.ref,
+          whitespace : config.whitespace
+        };
+        new JX.Workflow(config.uri, data)
+          .setHandler(JX.bind(null, onresponse, meta.id))
+          .start();
+      }
       if (meta.kill) {
         e.kill();
       }