Unify arguments for 'arc lint', 'arc unit'

Summary: See T645. These commands take inconsistent and overly-magical arguments
right now. Instead, make them behave consistently and allow them both to operate
on "arc <workflow> path path2 path3 ...", which is a generally useful workflow.

Test Plan: Ran "arc lint <path>", "arc unit <path>", "arc lint --rev
HEAD^^^^^^", "arc unit --rev HEAD^^^^^^^^^^^^", etc. Ran "arc diff --trace" and
verified --rev argument to child workflows.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, epriestley, btrahan

Maniphest Tasks: T645

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

scripts/arcanist.php

@@ -2,7 +2,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.

src/lint/engine/base/ArcanistLintEngine.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.

src/lint/linter/base/ArcanistLinter.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.

src/parser/bundle/ArcanistBundle.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.

src/repository/hookapi/base/ArcanistHookAPI.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.

src/repository/hookapi/subversion/ArcanistSubversionHookAPI.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.

src/workflow/base/ArcanistBaseWorkflow.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -936,4 +936,49 @@ class ArcanistBaseWorkflow {
     return ($working_copy->getConfig('immutable_history') === true);
   }
 
+  /**
+   * Workflows like 'lint' and 'unit' operate on a list of working copy paths.
+   * The user can either specify the paths explicitly ("a.js b.php"), or by
+   * specfifying a revision ("--rev a3f10f1f") to select all paths modified
+   * since that revision, or by omitting both and letting arc choose the
+   * default relative revision.
+   *
+   * This method takes the user's selections and returns the paths that the
+   * workflow should act upon.
+   *
+   * @param   list          List of explicitly provided paths.
+   * @param   string|null   Revision name, if provided.
+   * @return  list          List of paths the workflow should act on.
+   */
+  protected function selectPathsForWorkflow(array $paths, $rev) {
+    if ($paths) {
+      $working_copy = $this->getWorkingCopy();
+      foreach ($paths as $key => $path) {
+        $full_path = Filesystem::resolvePath($path);
+        if (!Filesystem::pathExists($full_path)) {
+          throw new ArcanistUsageException("Path '{$path}' does not exist!");
+        }
+        $relative_path = Filesystem::readablePath(
+          $full_path,
+          $working_copy->getProjectRoot());
+        $paths[$key] = $relative_path;
+      }
+    } else {
+      $repository_api = $this->getRepositoryAPI();
+      if ($rev) {
+        $repository_api->parseRelativeLocalCommit(array($rev));
+      }
+
+      $paths = $repository_api->getWorkingCopyStatus();
+      foreach ($paths as $path => $flags) {
+        if ($flags & ArcanistRepositoryAPI::FLAG_UNTRACKED) {
+          unset($paths[$path]);
+        }
+      }
+      $paths = array_keys($paths);
+    }
+
+    return array_values($paths);
+  }
+
 }

src/workflow/base/__init__.php

@@ -14,6 +14,7 @@ phutil_require_module('arcanist', 'parser/bundle');
 phutil_require_module('arcanist', 'parser/diff');
 phutil_require_module('arcanist', 'parser/diff/change');
 phutil_require_module('arcanist', 'parser/diff/changetype');
+phutil_require_module('arcanist', 'repository/api/base');
 
 phutil_require_module('phutil', 'conduit/client');
 phutil_require_module('phutil', 'console');

src/workflow/diff/ArcanistDiffWorkflow.php

