Integrate with file.upload over conduit to ship binary changes to Differential.

Summary: support binary upload in the new arc so we can show image diffs, e.g.

Test Plan:

Reviewers:

CC:

src/parser/diff/ArcanistDiffParser.php

@@ -541,7 +541,7 @@ class ArcanistDiffParser {
     if ($is_binary_add) {
       $this->nextLine(); // Cannot display: file marked as a binary type.
       $this->nextNonemptyLine(); // svn:mime-type = application/octet-stream
-      $this->pullBinaries($change);
+      $this->markBinary($change);
       return;
     }
 
@@ -552,7 +552,7 @@ class ArcanistDiffParser {
       $line);
     if ($is_binary_diff) {
       $this->nextNonemptyLine(); // Binary files x and y differ
-      $this->pullBinaries($change);
+      $this->markBinary($change);
       return;
     }
 
@@ -587,45 +587,9 @@ class ArcanistDiffParser {
     return $matches['path'];
   }
 
-  protected function pullBinaries(ArcanistDiffChange $change) {
+  protected function markBinary(ArcanistDiffChange $change) {
     $change->setFileType(ArcanistDiffChangeType::FILE_BINARY);
-
-    // TODO: Reimplement this.
-    return;
-
-/*
-    $api = $this->getRepositoryAPI();
-    if (!$api) {
-      return;
-    }
-
-    $is_image = Filesystem::isImageFilename($change->getCurrentPath());
-    if (!$is_image) {
-      // TODO: We could store binaries for reasonably-sized files.
-      return;
-    }
-
-    $change->setFileType(ArcanistDiffChangeType::FILE_IMAGE);
-
-    $old_data = $api->getOriginalFileData($change->getCurrentPath());
-    $new_data = $api->getCurrentFileData($change->getCurrentPath());
-
-    $old_fbid = $this->createAttachment($change->getOldPath(), $old_data);
-    $new_fbid = $this->createAttachment($change->getCurrentPath(), $new_data);
-
-    $info = array(
-      'tools-attachment-old-fbid'  => $old_fbid,
-      'tools-attachment-new-fbid'  => $new_fbid,
-    );
-
-    $change->setMetadata('attachment-data', $info);
-
-*/
-  }
-
-  protected function createAttachment($name, $data) {
-    // TODO: Implement attachments over conduit.
-    return null;
+    return $this;
   }
 
   protected function parseChangeset(ArcanistDiffChange $change) {

src/repository/api/base/ArcanistRepositoryAPI.php

@@ -132,5 +132,7 @@ abstract class ArcanistRepositoryAPI {
   abstract public function getBlame($path);
   abstract public function getWorkingCopyStatus();
   abstract public function getRawDiffText($path);
+  abstract public function getOriginalFileData($path);
+  abstract public function getCurrentFileData($path);
 
 }

src/repository/api/git/ArcanistGitAPI.php

@@ -287,4 +287,71 @@ class ArcanistGitAPI extends ArcanistRepositoryAPI {
     return $blame;
   }
 
+  public function getOriginalFileData($path) {
+    return $this->getFileDataAtRevision($path, $this->getRelativeCommit());
+  }
+
+  public function getCurrentFileData($path) {
+    return $this->getFileDataAtRevision($path, 'HEAD');
+  }
+
+  private function parseGitTree($stdout) {
+    $result = array();
+
+    $stdout = trim($stdout);
+    if (!strlen($stdout)) {
+      return $result;
+    }
+
+    $lines = explode("\n", $stdout);
+    foreach ($lines as $line) {
+      $matches = array();
+      $ok = preg_match(
+        '/^(\d{6}) (blob|tree) ([a-z0-9]{40})[\t](.*)$/',
+        $line,
+        $matches);
+      if (!$ok) {
+        throw new Exception("Failed to parse git ls-tree output!");
+      }
+      $result[$matches[4]] = array(
+        'mode' => $matches[1],
+        'type' => $matches[2],
+        'ref'  => $matches[3],
+      );
+    }
+    return $result;
+  }
+
+  private function getFileDataAtRevision($path, $revision) {
+
+    // NOTE: We don't want to just "git show {$revision}:{$path}" since if the
+    // path was a directory at the given revision we'll get a list of its files
+    // and treat it as though it as a file containing a list of other files,
+    // which is silly.
+
+    list($stdout) = execx(
+      '(cd %s && git ls-tree %s -- %s)',
+      $this->getPath(),
+      $revision,
+      $path);
+
+    $info = $this->parseGitTree($stdout);
+    if (empty($info[$path])) {
+      // No such path, or the path is a directory and we executed 'ls-tree dir/'
+      // and got a list of its contents back.
+      return null;
+    }
+
+    if ($info[$path]['type'] != 'blob') {
+      // Path is or was a directory, not a file.
+      return null;
+    }
+
+    list($stdout) = execx(
+      '(cd %s && git cat-file blob %s)',
+       $this->getPath(),
+       $info[$path]['ref']);
+    return $stdout;
+  }
+
 }

src/repository/api/subversion/ArcanistSubversionAPI.php

@@ -412,4 +412,26 @@ EODIFF;
     return $blame;
   }
   
+  public function getOriginalFileData($path) {
+    // SVN issues warnings for nonexistent paths, directories, etc., but still
+    // returns no error code. However, for new paths in the working copy it
+    // fails. Assume that failure means the original file does not exist.
+    list($err, $stdout) = exec_manual(
+      '(cd %s && svn cat %s@)',
+      $this->getPath(),
+      $path);
+    if ($err) {
+      return null;
+    }
+    return $stdout;
+  }
+  
+  public function getCurrentFileData($path) {
+    $full_path = $this->getPath($path);
+    if (Filesystem::pathExists($full_path)) {
+      return Filesystem::readFile($full_path);
+    }
+    return null;
+  }
+
 }

