Make ArcanistDiffParser automatically load synthetic changes if the working copy is available

Summary: Make this harder to get wrong. Instead of requiring a separate call for synthetic data, automatically load it if we can.

Test Plan: Unit tests; `arc diff`.

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T866

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

src/parser/ArcanistDiffParser.php

@@ -23,7 +23,7 @@
  */
 final class ArcanistDiffParser {
 
-  protected $api;
+  protected $repositoryAPI;
   protected $text;
   protected $line;
   protected $lineSaved;
@@ -37,15 +37,11 @@ final class ArcanistDiffParser {
   protected $changes = array();
   private $forcePath;
 
-  protected function setRepositoryAPI(ArcanistRepositoryAPI $api) {
+  public function setRepositoryAPI(ArcanistRepositoryAPI $repository_api) {
-    $this->api = $api;
+    $this->repositoryAPI = $repository_api;
     return $this;
   }
 
-  protected function getRepositoryAPI() {
-    return $this->api;
-  }
-
   public function setDetectBinaryFiles($detect) {
     $this->detectBinaryFiles = $detect;
     return $this;
@@ -339,6 +335,8 @@ final class ArcanistDiffParser {
 
     $this->didFinishParse();
 
+    $this->loadSyntheticData();
+
     return $this->changes;
   }
 
@@ -1144,27 +1142,34 @@ final class ArcanistDiffParser {
     }
   }
 
-  public function loadSyntheticData(
+  private function loadSyntheticData() {
-    array $changes,
+    if (!$this->changes) {
-    ArcanistRepositoryAPI $repository_api) {
+      return;
-    assert_instances_of($changes, 'ArcanistDiffChange');
+    }
 
+    $repository_api = $this->repositoryAPI;
+    if (!$repository_api) {
+      return;
+    }
+
+    $changes = $this->changes;
     foreach ($changes as $change) {
       $path = $change->getCurrentPath();
 
       // Certain types of changes (moves and copies) don't contain change data
       // when expressed in raw "git diff" form. Augment any such diffs with
       // textual data.
-      if ($change->getNeedsSyntheticGitHunks()) {
+      if ($change->getNeedsSyntheticGitHunks() &&
+          ($repository_api instanceof ArcanistGitAPI)) {
         $diff = $repository_api->getRawDiffText($path, $moves = false);
 
         // NOTE: We're reusing the parser and it doesn't reset change state
         // between parses because there's an oddball SVN workflow in Phabricator
         // which relies on being able to inject changes.
         // TODO: Fix this.
-        $this->setChanges(array());
+        $parser = clone $this;
-
+        $parser->setChanges(array());
-        $raw_changes = $this->parseDiff($diff);
+        $raw_changes = $parser->parseDiff($diff);
 
         foreach ($raw_changes as $raw_change) {
           if ($raw_change->getCurrentPath() == $path) {
@@ -1192,7 +1197,7 @@ final class ArcanistDiffParser {
       $change->setCurrentFileData($repository_api->getCurrentFileData($path));
     }
 
-    return $changes;
+    $this->changes = $changes;
   }
 
   /**

src/parser/__tests__/ArcanistBundleTestCase.php

@@ -78,13 +78,12 @@ final class ArcanistBundleTestCase extends ArcanistTestCase {
         $commit_hash,
         $commit_hash);
 
-      $parser = new ArcanistDiffParser();
-      $changes = $parser->parseDiff($diff);
-
       $repository_api = new ArcanistGitAPI($fixture->getPath());
-      $repository_api->setRelativeCommit($commit_hash.'^');
+      $repository_api->setDefaultBaseCommit();
 
-      $parser->loadSyntheticData($changes, $repository_api);
+      $parser = new ArcanistDiffParser();
+      $parser->setRepositoryAPI($repository_api);
+      $changes = $parser->parseDiff($diff);
 
       $bundle = ArcanistBundle::newFromChanges($changes);
 

src/workflow/ArcanistBaseWorkflow.php

@@ -1336,6 +1336,9 @@ abstract class ArcanistBaseWorkflow {
 
   protected function newDiffParser() {
     $parser = new ArcanistDiffParser();
+    if ($this->getRepositoryAPI()) {
+      $parser->setRepositoryAPI($this->getRepositoryAPI());
+    }
     $parser->setWriteDiffOnFailure(true);
     return $parser;
   }

src/workflow/ArcanistDiffWorkflow.php

@@ -1020,8 +1020,6 @@ EOTEXT
       }
     }
 
-    $changes = $parser->loadSyntheticData($changes, $repository_api);
-
     foreach ($changes as $change) {
       if ($change->getFileType() != ArcanistDiffChangeType::FILE_BINARY) {
         continue;

src/workflow/ArcanistExportWorkflow.php

@@ -190,6 +190,7 @@ EOTEXT
       case self::SOURCE_LOCAL:
         $repository_api = $this->getRepositoryAPI();
         $parser = new ArcanistDiffParser();
+        $parser->setRepositorAPI($repository_api);
 
         if ($repository_api instanceof ArcanistGitAPI) {
           $repository_api->parseRelativeLocalCommit(
@@ -204,8 +205,6 @@ EOTEXT
             $paths);
         }
 
-        $changes = $parser->loadSyntheticData($changes, $repository_api);
-
         $bundle = ArcanistBundle::newFromChanges($changes);
         $bundle->setProjectID($this->getWorkingCopy()->getProjectID());
         $bundle->setBaseRevision(