Improve arc handling of various binary file operations in Git

Summary:
See T866, D3521. Additional things this fixes:

  - `arc export` now exports binary data correctly.
  - `ArcanistBundle` unit tests now load and apply binary data correctly.
  - `arc patch` no longer relies on `base` configuration.
  - Adds tests to the tarball:

  commit df340e88d8aba12e8f2b8827f01f0cd9f35eb758
  Author: epriestley <git@epriestley.com>
  Date:   Wed Oct 17 15:46:11 2012 -0700

      Remove binary image.

  commit 3f5c6d735e64c25a04f83be48ef184b25b5282f0
  Author: epriestley <git@epriestley.com>
  Date:   Wed Oct 17 15:45:58 2012 -0700

      Copy binary image.

  commit b454edb3bb29890ee5b3af5ef66ce6a24d15d882
  Author: epriestley <git@epriestley.com>
  Date:   Wed Oct 17 15:45:35 2012 -0700

      Move binary image.

  commit 5de5f3dfda1b7db2eb054e57699f05aaf1f4483e
  Author: epriestley <git@epriestley.com>
  Date:   Wed Oct 17 15:45:09 2012 -0700

      Add a binary image.

Test Plan: Ran unit tests, `arc patch`, `arc export`, `arc diff`, `arc upgrade`.

Reviewers: vrana, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T866

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

src/parser/ArcanistBundle.php