src/workflow/diff/ArcanistDiffWorkflow.php

@@ -479,9 +479,86 @@ EOTEXT
       }
     }
 
+
+    // TODO: Ideally, we should do this later, after validating commit message
+    // fields (i.e., test plan), in case there are large/slow file upload steps
+    // involved.
+    foreach ($changes as $change) {
+      if ($change->getFileType() != ArcanistDiffChangeType::FILE_BINARY) {
+        continue;
+      }
+
+      $path = $change->getCurrentPath();
+      $old_file = $repository_api->getOriginalFileData($path);
+      $new_file = $repository_api->getCurrentFileData($path);
+
+      $old_dict = $this->uploadFile($old_file, basename($path), 'old binary');
+      $new_dict = $this->uploadFile($new_file, basename($path), 'new binary');
+
+      if ($old_dict['guid']) {
+        $change->setMetadata('old:binary-guid', $old_dict['guid']);
+      }
+      if ($new_dict['guid']) {
+        $change->setMetadata('new:binary-guid', $new_dict['guid']);
+      }
+
+      $change->setMetadata('old:file:size',      strlen($old_file));
+      $change->setMetadata('new:file:size',      strlen($new_file));
+      $change->setMetadata('old:file:mime-type', $old_dict['mime']);
+      $change->setMetadata('new:file:mime-type', $new_dict['mime']);
+
+      if (preg_match('@^image/@', $new_dict['mime'])) {
+        $change->setFileType(ArcanistDiffChangeType::FILE_IMAGE);
+      }
+    }
+
+
     return $changes;
   }
 
+  private function uploadFile($data, $name, $desc) {
+    $result = array(
+      'guid' => null,
+      'mime' => null,
+    );
+
+    if (!strlen($data)) {
+      return $result;
+    }
+
+    $future = new ExecFuture('file -ib -');
+    $future->write($data);
+    list($mime_type) = $future->resolvex();
+
+    $mime_type = trim($mime_type);
+    if (strpos($mime_type, ',') !== false) {
+      // TODO: This is kind of silly, but 'file -ib' goes crazy on executables.
+      $mime_type = reset(explode(',', $mime_type));
+    }
+      
+
+    $result['mime'] = $mime_type;
+
+    // TODO: Make this configurable.
+    $bin_limit = 1024 * 1024; // 1 MB limit
+    if (strlen($data) > $bin_limit) {
+      return $result;
+    }
+
+    $bytes = strlen($data);
+    echo "Uploading {$desc} '{$name}' ({$mime_type}, {$bytes} bytes)...\n";
+
+    $guid = $this->getConduit()->callMethodSynchronous(
+      'file.upload',
+      array(
+        'data_base64' => base64_encode($data),
+        'name'        => $name,
+      ));
+
+    $result['guid'] = $guid;
+    return $result;
+  }
+
   /**
    * Retrieve the git message in HEAD if it isn't a primary template message.
    */

src/workflow/diff/__init__.php

@@ -10,6 +10,7 @@ phutil_require_module('arcanist', 'differential/commitmessage');
 phutil_require_module('arcanist', 'exception/usage');
 phutil_require_module('arcanist', 'exception/usage/userabort');
 phutil_require_module('arcanist', 'parser/diff');
+phutil_require_module('arcanist', 'parser/diff/changetype');
 phutil_require_module('arcanist', 'repository/api/base');
 phutil_require_module('arcanist', 'workflow/base');
 phutil_require_module('arcanist', 'workflow/lint');
@@ -18,6 +19,7 @@ phutil_require_module('arcanist', 'workflow/unit');
 phutil_require_module('phutil', 'console');
 phutil_require_module('phutil', 'console/editor');
 phutil_require_module('phutil', 'filesystem/filelist');
+phutil_require_module('phutil', 'future/exec');
 phutil_require_module('phutil', 'utils');