Let ghostly inlines automatically expand changesets in enormous diffs

Summary:
Ref T7447. Ref T7870. When a diff affects more than 100 files, we collapse files by default, then expand files with inlines.

Ghostly inlines currently don't cause files to expand, but reasonably should.

Test Plan:
Before:

{F378065}

After:

{F378066}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: yelirekim, epriestley

Maniphest Tasks: T7447, T7870

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

src/applications/differential/controller/DifferentialChangesetViewController.php

@@ -65,6 +65,8 @@ final class DifferentialChangesetViewController extends DifferentialController {
       }
     }
 
+    $old = array();
+    $new = array();
     if (!$vs) {
       $right = $changeset;
       $left  = null;
@@ -75,6 +77,9 @@ final class DifferentialChangesetViewController extends DifferentialController {
       $left_new = false;
 
       $render_cache_key = $right->getID();
+
+      $old[] = $changeset;
+      $new[] = $changeset;
     } else if ($vs == -1) {
       $right = null;
       $left = $changeset;
@@ -85,6 +90,9 @@ final class DifferentialChangesetViewController extends DifferentialController {
       $left_new = true;
 
       $render_cache_key = null;
+
+      $old[] = $changeset;
+      $new[] = $changeset;
     } else {
       $right = $changeset;
       $left = $vs_changeset;
@@ -95,6 +103,9 @@ final class DifferentialChangesetViewController extends DifferentialController {
       $left_new = true;
 
       $render_cache_key = null;
+
+      $new[] = $left;
+      $new[] = $right;
     }
 
     if ($left) {
@@ -177,12 +188,11 @@ final class DifferentialChangesetViewController extends DifferentialController {
 
     // Load both left-side and right-side inline comments.
     if ($revision) {
-      $inlines = $this->loadInlineComments(
+      $query = id(new DifferentialInlineCommentQuery())
-        $revision,
+        ->setViewer($viewer)
-        array(nonempty($left, $right)),
+        ->withRevisionPHIDs(array($revision->getPHID()));
-        $left_new,
+      $inlines = $query->execute();
-        array($right),
+      $inlines = $query->adjustInlinesForChangesets($inlines, $old, $new);
-        $right_new);
     } else {
       $inlines = array();
     }
@@ -285,199 +295,6 @@ final class DifferentialChangesetViewController extends DifferentialController {
       ));
   }
 
-  private function loadInlineComments(
-    DifferentialRevision $revision,
-    array $left,
-    $left_new,
-    array $right,
-    $right_new) {
-
-    assert_instances_of($left, 'DifferentialChangeset');
-    assert_instances_of($right, 'DifferentialChangeset');
-
-    $viewer = $this->getViewer();
-    $all = array_merge($left, $right);
-
-    $inlines = id(new DifferentialInlineCommentQuery())
-      ->setViewer($viewer)
-      ->withRevisionPHIDs(array($revision->getPHID()))
-      ->execute();
-
-    $changeset_ids = mpull($inlines, 'getChangesetID');
-    $changeset_ids = array_unique($changeset_ids);
-    if ($changeset_ids) {
-      $changesets = id(new DifferentialChangesetQuery())
-        ->setViewer($viewer)
-        ->withIDs($changeset_ids)
-        ->execute();
-      $changesets = mpull($changesets, null, 'getID');
-    } else {
-      $changesets = array();
-    }
-    $changesets += mpull($all, null, 'getID');
-
-    $id_map = array();
-    foreach ($all as $changeset) {
-      $id_map[$changeset->getID()] = $changeset->getID();
-    }
-
-    // Generate filename maps for older and newer comments. If we're bringing
-    // an older comment forward in a diff-of-diffs, we want to put it on the
-    // left side of the screen, not the right side. Both sides are "new" files
-    // with the same name, so they're both appropriate targets, but the left
-    // is a better target conceptually for users because it's more consistent
-    // with the rest of the UI, which shows old information on the left and
-    // new information on the right.
-    $move_here = DifferentialChangeType::TYPE_MOVE_HERE;
-
-    $name_map_old = array();
-    $name_map_new = array();
-    $move_map = array();
-    foreach ($all as $changeset) {
-      $changeset_id = $changeset->getID();
-
-      $filenames = array();
-      $filenames[] = $changeset->getFilename();
-
-      // If this is the target of a move, also map comments on the old filename
-      // to this changeset.
-      if ($changeset->getChangeType() == $move_here) {
-        $old_file = $changeset->getOldFile();
-        $filenames[] = $old_file;
-        $move_map[$changeset_id][$old_file] = true;
-      }
-
-      foreach ($filenames as $filename) {
-        // We update the old map only if we don't already have an entry (oldest
-        // changeset persists).
-        if (empty($name_map_old[$filename])) {
-          $name_map_old[$filename] = $changeset_id;
-        }
-
-        // We always update the new map (newest changeset overwrites).
-        $name_map_new[$changeset->getFilename()] = $changeset_id;
-      }
-    }
-
-    // Find the smallest "new" changeset ID. We'll consider everything
-    // larger than this to be "newer", and everything smaller to be "older".
-    $first_new_id = min(mpull($right, 'getID'));
-
-    $results = array();
-    foreach ($inlines as $inline) {
-      $changeset_id = $inline->getChangesetID();
-      if (isset($id_map[$changeset_id])) {
-        // This inline is legitimately on one of the current changesets, so
-        // we can include it in the result set unmodified.
-        $results[] = $inline;
-        continue;
-      }
-
-      $changeset = idx($changesets, $changeset_id);
-      if (!$changeset) {
-        // Just discard this inline, as it has bogus data.
-        continue;
-      }
-
-      $target_id = null;
-
-      if ($changeset_id >= $first_new_id) {
-        $name_map = $name_map_new;
-        $is_new = true;
-      } else {
-        $name_map = $name_map_old;
-        $is_new = false;
-      }
-
-      $filename = $changeset->getFilename();
-      if (isset($name_map[$filename])) {
-        // This changeset is on a file with the same name as the current
-        // changeset, so we're going to port it forward or backward.
-        $target_id = $name_map[$filename];
-
-        $is_move = isset($move_map[$target_id][$filename]);
-        if ($is_new) {
-          if ($is_move) {
-            $reason = pht(
-              'This comment was made on a file with the same name as the '.
-              'file this file was moved from, but in a newer diff.');
-          } else {
-            $reason = pht(
-              'This comment was made on a file with the same name, but '.
-              'in a newer diff.');
-          }
-        } else {
-          if ($is_move) {
-            $reason = pht(
-              'This comment was made on a file with the same name as the '.
-              'file this file was moved from, but in an older diff.');
-          } else {
-            $reason = pht(
-              'This comment was made on a file with the same name, but '.
-              'in an older diff.');
-          }
-        }
-      }
-
-
-      // If we didn't find a target and this change is the target of a move,
-      // look for a match against the old filename.
-      if (!$target_id) {
-        if ($changeset->getChangeType() == $move_here) {
-          $filename = $changeset->getOldFile();
-          if (isset($name_map[$filename])) {
-            $target_id = $name_map[$filename];
-            if ($is_new) {
-              $reason = pht(
-                'This comment was made on a file which this file was moved '.
-                'to, but in a newer diff.');
-            } else {
-              $reason = pht(
-                'This comment was made on a file which this file was moved '.
-                'to, but in an older diff.');
-            }
-          }
-        }
-      }
-
-
-      // If we found a changeset to port this comment to, bring it forward
-      // or backward and mark it.
-      if ($target_id) {
-        $inline
-          ->makeEphemeral(true)
-          ->setChangesetID($target_id)
-          ->setIsGhost(
-            array(
-              'new' => $is_new,
-              'reason' => $reason,
-            ));
-
-        $results[] = $inline;
-      }
-    }
-
-    // Filter out the inlines we ported forward which won't be visible because
-    // they appear on the wrong side of a file.
-    $keep_map = array();
-    foreach ($left as $changeset) {
-      $keep_map[$changeset->getID()][(int)$left_new] = true;
-    }
-    foreach ($right as $changeset) {
-      $keep_map[$changeset->getID()][(int)$right_new] = true;
-    }
-    foreach ($results as $key => $inline) {
-      $is_new = (int)$inline->getIsNewFile();
-      $changeset_id = $inline->getChangesetID();
-      if (!isset($keep_map[$changeset_id][$is_new])) {
-        unset($results[$key]);
-        continue;
-      }
-    }
-
-    return $results;
-  }
-
   private function buildRawFileResponse(
     DifferentialChangeset $changeset,
     $is_new) {

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -95,9 +95,6 @@ final class DifferentialRevisionViewController extends DifferentialController {
       $target_manual->getID());
     $props = mpull($props, 'getData', 'getName');
 
-    $all_changesets = $changesets;
-    $inlines = $revision->loadInlineComments($all_changesets, $user);
-
     $object_phids = array_merge(
       $revision->getReviewers(),
       $revision->getCCPHIDs(),
@@ -157,15 +154,33 @@ final class DifferentialRevisionViewController extends DifferentialController {
           pht('Show All Files Inline'))));
       $warning = $warning->render();
 
-      $my_inlines = id(new DifferentialInlineCommentQuery())
+      $map = $vs_map;
+      if (!$map) {
+        $map = array_fill_keys(array_keys($changesets), 0);
+      }
+
+      $old = array();
+      $new = array();
+      foreach ($map as $id => $vs) {
+        if ($vs <= 0) {
+          $old[] = $id;
+          $new[] = $id;
+        } else {
+          $new[] = $id;
+          $new[] = $vs;
+        }
+      }
+      $old = array_select_keys($changesets, $old);
+      $new = array_select_keys($changesets, $new);
+
+      $query = id(new DifferentialInlineCommentQuery())
         ->setViewer($user)
-        ->withDrafts(true)
+        ->withRevisionPHIDs(array($revision->getPHID()));
-        ->withAuthorPHIDs(array($user->getPHID()))
+      $inlines = $query->execute();
-        ->withRevisionPHIDs(array($revision->getPHID()))
+      $inlines = $query->adjustInlinesForChangesets($inlines, $old, $new);
-        ->execute();
 
       $visible_changesets = array();
-      foreach ($inlines + $my_inlines as $inline) {
+      foreach ($inlines as $inline) {
         $changeset_id = $inline->getChangesetID();
         if (isset($changesets[$changeset_id])) {
           $visible_changesets[$changeset_id] = $changesets[$changeset_id];
@@ -270,8 +285,7 @@ final class DifferentialRevisionViewController extends DifferentialController {
     $comment_view = $this->buildTransactions(
       $revision,
       $diff_vs ? $diffs[$diff_vs] : $target,
-      $target,
+      $target);
-      $all_changesets);
 
     if (!$viewer_is_anonymous) {
       $comment_view->setQuoteRef('D'.$revision->getID());
@@ -917,8 +931,7 @@ final class DifferentialRevisionViewController extends DifferentialController {
   private function buildTransactions(
     DifferentialRevision $revision,
     DifferentialDiff $left_diff,
-    DifferentialDiff $right_diff,
+    DifferentialDiff $right_diff) {
-    array $changesets) {
 
     $timeline = $this->buildTransactionTimeline(
       $revision,

src/applications/differential/query/DifferentialInlineCommentQuery.php

@@ -137,4 +137,192 @@ final class DifferentialInlineCommentQuery
     return $this->formatWhereClause($where);
   }
 
+  public function adjustInlinesForChangesets(
+    array $inlines,
+    array $old,
+    array $new) {
+
+    assert_instances_of($inlines, 'DifferentialInlineComment');
+    assert_instances_of($old, 'DifferentialChangeset');
+    assert_instances_of($new, 'DifferentialChangeset');
+
+    $viewer = $this->getViewer();
+    $all = array_merge($old, $new);
+
+    $changeset_ids = mpull($inlines, 'getChangesetID');
+    $changeset_ids = array_unique($changeset_ids);
+    if ($changeset_ids) {
+      $changesets = id(new DifferentialChangesetQuery())
+        ->setViewer($viewer)
+        ->withIDs($changeset_ids)
+        ->execute();
+      $changesets = mpull($changesets, null, 'getID');
+    } else {
+      $changesets = array();
+    }
+    $changesets += mpull($all, null, 'getID');
+
+    $id_map = array();
+    foreach ($all as $changeset) {
+      $id_map[$changeset->getID()] = $changeset->getID();
+    }
+
+    // Generate filename maps for older and newer comments. If we're bringing
+    // an older comment forward in a diff-of-diffs, we want to put it on the
+    // left side of the screen, not the right side. Both sides are "new" files
+    // with the same name, so they're both appropriate targets, but the left
+    // is a better target conceptually for users because it's more consistent
+    // with the rest of the UI, which shows old information on the left and
+    // new information on the right.
+    $move_here = DifferentialChangeType::TYPE_MOVE_HERE;
+
+    $name_map_old = array();
+    $name_map_new = array();
+    $move_map = array();
+    foreach ($all as $changeset) {
+      $changeset_id = $changeset->getID();
+
+      $filenames = array();
+      $filenames[] = $changeset->getFilename();
+
+      // If this is the target of a move, also map comments on the old filename
+      // to this changeset.
+      if ($changeset->getChangeType() == $move_here) {
+        $old_file = $changeset->getOldFile();
+        $filenames[] = $old_file;
+        $move_map[$changeset_id][$old_file] = true;
+      }
+
+      foreach ($filenames as $filename) {
+        // We update the old map only if we don't already have an entry (oldest
+        // changeset persists).
+        if (empty($name_map_old[$filename])) {
+          $name_map_old[$filename] = $changeset_id;
+        }
+
+        // We always update the new map (newest changeset overwrites).
+        $name_map_new[$changeset->getFilename()] = $changeset_id;
+      }
+    }
+
+    // Find the smallest "new" changeset ID. We'll consider everything
+    // larger than this to be "newer", and everything smaller to be "older".
+    $first_new_id = min(mpull($new, 'getID'));
+
+    $results = array();
+    foreach ($inlines as $inline) {
+      $changeset_id = $inline->getChangesetID();
+      if (isset($id_map[$changeset_id])) {
+        // This inline is legitimately on one of the current changesets, so
+        // we can include it in the result set unmodified.
+        $results[] = $inline;
+        continue;
+      }
+
+      $changeset = idx($changesets, $changeset_id);
+      if (!$changeset) {
+        // Just discard this inline, as it has bogus data.
+        continue;
+      }
+
+      $target_id = null;
+
+      if ($changeset_id >= $first_new_id) {
+        $name_map = $name_map_new;
+        $is_new = true;
+      } else {
+        $name_map = $name_map_old;
+        $is_new = false;
+      }
+
+      $filename = $changeset->getFilename();
+      if (isset($name_map[$filename])) {
+        // This changeset is on a file with the same name as the current
+        // changeset, so we're going to port it forward or backward.
+        $target_id = $name_map[$filename];
+
+        $is_move = isset($move_map[$target_id][$filename]);
+        if ($is_new) {
+          if ($is_move) {
+            $reason = pht(
+              'This comment was made on a file with the same name as the '.
+              'file this file was moved from, but in a newer diff.');
+          } else {
+            $reason = pht(
+              'This comment was made on a file with the same name, but '.
+              'in a newer diff.');
+          }
+        } else {
+          if ($is_move) {
+            $reason = pht(
+              'This comment was made on a file with the same name as the '.
+              'file this file was moved from, but in an older diff.');
+          } else {
+            $reason = pht(
+              'This comment was made on a file with the same name, but '.
+              'in an older diff.');
+          }
+        }
+      }
+
+
+      // If we didn't find a target and this change is the target of a move,
+      // look for a match against the old filename.
+      if (!$target_id) {
+        if ($changeset->getChangeType() == $move_here) {
+          $filename = $changeset->getOldFile();
+          if (isset($name_map[$filename])) {
+            $target_id = $name_map[$filename];
+            if ($is_new) {
+              $reason = pht(
+                'This comment was made on a file which this file was moved '.
+                'to, but in a newer diff.');
+            } else {
+              $reason = pht(
+                'This comment was made on a file which this file was moved '.
+                'to, but in an older diff.');
+            }
+          }
+        }
+      }
+
+
+      // If we found a changeset to port this comment to, bring it forward
+      // or backward and mark it.
+      if ($target_id) {
+        $inline
+          ->makeEphemeral(true)
+          ->setChangesetID($target_id)
+          ->setIsGhost(
+            array(
+              'new' => $is_new,
+              'reason' => $reason,
+            ));
+
+        $results[] = $inline;
+      }
+    }
+
+    // Filter out the inlines we ported forward which won't be visible because
+    // they appear on the wrong side of a file.
+    $keep_map = array();
+    foreach ($old as $changeset) {
+      $keep_map[$changeset->getID()][0] = true;
+    }
+    foreach ($new as $changeset) {
+      $keep_map[$changeset->getID()][1] = true;
+    }
+
+    foreach ($results as $key => $inline) {
+      $is_new = (int)$inline->getIsNewFile();
+      $changeset_id = $inline->getChangesetID();
+      if (!isset($keep_map[$changeset_id][$is_new])) {
+        unset($results[$key]);
+        continue;
+      }
+    }
+
+    return $results;
+  }
+
 }