From c12db282519d5402bfe8d72819f52f349ff6a53c Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 27 Apr 2020 08:16:18 -0700
Subject: [PATCH] For changesets that affect binaries, use the new binary file
 content hash as an effect hash

Summary: Ref T13522. When changesets update an image, we currently compute no effect hash. A content hash of the image (or other binary file) is a reasonable effect hash, and enalbes effect-hash-based behavior, including hiding files in intradiffs.

Test Plan:
  - Created a revision affecting `cat.png` and `quack.txt` (currently, there must be 2+ changesets to trigger the hide logic).
  - Updated it with the exact same changes.
  - Viewed revision:
    - Saw the image renderered in the interdiff.
  - Applied patch.
  - Ran `bin/differential rebuild-changesets ...`.
  - Viewed revision:
    - Saw both changesets collapse as unchanged.

Maniphest Tasks: T13522

Differential Revision: https://secure.phabricator.com/D21174
---
 .../engine/DifferentialChangesetEngine.php    | 66 +++++++++++++++++++
 ...rDifferentialRebuildChangesetsWorkflow.php |  1 +
 .../parser/DifferentialChangesetParser.php    |  2 -
 .../storage/DifferentialChangeset.php         | 31 +++++++++
 .../differential/storage/DifferentialDiff.php |  5 ++
 5 files changed, 103 insertions(+), 2 deletions(-)

