From 17426a60f00aa09d0483a99fc92474e785baa0e7 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 4 May 2020 07:35:21 -0700
Subject: [PATCH] Fix an issue where text intradiff bodies may not render

Summary:
Ref T13523. In the caching layer, there's a tricky clause about filetypes that skips some body rendering behavior.

Provide file type information which at least has a better chance of representing all changes (e.g., an image file may be replaced with a text file, but this can not be represented by a single file type).

Formalize "hasSourceTextBody()", to mean the changeset parser should engage the change as source text.

Test Plan: Intradiffed text changes, saw the body render properly.

Maniphest Tasks: T13523

Differential Revision: https://secure.phabricator.com/D21210
---
 .../parser/DifferentialChangesetParser.php    | 28 ++++++-----
 .../storage/DifferentialChangeset.php         | 49 ++++++++++++++++++-
 2 files changed, 62 insertions(+), 15 deletions(-)

diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php
index 686979bca0..fb7c9394af 100644
--- a/src/applications/differential/parser/DifferentialChangesetParser.php
+++ b/src/applications/differential/parser/DifferentialChangesetParser.php
@@ -592,6 +592,17 @@ final class DifferentialChangesetParser extends Phobject {
   }
 
   private function tryCacheStuff() {
+    $changeset = $this->getChangeset();
+    if (!$changeset->hasSourceTextBody()) {
+
+      // TODO: This isn't really correct (the change is not "generated"), the
+      // intent is just to not render a text body for Subversion directory
+      // changes, etc.
+      $this->markGenerated();
+
+      return;
+    }
+
     $viewstate = $this->getViewState();
 
     $skip_cache = false;
@@ -610,19 +621,10 @@ final class DifferentialChangesetParser extends Phobject {
       $skip_cache = true;
     }
 
-    $changeset = $this->changeset;
-
-    if ($changeset->getFileType() != DifferentialChangeType::FILE_TEXT &&
-        $changeset->getFileType() != DifferentialChangeType::FILE_SYMLINK) {
-
-      $this->markGenerated();
-
-    } else {
-      if ($skip_cache || !$this->loadCache()) {
-        $this->process();
-        if (!$skip_cache) {
-          $this->saveCache();
-        }
+    if ($skip_cache || !$this->loadCache()) {
+      $this->process();
+      if (!$skip_cache) {
+        $this->saveCache();
       }
     }
   }
diff --git a/src/applications/differential/storage/DifferentialChangeset.php b/src/applications/differential/storage/DifferentialChangeset.php
index 966e93fd28..fa4cc37ec8 100644
--- a/src/applications/differential/storage/DifferentialChangeset.php
+++ b/src/applications/differential/storage/DifferentialChangeset.php
@@ -32,6 +32,8 @@ final class DifferentialChangeset
   private $hasNewState;
   private $oldStateMetadata;
   private $newStateMetadata;
+  private $oldFileType;
+  private $newFileType;
 
   const TABLE_CACHE = 'differential_changeset_parse_cache';
 
@@ -550,17 +552,20 @@ final class DifferentialChangeset
     $left_metadata = $left->getNewStateMetadata();
     $left_state = $left->hasNewState();
     $shared_metadata = $left->getMetadata();
+    $left_type = $left->getNewFileType();
     if ($right) {
       $right_data = $right->makeNewFile();
       $right_properties = $right->getNewProperties();
       $right_metadata = $right->getNewStateMetadata();
       $right_state = $right->hasNewState();
       $shared_metadata = $right->getMetadata();
+      $right_type = $right->getNewFileType();
     } else {
       $right_data = $left->makeOldFile();
       $right_properties = $left->getOldProperties();
       $right_metadata = $left->getOldStateMetadata();
       $right_state = $left->hasOldState();
+      $right_type = $left->getOldFileType();
     }
 
     $engine = new PhabricatorDifferenceEngine();
@@ -576,7 +581,6 @@ final class DifferentialChangeset
       ->setFilename($right->getFilename());
 
     // TODO: Change type?
-    // TODO: File type?
     // TODO: Away paths?
     // TODO: View state key?
 
@@ -589,7 +593,9 @@ final class DifferentialChangeset
       ->setOldStateMetadata($left_metadata)
       ->setNewStateMetadata($right_metadata)
       ->setHasOldState($left_state)
-      ->setHasNewState($right_state);
+      ->setHasNewState($right_state)
+      ->setOldFileType($left_type)
+      ->setNewFileType($right_type);
 
     // NOTE: Some metadata is not stored statefully, like the "generated"
     // flag. For now, use the rightmost "new state" metadata to fill in these
@@ -602,6 +608,45 @@ final class DifferentialChangeset
     return $comparison;
   }
 
+
+  public function setNewFileType($new_file_type) {
+    $this->newFileType = $new_file_type;
+    return $this;
+  }
+
+  public function getNewFileType() {
+    if ($this->newFileType !== null) {
+      return $this->newFileType;
+    }
+
+    return $this->getFiletype();
+  }
+
+  public function setOldFileType($old_file_type) {
+    $this->oldFileType = $old_file_type;
+    return $this;
+  }
+
+  public function getOldFileType() {
+    if ($this->oldFileType !== null) {
+      return $this->oldFileType;
+    }
+
+    return $this->getFileType();
+  }
+
+  public function hasSourceTextBody() {
+    $type_map = array(
+      DifferentialChangeType::FILE_TEXT => true,
+      DifferentialChangeType::FILE_SYMLINK => true,
+    );
+
+    $old_body = isset($type_map[$this->getOldFileType()]);
+    $new_body = isset($type_map[$this->getNewFileType()]);
+
+    return ($old_body || $new_body);
+  }
+
   public function getNewStateMetadata() {
     return $this->getMetadataWithPrefix('new:');
   }