@@ -956,9 +956,8 @@ EOTEXT
     echo "Linting...\n";
     try {
       $argv = $this->getPassthruArgumentsAsArgv('lint');
-      if ($repository_api instanceof ArcanistSubversionAPI) {
+      if ($repository_api->supportsRelativeLocalCommits()) {
-        $argv = array_merge($argv, array_keys($paths));
+        $argv[] = '--rev';
-      } else {
         $argv[] = $repository_api->getRelativeCommit();
       }
       $lint_workflow = $this->buildChildWorkflow('lint', $argv);
@@ -1021,8 +1020,9 @@ EOTEXT
     echo "Running unit tests...\n";
     try {
       $argv = $this->getPassthruArgumentsAsArgv('unit');
-      if ($repository_api instanceof ArcanistSubversionAPI) {
+      if ($repository_api->supportsRelativeLocalCommits()) {
-        $argv = array_merge($argv, array_keys($paths));
+        $argv[] = '--rev';
+        $argv[] = $repository_api->getRelativeCommit();
       }
       $this->unitWorkflow = $this->buildChildWorkflow('unit', $argv);
       $unit_result = $this->unitWorkflow->run();

src/workflow/lint/ArcanistLintWorkflow.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -38,9 +38,9 @@ class ArcanistLintWorkflow extends ArcanistBaseWorkflow {
 
   public function getCommandHelp() {
     return phutil_console_format(<<<EOTEXT
-      **lint** [__options__] [__paths__] (svn)
+      **lint** [__options__] [__paths__]
-      **lint** [__options__] [__commit_range__] (git)
+      **lint** [__options__] --rev [__rev__]
-          Supports: git, svn
+          Supports: git, svn, hg
           Run static analysis on changes to check for mistakes. If no files
           are specified, lint will be run on all files which have been modified.
 EOTEXT
@@ -53,6 +53,17 @@ EOTEXT
         'help' =>
           "Show all lint warnings, not just those on changed lines."
       ),
+      'rev' => array(
+        'param' => 'revision',
+        'help' => "Lint changes since a specific revision.",
+        'supports' => array(
+          'git',
+          'hg',
+        ),
+        'nosupport' => array(
+          'svn' => "Lint does not currently support --rev in SVN.",
+        ),
+      ),
       'output' => array(
         'param' => 'format',
         'help' =>
@@ -90,6 +101,10 @@ EOTEXT
     return true;
   }
 
+  public function requiresRepositoryAPI() {
+    return true;
+  }
+
   public function run() {
     $working_copy = $this->getWorkingCopy();
 
@@ -103,61 +118,23 @@ EOTEXT
       }
     }
 
-    $should_lint_all = $this->getArgument('lintall');
+    $rev = $this->getArgument('rev');
+    $paths = $this->getArgument('paths');
 
-    $repository_api = null;
+    if ($rev && $paths) {
-    if (!$should_lint_all) {
+      throw new ArcanistUsageException("Specify either --rev or paths.");
-      try {
-        $repository_api = ArcanistRepositoryAPI::newAPIFromWorkingCopyIdentity(
-          $working_copy);
-        $this->setRepositoryAPI($repository_api);
-      } catch (ArcanistUsageException $ex) {
-        throw new ArcanistUsageException(
-          $ex->getMessage()."\n\n".
-          "Use '--lintall' to ignore working copy changes when running lint.");
-      }
-
-      if ($repository_api instanceof ArcanistSubversionAPI) {
-        $paths = $repository_api->getWorkingCopyStatus();
-        $list  = new FileList($this->getArgument('paths'));
-        foreach ($paths as $path => $flags) {
-          if (!$list->contains($path)) {
-            unset($paths[$path]);
-          }
-        }
-      } else if ($repository_api->supportsRelativeLocalCommits()) {
-        $repository_api->parseRelativeLocalCommit(
-          $this->getArgument('paths', array()));
-        $paths = $repository_api->getWorkingCopyStatus();
-      } else {
-        throw new Exception("Unknown VCS!");
-      }
-
-      foreach ($paths as $path => $flags) {
-        if ($flags & ArcanistRepositoryAPI::FLAG_UNTRACKED) {
-          unset($paths[$path]);
-        }
-      }
-
-      $paths = array_keys($paths);
-    } else {
-      $paths = $this->getArgument('paths');
-      if (empty($paths)) {
-        throw new ArcanistUsageException(
-          "You must specify one or more files to lint when using '--lintall'.");
-      }
-      foreach ($paths as $key => $path) {
-        $full_path = Filesystem::resolvePath($path);
-        if (!Filesystem::pathExists($full_path)) {
-          throw new ArcanistUsageException("Path '{$path}' does not exist!");
-        }
-        $relative_path = Filesystem::readablePath(
-          $full_path,
-          $working_copy->getProjectRoot());
-        $paths[$key] = $relative_path;
-      }
     }
 
+    $should_lint_all = $this->getArgument('lintall');
+    if ($paths) {
+      // NOTE: When the user specifies paths, we imply --lintall and show all
+      // warnings for the paths in question. This is easier to deal with for
+      // us and less confusing for users.
+      $should_lint_all = true;
+    }
+
+    $paths = $this->selectPathsForWorkflow($paths, $rev);
+
     PhutilSymbolLoader::loadClass($engine);
     if (!is_subclass_of($engine, 'ArcanistLintEngine')) {
       throw new ArcanistUsageException(
@@ -262,6 +239,7 @@ EOTEXT
       }
     }
 
+    $repository_api = $this->getRepositoryAPI();
     if ($wrote_to_disk &&
         ($repository_api instanceof ArcanistGitAPI) &&
         $this->shouldAmendChanges) {

src/workflow/lint/__init__.php

@@ -11,12 +11,10 @@ phutil_require_module('arcanist', 'exception/usage/noengine');
 phutil_require_module('arcanist', 'lint/patcher');
 phutil_require_module('arcanist', 'lint/renderer');
 phutil_require_module('arcanist', 'lint/severity');
-phutil_require_module('arcanist', 'repository/api/base');
 phutil_require_module('arcanist', 'workflow/base');
 
 phutil_require_module('phutil', 'console');
 phutil_require_module('phutil', 'filesystem');
-phutil_require_module('phutil', 'filesystem/filelist');
 phutil_require_module('phutil', 'filesystem/tempfile');
 phutil_require_module('phutil', 'future/exec');
 phutil_require_module('phutil', 'symbols');

src/workflow/shell-complete/ArcanistShellCompleteWorkflow.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.

src/workflow/svn-hook-pre-commit/ArcanistSvnHookPreCommitWorkflow.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.

src/workflow/unit/ArcanistUnitWorkflow.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -34,8 +34,9 @@ class ArcanistUnitWorkflow extends ArcanistBaseWorkflow {
 
   public function getCommandHelp() {
     return phutil_console_format(<<<EOTEXT
-      **unit** [__paths__]
+      **unit** [__options__] [__paths__]
-          Supports: git, svn
+      **unit** [__options__] --rev [__rev__]
+          Supports: git, svn, hg
           Run unit tests that cover specified paths. If no paths are specified,
           unit tests covering all modified files will be run.
 EOTEXT
@@ -44,6 +45,17 @@ EOTEXT
 
   public function getArguments() {
     return array(
+      'rev' => array(
+        'param' => 'revision',
+        'help' => "Run unit tests covering changes since a specific revision.",
+        'supports' => array(
+          'git',
+          'hg',
+        ),
+        'nosupport' => array(
+          'svn' => "Arc unit does not currently support --rev in SVN.",
+        ),
+      ),
       'engine' => array(
         'param' => 'classname',
         'help' =>
@@ -79,23 +91,10 @@ EOTEXT
         "to specify a unit test engine.");
     }
 
-    $repository_api = $this->getRepositoryAPI();
+    $paths = $this->getArgument('paths');
+    $rev = $this->getArgument('rev');
 
-    if ($this->getArgument('paths')) {
+    $paths = $this->selectPathsForWorkflow($paths, $rev);
-      // TODO: deal with git stuff
-      $paths = $this->getArgument('paths');
-    } else {
-      $paths = $repository_api->getWorkingCopyStatus();
-
-      // TODO: clean this up
-      foreach ($paths as $path => $mask) {
-        if ($mask & ArcanistRepositoryAPI::FLAG_UNTRACKED) {
-          unset($paths[$path]);
-        }
-      }
-
-      $paths = array_keys($paths);
-    }
 
     PhutilSymbolLoader::loadClass($engine_class);
     if (!is_subclass_of($engine_class, 'ArcanistBaseUnitTestEngine')) {

src/workflow/unit/__init__.php

@@ -8,7 +8,6 @@
 
 phutil_require_module('arcanist', 'exception/usage');
 phutil_require_module('arcanist', 'exception/usage/noengine');
-phutil_require_module('arcanist', 'repository/api/base');
 phutil_require_module('arcanist', 'unit/result');
 phutil_require_module('arcanist', 'workflow/base');