Improve the construction of synthetic "comparison/intradiff" changesets

Summary:
Ref T13523. Currently, when building a "comparison" changeset, metadata is taken from the left changeset. This is somewhat arbitrary.

This means that intradiffs of images don't work properly because the rendered changeset has only the left (usually "old") information.

Later, some of the code attempts to ignore the file data stored on the changeset and reconstruct the correct file data, which is how the result ends up not-completely-wrong.

Be more careful about building sensible-ish metadata, and then just use it directly later on. This fixes the "spooky" code referencing D955 + D6851.

There are some related issues, where "change type" and "file type" are selected arbitrarily and then used to determine whether the change has an "old/new" state or not (i.e., is the left side of the diff empty, since the change creates the file)?

In many cases, neither of the original changesets have a "change type" which will answer this question correctly. Separate this concept from "has state" from "change type", and make more of the code ask narrower questions about the specific conditions or states it cares about, rather than "change type".

Test Plan:
  - Created a revision with Diff 1, Diff 2, and Diff 3. Diff 1 takes an image from "null -> A". Diff 2 takes the same image from "null -> B". Diff 3 takes the same image from "A -> B'.
  - Intradiffed 1v2 and 1v3.
  - Before patch:
    - Left side usually missing, which is incorrect (should always be "A").
    - Change properties are a mess ("null -> image/png" for MIME type, e.g.)
    - Uninteresting/incorrect "unix:filemode" stuff.
  - After patch;
    - Left side shows state "A".
    - Change properties only show size changes (which is correct).

{F7402012}

Maniphest Tasks: T13523

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

src/applications/differential/controller/DifferentialChangesetViewController.php

