From 3eafe9e3bbf4d9791d73576e86b49c9e90e81fc6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 20 Jul 2011 11:43:44 -0700 Subject: [PATCH] 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 --- .../query/diff/svn/DiffusionSvnDiffQuery.php | 19 +-- .../diffusion/query/diff/svn/__init__.php | 3 +- .../engine/PhabricatorDifferenceEngine.php | 135 ++++++++++++------ 3 files changed, 102 insertions(+), 55 deletions(-) diff --git a/src/applications/diffusion/query/diff/svn/DiffusionSvnDiffQuery.php b/src/applications/diffusion/query/diff/svn/DiffusionSvnDiffQuery.php index afe006935c..1d7dd7f1e6 100644 --- a/src/applications/diffusion/query/diff/svn/DiffusionSvnDiffQuery.php +++ b/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(); - $new_tmp = new TempFile(); - - Filesystem::writeFile($old_tmp, $old_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); + $engine = new PhabricatorDifferenceEngine(); + $engine->setOldName($old_name); + $engine->setNewName($new_name); + $raw_diff = $engine->generateRawDiffFromFileContent($old_data, $new_data); $parser = new ArcanistDiffParser(); $parser->setDetectBinaryFiles(true); diff --git a/src/applications/diffusion/query/diff/svn/__init__.php b/src/applications/diffusion/query/diff/svn/__init__.php index 114f3781e6..a87b1cbdfe 100644 --- a/src/applications/diffusion/query/diff/svn/__init__.php +++ b/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'); diff --git a/src/infrastructure/diff/engine/PhabricatorDifferenceEngine.php b/src/infrastructure/diff/engine/PhabricatorDifferenceEngine.php index b5226f44ef..a9847c259a 100644 --- a/src/infrastructure/diff/engine/PhabricatorDifferenceEngine.php +++ b/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) { - - $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"; - } + $diff = $this->generateRawDiffFromFileContent($old, $new); $changes = id(new ArcanistDiffParser())->parseDiff($diff); $diff = DifferentialDiff::newFromRawChanges($changes);