When showing a diff-of-diffs, hide files which didn't get any more changes and have no inlines

Summary:
Ref T13137. See that task for discussion.

When we show a diff-of-diffs, we often render stubs for files which didn't change between the diffs. These stubs usually aren't a big deal, but for certain types of changes (like refactors) they can create a lot of clutter.

Instead, hide these stubs and show a notice that we hid them.

Test Plan:
  - Created a revision affecting 4 files.
  - Updated it with a diff that changed only 1 of the 4 files.
  - Added an inline comment to a different file.
  - Viewed the diff of diffs.
    - Before: 4 changesets with two "nothing changed" stubs.
    - After: 2 changesets with the stubs hidden.

{F5621083}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13137

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

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -5,6 +5,7 @@ final class DifferentialRevisionViewController
 
   private $revisionID;
   private $changesetCount;
+  private $hiddenChangesets;
 
   public function shouldAllowPublic() {
     return true;
@@ -169,6 +170,7 @@ final class DifferentialRevisionViewController
     }
 
     $handles = $this->loadViewerHandles($object_phids);
+    $warnings = array();
 
     $request_uri = $request->getRequestURI();
 
@@ -203,11 +205,19 @@ final class DifferentialRevisionViewController
             pht('Expand All Files'))),
       );
 
-      $warning = id(new PHUIInfoView())
+      $warnings[] = id(new PHUIInfoView())
         ->setTitle(pht('Large Diff'))
         ->setSeverity(PHUIInfoView::SEVERITY_WARNING)
         ->appendChild($message);
 
+      $folded_changesets = $changesets;
+    } else {
+      $folded_changesets = array();
+    }
+
+    // Don't hide or fold changesets which have inline comments.
+    $hidden_changesets = $this->hiddenChangesets;
+    if ($hidden_changesets || $folded_changesets) {
       $old = array_select_keys($changesets, $old_ids);
       $new = array_select_keys($changesets, $new_ids);
 
@@ -222,16 +232,47 @@ final class DifferentialRevisionViewController
         $new,
         $revision);
 
-      $visible_changesets = array();
       foreach ($inlines as $inline) {
         $changeset_id = $inline->getChangesetID();
-        if (isset($changesets[$changeset_id])) {
+        if (!isset($changesets[$changeset_id])) {
-          $visible_changesets[$changeset_id] = $changesets[$changeset_id];
+          continue;
+        }
+
+        unset($hidden_changesets[$changeset_id]);
+        unset($folded_changesets[$changeset_id]);
       }
     }
-    } else {
+
-      $warning = null;
+    // If we would hide only one changeset, don't hide anything. The notice
-      $visible_changesets = $changesets;
+    // we'd render about it is about the same size as the changeset.
+    if (count($hidden_changesets) < 2) {
+      $hidden_changesets = array();
+    }
+
+    // Update the set of hidden changesets, since we may have just un-hidden
+    // some of them.
+    if ($hidden_changesets) {
+      $warnings[] = id(new PHUIInfoView())
+        ->setTitle(pht('Showing Only Differences'))
+        ->setSeverity(PHUIInfoView::SEVERITY_NOTICE)
+        ->appendChild(
+          pht(
+            'This revision modifies %s more files that are hidden because '.
+            'they were not modified between selected diffs and they have no '.
+            'inline comments.',
+            phutil_count($hidden_changesets)));
+    }
+
+    // Compute the unfolded changesets. By default, everything is unfolded.
+    $unfolded_changesets = $changesets;
+    foreach ($folded_changesets as $changeset_id => $changeset) {
+      unset($unfolded_changesets[$changeset_id]);
+    }
+
+    // Throw away any hidden changesets.
+    foreach ($hidden_changesets as $changeset_id => $changeset) {
+      unset($changesets[$changeset_id]);
+      unset($unfolded_changesets[$changeset_id]);
     }
 
     $commit_hashes = mpull($diffs, 'getSourceControlBaseRevision');
@@ -267,7 +308,7 @@ final class DifferentialRevisionViewController
     if ($repository) {
       $symbol_indexes = $this->buildSymbolIndexes(
         $repository,
-        $visible_changesets);
+        $unfolded_changesets);
     } else {
       $symbol_indexes = array();
     }
