Fix Diffusion rendering of SVN files which did not change

Summary:
Share code with the new PhabricatorDifferenceEngine, which handles diffs with no
changes correctly.

(This isn't the same issue as file moves, but I ran into it while generating a
repro case.)

Test Plan: Previously, changes which didn't change file content (e.g., property
changes) would throw. Now they work.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 698

src/applications/diffusion/query/diff/svn/DiffusionSvnDiffQuery.php

@@ -90,24 +90,17 @@ final class DiffusionSvnDiffQuery extends DiffusionDiffQuery {
     $futures = array_filter($futures);
 
     foreach (Futures($futures) as $key => $future) {
-      $futures[$key] = $future->resolvex();
+      list($stdout) = $future->resolvex();
+      $futures[$key] = $stdout;
     }
 
     $old_data = idx($futures, 'old', '');
     $new_data = idx($futures, 'new', '');
 
-    $old_tmp = new TempFile();
+    $engine = new PhabricatorDifferenceEngine();
-    $new_tmp = new TempFile();
+    $engine->setOldName($old_name);
-
+    $engine->setNewName($new_name);
-    Filesystem::writeFile($old_tmp, $old_data);
+    $raw_diff = $engine->generateRawDiffFromFileContent($old_data, $new_data);
-    Filesystem::writeFile($new_tmp, $new_data);
-
-    list($err, $raw_diff) = exec_manual(
-      'diff -L %s -L %s -U65535 %s %s',
-      nonempty($old_name, '/dev/universe').' 9999-99-99',
-      nonempty($new_name, '/dev/universe').' 9999-99-99',
-      $old_tmp,
-      $new_tmp);
 
     $parser = new ArcanistDiffParser();
     $parser->setDetectBinaryFiles(true);

src/applications/diffusion/query/diff/svn/__init__.php

@@ -13,9 +13,8 @@ phutil_require_module('phabricator', 'applications/differential/storage/diff');
 phutil_require_module('phabricator', 'applications/diffusion/data/pathchange');
 phutil_require_module('phabricator', 'applications/diffusion/query/diff/base');
 phutil_require_module('phabricator', 'applications/diffusion/query/pathchange/base');
+phutil_require_module('phabricator', 'infrastructure/diff/engine');
 
-phutil_require_module('phutil', 'filesystem');
-phutil_require_module('phutil', 'filesystem/tempfile');
 phutil_require_module('phutil', 'future');
 phutil_require_module('phutil', 'future/exec');
 phutil_require_module('phutil', 'utils');

src/infrastructure/diff/engine/PhabricatorDifferenceEngine.php

@@ -25,7 +25,10 @@
  */
 final class PhabricatorDifferenceEngine {
 
+
   private $ignoreWhitespace;
+  private $oldName;
+  private $newName;
 
 
 /* -(  Configuring the Engine  )--------------------------------------------- */
@@ -44,9 +47,100 @@ final class PhabricatorDifferenceEngine {
   }
 
 
+  /**
+   * Set the name to identify the old file with. Primarily cosmetic.
+   *
+   * @param  string Old file name.
+   * @return this
+   * @task config
+   */
+  public function setOldName($old_name) {
+    $this->oldName = $old_name;
+    return $this;
+  }
+
+
+  /**
+   * Set the name to identify the new file with. Primarily cosmetic.
+   *
+   * @param  string New file name.
+   * @return this
+   * @task config
+   */
+  public function setNewName($new_name) {
+    $this->newName = $new_name;
+    return $this;
+  }
+
+
 /* -(  Generating Diffs  )--------------------------------------------------- */
 
 
+  /**
+   * Generate a raw diff from two raw files. This is a lower-level API than
+   * @{method:generateChangesetFromFileContent}, but may be useful if you need
+   * to use a custom parser configuration, as with Diffusion.
+   *
+   * @param string Entire previous file content.
+   * @param string Entire current file content.
+   * @return string Raw diff between the two files.
+   * @task diff
+   */
+  public function generateRawDiffFromFileContent($old, $new) {
+
+    $options = array();
+    if ($this->ignoreWhitespace) {
+      $options[] = '-bw';
+    }
+
+    // Generate diffs with full context.
+    $options[] = '-U65535';
+
+    $old_name = nonempty($this->oldName, '/dev/universe').' 9999-99-99';
+    $new_name = nonempty($this->newName, '/dev/universe').' 9999-99-99';
+
+    $options[] = '-L';
+    $options[] = $old_name;
+    $options[] = '-L';
+    $options[] = $new_name;
+
+    $old_tmp = new TempFile();
+    $new_tmp = new TempFile();
+
+    Filesystem::writeFile($old_tmp, $old);
+    Filesystem::writeFile($new_tmp, $new);
+    list($err, $diff) = exec_manual(
+      '/usr/bin/diff %Ls %s %s',
+      $options,
+      $old_tmp,
+      $new_tmp);
+
+    if (!$err) {
+      // This indicates that the two files are the same (or, possibly, the
+      // same modulo whitespace differences, which is why we can't do this
+      // check trivially before running `diff`). Build a synthetic, changeless
+      // diff so that we can still render the raw, unchanged file instead of
+      // being forced to just say "this file didn't change" since we don't have
+      // the content.
+
+      $entire_file = explode("\n", $old);
+      foreach ($entire_file as $k => $line) {
+        $entire_file[$k] = ' '.$line;
+      }
+      $len = count($entire_file);
+      $entire_file = implode("\n", $entire_file);
+
+      // This is a bit hacky but the diff parser can handle it.
+      $diff = "--- {$old_name}\n".
+              "+++ {$new_name}\n".
+              "@@ -1,{$len} +1,{$len} @@\n".
+              $entire_file."\n";
+    }
+
+    return $diff;
+  }
+
+
   /**
    * Generate an @{class:DifferentialChangeset} from two raw files. This is
    * principally useful because you can feed the output to
@@ -58,46 +152,7 @@ final class PhabricatorDifferenceEngine {
    * @task diff
    */
   public function generateChangesetFromFileContent($old, $new) {
-
+    $diff = $this->generateRawDiffFromFileContent($old, $new);
-    $options = array();
-    if ($this->ignoreWhitespace) {
-      $options[] = '-bw';
-    }
-
-    // Generate diffs with full context.
-    $options[] = '-U65535';
-
-    $old_tmp = new TempFile();
-    $new_tmp = new TempFile();
-
-    Filesystem::writeFile($old_tmp, $old);
-    Filesystem::writeFile($new_tmp, $new);
-    list($err, $diff) = exec_manual(
-      '/usr/bin/diff %Ls %s %s',
-      $options,
-      $old_tmp,
-      $new_tmp);
-
-    if (!strlen($diff)) {
-      // This indicates that the two files are the same (or, possibly, the
-      // same modulo whitespace differences, which is why we can't do this
-      // check trivially before running `diff`). Build a synthetic, changeless
-      // diff so that we can still render the raw, unchanged file instead of
-      // being forced to just say "this file didn't change" since we don't have
-      // the content.
-      $entire_file = explode("\n", $old);
-      foreach ($entire_file as $k => $line) {
-        $entire_file[$k] = ' '.$line;
-      }
-      $len = count($entire_file);
-      $entire_file = implode("\n", $entire_file);
-
-      // This is a bit hacky but the diff parser can handle it.
-      $diff = "--- ignored 9999-99-99\n".
-              "+++ ignored 9999-99-99\n".
-              "@@ -1,{$len} +1,{$len} @@\n".
-              $entire_file."\n";
-    }
 
     $changes = id(new ArcanistDiffParser())->parseDiff($diff);
     $diff = DifferentialDiff::newFromRawChanges($changes);