From 964630facc5fa325cabc15281f85d6dd00bd9c74 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 11 Jan 2011 13:02:38 -0800 Subject: [PATCH] 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 | 44 +---------- .../api/base/ArcanistRepositoryAPI.php | 2 + src/repository/api/git/ArcanistGitAPI.php | 67 ++++++++++++++++ .../api/subversion/ArcanistSubversionAPI.php | 22 ++++++ src/workflow/diff/ArcanistDiffWorkflow.php | 77 +++++++++++++++++++ src/workflow/diff/__init__.php | 2 + 6 files changed, 174 insertions(+), 40 deletions(-) diff --git a/src/parser/diff/ArcanistDiffParser.php b/src/parser/diff/ArcanistDiffParser.php index dc7472f1..cbc92087 100644 --- a/src/parser/diff/ArcanistDiffParser.php +++ b/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) { diff --git a/src/repository/api/base/ArcanistRepositoryAPI.php b/src/repository/api/base/ArcanistRepositoryAPI.php index 59523666..d58a8bad 100644 --- a/src/repository/api/base/ArcanistRepositoryAPI.php +++ b/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); } diff --git a/src/repository/api/git/ArcanistGitAPI.php b/src/repository/api/git/ArcanistGitAPI.php index bf453221..9eff1f51 100644 --- a/src/repository/api/git/ArcanistGitAPI.php +++ b/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; + } + } diff --git a/src/repository/api/subversion/ArcanistSubversionAPI.php b/src/repository/api/subversion/ArcanistSubversionAPI.php index ce76fd7b..84430686 100644 --- a/src/repository/api/subversion/ArcanistSubversionAPI.php +++ b/src/repository/api/subversion/ArcanistSubversionAPI.php @@ -411,5 +411,27 @@ 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; + } } diff --git a/src/workflow/diff/ArcanistDiffWorkflow.php b/src/workflow/diff/ArcanistDiffWorkflow.php index b9e1e151..15c4da93 100644 --- a/src/workflow/diff/ArcanistDiffWorkflow.php +++ b/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. */ diff --git a/src/workflow/diff/__init__.php b/src/workflow/diff/__init__.php index 5a60aac7..029b9fae 100644 --- a/src/workflow/diff/__init__.php +++ b/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');