@@ -109,28 +109,7 @@ final class DifferentialChangesetViewController extends DifferentialController {
     }
 
     if ($left) {
-      $left_data = $left->makeNewFile();
-      $left_properties = $left->getNewProperties();
-      if ($right) {
-        $right_data = $right->makeNewFile();
-        $right_properties = $right->getNewProperties();
-      } else {
-        $right_data = $left->makeOldFile();
-        $right_properties = $left->getOldProperties();
-      }
-
-      $engine = new PhabricatorDifferenceEngine();
-      $synthetic = $engine->generateChangesetFromFileContent(
-        $left_data,
-        $right_data);
-
-      $choice = clone nonempty($left, $right);
-      $choice->attachHunks($synthetic->getHunks());
-
-      $choice->setOldProperties($left_properties);
-      $choice->setNewProperties($right_properties);
-
-      $changeset = $choice;
+      $changeset = $left->newComparisonChangeset($right);
     }
 
     if ($left_new || $right_new) {

src/applications/differential/parser/DifferentialChangesetParser.php

@@ -1694,27 +1694,10 @@ final class DifferentialChangesetParser extends Phobject {
     $changeset = $this->changeset;
     $viewer = $this->getViewer();
 
-    // TODO: This should probably be made non-optional in the future.
-    if (!$viewer) {
-      return null;
-    }
-
-    $old_file = null;
-    $new_file = null;
-
-    switch ($changeset->getFileType()) {
-      case DifferentialChangeType::FILE_IMAGE:
-      case DifferentialChangeType::FILE_BINARY:
     list($old_file, $new_file) = $this->loadFileObjectsForChangeset();
-        break;
-    }
 
-    $type_delete = DifferentialChangeType::TYPE_DELETE;
-    $type_add = DifferentialChangeType::TYPE_ADD;
-    $change_type = $changeset->getChangeType();
-
-    $no_old = ($change_type == $type_add);
-    $no_new = ($change_type == $type_delete);
+    $no_old = !$changeset->hasOldState();
+    $no_new = !$changeset->hasNewState();
 
     if ($no_old) {
       $old_ref = null;
@@ -1742,7 +1725,6 @@ final class DifferentialChangesetParser extends Phobject {
       }
     }
 
-
     $old_engines = null;
     if ($old_ref) {
       $old_engines = PhabricatorDocumentEngine::getEnginesForRef(
@@ -1813,44 +1795,12 @@ final class DifferentialChangesetParser extends Phobject {
     $changeset = $this->changeset;
     $viewer = $this->getViewer();
 
+    $old_phid = $changeset->getOldFileObjectPHID();
+    $new_phid = $changeset->getNewFileObjectPHID();
+
     $old_file = null;
     $new_file = null;
 
-    // TODO: Improve the architectural issue as discussed in D955
-    // https://secure.phabricator.com/D955
-    $reference = $this->getRenderingReference();
-    $parts = explode('/', $reference);
-    if (count($parts) == 2) {
-      list($id, $vs) = $parts;
-    } else {
-      $id = $parts[0];
-      $vs = 0;
-    }
-    $id = (int)$id;
-    $vs = (int)$vs;
-
-    if (!$vs) {
-      $metadata = $this->changeset->getMetadata();
-      $old_phid = idx($metadata, 'old:binary-phid');
-      $new_phid = idx($metadata, 'new:binary-phid');
-    } else {
-      $vs_changeset = id(new DifferentialChangeset())->load($vs);
-      $old_phid = null;
-      $new_phid = null;
-
-      // TODO: This is spooky, see D6851
-      if ($vs_changeset) {
-        $vs_metadata = $vs_changeset->getMetadata();
-        $old_phid = idx($vs_metadata, 'new:binary-phid');
-      }
-
-      $changeset = id(new DifferentialChangeset())->load($id);
-      if ($changeset) {
-        $metadata = $changeset->getMetadata();
-        $new_phid = idx($metadata, 'new:binary-phid');
-      }
-    }
-
     if ($old_phid || $new_phid) {
       $file_phids = array();
       if ($old_phid) {
@@ -1864,13 +1814,28 @@ final class DifferentialChangesetParser extends Phobject {
         ->setViewer($viewer)
         ->withPHIDs($file_phids)
         ->execute();
+      $files = mpull($files, null, 'getPHID');
 
-      foreach ($files as $file) {
-        if ($file->getPHID() == $old_phid) {
-          $old_file = $file;
-        } else if ($file->getPHID() == $new_phid) {
-          $new_file = $file;
+      if ($old_phid) {
+        $old_file = idx($files, $old_phid);
+        if (!$old_file) {
+          throw new Exception(
+            pht(
+              'Failed to load file data for changeset ("%s").',
+              $old_phid));
         }
+        $changeset->attachOldFileObject($old_file);
+      }
+
+      if ($new_phid) {
+        $new_file = idx($files, $new_phid);
+        if (!$new_file) {
+          throw new Exception(
+            pht(
+              'Failed to load file data for changeset ("%s").',
+              $new_phid));
+        }
+        $changeset->attachNewFileObject($new_file);
       }
     }
 

src/applications/differential/render/DifferentialChangesetRenderer.php

@@ -647,16 +647,26 @@ abstract class DifferentialChangesetRenderer extends Phobject {
     $old = $changeset->getOldProperties();
     $new = $changeset->getNewProperties();
 
-    // When adding files, don't show the uninteresting 644 filemode change.
-    if ($changeset->getChangeType() == DifferentialChangeType::TYPE_ADD &&
-        $new == array('unix:filemode' => '100644')) {
-      unset($new['unix:filemode']);
-    }
+    // If a property has been changed, but is not present on one side of the
+    // change and has an uninteresting default value on the other, remove it.
+    // This most commonly happens when a change adds or removes a file: the
+    // side of the change with the file has a "100644" filemode in Git.
 
-    // Likewise when removing files.
-    if ($changeset->getChangeType() == DifferentialChangeType::TYPE_DELETE &&
-        $old == array('unix:filemode' => '100644')) {
-      unset($old['unix:filemode']);
+    $defaults = array(
+      'unix:filemode' => '100644',
+    );
+
+    foreach ($defaults as $default_key => $default_value) {
+      $old_value = idx($old, $default_key, $default_value);
+      $new_value = idx($new, $default_key, $default_value);
+
+      $old_default = ($old_value === $default_value);
+      $new_default = ($new_value === $default_value);
+
+      if ($old_default && $new_default) {
+        unset($old[$default_key]);
+        unset($new[$default_key]);
+      }
     }
 
     $metadata = $changeset->getMetadata();

src/applications/differential/storage/DifferentialChangeset.php

@@ -28,6 +28,11 @@ final class DifferentialChangeset
   private $newFileObject = self::ATTACHABLE;
   private $oldFileObject = self::ATTACHABLE;
 
+  private $hasOldState;
+  private $hasNewState;
+  private $oldStateMetadata;
+  private $newStateMetadata;
+
   const TABLE_CACHE = 'differential_changeset_parse_cache';
 
   const METADATA_TRUSTED_ATTRIBUTES = 'attributes.trusted';
@@ -134,6 +139,34 @@ final class DifferentialChangeset
     return $this->changesetPackages;
   }
 
+  public function setHasOldState($has_old_state) {
+    $this->hasOldState = $has_old_state;
+    return $this;
+  }
+
+  public function setHasNewState($has_new_state) {
+    $this->hasNewState = $has_new_state;
+    return $this;
+  }
+
+  public function hasOldState() {
+    if ($this->hasOldState !== null) {
+      return $this->hasOldState;
+    }
+
+    $change_type = $this->getChangeType();
+    return !DifferentialChangeType::isCreateChangeType($change_type);
+  }
+
+  public function hasNewState() {
+    if ($this->hasNewState !== null) {
+      return $this->hasNewState;
+    }
+
+    $change_type = $this->getChangeType();
+    return !DifferentialChangeType::isDeleteChangeType($change_type);
+  }
+
   public function save() {
     $this->openTransaction();
       $ret = parent::save();
@@ -490,6 +523,110 @@ final class DifferentialChangeset
     return $this->assertAttached($this->oldFileObject);
   }
 
+  public function newComparisonChangeset(
+    DifferentialChangeset $against = null) {
+
+    $left = $this;
+    $right = $against;
+
+    $left_data = $left->makeNewFile();
+    $left_properties = $left->getNewProperties();
+    $left_metadata = $left->getNewStateMetadata();
+    $left_state = $left->hasNewState();
+    $shared_metadata = $left->getMetadata();
+    if ($right) {
+      $right_data = $right->makeNewFile();
+      $right_properties = $right->getNewProperties();
+      $right_metadata = $right->getNewStateMetadata();
+      $right_state = $right->hasNewState();
+      $shared_metadata = $right->getMetadata();
+    } else {
+      $right_data = $left->makeOldFile();
+      $right_properties = $left->getOldProperties();
+      $right_metadata = $left->getOldStateMetadata();
+      $right_state = $left->hasOldState();
+    }
+
+    $engine = new PhabricatorDifferenceEngine();
+
+    $synthetic = $engine->generateChangesetFromFileContent(
+      $left_data,
+      $right_data);
+
+    $comparison = id(new self())
+      ->makeEphemeral(true)
+      ->attachDiff($left->getDiff())
+      ->setOldFile($left->getFilename())
+      ->setFilename($right->getFilename());
+
+    // TODO: Change type?
+    // TODO: File type?
+    // TODO: Away paths?
+    // TODO: View state key?
+
+    $comparison->attachHunks($synthetic->getHunks());
+
+    $comparison->setOldProperties($left_properties);
+    $comparison->setNewProperties($right_properties);
+
+    $comparison
+      ->setOldStateMetadata($left_metadata)
+      ->setNewStateMetadata($right_metadata)
+      ->setHasOldState($left_state)
+      ->setHasNewState($right_state);
+
+    // NOTE: Some metadata is not stored statefully, like the "generated"
+    // flag. For now, use the rightmost "new state" metadata to fill in these
+    // values.
+
+    $metadata = $comparison->getMetadata();
+    $metadata = $metadata + $shared_metadata;
+    $comparison->setMetadata($metadata);
+
+    return $comparison;
+  }
+
+  public function getNewStateMetadata() {
+    return $this->getMetadataWithPrefix('new:');
+  }
+
+  public function setNewStateMetadata(array $metadata) {
+    return $this->setMetadataWithPrefix($metadata, 'new:');
+  }
+
+  public function getOldStateMetadata() {
+    return $this->getMetadataWithPrefix('old:');
+  }
+
+  public function setOldStateMetadata(array $metadata) {
+    return $this->setMetadataWithPrefix($metadata, 'old:');
+  }
+
+  private function getMetadataWithPrefix($prefix) {
+    $length = strlen($prefix);
+
+    $result = array();
+    foreach ($this->getMetadata() as $key => $value) {
+      if (strncmp($key, $prefix, $length)) {
+        continue;
+      }
+
+      $key = substr($key, $length);
+      $result[$key] = $value;
+    }
+
+    return $result;
+  }
+
+  private function setMetadataWithPrefix(array $metadata, $prefix) {
+    foreach ($metadata as $key => $value) {
+      $key = $prefix.$key;
+      $this->metadata[$key] = $value;
+    }
+
+    return $this;
+  }
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */