Upgrade "arc download" to Toolsets

Summary:
Ref T13490. This is mostly straightforward.

  - Drop "--show" in favor of "--as -".
  - Drop support for 4+ year old "file.info" API.
  - Use modern stream-to-disk support so we get a real progress bar and don't need to buffer files into memory.

Test Plan: Downloaded various files, including large files.

Maniphest Tasks: T13490

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

src/__phutil_library_map__.php

@@ -190,6 +190,10 @@ phutil_register_library_map(array(
     'ArcanistFeatureWorkflow' => 'workflow/ArcanistFeatureWorkflow.php',
     'ArcanistFileConfigurationSource' => 'config/source/ArcanistFileConfigurationSource.php',
     'ArcanistFileDataRef' => 'upload/ArcanistFileDataRef.php',
+    'ArcanistFileRef' => 'ref/file/ArcanistFileRef.php',
+    'ArcanistFileSymbolHardpointQuery' => 'ref/file/ArcanistFileSymbolHardpointQuery.php',
+    'ArcanistFileSymbolRef' => 'ref/file/ArcanistFileSymbolRef.php',
+    'ArcanistFileSymbolRefInspector' => 'ref/file/ArcanistFileSymbolRefInspector.php',
     'ArcanistFileUploader' => 'upload/ArcanistFileUploader.php',
     'ArcanistFilenameLinter' => 'lint/linter/ArcanistFilenameLinter.php',
     'ArcanistFilenameLinterTestCase' => 'lint/linter/__tests__/ArcanistFilenameLinterTestCase.php',
@@ -1149,7 +1153,7 @@ phutil_register_library_map(array(
     ),
     'ArcanistDoubleQuoteXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistDoubleQuoteXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
-    'ArcanistDownloadWorkflow' => 'ArcanistWorkflow',
+    'ArcanistDownloadWorkflow' => 'ArcanistArcWorkflow',
     'ArcanistDuplicateKeysInArrayXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistDuplicateKeysInArrayXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistDuplicateSwitchCaseXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
@@ -1173,6 +1177,13 @@ phutil_register_library_map(array(
     'ArcanistFeatureWorkflow' => 'ArcanistFeatureBaseWorkflow',
     'ArcanistFileConfigurationSource' => 'ArcanistFilesystemConfigurationSource',
     'ArcanistFileDataRef' => 'Phobject',
+    'ArcanistFileRef' => array(
+      'ArcanistRef',
+      'ArcanistDisplayRefInterface',
+    ),
+    'ArcanistFileSymbolHardpointQuery' => 'ArcanistRuntimeHardpointQuery',
+    'ArcanistFileSymbolRef' => 'ArcanistSymbolRef',
+    'ArcanistFileSymbolRefInspector' => 'ArcanistRefInspector',
     'ArcanistFileUploader' => 'Phobject',
     'ArcanistFilenameLinter' => 'ArcanistLinter',
     'ArcanistFilenameLinterTestCase' => 'ArcanistLinterTestCase',

src/ref/file/ArcanistFileRef.php

@@ -0,0 +1,52 @@
+<?php
+
+final class ArcanistFileRef
+  extends ArcanistRef
+  implements
+    ArcanistDisplayRefInterface {
+
+  private $parameters;
+
+  public function getRefDisplayName() {
+    return pht('File "%s"', $this->getMonogram());
+  }
+
+  public static function newFromConduit(array $parameters) {
+    $ref = new self();
+    $ref->parameters = $parameters;
+    return $ref;
+  }
+
+  public function getID() {
+    return idx($this->parameters, 'id');
+  }
+
+  public function getPHID() {
+    return idx($this->parameters, 'phid');
+  }
+
+  public function getName() {
+    return idxv($this->parameters, array('fields', 'name'));
+  }
+
+  public function getDataURI() {
+    return idxv($this->parameters, array('fields', 'dataURI'));
+  }
+
+  public function getSize() {
+    return idxv($this->parameters, array('fields', 'size'));
+  }
+
+  public function getMonogram() {
+    return 'F'.$this->getID();
+  }
+
+  public function getDisplayRefObjectName() {
+    return $this->getMonogram();
+  }
+
+  public function getDisplayRefTitle() {
+    return $this->getName();
+  }
+
+}

src/ref/file/ArcanistFileSymbolHardpointQuery.php

@@ -0,0 +1,90 @@
+<?php
+
+final class ArcanistFileSymbolHardpointQuery
+  extends ArcanistRuntimeHardpointQuery {
+
+  public function getHardpoints() {
+    return array(
+      ArcanistFileSymbolRef::HARDPOINT_OBJECT,
+    );
+  }
+
+  protected function canLoadRef(ArcanistRef $ref) {
+    return ($ref instanceof ArcanistFileSymbolRef);
+  }
+
+  public function loadHardpoint(array $refs, $hardpoint) {
+    $id_map = array();
+    $phid_map = array();
+
+    foreach ($refs as $key => $ref) {
+      switch ($ref->getSymbolType()) {
+        case ArcanistFileSymbolRef::TYPE_ID:
+          $id_map[$key] = $ref->getSymbol();
+          break;
+        case ArcanistFileSymbolRef::TYPE_PHID:
+          $phid_map[$key] = $ref->getSymbol();
+          break;
+      }
+    }
+
+    $futures = array();
+
+    if ($id_map) {
+      $id_future = $this->newConduitSearch(
+        'file.search',
+        array(
+          'ids' => array_values(array_fuse($id_map)),
+        ));
+
+      $futures[] = $id_future;
+    } else {
+      $id_future = null;
+    }
+
+    if ($phid_map) {
+      $phid_future = $this->newConduitSearch(
+        'file.search',
+        array(
+         'phids' => array_values(array_fuse($phid_map)),
+        ));
+
+      $futures[] = $phid_future;
+    } else {
+      $phid_future = null;
+    }
+
+    yield $this->yieldFutures($futures);
+
+    $result_map = array();
+
+    if ($id_future) {
+      $id_results = $id_future->resolve();
+      $id_results = ipull($id_results, null, 'id');
+
+      foreach ($id_map as $key => $id) {
+        $result_map[$key] = idx($id_results, $id);
+      }
+    }
+
+    if ($phid_future) {
+      $phid_results = $phid_future->resolve();
+      $phid_results = ipull($phid_results, null, 'phid');
+
+      foreach ($phid_map as $key => $phid) {
+        $result_map[$key] = idx($phid_results, $phid);
+      }
+    }
+
+    foreach ($result_map as $key => $raw_result) {
+      if ($raw_result === null) {
+        continue;
+      }
+
+      $result_map[$key] = ArcanistFileRef::newFromConduit($raw_result);
+    }
+
+    yield $this->yieldMap($result_map);
+  }
+
+}

src/ref/file/ArcanistFileSymbolRef.php

@@ -0,0 +1,47 @@
+<?php
+
+final class ArcanistFileSymbolRef
+  extends ArcanistSymbolRef {
+
+  private $type;
+
+  const TYPE_ID = 'id';
+  const TYPE_PHID = 'phid';
+
+  public function getRefDisplayName() {
+    return pht('File Symbol "%s"', $this->getSymbol());
+  }
+
+  protected function newCacheKeyParts() {
+    return array(
+      sprintf('type(%s)', $this->type),
+    );
+  }
+
+  public function getSymbolType() {
+    return $this->type;
+  }
+
+  protected function resolveSymbol($symbol) {
+    $matches = null;
+
+    $is_id = preg_match('/^[Ff]?([1-9]\d*)\z/', $symbol, $matches);
+    if ($is_id) {
+      $this->type = self::TYPE_ID;
+      return (int)$matches[1];
+    }
+
+    $is_phid = preg_match('/^PHID-FILE-\S+\z/', $symbol, $matches);
+    if ($is_phid) {
+      $this->type = self::TYPE_PHID;
+      return $matches[0];
+    }
+
+    throw new PhutilArgumentUsageException(
+      pht(
+        'The format of file symbol "%s" is unrecognized. Expected a '.
+        'monogram like "F123", or an ID like "123", or a file PHID.',
+        $symbol));
+  }
+
+}

src/ref/file/ArcanistFileSymbolRefInspector.php

@@ -0,0 +1,22 @@
+<?php
+
+final class ArcanistFileSymbolRefInspector
+  extends ArcanistRefInspector {
+
+  public function getInspectFunctionName() {
+    return 'file';
+  }
+
+  public function newInspectRef(array $argv) {
+    if (count($argv) !== 1) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'Expected exactly one argument to "file(...)" with a '.
+          'file symbol.'));
+    }
+
+    return id(new ArcanistFileSymbolRef())
+      ->setSymbol($argv[0]);
+  }
+
+}

src/ref/symbol/ArcanistSymbolEngine.php

@@ -48,6 +48,17 @@ final class ArcanistSymbolEngine
       $symbols);
   }
 
