(stable) Make "bin/files" parsing of working set arguments more consistent

Summary:
Fixes T13326. In D20571, I slightly generalized construction of an iterator over a set of files, but missed some code in other "bin/files ..." commands which was also affected.

Today, basically all of these workflows define their own "--all" and "names" flags. Pull these definitions up and implement them more consistently.

Test Plan: Ran multiple different `bin/files` commands with different combinations of arguments, saw consistent handling of iterator construction.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13326

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

src/applications/files/management/PhabricatorFilesManagementCompactWorkflow.php

@@ -4,41 +4,25 @@ final class PhabricatorFilesManagementCompactWorkflow
   extends PhabricatorFilesManagementWorkflow {
 
   protected function didConstruct() {
+    $arguments = $this->newIteratorArguments();
+    $arguments[] = array(
+      'name' => 'dry-run',
+      'help' => pht('Show what would be compacted.'),
+    );
+
     $this
       ->setName('compact')
       ->setSynopsis(
         pht(
           'Merge identical files to share the same storage. In some cases, '.
           'this can repair files with missing data.'))
-      ->setArguments(
+      ->setArguments($arguments);
-        array(
-          array(
-            'name'      => 'dry-run',
-            'help'      => pht('Show what would be compacted.'),
-          ),
-          array(
-            'name'      => 'all',
-            'help'      => pht('Compact all files.'),
-          ),
-          array(
-            'name'      => 'names',
-            'wildcard'  => true,
-          ),
-        ));
   }
 
   public function execute(PhutilArgumentParser $args) {
     $console = PhutilConsole::getConsole();
 
     $iterator = $this->buildIterator($args);
-    if (!$iterator) {
-      throw new PhutilArgumentUsageException(
-        pht(
-          'Either specify a list of files to compact, or use `%s` '.
-          'to compact all files.',
-          '--all'));
-    }
-
     $is_dry_run = $args->getArg('dry-run');
 
     foreach ($iterator as $file) {

src/applications/files/management/PhabricatorFilesManagementCycleWorkflow.php

@@ -4,36 +4,22 @@ final class PhabricatorFilesManagementCycleWorkflow
   extends PhabricatorFilesManagementWorkflow {
 
   protected function didConstruct() {
+    $arguments = $this->newIteratorArguments();
+    $arguments[] = array(
+      'name' => 'key',
+      'param' => 'keyname',
+      'help' => pht('Select a specific storage key to cycle to.'),
+    );
+
     $this
       ->setName('cycle')
       ->setSynopsis(
         pht('Cycle master key for encrypted files.'))
-      ->setArguments(
+      ->setArguments($arguments);
-        array(
-          array(
-            'name' => 'key',
-            'param' => 'keyname',
-            'help' => pht('Select a specific storage key to cycle to.'),
-          ),
-          array(
-            'name' => 'all',
-            'help' => pht('Change encoding for all files.'),
-          ),
-          array(
-            'name' => 'names',
-            'wildcard' => true,
-          ),
-        ));
   }
 
   public function execute(PhutilArgumentParser $args) {
     $iterator = $this->buildIterator($args);
-    if (!$iterator) {
-      throw new PhutilArgumentUsageException(
-        pht(
-          'Either specify a list of files to cycle, or use --all to cycle '.
-          'all files.'));
-    }
 
     $format_map = PhabricatorFileStorageFormat::getAllFormats();
     $engines = PhabricatorFileStorageEngine::loadAllEngines();

src/applications/files/management/PhabricatorFilesManagementEncodeWorkflow.php

@@ -4,47 +4,36 @@ final class PhabricatorFilesManagementEncodeWorkflow
   extends PhabricatorFilesManagementWorkflow {
 
   protected function didConstruct() {
+    $arguments = $this->newIteratorArguments();
+
+    $arguments[] = array(
+      'name' => 'as',
+      'param' => 'format',
+      'help' => pht('Select the storage format to use.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'key',
+      'param' => 'keyname',
+      'help' => pht('Select a specific storage key.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'force',
+      'help' => pht(
+        'Re-encode files which are already stored in the target '.
+        'encoding.'),
+    );
+
     $this
       ->setName('encode')
       ->setSynopsis(
         pht('Change the storage encoding of files.'))
-      ->setArguments(
+      ->setArguments($arguments);
-        array(
-          array(
-            'name' => 'as',
-            'param' => 'format',
-            'help' => pht('Select the storage format to use.'),
-          ),
-          array(
-            'name' => 'key',
-            'param' => 'keyname',
-            'help' => pht('Select a specific storage key.'),
-          ),
-          array(
-            'name' => 'all',
-            'help' => pht('Change encoding for all files.'),
-          ),
-          array(
-            'name' => 'force',
-            'help' => pht(
-              'Re-encode files which are already stored in the target '.
-              'encoding.'),
-          ),
-          array(
-            'name' => 'names',
-            'wildcard' => true,
-          ),
-        ));
   }
 
   public function execute(PhutilArgumentParser $args) {
     $iterator = $this->buildIterator($args);
-    if (!$iterator) {
-      throw new PhutilArgumentUsageException(
-        pht(
-          'Either specify a list of files to encode, or use --all to '.
-          'encode all files.'));
-    }
 
     $force = (bool)$args->getArg('force');
 

src/applications/files/management/PhabricatorFilesManagementIntegrityWorkflow.php

@@ -4,52 +4,50 @@ final class PhabricatorFilesManagementIntegrityWorkflow
   extends PhabricatorFilesManagementWorkflow {
 
   protected function didConstruct() {
+    $arguments = $this->newIteratorArguments();
+
+    $arguments[] = array(
+      'name' => 'strip',
+      'help' => pht(
+        'DANGEROUS. Strip integrity hashes from files. This makes '.
+        'files vulnerable to corruption or tampering.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'corrupt',
+      'help' => pht(
+        'Corrupt integrity hashes for given files. This is intended '.
+        'for debugging.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'compute',
+      'help' => pht(
+        'Compute and update integrity hashes for files which do not '.
+        'yet have them.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'overwrite',
+      'help' => pht(
+        'DANGEROUS. Recompute and update integrity hashes, overwriting '.
+        'invalid hashes. This may mark corrupt or dangerous files as '.
+        'valid.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'force',
+      'short' => 'f',
+      'help' => pht(
+        'Execute dangerous operations without prompting for '.
+        'confirmation.'),
+    );
+
+
     $this
       ->setName('integrity')
       ->setSynopsis(pht('Verify or recalculate file integrity hashes.'))
-      ->setArguments(
+      ->setArguments($arguments);
-        array(
-          array(
-            'name'      => 'all',
-            'help'      => pht('Affect all files.'),
-          ),
-          array(
-            'name' => 'strip',
-            'help' => pht(
-              'DANGEROUS. Strip integrity hashes from files. This makes '.
-              'files vulnerable to corruption or tampering.'),
-          ),
-          array(
-            'name' => 'corrupt',
-            'help' => pht(
-              'Corrupt integrity hashes for given files. This is intended '.
-              'for debugging.'),
-          ),
-          array(
-            'name' => 'compute',
-            'help' => pht(
-              'Compute and update integrity hashes for files which do not '.
-              'yet have them.'),
-          ),
-          array(
-            'name' => 'overwrite',
-            'help' => pht(
-              'DANGEROUS. Recompute and update integrity hashes, overwriting '.
-              'invalid hashes. This may mark corrupt or dangerous files as '.
-              'valid.'),
-          ),
-          array(
-            'name' => 'force',
-            'short' => 'f',
-            'help' => pht(
-              'Execute dangerous operations without prompting for '.
-              'confirmation.'),
-          ),
-          array(
-            'name'      => 'names',
-            'wildcard'  => true,
-          ),
-        ));
   }
 
   public function execute(PhutilArgumentParser $args) {
@@ -120,12 +118,6 @@ final class PhabricatorFilesManagementIntegrityWorkflow
     }
 
     $iterator = $this->buildIterator($args);
-    if (!$iterator) {
-      throw new PhutilArgumentUsageException(
-        pht(
-          'Either specify a list of files to affect, or use "--all" to '.
-          'affect all files.'));
-    }
 
     $failure_count = 0;
     $total_count = 0;

src/applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php

@@ -4,61 +4,54 @@ final class PhabricatorFilesManagementMigrateWorkflow
   extends PhabricatorFilesManagementWorkflow {
 
   protected function didConstruct() {
+    $arguments = $this->newIteratorArguments();
+
+    $arguments[] = array(
+      'name' => 'engine',
+      'param' => 'storage-engine',
+      'help' => pht('Migrate to the named storage engine.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'dry-run',
+      'help' => pht('Show what would be migrated.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'min-size',
+      'param' => 'bytes',
+      'help' => pht(
+        'Do not migrate data for files which are smaller than a given '.
+        'filesize.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'max-size',
+      'param' => 'bytes',
+      'help' => pht(
+        'Do not migrate data for files which are larger than a given '.
+        'filesize.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'copy',
+      'help' => pht(
+        'Copy file data instead of moving it: after migrating, do not '.
+        'remove the old data even if it is no longer referenced.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'local-disk-source',
+      'param' => 'path',
+      'help' => pht(
+        'When migrating from a local disk source, use the specified '.
+        'path as the root directory.'),
+    );
+
     $this
       ->setName('migrate')
       ->setSynopsis(pht('Migrate files between storage engines.'))
-      ->setArguments(
+      ->setArguments($arguments);
-        array(
-          array(
-            'name'      => 'engine',
-            'param'     => 'storage_engine',
-            'help'      => pht('Migrate to the named storage engine.'),
-          ),
-          array(
-            'name'      => 'dry-run',
-            'help'      => pht('Show what would be migrated.'),
-          ),
-          array(
-            'name' => 'min-size',
-            'param' => 'bytes',
-            'help' => pht(
-              'Do not migrate data for files which are smaller than a given '.
-              'filesize.'),
-          ),
-          array(
-            'name' => 'max-size',
-            'param' => 'bytes',
-            'help' => pht(
-              'Do not migrate data for files which are larger than a given '.
-              'filesize.'),
-          ),
-          array(
-            'name'      => 'all',
-            'help'      => pht('Migrate all files.'),
-          ),
-          array(
-            'name' => 'copy',
-            'help' => pht(
-              'Copy file data instead of moving it: after migrating, do not '.
-              'remove the old data even if it is no longer referenced.'),
-          ),
-          array(
-            'name'      => 'names',
-            'wildcard'  => true,
-          ),
-          array(
-            'name' => 'from-engine',
-            'param' => 'engine',
-            'help' => pht('Migrate files from the named storage engine.'),
-          ),
-          array(
-            'name' => 'local-disk-source',
-            'param' => 'path',
-            'help' => pht(
-              'When migrating from a local disk source, use the specified '.
-              'path as the root directory.'),
-          ),
-        ));
   }
 
   public function execute(PhutilArgumentParser $args) {
@@ -97,14 +90,6 @@ final class PhabricatorFilesManagementMigrateWorkflow
     $target_engine = PhabricatorFile::buildEngine($target_key);
 
     $iterator = $this->buildIterator($args);
-    if (!$iterator) {
-      throw new PhutilArgumentUsageException(
-        pht(
-          'Either specify a list of files to migrate, or use `%s` '.
-          'to migrate all files.',
-          '--all'));
-    }
-
     $is_dry_run = $args->getArg('dry-run');
 
     $min_size = (int)$args->getArg('min-size');

src/applications/files/management/PhabricatorFilesManagementRebuildWorkflow.php

@@ -4,45 +4,33 @@ final class PhabricatorFilesManagementRebuildWorkflow
   extends PhabricatorFilesManagementWorkflow {
 
   protected function didConstruct() {
+    $arguments = $this->newIteratorArguments();
+
+    $arguments[] = array(
+      'name' => 'dry-run',
+      'help' => pht('Show what would be updated.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'rebuild-mime',
+      'help' => pht('Rebuild MIME information.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'rebuild-dimensions',
+      'help' => pht('Rebuild image dimension information.'),
+    );
+
     $this
       ->setName('rebuild')
       ->setSynopsis(pht('Rebuild metadata of old files.'))
-      ->setArguments(
+      ->setArguments($arguments);
-        array(
-          array(
-            'name'      => 'all',
-            'help'      => pht('Update all files.'),
-          ),
-          array(
-            'name'      => 'dry-run',
-            'help'      => pht('Show what would be updated.'),
-          ),
-          array(
-            'name'      => 'rebuild-mime',
-            'help'      => pht('Rebuild MIME information.'),
-          ),
-          array(
-            'name'      => 'rebuild-dimensions',
-            'help'      => pht('Rebuild image dimension information.'),
-          ),
-          array(
-            'name'      => 'names',
-            'wildcard'  => true,
-          ),
-        ));
   }
 
   public function execute(PhutilArgumentParser $args) {
     $console = PhutilConsole::getConsole();
 
     $iterator = $this->buildIterator($args);
-    if (!$iterator) {
-      throw new PhutilArgumentUsageException(
-        pht(
-          'Either specify a list of files to update, or use `%s` '.
-          'to update all files.',
-          '--all'));
-    }
 
     $update = array(
       'mime'          => $args->getArg('rebuild-mime'),

src/applications/files/management/PhabricatorFilesManagementWorkflow.php

@@ -3,11 +3,30 @@
 abstract class PhabricatorFilesManagementWorkflow
   extends PhabricatorManagementWorkflow {
 
+  protected function newIteratorArguments() {
+    return array(
+      array(
+        'name' => 'all',
+        'help' => pht('Operate on all files.'),
+      ),
+      array(
+        'name' => 'names',
+        'wildcard' => true,
+      ),
+      array(
+        'name' => 'from-engine',
+        'param' => 'storage-engine',
+        'help' => pht('Operate on files stored in a specified engine.'),
+      ),
+    );
+  }
+
   protected function buildIterator(PhutilArgumentParser $args) {
     $viewer = $this->getViewer();
-    $names = $args->getArg('names');
 
     $is_all = $args->getArg('all');
+
+    $names = $args->getArg('names');
     $from_engine = $args->getArg('from-engine');
 
     $any_constraint = ($from_engine || $names);
@@ -15,15 +34,16 @@ abstract class PhabricatorFilesManagementWorkflow
     if (!$is_all && !$any_constraint) {
       throw new PhutilArgumentUsageException(
         pht(
-          'Use "--all" to migrate all files, or choose files to migrate '.
+          'Specify which files to operate on, or use "--all" to operate on '.
-          'with "--names" or "--from-engine".'));
+          'all files.'));
     }
 
     if ($is_all && $any_constraint) {
       throw new PhutilArgumentUsageException(
         pht(
-          'You can not migrate all files with "--all" and also migrate only '.
+          'You can not operate on all files with "--all" and also operate '.
-          'a subset of files with "--from-engine" or "--names".'));
+          'on a subset of files by naming them explicitly or using '.
+          'constraint flags like "--from-engine".'));
     }
 
     // If we're migrating specific named files, convert the names into IDs