diff --git a/src/applications/differential/engine/DifferentialChangesetEngine.php b/src/applications/differential/engine/DifferentialChangesetEngine.php
index 23382e6a81..c5c9fc57eb 100644
--- a/src/applications/differential/engine/DifferentialChangesetEngine.php
+++ b/src/applications/differential/engine/DifferentialChangesetEngine.php
@@ -2,9 +2,22 @@
 
 final class DifferentialChangesetEngine extends Phobject {
 
+  private $viewer;
+
+  public function setViewer(PhabricatorUser $viewer) {
+    $this->viewer = $viewer;
+    return $this;
+  }
+
+  public function getViewer() {
+    return $this->viewer;
+  }
+
   public function rebuildChangesets(array $changesets) {
     assert_instances_of($changesets, 'DifferentialChangeset');
 
+    $changesets = $this->loadChangesetFiles($changesets);
+
     foreach ($changesets as $changeset) {
       $this->detectGeneratedCode($changeset);
       $this->computeHashes($changeset);
@@ -13,6 +26,45 @@ final class DifferentialChangesetEngine extends Phobject {
     $this->detectCopiedCode($changesets);
   }
 
+  private function loadChangesetFiles(array $changesets) {
+    $viewer = $this->getViewer();
+
+    $file_phids = array();
+    foreach ($changesets as $changeset) {
+      $file_phid = $changeset->getNewFileObjectPHID();
+      if ($file_phid !== null) {
+        $file_phids[] = $file_phid;
+      }
+    }
+
+    if ($file_phids) {
+      $files = id(new PhabricatorFileQuery())
+        ->setViewer($viewer)
+        ->withPHIDs($file_phids)
+        ->execute();
+      $files = mpull($files, null, 'getPHID');
+    } else {
+      $files = array();
+    }
+
+    foreach ($changesets as $changeset_key => $changeset) {
+      $file_phid = $changeset->getNewFileObjectPHID();
+      if ($file_phid === null) {
+        continue;
+      }
+
+      $file = idx($files, $file_phid);
+      if (!$file) {
+        unset($changesets[$changeset_key]);
+        continue;
+      }
+
+      $changeset->attachNewFileObject($file);
+    }
+
+    return $changesets;
+  }
+
 
 /* -(  Generated Code  )----------------------------------------------------- */
 
@@ -86,6 +138,20 @@ final class DifferentialChangesetEngine extends Phobject {
       return PhabricatorHash::digestForIndex($new_data);
     }
 
+    if ($changeset->getNewFileObjectPHID()) {
+      $file = $changeset->getNewFileObject();
+
+      // See T13522. For now, the "contentHash" is not really a content hash
+      // for files >4MB. This is okay: we will just always detect them as
+      // changed, which is the safer behavior.
+
+      $hash = $file->getContentHash();
+      if ($hash !== null) {
+        $hash = sprintf('file-hash:%s', $hash);
+        return PhabricatorHash::digestForIndex($hash);
+      }
+    }
+
     return null;
   }
 
diff --git a/src/applications/differential/management/PhabricatorDifferentialRebuildChangesetsWorkflow.php b/src/applications/differential/management/PhabricatorDifferentialRebuildChangesetsWorkflow.php
index 068771284b..80a8ac29a8 100644
--- a/src/applications/differential/management/PhabricatorDifferentialRebuildChangesetsWorkflow.php
+++ b/src/applications/differential/management/PhabricatorDifferentialRebuildChangesetsWorkflow.php
@@ -80,6 +80,7 @@ final class PhabricatorDifferentialRebuildChangesetsWorkflow
       }
 
       id(new DifferentialChangesetEngine())
+        ->setViewer($viewer)
         ->rebuildChangesets($changesets);
 
       foreach ($changesets as $changeset) {
diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php
index 8cee8013a6..850a7e26d1 100644
--- a/src/applications/differential/parser/DifferentialChangesetParser.php
+++ b/src/applications/differential/parser/DifferentialChangesetParser.php
@@ -1827,8 +1827,6 @@ final class DifferentialChangesetParser extends Phobject {
 
     if (!$vs) {
       $metadata = $this->changeset->getMetadata();
-      $data = idx($metadata, 'attachment-data');
-
       $old_phid = idx($metadata, 'old:binary-phid');
       $new_phid = idx($metadata, 'new:binary-phid');
     } else {
diff --git a/src/applications/differential/storage/DifferentialChangeset.php b/src/applications/differential/storage/DifferentialChangeset.php
index f580464fc5..682dffdd88 100644
--- a/src/applications/differential/storage/DifferentialChangeset.php
+++ b/src/applications/differential/storage/DifferentialChangeset.php
@@ -25,6 +25,9 @@ final class DifferentialChangeset
   private $authorityPackages;
   private $changesetPackages;
 
+  private $newFileObject = self::ATTACHABLE;
+  private $oldFileObject = self::ATTACHABLE;
+
   const TABLE_CACHE = 'differential_changeset_parse_cache';
 
   const METADATA_TRUSTED_ATTRIBUTES = 'attributes.trusted';
@@ -459,6 +462,34 @@ final class DifferentialChangeset
     return $this->getChangesetAttribute(self::ATTRIBUTE_GENERATED);
   }
 
+  public function getNewFileObjectPHID() {
+    $metadata = $this->getMetadata();
+    return idx($metadata, 'new:binary-phid');
+  }
+
+  public function getOldFileObjectPHID() {
+    $metadata = $this->getMetadata();
+    return idx($metadata, 'old:binary-phid');
+  }
+
+  public function attachNewFileObject(PhabricatorFile $file) {
+    $this->newFileObject = $file;
+    return $this;
+  }
+
+  public function getNewFileObject() {
+    return $this->assertAttached($this->newFileObject);
+  }
+
+  public function attachOldFileObject(PhabricatorFile $file) {
+    $this->oldFileObject = $file;
+    return $this;
+  }
+
+  public function getOldFileObject() {
+    return $this->assertAttached($this->oldFileObject);
+  }
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 
diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php
index 1922617ce8..747407ce75 100644
--- a/src/applications/differential/storage/DifferentialDiff.php
+++ b/src/applications/differential/storage/DifferentialDiff.php
@@ -232,7 +232,12 @@ final class DifferentialDiff
 
     $changesets = $diff->getChangesets();
 
+    // TODO: This is "safe", but it would be better to propagate a real user
+    // down the stack.
+    $viewer = PhabricatorUser::getOmnipotentUser();
+
     id(new DifferentialChangesetEngine())
+      ->setViewer($viewer)
       ->rebuildChangesets($changesets);
 
     return $diff;