@@ -328,7 +369,7 @@ final class DifferentialRevisionViewController
     } else {
       $changeset_view = id(new DifferentialChangesetListView())
         ->setChangesets($changesets)
-        ->setVisibleChangesets($visible_changesets)
+        ->setVisibleChangesets($unfolded_changesets)
         ->setStandaloneURI('/differential/changeset/')
         ->setRawFileURIs(
           '/differential/changeset/?view=old',
@@ -405,7 +446,7 @@ final class DifferentialRevisionViewController
 
       $toc_view = $this->buildTableOfContents(
         $changesets,
-        $visible_changesets,
+        $unfolded_changesets,
         $target->loadCoverageMap($viewer));
 
       // Attach changesets to each reviewer so we can show which Owners package
@@ -520,7 +561,7 @@ final class DifferentialRevisionViewController
 
       $footer[] = array(
         $anchor,
-        $warning,
+        $warnings,
         $tab_view,
         $changeset_view,
       );
@@ -807,6 +848,7 @@ final class DifferentialRevisionViewController
     DifferentialDiff $target,
     DifferentialDiff $diff_vs = null,
     PhabricatorRepository $repository = null) {
+    $viewer = $this->getViewer();
 
     $load_diffs = array($target);
     if ($diff_vs) {
@@ -814,7 +856,7 @@ final class DifferentialRevisionViewController
     }
 
     $raw_changesets = id(new DifferentialChangesetQuery())
-      ->setViewer($this->getRequest()->getUser())
+      ->setViewer($viewer)
       ->withDiffs($load_diffs)
       ->execute();
     $changeset_groups = mgroup($raw_changesets, 'getDiffID');
@@ -825,6 +867,7 @@ final class DifferentialRevisionViewController
     $refs = array();
     $vs_map = array();
     $vs_changesets = array();
+    $must_compare = array();
     if ($diff_vs) {
       $vs_id = $diff_vs->getID();
       $vs_changesets_path_map = array();
@@ -833,6 +876,7 @@ final class DifferentialRevisionViewController
         $vs_changesets_path_map[$path] = $changeset;
         $vs_changesets[$changeset->getID()] = $changeset;
       }
+
       foreach ($changesets as $key => $changeset) {
         $path = $changeset->getAbsoluteRepositoryPath($repository, $target);
         if (isset($vs_changesets_path_map[$path])) {
@@ -841,15 +885,20 @@ final class DifferentialRevisionViewController
           $refs[$changeset->getID()] =
             $changeset->getID().'/'.$vs_changesets_path_map[$path]->getID();
           unset($vs_changesets_path_map[$path]);
+
+          $must_compare[] = $changeset->getID();
+
         } else {
           $refs[$changeset->getID()] = $changeset->getID();
         }
       }
+
       foreach ($vs_changesets_path_map as $path => $changeset) {
         $changesets[$changeset->getID()] = $changeset;
         $vs_map[$changeset->getID()] = -1;
         $refs[$changeset->getID()] = $changeset->getID().'/-1';
       }
+
     } else {
       foreach ($changesets as $changeset) {
         $refs[$changeset->getID()] = $changeset->getID();
@@ -858,13 +907,25 @@ final class DifferentialRevisionViewController
 
     $changesets = msort($changesets, 'getSortKey');
 
+    // See T13137. When displaying the diff between two updates, hide any
+    // changesets which haven't actually changed.
+    $this->hiddenChangesets = array();
+    foreach ($must_compare as $changeset_id) {
+      $changeset = $changesets[$changeset_id];
+      $vs_changeset = $vs_changesets[$vs_map[$changeset_id]];
+
+      if ($changeset->hasSameEffectAs($vs_changeset)) {
+        $this->hiddenChangesets[$changeset_id] = $changesets[$changeset_id];
+      }
+    }
+
     return array($changesets, $vs_map, $vs_changesets, $refs);
   }
 
   private function buildSymbolIndexes(
     PhabricatorRepository $repository,
-    array $visible_changesets) {
+    array $unfolded_changesets) {
-    assert_instances_of($visible_changesets, 'DifferentialChangeset');
+    assert_instances_of($unfolded_changesets, 'DifferentialChangeset');
 
     $engine = PhabricatorSyntaxHighlighter::newEngine();
 
@@ -889,7 +950,7 @@ final class DifferentialRevisionViewController
       $sources);
 
     $indexed_langs = array_fill_keys($langs, true);
-    foreach ($visible_changesets as $key => $changeset) {
+    foreach ($unfolded_changesets as $key => $changeset) {
       $lang = $engine->getLanguageFromFilename($changeset->getFilename());
       if (empty($indexed_langs) || isset($indexed_langs[$lang])) {
         $symbol_indexes[$key] = array(

src/applications/differential/engine/DifferentialChangesetEngine.php

@@ -7,6 +7,7 @@ final class DifferentialChangesetEngine extends Phobject {
 
     foreach ($changesets as $changeset) {
       $this->detectGeneratedCode($changeset);
+      $this->computeHashes($changeset);
     }
 
     $this->detectCopiedCode($changesets);
@@ -59,6 +60,30 @@ final class DifferentialChangesetEngine extends Phobject {
   }
 
 
+/* -(  Content Hashes  )----------------------------------------------------- */
+
+
+  private function computeHashes(DifferentialChangeset $changeset) {
+
+    $effect_key = DifferentialChangeset::METADATA_EFFECT_HASH;
+
+    $effect_hash = $this->newEffectHash($changeset);
+    if ($effect_hash !== null) {
+      $changeset->setChangesetMetadata($effect_key, $effect_hash);
+    }
+  }
+
+  private function newEffectHash(DifferentialChangeset $changeset) {
+
+    if ($changeset->getHunks()) {
+      $new_data = $changeset->makeNewFile();
+      return PhabricatorHash::digestForIndex($new_data);
+    }
+
+    return null;
+  }
+
+
 /* -(  Copied Code  )-------------------------------------------------------- */
 
 

src/applications/differential/management/PhabricatorDifferentialRebuildChangesetsWorkflow.php

@@ -82,6 +82,10 @@ final class PhabricatorDifferentialRebuildChangesetsWorkflow
       id(new DifferentialChangesetEngine())
         ->rebuildChangesets($changesets);
 
+      foreach ($changesets as $changeset) {
+        $changeset->save();
+      }
+
       echo tsprintf(
         "%s\n",
         pht('Done.'));

src/applications/differential/storage/DifferentialChangeset.php

@@ -26,6 +26,7 @@ final class DifferentialChangeset
 
   const METADATA_TRUSTED_ATTRIBUTES = 'attributes.trusted';
   const METADATA_UNTRUSTED_ATTRIBUTES = 'attributes.untrusted';
+  const METADATA_EFFECT_HASH = 'hash.effect';
 
   const ATTRIBUTE_GENERATED = 'generated';
 
@@ -143,6 +144,48 @@ final class DifferentialChangeset
     return $ret;
   }
 
+  /**
+   * Test if this changeset and some other changeset put the affected file in
+   * the same state.
+   *
+   * @param DifferentialChangeset Changeset to compare against.
+   * @return bool True if the two changesets have the same effect.
+   */
+  public function hasSameEffectAs(DifferentialChangeset $other) {
+    if ($this->getFilename() !== $other->getFilename()) {
+      return false;
+    }
+
+    $hash_key = self::METADATA_EFFECT_HASH;
+
+    $u_hash = $this->getChangesetMetadata($hash_key);
+    if ($u_hash === null) {
+      return false;
+    }
+
+    $v_hash = $other->getChangesetMetadata($hash_key);
+    if ($v_hash === null) {
+      return false;
+    }
+
+    if ($u_hash !== $v_hash) {
+      return false;
+    }
+
+    // Make sure the final states for the file properties (like the "+x"
+    // executable bit) match one another.
+    $u_props = $this->getNewProperties();
+    $v_props = $other->getNewProperties();
+    ksort($u_props);
+    ksort($v_props);
+
+    if ($u_props !== $v_props) {
+      return false;
+    }
+
+    return true;
+  }
+
   public function getSortKey() {
     $sort_key = $this->getFilename();
     // Sort files with ".h" in them first, so headers (.h, .hpp) come before