From 82b7cd778a288dfb2b9035b8395d97c8f6185e2b Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 4 Apr 2017 09:50:00 -0700 Subject: [PATCH 1/3] Make "arc download" use "file.search" if available Summary: Fixes T8348. Just use normal HTTP GET to download files if the server is sufficiently modern. Test Plan: Downloaded various files with `--as`, `--show`, large files, small files, old server, new server. Reviewers: chad Reviewed By: chad Maniphest Tasks: T8348 Differential Revision: https://secure.phabricator.com/D17614 --- src/workflow/ArcanistDownloadWorkflow.php | 157 ++++++++++++++++++++++ 1 file changed, 157 insertions(+) diff --git a/src/workflow/ArcanistDownloadWorkflow.php b/src/workflow/ArcanistDownloadWorkflow.php index f3eb466b..66fa7331 100644 --- a/src/workflow/ArcanistDownloadWorkflow.php +++ b/src/workflow/ArcanistDownloadWorkflow.php @@ -79,6 +79,163 @@ EOTEXT public function run() { $conduit = $this->getConduit(); + $id = $this->id; + $display_name = 'F'.$id; + $is_show = $this->show; + $save_as = $this->saveAs; + + try { + $file = $conduit->callMethodSynchronous( + 'file.search', + array( + 'constraints' => array( + 'ids' => array($id), + ), + )); + + $data = $file['data']; + if (!$data) { + throw new ArcanistUsageException( + pht( + 'File "%s" is not a valid file, or not visible.', + $display_name)); + } + + $file = head($data); + $data_uri = idxv($file, array('fields', 'dataURI')); + + if ($data_uri === null) { + throw new ArcanistUsageException( + pht( + 'File "%s" can not be downloaded.', + $display_name)); + } + + if ($is_show) { + // Skip all the file path stuff if we're just going to echo the + // file contents. + } else { + if ($save_as !== null) { + $path = Filesystem::resolvePath($save_as); + + $try_unique = false; + } else { + $path = idxv($file, array('fields', 'name'), $display_name); + $path = basename($path); + $path = Filesystem::resolvePath($path); + + $try_unique = true; + } + + if ($try_unique) { + $path = Filesystem::writeUniqueFile($path, ''); + } else { + if (Filesystem::pathExists($path)) { + throw new ArcanistUsageException( + pht( + 'File "%s" already exists.', + $save_as)); + } + + Filesystem::writeFile($path, ''); + } + + $display_path = Filesystem::readablePath($path); + } + + $size = idxv($file, array('fields', 'size'), 0); + + if ($is_show) { + $file_handle = null; + } else { + $file_handle = fopen($path, 'ab+'); + if ($file_handle === false) { + throw new Exception( + pht( + 'Failed to open file "%s" for writing.', + $path)); + } + + $this->writeInfo( + pht('DATA'), + pht( + 'Downloading "%s" (%s byte(s))...', + $display_name, + new PhutilNumber($size))); + } + + $future = new HTTPSFuture($data_uri); + + // For small files, don't bother drawing a progress bar. + $minimum_bar_bytes = (1024 * 1024 * 4); + + if ($is_show || ($size < $minimum_bar_bytes)) { + $bar = null; + } else { + $bar = id(new PhutilConsoleProgressBar()) + ->setTotal($size); + } + + // TODO: We should stream responses to disk, but cURL gives us the raw + // HTTP response data and BaseHTTPFuture can not currently parse it in + // a stream-oriented way. Until this is resolved, buffer the file data + // in memory and write it to disk in one shot. + + list($status, $data) = $future->resolve(); + if ($status->getStatusCode() !== 200) { + throw new Exception( + pht( + 'Got HTTP %d status response, expected HTTP 200.', + $status)); + } + + if (strlen($data)) { + if ($is_show) { + echo $data; + } else { + $ok = fwrite($file_handle, $data); + if ($ok === false) { + throw new Exception( + pht( + 'Failed to write file data to "%s".', + $path)); + } + } + } + + if ($bar) { + $bar->update(strlen($data)); + } + + if ($bar) { + $bar->done(); + } + + if ($file_handle) { + $ok = fclose($file_handle); + if ($ok === false) { + throw new Exception( + pht( + 'Failed to close file handle for "%s".', + $path)); + } + } + + if (!$is_show) { + $this->writeOkay( + pht('DONE'), + pht( + 'Saved "%s" as "%s".', + $display_name, + $display_path)); + } + + return 0; + } catch (Exception $ex) { + // If we fail for any reason, fall back to the older mechanism using + // "file.info" and "file.download". + } + $this->writeStatusMessage(pht('Getting file information...')."\n"); $info = $conduit->callMethodSynchronous( 'file.info', From c4e84550fcd33f0e0ea0a245044352dac52accfe Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 4 Apr 2017 15:46:31 -0700 Subject: [PATCH 2/3] Allow "arc upload" to work correctly if it can not hash content Summary: Ref T12464. This is similar to D17619 and prepares us to move to SHA256 in the client. Note that it's fine if `arc` and Phabricator disagree about hashing algorithms. We don't really trust the client anyway, so if things are mismatched clients will just end up transferring a bit more data instead of getting to cheat when Phabricator already has copies of data. Test Plan: Ran `arc upload`, got a clean upload. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12464 Differential Revision: https://secure.phabricator.com/D17621 --- src/upload/ArcanistFileDataRef.php | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/upload/ArcanistFileDataRef.php b/src/upload/ArcanistFileDataRef.php index 318418ec..1a79e143 100644 --- a/src/upload/ArcanistFileDataRef.php +++ b/src/upload/ArcanistFileDataRef.php @@ -222,15 +222,6 @@ final class ArcanistFileDataRef extends Phobject { $path)); } - $hash = @sha1_file($path); - if ($hash === false) { - throw new Exception( - pht( - 'Unable to upload file: failed to calculate file data hash for '. - 'path "%s".', - $path)); - } - $size = @filesize($path); if ($size === false) { throw new Exception( @@ -240,11 +231,11 @@ final class ArcanistFileDataRef extends Phobject { $path)); } - $this->hash = $hash; + $this->hash = $this->newFileHash($path); $this->size = $size; } else { $data = $this->data; - $this->hash = sha1($data); + $this->hash = $this->newDataHash($data); $this->size = strlen($data); } } @@ -354,4 +345,12 @@ final class ArcanistFileDataRef extends Phobject { return $data; } + private function newFileHash($path) { + return null; + } + + private function newDataHash($data) { + return null; + } + } From a59cfca5f190c44403dfc7449c678a2aa1626bb4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 4 Apr 2017 15:52:46 -0700 Subject: [PATCH 3/3] Upgrade "arc upload" to use SHA256 Summary: Fixes T12464. Moves "arc upload" to SHA256 where applicable. Test Plan: Ran `arc upload` against a server with D17620 twice, saw it skip the actual upload the second time. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12464 Differential Revision: https://secure.phabricator.com/D17622 --- src/upload/ArcanistFileDataRef.php | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/upload/ArcanistFileDataRef.php b/src/upload/ArcanistFileDataRef.php index 1a79e143..99ed03d9 100644 --- a/src/upload/ArcanistFileDataRef.php +++ b/src/upload/ArcanistFileDataRef.php @@ -346,11 +346,23 @@ final class ArcanistFileDataRef extends Phobject { } private function newFileHash($path) { - return null; + $hash = hash_file('sha256', $path, $raw_output = false); + + if ($hash === false) { + return null; + } + + return $hash; } private function newDataHash($data) { - return null; + $hash = hash('sha256', $data, $raw_output = false); + + if ($hash === false) { + return null; + } + + return $hash; } }