From f3eccfbe81f1a6ae4a6755d10fdc42c632b96327 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Jan 2012 12:40:50 -0800 Subject: [PATCH] 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 path path2 path3 ...", which is a generally useful workflow. Test Plan: Ran "arc lint ", "arc unit ", "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 +- src/lint/engine/base/ArcanistLintEngine.php | 2 +- src/lint/linter/base/ArcanistLinter.php | 2 +- src/parser/bundle/ArcanistBundle.php | 2 +- .../hookapi/base/ArcanistHookAPI.php | 2 +- .../subversion/ArcanistSubversionHookAPI.php | 2 +- src/workflow/base/ArcanistBaseWorkflow.php | 47 +++++++++- src/workflow/base/__init__.php | 1 + src/workflow/diff/ArcanistDiffWorkflow.php | 10 +-- src/workflow/lint/ArcanistLintWorkflow.php | 90 +++++++------------ src/workflow/lint/__init__.php | 2 - .../ArcanistShellCompleteWorkflow.php | 2 +- .../ArcanistSvnHookPreCommitWorkflow.php | 2 +- src/workflow/unit/ArcanistUnitWorkflow.php | 37 ++++---- src/workflow/unit/__init__.php | 1 - 15 files changed, 112 insertions(+), 92 deletions(-) diff --git a/scripts/arcanist.php b/scripts/arcanist.php index 1ef7f431..4f772a63 100755 --- a/scripts/arcanist.php +++ b/scripts/arcanist.php @@ -2,7 +2,7 @@ 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); + } + } diff --git a/src/workflow/base/__init__.php b/src/workflow/base/__init__.php index b59c83a1..f23596cf 100644 --- a/src/workflow/base/__init__.php +++ b/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'); diff --git a/src/workflow/diff/ArcanistDiffWorkflow.php b/src/workflow/diff/ArcanistDiffWorkflow.php index 449ba706..f538978b 100644 --- a/src/workflow/diff/ArcanistDiffWorkflow.php +++ b/src/workflow/diff/ArcanistDiffWorkflow.php @@ -956,9 +956,8 @@ EOTEXT echo "Linting...\n"; try { $argv = $this->getPassthruArgumentsAsArgv('lint'); - if ($repository_api instanceof ArcanistSubversionAPI) { - $argv = array_merge($argv, array_keys($paths)); - } else { + if ($repository_api->supportsRelativeLocalCommits()) { + $argv[] = '--rev'; $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) { - $argv = array_merge($argv, array_keys($paths)); + if ($repository_api->supportsRelativeLocalCommits()) { + $argv[] = '--rev'; + $argv[] = $repository_api->getRelativeCommit(); } $this->unitWorkflow = $this->buildChildWorkflow('unit', $argv); $unit_result = $this->unitWorkflow->run(); diff --git a/src/workflow/lint/ArcanistLintWorkflow.php b/src/workflow/lint/ArcanistLintWorkflow.php index f50a88cf..8dc5c937 100644 --- a/src/workflow/lint/ArcanistLintWorkflow.php +++ b/src/workflow/lint/ArcanistLintWorkflow.php @@ -1,7 +1,7 @@ "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 (!$should_lint_all) { - 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; - } + if ($rev && $paths) { + throw new ArcanistUsageException("Specify either --rev or paths."); } + $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) { diff --git a/src/workflow/lint/__init__.php b/src/workflow/lint/__init__.php index 4560471f..b5c858d9 100644 --- a/src/workflow/lint/__init__.php +++ b/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'); diff --git a/src/workflow/shell-complete/ArcanistShellCompleteWorkflow.php b/src/workflow/shell-complete/ArcanistShellCompleteWorkflow.php index fa185391..7cd8463e 100644 --- a/src/workflow/shell-complete/ArcanistShellCompleteWorkflow.php +++ b/src/workflow/shell-complete/ArcanistShellCompleteWorkflow.php @@ -1,7 +1,7 @@ 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')) { - // 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); - } + $paths = $this->selectPathsForWorkflow($paths, $rev); PhutilSymbolLoader::loadClass($engine_class); if (!is_subclass_of($engine_class, 'ArcanistBaseUnitTestEngine')) { diff --git a/src/workflow/unit/__init__.php b/src/workflow/unit/__init__.php index cafa3c73..bc9e52a6 100644 --- a/src/workflow/unit/__init__.php +++ b/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');