+  public function loadFileForSymbol($symbol) {
+    $refs = $this->loadFilesForSymbols(array($symbol));
+    return head($refs)->getObject();
+  }
+
+  public function loadFilesForSymbols(array $symbols) {
+    return $this->loadRefsForSymbols(
+      new ArcanistFileSymbolRef(),
+      $symbols);
+  }
+
   public function loadRefsForSymbols(
     ArcanistSymbolRef $template,
     array $symbols) {

src/runtime/ArcanistRuntime.php

@@ -113,7 +113,7 @@ final class ArcanistRuntime {
     $workflows = $this->newWorkflows($toolset);
     $this->workflows = $workflows;
 
-    $conduit_engine = $this->newConduitEngine($config);
+    $conduit_engine = $this->newConduitEngine($config, $args);
     $this->conduitEngine = $conduit_engine;
 
     $phutil_workflows = array();
@@ -698,16 +698,34 @@ final class ArcanistRuntime {
     return last($this->stack);
   }
 
-  private function newConduitEngine(ArcanistConfigurationSourceList $config) {
+  private function newConduitEngine(
+    ArcanistConfigurationSourceList $config,
+    PhutilArgumentParser $args) {
 
-    $conduit_uri = $config->getConfig('phabricator.uri');
+    try {
-    if ($conduit_uri === null) {
+      $force_uri = $args->getArg('conduit-uri');
-      // For now, read this older config from raw storage. There is currently
+    } catch (PhutilArgumentSpecificationException $ex) {
-      // no definition of this option in the "toolsets" config list, and it
+      $force_uri = null;
-      // would be nice to get rid of it.
+    }
-      $default_list = $config->getStorageValueList('default');
+
-      if ($default_list) {
+    try {
-        $conduit_uri = last($default_list)->getValue();
+      $force_token = $args->getArg('conduit-token');
+    } catch (PhutilArgumentSpecificationException $ex) {
+      $force_token = null;
+    }
+
+    if ($force_uri !== null) {
+      $conduit_uri = $force_uri;
+    } else {
+      $conduit_uri = $config->getConfig('phabricator.uri');
+      if ($conduit_uri === null) {
+        // For now, read this older config from raw storage. There is currently
+        // no definition of this option in the "toolsets" config list, and it
+        // would be nice to get rid of it.
+        $default_list = $config->getStorageValueList('default');
+        if ($default_list) {
+          $conduit_uri = last($default_list)->getValue();
+        }
       }
     }
 
@@ -731,17 +749,20 @@ final class ArcanistRuntime {
     // TODO: This isn't using "getConfig()" because we aren't defining a
     // real config entry for the moment.
 
-    $hosts = array();
+    if ($force_token !== null) {
+      $conduit_token = $force_token;
+    } else {
+      $hosts = array();
 
-    $hosts_list = $config->getStorageValueList('hosts');
+      $hosts_list = $config->getStorageValueList('hosts');
-    foreach ($hosts_list as $hosts_config) {
+      foreach ($hosts_list as $hosts_config) {
-      $hosts += $hosts_config->getValue();
+        $hosts += $hosts_config->getValue();
+      }
+
+      $host_config = idx($hosts, $conduit_uri, array());
+      $conduit_token = idx($host_config, 'token');
     }
 
-    $host_config = idx($hosts, $conduit_uri, array());
-    $user_name = idx($host_config, 'user');
-    $conduit_token = idx($host_config, 'token');
-
     if ($conduit_token !== null) {
       $engine->setConduitToken($conduit_token);
     }

src/workflow/ArcanistDownloadWorkflow.php

@@ -1,53 +1,31 @@
 <?php
 
-/**
+final class ArcanistDownloadWorkflow
- * Download a file from Phabricator.
+  extends ArcanistArcWorkflow {
- */
-final class ArcanistDownloadWorkflow extends ArcanistWorkflow {
-
-  private $id;
-  private $saveAs;
-  private $show;
 
   public function getWorkflowName() {
     return 'download';
   }
 
-  public function getCommandSynopses() {
+  public function getWorkflowInformation() {
-    return phutil_console_format(<<<EOTEXT
+    $help = pht(<<<EOTEXT
-      **download** __file__ [--as __name__] [--show]
+Download a file to local disk.
 EOTEXT
       );
+
+    return $this->newWorkflowInformation()
+      ->setSynopsis(pht('Download a file to local disk.'))
+      ->addExample(pht('**download** [__options__] -- __file__'))
+      ->setHelp($help);
   }
 
-  public function getCommandHelp() {
+  public function getWorkflowArguments() {
-    return phutil_console_format(<<<EOTEXT
-          Supports: filesystems
-          Download a file to local disk, e.g.:
-
-            $ arc download F33              # Download file 'F33'
-EOTEXT
-      );
-  }
-
-  public function getArguments() {
     return array(
-      'show' => array(
+      $this->newWorkflowArgument('as')
-        'conflicts' => array(
+        ->setParameter('path')
-          'as' => pht(
+        ->setHelp(pht('Save the file to a specific location.')),
-            'Use %s to direct the file to stdout, or %s to direct '.
+      $this->newWorkflowArgument('argv')
-            'it to a named location.',
+        ->setWildcard(true),
-            '--show',
-            '--as'),
-        ),
-        'help' => pht('Write file to stdout instead of to disk.'),
-      ),
-      'as' => array(
-        'param' => 'name',
-        'help' => pht(
-          'Save the file with a specific name rather than the default.'),
-      ),
-      '*' => 'argv',
     );
   }
 
@@ -72,207 +50,143 @@ EOTEXT
     $this->show = $this->getArgument('show');
   }
 
-  public function requiresAuthentication() {
-    return true;
-  }
 
-  public function run() {
+  public function runWorkflow() {
-    $conduit = $this->getConduit();
+    $file_symbols = $this->getArgument('argv');
 
-    $id = $this->id;
+    if (!$file_symbols) {
-    $display_name = 'F'.$id;
+      throw new PhutilArgumentUsageException(
-    $is_show = $this->show;
+        pht(
-    $save_as = $this->saveAs;
+          'Specify a file to download, like "F123".'));
+    }
+
+    if (count($file_symbols) > 1) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'Specify exactly one file to download.'));
+    }
+
+    $file_symbol = head($file_symbols);
+
+    $symbols = $this->getSymbolEngine();
+    $file_ref = $symbols->loadFileForSymbol($file_symbol);
+    if (!$file_ref) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'File "%s" does not exist, or you do not have permission to '.
+          'view it.',
+          $file_symbol));
+    }
+
+    $is_stdout = false;
     $path = null;
 
-    try {
+    $save_as = $this->getArgument('as');
-      $file = $conduit->callMethodSynchronous(
+    if ($save_as === '-') {
-        'file.search',
+      $is_stdout = true;
-        array(
+    } else if ($save_as === null) {
-          'constraints' => array(
+      $path = $file_ref->getName();
-            'ids' => array($id),
+      $path = basename($path);
-          ),
+      $path = Filesystem::resolvePath($path);
-        ));
 
-      $data = $file['data'];
+      $try_unique = true;
-      if (!$data) {
+    } else {
-        throw new ArcanistUsageException(
+      $path = Filesystem::resolvePath($save_as);
-          pht(
-            'File "%s" is not a valid file, or not visible.',
-            $display_name));
-      }
 
-      $file = head($data);
+      $try_unique = false;
-      $data_uri = idxv($file, array('fields', 'dataURI'));
+    }
 
-      if ($data_uri === null) {
+    $file_handle = null;
-        throw new ArcanistUsageException(
+    if (!$is_stdout) {
-          pht(
+      if ($try_unique) {
-            'File "%s" can not be downloaded.',
+        $path = Filesystem::writeUniqueFile($path, '');
-            $display_name));
+        Filesystem::remove($path);
-      }
-
-      if ($is_show) {
-        // Skip all the file path stuff if we're just going to echo the
-        // file contents.
       } else {
-        if ($save_as !== null) {
+        if (Filesystem::pathExists($path)) {
-          $path = Filesystem::resolvePath($save_as);
+          throw new PhutilArgumentUsageException(
-
-          $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.',
+              'File "%s" already exists.',
               $path));
         }
-
-        $this->writeInfo(
-          pht('DATA'),
-          pht(
-            'Downloading "%s" (%s byte(s))...',
-            $display_name,
-            new PhutilNumber($size)));
       }
+    }
 
-      $future = new HTTPSFuture($data_uri);
+    $display_path = Filesystem::readablePath($path);
 
+    $display_name = $file_ref->getName();
+    if (!strlen($display_name)) {
+      $display_name = $file_ref->getMonogram();
+    }
+
+    $expected_bytes = $file_ref->getSize();
+    $log = $this->getLogEngine();
+
+    if (!$is_stdout) {
+      $log->writeStatus(
+        pht('DATA'),
+        pht(
+          'Downloading "%s" (%s byte(s)) to "%s"...',
+          $display_name,
+          new PhutilNumber($expected_bytes),
+          $display_path));
+    }
+
+    $data_uri = $file_ref->getDataURI();
+    $future = new HTTPSFuture($data_uri);
+
+    if (!$is_stdout) {
       // For small files, don't bother drawing a progress bar.
       $minimum_bar_bytes = (1024 * 1024 * 4);
+      if ($expected_bytes > $minimum_bar_bytes) {
+        $progress = id(new PhutilConsoleProgressSink())
+          ->setTotalWork($expected_bytes);
 
-      if ($is_show || ($size < $minimum_bar_bytes)) {
+        $future->setProgressSink($progress);
-        $bar = null;
-      } else {
-        $bar = id(new PhutilConsoleProgressBar())
-          ->setTotal($size);
       }
 
-      // TODO: We should stream responses to disk, but cURL gives us the raw
+      // Compute a timeout based on the expected filesize.
-      // HTTP response data and BaseHTTPFuture can not currently parse it in
+      $transfer_rate = 32 * 1024;
-      // a stream-oriented way. Until this is resolved, buffer the file data
+      $timeout = (int)(120 + ($expected_bytes / $transfer_rate));
-      // in memory and write it to disk in one shot.
 
-      list($status, $data) = $future->resolve();
+      $future
-      if ($status->getStatusCode() !== 200) {
+        ->setTimeout($timeout)
-        throw new Exception(
+        ->setDownloadPath($path);
-          pht(
+    }
-            'Got HTTP %d status response, expected HTTP 200.',
-            $status->getStatusCode()));
-      }
 
-      if (strlen($data)) {
+    try {
-        if ($is_show) {
+      list($data) = $future->resolvex();
-          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) {
-
+      Filesystem::removePath($path);
-      // If we created an empty file, clean it up.
+      throw $ex;
-      if (!$is_show) {
-        if ($path !== null) {
-          Filesystem::remove($path);
-        }
-      }
-
-      // 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");
+    if ($is_stdout) {
-    $info = $conduit->callMethodSynchronous(
+      $file_bytes = strlen($data);
-      'file.info',
+    } else {
-      array(
+      // TODO: This has various potential problems with clearstatcache() and
-        'id' => $this->id,
+      // 32-bit systems, but just ignore them for now.
-      ));
+      $file_bytes = filesize($path);
-
-    $desc = pht('(%s bytes)', new PhutilNumber($info['byteSize']));
-    if ($info['name']) {
-      $desc = "'".$info['name']."' ".$desc;
     }
 
-    $this->writeStatusMessage(pht('Downloading file %s...', $desc)."\n");
+    if ($file_bytes !== $expected_bytes) {
-    $data = $conduit->callMethodSynchronous(
+      throw new Exception(
-      'file.download',
+        pht(
-      array(
+          'Downloaded file size (%s bytes) does not match expected '.
-        'phid' => $info['phid'],
+          'file size (%s bytes). This download may be incomplete or '.
-      ));
+          'corrupt.',
+          new PhutilNumber($file_bytes),
+          new PhutilNumber($expected_bytes)));
+    }
 
-    $data = base64_decode($data);
+    if ($is_stdout) {
-
-    if ($this->show) {
       echo $data;
     } else {
-      $path = Filesystem::writeUniqueFile(
+      $log->writeStatus(
-        nonempty($this->saveAs, $info['name'], 'file'),
+        pht('DONE'),
-        $data);
+        pht(
-      $this->writeStatusMessage(pht("Saved file as '%s'.", $path)."\n");
+          'Saved "%s" as "%s".',
+          $display_name,
+          $display_path));
     }
 
     return 0;