@@ -312,15 +312,14 @@ final class ArcanistBundle {
 
     $old_file_phids = array();
     foreach ($changes as $change) {
+      if (!$this->isGitBinaryChange($change)) {
+        continue;
+      }
+
       $type = $change->getType();
       if ($type == ArcanistDiffChangeType::TYPE_MOVE_AWAY) {
-        $file_type = $change->getFileType();
+        foreach ($change->getAwayPaths() as $path) {
-        $is_binary = ($file_type == ArcanistDiffChangeType::FILE_BINARY ||
+          $old_file_phids[$path] = $change->getMetadata('old:binary-phid');
-                      $file_type == ArcanistDiffChangeType::FILE_IMAGE);
-        if ($is_binary) {
-          foreach ($change->getAwayPaths() as $path) {
-            $old_file_phids[$path] = $change->getMetadata('old:binary-phid');
-          }
         }
       }
     }
@@ -348,8 +347,7 @@ final class ArcanistBundle {
       $old_mode = idx($change->getOldProperties(), 'unix:filemode', '100644');
       $new_mode = idx($change->getNewProperties(), 'unix:filemode', '100644');
 
-      $is_binary = ($file_type == ArcanistDiffChangeType::FILE_BINARY ||
+      $is_binary = $this->isGitBinaryChange($change);
-                    $file_type == ArcanistDiffChangeType::FILE_IMAGE);
 
       if ($is_binary) {
         $old_phid = idx($old_file_phids, $this->getCurrentPath($change));
@@ -427,6 +425,12 @@ final class ArcanistBundle {
     return $this->convertNonUTF8Diff($diff);
   }
 
+  private function isGitBinaryChange(ArcanistDiffChange $change) {
+    $file_type = $change->getFileType();
+    return ($file_type == ArcanistDiffChangeType::FILE_BINARY ||
+            $file_type == ArcanistDiffChangeType::FILE_IMAGE);
+  }
+
   private function convertNonUTF8Diff($diff) {
     if ($this->encoding) {
       $diff = phutil_utf8_convert($diff, $this->encoding, 'UTF-8');
@@ -615,7 +619,7 @@ final class ArcanistBundle {
     return $this;
   }
 
-  private function getBlob($phid) {
+  private function getBlob($phid, $name = null) {
     if ($this->loadFileDataCallback) {
       return call_user_func($this->loadFileDataCallback, $phid);
     }
@@ -625,8 +629,14 @@ final class ArcanistBundle {
       return $blob_data;
     }
 
+    $console = PhutilConsole::getConsole();
+
     if ($this->conduit) {
-      echo "Downloading binary data...\n";
+      if ($name) {
+        $console->writeErr("Downloading binary data for '%s'...\n", $name);
+      } else {
+        $console->writeErr("Downloading binary data...\n");
+      }
       $data_base64 = $this->conduit->callMethodSynchronous(
         'file.download',
         array(
@@ -642,23 +652,37 @@ final class ArcanistBundle {
     $old_phid = idx($change->getAllMetadata(), 'old:binary-phid', $old_phid);
     $new_phid = $change->getMetadata('new:binary-phid');
 
-    if (!$old_phid) {
+    $old_data = null;
+    if ($change->getOriginalFileData() !== null) {
+      $old_data = $change->getOriginalFileData();
+    } else if ($old_phid) {
+      $name = basename($change->getOldPath());
+      $old_data = $this->getBlob($old_phid, $name);
+    }
+
+    $old_length = strlen($old_data);
+
+    if ($old_data === null) {
       $old_data = '';
-      $old_length = 0;
       $old_sha1 = str_repeat('0', 40);
     } else {
-      $old_data = $this->getBlob($old_phid);
-      $old_length = strlen($old_data);
       $old_sha1 = sha1("blob {$old_length}\0{$old_data}");
     }
 
-    if (!$new_phid) {
+    $new_data = null;
+    if ($change->getCurrentFileData() !== null) {
+      $new_data = $change->getCurrentFileData();
+    } else if ($new_phid) {
+      $name = basename($change->getCurrentPath());
+      $new_data = $this->getBlob($new_phid, $name);
+    }
+
+    $new_length = strlen($new_data);
+
+    if ($new_data === null) {
       $new_data = '';
-      $new_length = 0;
       $new_sha1 = str_repeat('0', 40);
     } else {
-      $new_data = $this->getBlob($new_phid);
-      $new_length = strlen($new_data);
       $new_sha1 = sha1("blob {$new_length}\0{$new_data}");
     }
 

src/parser/ArcanistDiffParser.php

@@ -1140,4 +1140,49 @@ final class ArcanistDiffParser {
       return $name;
     }
   }
+
+  public function loadSyntheticData(
+    array $changes,
+    ArcanistRepositoryAPI $repository_api) {
+    assert_instances_of($changes, 'ArcanistDiffChange');
+
+    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()) {
+        $diff = $repository_api->getRawDiffText($path, $moves = false);
+
+        $raw_changes = $this->parseDiff($diff);
+        foreach ($raw_changes as $raw_change) {
+          if ($raw_change->getCurrentPath() == $path) {
+            $change->setFileType($raw_change->getFileType());
+            foreach ($raw_change->getHunks() as $hunk) {
+              // Git thinks that this file has been added. But we know that it
+              // has been moved or copied without a change.
+              $hunk->setCorpus(
+                preg_replace('/^\+/m', ' ', $hunk->getCorpus()));
+              $change->addHunk($hunk);
+            }
+            break;
+          }
+        }
+
+        $change->setNeedsSyntheticGitHunks(false);
+      }
+
+      if ($change->getFileType() != ArcanistDiffChangeType::FILE_BINARY &&
+          $change->getFileType() != ArcanistDiffChangeType::FILE_IMAGE) {
+        continue;
+      }
+
+      $change->setOriginalFileData($repository_api->getOriginalFileData($path));
+      $change->setCurrentFileData($repository_api->getCurrentFileData($path));
+    }
+
+    return $changes;
+  }
+
 }

src/parser/__tests__/ArcanistBundleTestCase.php

@@ -71,6 +71,8 @@ final class ArcanistBundleTestCase extends ArcanistTestCase {
 
     foreach ($commits as $commit) {
       list($commit_hash, $tree_hash, $subject) = explode(' ', $commit, 3);
+      execx('git reset --hard %s --', $commit_hash);
+
       list($diff) = execx(
         'git diff %s^ %s --',
         $commit_hash,
@@ -78,12 +80,20 @@ final class ArcanistBundleTestCase extends ArcanistTestCase {
 
       $parser = new ArcanistDiffParser();
       $changes = $parser->parseDiff($diff);
+
+      $repository_api = new ArcanistGitAPI($fixture->getPath());
+      $repository_api->setRelativeCommit($commit_hash.'^');
+
+      $parser->loadSyntheticData($changes, $repository_api);
+
       $bundle = ArcanistBundle::newFromChanges($changes);
 
       execx('git reset --hard %s^ --', $commit_hash);
 
+      $patch = $bundle->toGitPatch();
+
       id(new ExecFuture('git apply --index --reject'))
-        ->write($bundle->toGitPatch())
+        ->write($patch)
         ->resolvex();
 
       execx('git commit -m %s', $subject);

src/parser/diff/ArcanistDiffChange.php

@@ -40,6 +40,27 @@ final class ArcanistDiffChange {
 
   private $needsSyntheticGitHunks;
 
+  private $currentFileData;
+  private $originalFileData;
+
+  public function setOriginalFileData($original_file_data) {
+    $this->originalFileData = $original_file_data;
+    return $this;
+  }
+
+  public function getOriginalFileData() {
+    return $this->originalFileData;
+  }
+
+  public function setCurrentFileData($current_file_data) {
+    $this->currentFileData = $current_file_data;
+    return $this;
+  }
+
+  public function getCurrentFileData() {
+    return $this->currentFileData;
+  }
+
   public function toDictionary() {
     $hunks = array();
     foreach ($this->hunks as $hunk) {

src/repository/api/ArcanistGitAPI.php

@@ -689,6 +689,11 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     return true;
   }
 
+  public function setDefaultBaseCommit() {
+    $this->setRelativeCommit('HEAD^');
+    return $this;
+  }
+
   public function hasLocalCommit($commit) {
     try {
       if (!$this->getCanonicalRevisionName($commit)) {

src/repository/api/ArcanistMercurialAPI.php

@@ -427,6 +427,11 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI {
     return true;
   }
 
+  public function setDefaultBaseCommit() {
+    $this->setRelativeCommit('.^');
+    return $this;
+  }
+
   public function hasLocalCommit($commit) {
     try {
       $this->getCanonicalRevisionName($commit);

src/repository/api/ArcanistRepositoryAPI.php

@@ -182,6 +182,21 @@ abstract class ArcanistRepositoryAPI {
     ConduitClient $conduit,
     array $query);
 
+  /**
+   * Set the base commit to a reasonable default value so that working copy
+   * status checks can do something meaningful and won't invoke configured
+   * 'base' rules.
+   *
+   * This is primarily useful for workflows which do not operate on commit
+   * ranges but need to verify the working copy is not dirty, like "amend",
+   * "upgrade" and "patch".
+   *
+   * @return this
+   */
+  public function setDefaultBaseCommit() {
+    throw new ArcanistCapabilityNotSupportedException($this);
+  }
+
   public function amendCommit($message) {
     throw new ArcanistCapabilityNotSupportedException($this);
   }

src/workflow/ArcanistAmendWorkflow.php

@@ -106,10 +106,8 @@ EOTEXT
       $revision_id = $this->normalizeRevisionID($this->getArgument('revision'));
     }
 
-    if ($repository_api instanceof ArcanistGitAPI) {
+    if ($repository_api->supportsRelativeLocalCommits()) {
-      $repository_api->setRelativeCommit('HEAD^');
+      $repository_api->setDefaultBaseCommit();
-    } else if ($repository_api instanceof ArcanistMercurialAPI) {
-      $repository_api->setRelativeCommit('.^');
     }
 
     $in_working_copy = $repository_api->loadWorkingCopyDifferentialRevisions(

src/workflow/ArcanistDiffWorkflow.php

@@ -1019,41 +1019,18 @@ EOTEXT
       }
     }
 
+    $changes = $parser->loadSyntheticData($changes, $repository_api);
+
     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()) {
-        $diff = $repository_api->getRawDiffText($path, $moves = false);
-        $parser = $this->newDiffParser();
-
-        $raw_changes = $parser->parseDiff($diff);
-        foreach ($raw_changes as $raw_change) {
-          if ($raw_change->getCurrentPath() == $path) {
-            $change->setFileType($raw_change->getFileType());
-            foreach ($raw_change->getHunks() as $hunk) {
-              // Git thinks that this file has been added. But we know that it
-              // has been moved or copied without a change.
-              $hunk->setCorpus(
-                preg_replace('/^\+/m', ' ', $hunk->getCorpus()));
-              $change->addHunk($hunk);
-            }
-            break;
-          }
-        }
-
-        $change->setNeedsSyntheticGitHunks(false);
-      }
-
       if ($change->getFileType() != ArcanistDiffChangeType::FILE_BINARY) {
         continue;
       }
 
+      $path = $change->getCurrentPath();
+
       $name = basename($path);
 
-      $old_file = $repository_api->getOriginalFileData($path);
+      $old_file = $change->getOriginalFileData();
       $old_dict = $this->uploadFile($old_file, $name, 'old binary');
       if ($old_dict['guid']) {
         $change->setMetadata('old:binary-phid', $old_dict['guid']);
@@ -1061,7 +1038,7 @@ EOTEXT
       $change->setMetadata('old:file:size',      $old_dict['size']);
       $change->setMetadata('old:file:mime-type', $old_dict['mime']);
 
-      $new_file = $repository_api->getCurrentFileData($path);
+      $new_file = $change->getCurrentFileData();
       $new_dict = $this->uploadFile($new_file, $name, 'new binary');
       if ($new_dict['guid']) {
         $change->setMetadata('new:binary-phid', $new_dict['guid']);

src/workflow/ArcanistExportWorkflow.php

@@ -204,6 +204,8 @@ EOTEXT
             $paths);
         }
 
+        $changes = $parser->loadSyntheticData($changes, $repository_api);
+
         $bundle = ArcanistBundle::newFromChanges($changes);
         $bundle->setProjectID($this->getWorkingCopy()->getProjectID());
         $bundle->setBaseRevision(

src/workflow/ArcanistPatchWorkflow.php

@@ -837,6 +837,11 @@ EOTEXT
    * Do the best we can to prevent PEBKAC and id10t issues.
    */
   private function sanityCheck(ArcanistBundle $bundle) {
+    $repository_api = $this->getRepositoryAPI();
+
+    if ($repository_api->supportsRelativeLocalCommits()) {
+      $repository_api->setDefaultBaseCommit();
+    }
 
     // Require clean working copy
     $this->requireCleanWorkingCopy();
@@ -870,7 +875,6 @@ EOTEXT
 
     // Check to see if the bundle's base revision matches the working copy
     // base revision
-    $repository_api = $this->getRepositoryAPI();
     if ($repository_api->supportsRelativeLocalCommits()) {
       $bundle_base_rev = $bundle->getBaseRevision();
       if (empty($bundle_base_rev)) {

src/workflow/ArcanistUpgradeWorkflow.php

@@ -66,10 +66,10 @@ EOTEXT
       $repository_api = ArcanistRepositoryAPI::newAPIFromWorkingCopyIdentity(
         $working_copy);
 
-      // Force the range to HEAD^..HEAD, which is meaningless but keeps us
+      if ($repository_api->supportsRelativeLocalCommits()) {
-      // from triggering "base" rules or other commit range resolution rules
+        $repository_api->setDefaultBaseCommit();
-      // that might prompt the user when we pull the working copy status.
+      }
-      $repository_api->setRelativeCommit('HEAD^');
+
       $this->setRepositoryAPI($repository_api);
 
       // Require no local changes.