diff --git a/scripts/phage.php b/scripts/phage.php deleted file mode 100755 --- a/scripts/phage.php +++ /dev/null @@ -1,29 +0,0 @@ -#!/usr/bin/env php -parseStandardArguments(); - -$args->parsePartial(array()); - - -// TODO: This is pretty minimal and should be shared with "arc". -$working_directory = getcwd(); -$working_copy = ArcanistWorkingCopyIdentity::newFromPath($working_directory); -$config = id(new ArcanistConfigurationManager()) - ->setWorkingCopyIdentity($working_copy); - -foreach ($config->getProjectConfig('load') as $load) { - $load = Filesystem::resolvePath($working_copy->getProjectRoot().'/'.$load); - phutil_load_library($load); -} - - -$workflows = id(new PhutilClassMapQuery()) - ->setAncestorClass('PhageWorkflow') - ->execute(); -$workflows[] = new PhutilHelpArgumentWorkflow(); -$args->parseWorkflows($workflows); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -301,6 +301,7 @@ 'ArcanistMergeConflictLinter' => 'lint/linter/ArcanistMergeConflictLinter.php', 'ArcanistMergeConflictLinterTestCase' => 'lint/linter/__tests__/ArcanistMergeConflictLinterTestCase.php', 'ArcanistMessageRevisionHardpointLoader' => 'loader/ArcanistMessageRevisionHardpointLoader.php', + 'ArcanistMissingArgumentTerminatorException' => 'exception/ArcanistMissingArgumentTerminatorException.php', 'ArcanistMissingLinterException' => 'lint/linter/exception/ArcanistMissingLinterException.php', 'ArcanistModifierOrderingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistModifierOrderingXHPASTLinterRule.php', 'ArcanistModifierOrderingXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistModifierOrderingXHPASTLinterRuleTestCase.php', @@ -885,6 +886,7 @@ 'phutil_ini_decode' => 'utils/utils.php', 'phutil_is_hiphop_runtime' => 'utils/utils.php', 'phutil_is_natural_list' => 'utils/utils.php', + 'phutil_is_noninteractive' => 'utils/utils.php', 'phutil_is_system_locale_available' => 'utils/utf8.php', 'phutil_is_utf8' => 'utils/utf8.php', 'phutil_is_utf8_slowly' => 'utils/utf8.php', @@ -1240,6 +1242,7 @@ 'ArcanistMergeConflictLinter' => 'ArcanistLinter', 'ArcanistMergeConflictLinterTestCase' => 'ArcanistLinterTestCase', 'ArcanistMessageRevisionHardpointLoader' => 'ArcanistHardpointLoader', + 'ArcanistMissingArgumentTerminatorException' => 'Exception', 'ArcanistMissingLinterException' => 'Exception', 'ArcanistModifierOrderingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistModifierOrderingXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', diff --git a/src/exception/ArcanistMissingArgumentTerminatorException.php b/src/exception/ArcanistMissingArgumentTerminatorException.php new file mode 100644 --- /dev/null +++ b/src/exception/ArcanistMissingArgumentTerminatorException.php @@ -0,0 +1,4 @@ +mergeSpecs($specs); - $specs_by_name = mpull($specs, null, 'getName'); - $specs_by_short = mpull($specs, null, 'getShortAlias'); + // Wildcard arguments have a name like "argv", but we don't want to + // parse a corresponding flag like "--argv". Filter them out before + // building a list of available flags. + $non_wildcard = array(); + foreach ($specs as $spec_key => $spec) { + if ($spec->getWildcard()) { + continue; + } + + $non_wildcard[$spec_key] = $spec; + } + + $specs_by_name = mpull($non_wildcard, null, 'getName'); + $specs_by_short = mpull($non_wildcard, null, 'getShortAlias'); unset($specs_by_short[null]); $argv = $this->argv; @@ -144,6 +158,7 @@ // Non-string argument; pass it through as-is. } else if ($arg == '--') { // This indicates "end of flags". + $this->sawTerminator = true; break; } else if ($arg == '-') { // This is a normal argument (e.g., stdin). @@ -174,7 +189,8 @@ $corrections = PhutilArgumentSpellingCorrector::newFlagCorrector() ->correctSpelling($arg, $options); - if (count($corrections) == 1) { + $should_autocorrect = $this->shouldAutocorrect(); + if (count($corrections) == 1 && $should_autocorrect) { $corrected = head($corrections); $this->logMessage( @@ -206,7 +222,7 @@ if ($param_name === null) { throw new PhutilArgumentUsageException( pht( - "Argument '%s' does not take a parameter.", + 'Argument "%s" does not take a parameter.', "{$pre}{$arg}")); } } else { @@ -218,7 +234,7 @@ } else { throw new PhutilArgumentUsageException( pht( - "Argument '%s' requires a parameter.", + 'Argument "%s" requires a parameter.', "{$pre}{$arg}")); } } else { @@ -230,7 +246,7 @@ if (array_key_exists($spec->getName(), $this->results)) { throw new PhutilArgumentUsageException( pht( - "Argument '%s' was provided twice.", + 'Argument "%s" was provided twice.', "{$pre}{$arg}")); } } @@ -247,7 +263,7 @@ throw new PhutilArgumentUsageException( pht( - "Argument '%s' conflicts with argument '%s'%s", + 'Argument "%s" conflicts with argument "%s"%s', "{$pre}{$arg}", "--{$conflict}", $reason)); @@ -279,6 +295,7 @@ } $this->argv = array_values($argv); + return $this; } @@ -300,10 +317,20 @@ public function parseFull(array $specs) { $this->parseInternal($specs, true, false); - if (count($this->argv)) { - $arg = head($this->argv); + // If we have remaining unconsumed arguments other than a single "--", + // fail. + $argv = $this->filterWildcardArgv($this->argv); + if ($argv) { throw new PhutilArgumentUsageException( - pht("Unrecognized argument '%s'.", $arg)); + pht( + 'Unrecognized argument "%s".', + head($argv))); + } + + if ($this->getRequireArgumentTerminator()) { + if (!$this->sawTerminator) { + throw new ArcanistMissingArgumentTerminatorException(); + } } if ($this->showHelp) { @@ -408,7 +435,8 @@ $corrected = PhutilArgumentSpellingCorrector::newCommandCorrector() ->correctSpelling($flow, array_keys($this->workflows)); - if (count($corrected) == 1) { + $should_autocorrect = $this->shouldAutocorrect(); + if (count($corrected) == 1 && $should_autocorrect) { $corrected = head($corrected); $this->logMessage( @@ -551,7 +579,7 @@ if ($xprofile) { if (!function_exists('xhprof_enable')) { throw new Exception( - pht("To use '%s', you must install XHProf.", '--xprofile')); + pht('To use "--xprofile", you must install XHProf.')); } xhprof_enable(0); @@ -580,7 +608,7 @@ public function getArg($name) { if (empty($this->specs[$name])) { throw new PhutilArgumentSpecificationException( - pht("No specification exists for argument '%s'!", $name)); + pht('No specification exists for argument "%s"!', $name)); } if (idx($this->results, $name) !== null) { @@ -602,6 +630,14 @@ /* -( Command Help )------------------------------------------------------- */ + public function setRequireArgumentTerminator($require) { + $this->requireArgumentTerminator = $require; + return $this; + } + + public function getRequireArgumentTerminator() { + return $this->requireArgumentTerminator; + } public function setSynopsis($synopsis) { $this->synopsis = $synopsis; @@ -750,8 +786,8 @@ throw new PhutilArgumentUsageException( pht( - "Argument '%s' is unrecognized. Use '%s' to indicate ". - "the end of flags.", + 'Argument "%s" is unrecognized. Use "%s" to indicate '. + 'the end of flags.', $value, '--')); } @@ -778,7 +814,9 @@ if (isset($this->specs[$name])) { throw new PhutilArgumentSpecificationException( - pht("Two argument specifications have the same name ('%s').", $name)); + pht( + 'Two argument specifications have the same name ("%s").', + $name)); } $short = $spec->getShortAlias(); @@ -786,7 +824,7 @@ if (isset($short_map[$short])) { throw new PhutilArgumentSpecificationException( pht( - "Two argument specifications have the same short alias ('%s').", + 'Two argument specifications have the same short alias ("%s").', $short)); } $short_map[$short] = $spec; @@ -811,13 +849,15 @@ if (empty($this->specs[$conflict])) { throw new PhutilArgumentSpecificationException( pht( - "Argument '%s' conflicts with unspecified argument '%s'.", + 'Argument "%s" conflicts with unspecified argument "%s".', $name, $conflict)); } if ($conflict == $name) { throw new PhutilArgumentSpecificationException( - pht("Argument '%s' conflicts with itself!", $name)); + pht( + 'Argument "%s" conflicts with itself!', + $name)); } } } @@ -925,11 +965,15 @@ "%B\n%s", $message, pht( - 'For details on available commands, run `%s`.', + 'For details on available commands, run "%s".', "{$binary} help")); } throw new PhutilArgumentUsageException($message); } + private function shouldAutocorrect() { + return !phutil_is_noninteractive(); + } + } diff --git a/src/runtime/ArcanistRuntime.php b/src/runtime/ArcanistRuntime.php --- a/src/runtime/ArcanistRuntime.php +++ b/src/runtime/ArcanistRuntime.php @@ -68,6 +68,14 @@ $args = id(new PhutilArgumentParser($argv)) ->parseStandardArguments(); + // If we can test whether STDIN is a TTY, and it isn't, require that "--" + // appear in the argument list. This is intended to make it very hard to + // write unsafe scripts on top of Arcanist. + + if (phutil_is_noninteractive()) { + $args->setRequireArgumentTerminator(true); + } + $is_trace = $args->getArg('trace'); $log->setShowTraceMessages($is_trace); @@ -138,6 +146,29 @@ try { return $args->parseWorkflowsFull($phutil_workflows); + } catch (ArcanistMissingArgumentTerminatorException $terminator_exception) { + $log->writeHint( + pht('USAGE'), + pht( + '"%s" is being run noninteractively, but the argument list is '. + 'missing "--" to indicate end of flags.', + $toolset->getToolsetKey())); + + $log->writeHint( + pht('USAGE'), + pht( + 'When running noninteractively, you MUST provide "--" to all '. + 'commands (even if they take no arguments).')); + + $log->writeHint( + pht('USAGE'), + tsprintf( + '%s <__%s__>', + pht('Learn More:'), + 'https://phurl.io/u/noninteractive')); + + throw new PhutilArgumentUsageException( + pht('Missing required "--" in argument list.')); } catch (PhutilArgumentUsageException $usage_exception) { // TODO: This is very, very hacky; we're trying to let errors like diff --git a/src/toolset/workflow/ArcanistAliasWorkflow.php b/src/toolset/workflow/ArcanistAliasWorkflow.php --- a/src/toolset/workflow/ArcanistAliasWorkflow.php +++ b/src/toolset/workflow/ArcanistAliasWorkflow.php @@ -145,6 +145,8 @@ // TOOLSETS: Check if the user already has an alias for this trigger, and // prompt them to overwrite it. Needs prompting to work. + // TOOLSETS: Don't let users set aliases which don't resolve to anything. + $aliases[] = $alias; $this->writeAliases($aliases); diff --git a/src/utils/utils.php b/src/utils/utils.php --- a/src/utils/utils.php +++ b/src/utils/utils.php @@ -1918,3 +1918,11 @@ function phutil_unescape_uri_path_component($string) { return rawurldecode($string); } + +function phutil_is_noninteractive() { + if (function_exists('posix_isatty') && !posix_isatty(STDIN)) { + return true; + } + + return false; +} diff --git a/src/workflow/ArcanistWeldWorkflow.php b/src/workflow/ArcanistWeldWorkflow.php --- a/src/workflow/ArcanistWeldWorkflow.php +++ b/src/workflow/ArcanistWeldWorkflow.php @@ -87,8 +87,20 @@ $u = rtrim($u, "\r\n"); $v = rtrim($v, "\r\n"); - $u = phutil_utf8v_combined($u); - $v = phutil_utf8v_combined($v); + // If the inputs are UTF8, split glyphs (so two valid UTF8 inputs always + // produce a sensible, valid UTF8 output). If they aren't, split bytes. + + if (phutil_is_utf8($u)) { + $u = phutil_utf8v_combined($u); + } else { + $u = str_split($u); + } + + if (phutil_is_utf8($v)) { + $v = phutil_utf8v_combined($v); + } else { + $v = str_split($v); + } $len = max(count($u), count($v)); diff --git a/src/workflow/ArcanistWorkflow.php b/src/workflow/ArcanistWorkflow.php --- a/src/workflow/ArcanistWorkflow.php +++ b/src/workflow/ArcanistWorkflow.php @@ -2282,4 +2282,8 @@ return $this->getConfigurationSourceList()->getConfig($key); } + final public function canHandleSignal($signo) { + return false; + } + }