Require "--" as an argument terminator when running noninteractively

Summary:
Depends on D21001. Ref T13490.

  - Require "--" to terminate arguments when running noninteractively.
  - Don't correct spelling when running noninteractively (we still suggest corrections).
  - Remove old "phage" wrapper script.
  - Fix an issue where "argv" could implicitly generate a parseable "--argv" flag.
  - Accept "--" as an argument terminator even when there is no argument list.
  - Update some strings to use more modern quoting style.
  - Make workflows decline to handle signals by default.
  - Allow "arc weld" to weld non-UTF8 files together.

Test Plan: Ran various commands. Welded non-UTF8 files.

Maniphest Tasks: T13490

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

scripts/phage.php

@@ -1,29 +0,0 @@
-#!/usr/bin/env php
-<?php
-
-require_once dirname(__FILE__).'/__init_script__.php';
-ini_set('memory_limit', -1);
-
-$args = new PhutilArgumentParser($argv);
-$args->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);

src/__phutil_library_map__.php

@@ -301,6 +301,7 @@ phutil_register_library_map(array(
     '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_register_library_map(array(
     '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 @@ phutil_register_library_map(array(
     'ArcanistMergeConflictLinter' => 'ArcanistLinter',
     'ArcanistMergeConflictLinterTestCase' => 'ArcanistLinterTestCase',
     'ArcanistMessageRevisionHardpointLoader' => 'ArcanistHardpointLoader',
+    'ArcanistMissingArgumentTerminatorException' => 'Exception',
     'ArcanistMissingLinterException' => 'Exception',
     'ArcanistModifierOrderingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistModifierOrderingXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',

src/exception/ArcanistMissingArgumentTerminatorException.php

@@ -0,0 +1,4 @@
+<?php
+
+final class ArcanistMissingArgumentTerminatorException
+  extends Exception {}

src/parser/argument/PhutilArgumentParser.php

@@ -78,6 +78,8 @@ final class PhutilArgumentParser extends Phobject {
   private $synopsis;
   private $workflows;
   private $showHelp;
+  private $requireArgumentTerminator = false;
+  private $sawTerminator = false;
 
   const PARSE_ERROR_CODE = 77;
 
@@ -129,8 +131,20 @@ final class PhutilArgumentParser extends Phobject {
     $specs = PhutilArgumentSpecification::newSpecsFromList($specs);
     $this->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 @@ final class PhutilArgumentParser extends Phobject {
         // 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 @@ final class PhutilArgumentParser extends Phobject {
           $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 @@ final class PhutilArgumentParser extends Phobject {
             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 @@ final class PhutilArgumentParser extends Phobject {
               } else {
                 throw new PhutilArgumentUsageException(
                   pht(
-                    "Argument '%s' requires a parameter.",
+                    'Argument "%s" requires a parameter.',
                     "{$pre}{$arg}"));
               }
             } else {
@@ -230,7 +246,7 @@ final class PhutilArgumentParser extends Phobject {
             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 @@ final class PhutilArgumentParser extends Phobject {
 
               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 @@ final class PhutilArgumentParser extends Phobject {
     }
 
     $this->argv = array_values($argv);
+
     return $this;
   }
 
@@ -300,10 +317,20 @@ final class PhutilArgumentParser extends Phobject {
   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 @@ final class PhutilArgumentParser extends Phobject {
       $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 @@ final class PhutilArgumentParser extends Phobject {
     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 @@ final class PhutilArgumentParser extends Phobject {
   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 @@ final class PhutilArgumentParser extends Phobject {
 
 /* -(  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 @@ final class PhutilArgumentParser extends Phobject {
 
         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 @@ final class PhutilArgumentParser extends Phobject {
 
       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 @@ final class PhutilArgumentParser extends Phobject {
         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 @@ final class PhutilArgumentParser extends Phobject {
         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 @@ final class PhutilArgumentParser extends Phobject {
         "%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();
+  }
+
 }

src/runtime/ArcanistRuntime.php

@@ -68,6 +68,14 @@ final class ArcanistRuntime {
     $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 @@ final class ArcanistRuntime {
 
     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

src/toolset/workflow/ArcanistAliasWorkflow.php

@@ -145,6 +145,8 @@ EOTEXT
     // 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);

src/utils/utils.php

@@ -1918,3 +1918,11 @@ function phutil_escape_uri_path_component($string) {
 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;
+}

src/workflow/ArcanistWeldWorkflow.php

@@ -87,8 +87,20 @@ EOTEXT
     $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));
 

src/workflow/ArcanistWorkflow.php

@@ -2282,4 +2282,8 @@ abstract class ArcanistWorkflow extends Phobject {
     return $this->getConfigurationSourceList()->getConfig($key);
   }
 
+  final public function canHandleSignal($signo) {
+    return false;
+  }
+
 }