implement -nnn style flags in argparse

This implements support for numeric flags without an associated short or
long flag name. This pattern is used by many commands. For example `head
-3 /a/file` to emit the first three lines of the file.

Fixes #4214

doc_src/argparse.txt

@@ -60,7 +60,7 @@ The first `--` seen is what allows the `argparse` command to reliably seperate t
 
 Each option specification is a string composed of
 
-- A short flag letter (which is mandatory).
+- A short flag letter (which is mandatory). It must be an alphanumeric or "#". The "#" character is special and means that a flag of the form `-123` is valid. The short flag "#" must be followed by "-" (since the short name isn't otherwise valid since `_flag_#` is not a valid var name) and must but followed by a long flag name with no modifiers.
 
 - A `/` if the short flag can be used by someone invoking your command else `-` if it should not be exposed as a valid short flag. If there is no long flag name these characters should be omitted.
 
@@ -98,7 +98,9 @@ Some OPTION_SPEC examples:
 
 - `x-` is not valid since there is no long flag name and therefore the short flag, `-x`, has to be usable. This is obviously true whether or not the specification also includes one of `:`, `::`, `+`.
 
-After parsing the arguments the argv var is set (with local scope) to any values not already consumed during flag processing.
+- `#-max` means that flags matching the regex "^--?\d+$" are valid. When seen they are assigned to the variable `_flag_max`. This allows any valid positive or negative integer to be specified by prefixing it with a single "-". Many commands support this idiom. For example `head -3 /a/file` to emit only the first three lines of /a/file.
+
+After parsing the arguments the `argv` var is set with local scope to any values not already consumed during flag processing. If there are not unbound values the var is set but `count $argv` will be zero.
 
 If an error occurs during argparse processing it will exit with a non-zero status and appropriate error messages are written to stderr.
 

src/builtin_argparse.cpp

@@ -173,6 +173,12 @@ static int parse_exclusive_args(argparse_cmd_opts_t &opts, io_streams_t &streams
 static bool parse_flag_modifiers(argparse_cmd_opts_t &opts, option_spec_t *opt_spec,
                                  const wcstring &option_spec, const wchar_t *s,
                                  io_streams_t &streams) {
+    if (opt_spec->short_flag == L'#' && *s) {
+        streams.err.append_format(_(L"%ls: Short flag '#' does not allow modifiers like '%lc'\n"),
+                                  opts.name.c_str(), *s);
+        return false;
+    }
+
     if (*s == L'=') {
         s++;
         if (*s == L'?') {
@@ -200,19 +206,33 @@ static bool parse_flag_modifiers(argparse_cmd_opts_t &opts, option_spec_t *opt_s
 static bool parse_option_spec(argparse_cmd_opts_t &opts, wcstring option_spec,
                               io_streams_t &streams) {
     if (option_spec.empty()) {
-        streams.err.append_format(_(L"%s: An option spec must have a short flag letter\n"),
+        streams.err.append_format(_(L"%ls: An option spec must have a short flag letter\n"),
                                   opts.name.c_str());
         return false;
     }
 
     const wchar_t *s = option_spec.c_str();
+    if (!iswalnum(*s) && *s != L'#') {
+        streams.err.append_format(_(L"%ls: Short flag '%lc' invalid, must be alphanum or '#'\n"),
+                                  opts.name.c_str(), *s);
+        return false;
+    }
     option_spec_t *opt_spec = new option_spec_t(*s++);
 
-    if (*s == L'/') {
-        s++;  // the struct is initialized assuming short_flag_valid should be true
+    if (*(s - 1) == L'#') {
+        if (*s != L'-') {
+            streams.err.append_format(
+                    _(L"%ls: Short flag '#' must be followed by '-' and a long name\n"),
+                    opts.name.c_str());
+            return false;
+        }
+        opt_spec->short_flag_valid = false;
+        s++;
     } else if (*s == L'-') {
         opt_spec->short_flag_valid = false;
         s++;
+    } else if (*s == L'/') {
+        s++;  // the struct is initialized assuming short_flag_valid should be true
     } else {
         // Long flag name not allowed if second char isn't '/' or '-' so just check for
         // behavior modifier chars.
@@ -361,6 +381,8 @@ static void populate_option_strings(
 static void update_bool_flag_counts(argparse_cmd_opts_t &opts) {
     for (auto it : opts.options) {
         auto opt_spec = it.second;
+        // The '#' short flag is special. It doesn't take any values but isn't a boolean arg.
+        if (opt_spec->short_flag == L'#') continue;
         if (opt_spec->num_allowed != 0 || opt_spec->num_seen == 0) continue;
         wchar_t count[20];
         swprintf(count, sizeof count / sizeof count[0], L"%d", opt_spec->num_seen);
@@ -380,6 +402,21 @@ static int argparse_parse_flags(argparse_cmd_opts_t &opts, const wchar_t *short_
             return STATUS_INVALID_ARGS;
         }
         if (opt == '?') {
+            auto found = opts.options.find(L'#');
+            if (found != opts.options.end()) {
+                // Try to parse it as a number; e.g., "-123".
+                long x = fish_wcstol(argv[w.woptind - 1] + 1);
+                if (!errno) {
+                    wchar_t val[20];
+                    swprintf(val, sizeof val / sizeof val[0], L"%ld", x);
+                    auto opt_spec = found->second;
+                    opt_spec->vals.clear();
+                    opt_spec->vals.push_back(wcstring(val));
+                    opt_spec->num_seen++;
+                    w.nextchar = NULL;
+                    continue;
+                }
+            }
             builtin_unknown_option(parser, streams, cmd, argv[w.woptind - 1]);
             return STATUS_INVALID_ARGS;
         }
@@ -446,11 +483,10 @@ static int argparse_parse_args(argparse_cmd_opts_t &opts, const wcstring_list_t
     return STATUS_CMD_OK;
 }
 
-static int check_min_max_args_constraints(argparse_cmd_opts_t &opts, const wcstring_list_t &args,
-                                          parser_t &parser, io_streams_t &streams) {
+static int check_min_max_args_constraints(argparse_cmd_opts_t &opts, parser_t &parser,
+                                          io_streams_t &streams) {
     UNUSED(parser);
     const wchar_t *cmd = opts.name.c_str();
-    int argc = static_cast<int>(args.size());
 
     if (opts.argv.size() < opts.min_args) {
         streams.err.append_format(BUILTIN_ERR_MIN_ARG_COUNT1, cmd, opts.min_args, opts.argv.size());
@@ -464,6 +500,34 @@ static int check_min_max_args_constraints(argparse_cmd_opts_t &opts, const wcstr
     return STATUS_CMD_OK;
 }
 
+/// Put the result of parsing the supplied args into the caller environment as local vars.
+static void set_argparse_result_vars(argparse_cmd_opts_t &opts) {
+    for (auto it : opts.options) {
+        option_spec_t *opt_spec = it.second;
+        if (!opt_spec->num_seen) continue;
+
+        wcstring var_name_prefix = L"_flag_";
+        auto val = list_to_array_val(opt_spec->vals);
+        if (opt_spec->short_flag_valid) {
+            env_set(var_name_prefix + opt_spec->short_flag, *val == ENV_NULL ? NULL : val->c_str(),
+                    ENV_LOCAL);
+        }
+        if (!opt_spec->long_flag.empty()) {
+            // We do a simple replacement of all non alphanum chars rather than calling
+            // escape_string(long_flag, 0, STRING_STYLE_VAR).
+            wcstring long_flag = opt_spec->long_flag;
+            for (size_t pos = 0; pos < long_flag.size(); pos++) {
+                if (!iswalnum(long_flag[pos])) long_flag[pos] = L'_';
+            }
+            env_set(var_name_prefix + long_flag, *val == ENV_NULL ? NULL : val->c_str(),
+                    ENV_LOCAL);
+        }
+    }
+
+    auto val = list_to_array_val(opts.argv);
+    env_set(L"argv", *val == ENV_NULL ? NULL : val->c_str(), ENV_LOCAL);
+}
+
 /// The argparse builtin. This is explicitly not compatible with the BSD or GNU version of this
 /// command. That's because fish doesn't have the weird quoting problems of POSIX shells. So we
 /// don't need to support flags like `--unquoted`. Similarly we don't want to support introducing
@@ -485,14 +549,6 @@ int builtin_argparse(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
         return STATUS_CMD_OK;
     }
 
-#if 0
-    if (optind == argc) {
-        // Apparently we weren't handed any arguments to be parsed according to the option specs we
-        // just collected. So there isn't anything for us to do.
-        return STATUS_CMD_OK;
-    }
-#endif
-
     wcstring_list_t args;
     args.push_back(opts.name);
     while (optind < argc) args.push_back(argv[optind++]);
@@ -503,31 +559,9 @@ int builtin_argparse(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
     retval = argparse_parse_args(opts, args, parser, streams);
     if (retval != STATUS_CMD_OK) return retval;
 
-    retval = check_min_max_args_constraints(opts, args, parser, streams);
+    retval = check_min_max_args_constraints(opts, parser, streams);
     if (retval != STATUS_CMD_OK) return retval;
 
-    for (auto it : opts.options) {
-        option_spec_t *opt_spec = it.second;
-        if (!opt_spec->num_seen) continue;
-
-        wcstring var_name_prefix = L"_flag_";
-        auto val = list_to_array_val(opt_spec->vals);
-        env_set(var_name_prefix + opt_spec->short_flag, *val == ENV_NULL ? NULL : val->c_str(),
-                ENV_LOCAL);
-        if (!opt_spec->long_flag.empty()) {
-            // We do a simple replacement of all non alphanum chars rather than calling
-            // escape_string(long_flag, 0, STRING_STYLE_VAR).
-            wcstring long_flag = opt_spec->long_flag;
-            for (size_t pos = 0; pos < long_flag.size(); pos++) {
-                if (!iswalnum(long_flag[pos])) long_flag[pos] = L'_';
-            }
-            env_set(var_name_prefix + long_flag, *val == ENV_NULL ? NULL : val->c_str(),
-                    ENV_LOCAL);
-        }
-    }
-
-    auto val = list_to_array_val(opts.argv);
-    env_set(L"argv", *val == ENV_NULL ? NULL : val->c_str(), ENV_LOCAL);
-
+    set_argparse_result_vars(opts);
     return retval;
 }

tests/argparse.err

@@ -6,7 +6,7 @@ argparse: Missing -- separator
 argparse: No option specs were provided
 # Invalid option specs
 argparse: Invalid option spec 'h-' at char '-'
-argparse: Invalid option spec '+help' at char 'h'
+argparse: Short flag '+' invalid, must be alphanum or '#'
 argparse: Invalid option spec 'h/help:' at char ':'
 argparse: Invalid option spec 'h-help::' at char ':'
 argparse: Invalid option spec 'h-help=x' at char 'x'
@@ -14,3 +14,10 @@ argparse: Invalid option spec 'h-help=x' at char 'x'
 min-max: Expected at least 1 args, got only 0
 min-max: Expected at most 3 args, got 4
 min-max: Expected at most 1 args, got 2
+# Invalid "#-val" spec
+argparse: Short flag '#' does not allow modifiers like '='
+# Invalid arg in the face of a "#-val" spec
+argparse: Unknown option '-x'
+Standard input (line 38): 
+argparse '#-val' -- abc -x def
+^

tests/argparse.in

@@ -31,6 +31,12 @@ begin
     argparse --name min-max --max-args 1 h/help -- arg1 arg2
 end
 
+echo '# Invalid "#-val" spec' >&2
+argparse '#-val=' -- abc -x def
+
+echo '# Invalid arg in the face of a "#-val" spec' >&2
+argparse '#-val' -- abc -x def
+
 ##########
 # Now verify that validly formed invocations work as expected.
 
@@ -64,3 +70,9 @@ begin
     set -l
     show $argv
 end
+
+echo '# A "#-val" spec works'
+argparse '#-val' -- abc -123 def
+set -l
+argparse 'v/verbose' '#-val' 't/token=' -- -123 a1 --token woohoo --234 -v a2 --verbose
+set -l

tests/argparse.out

@@ -34,3 +34,12 @@ count=3
 |non-opt|
 |second non-opt|
 |--help|
+# A "#-val" spec works
+_flag_val 123
+argv 'abc'  'def'
+_flag_t woohoo
+_flag_token woohoo
+_flag_v 2
+_flag_val -234
+_flag_verbose 2
+argv 'a1'  'a2'

tests/string.err

@@ -5,7 +5,7 @@ string match: ^
 
 # string invalidarg
 string: Subcommand 'invalidarg' is not valid
-Standard input (line 258): 
+Standard input (line 251): 
 string invalidarg; and echo "unexpected exit 0" >&2
 ^
 
@@ -29,6 +29,6 @@ string repeat: Expected argument
 
 # string repeat -l fakearg 2>&1
 string repeat: Unknown option '-l'
-Standard input (line 359): 
+Standard input (line 352): 
 string repeat -l fakearg
 ^

tests/string.in

@@ -1,12 +1,5 @@
 # Tests for string builtin. Mostly taken from man page examples.
 
-# We don't want syntax errors to emit command usage help. This makes the
-# stderr output considerably shorter and makes it easier to updates the tests
-# and documentation without having to make pointless changes to the test
-# output files.
-function __fish_print_help
-end
-
 echo '# string match -r -v "c.*" dog can cat diz'
 string match -r -v "c.*" dog can cat diz; and echo "exit 0"
 

tests/test_functions/__fish_print_help.fish

@@ -0,0 +1,6 @@
+# We don't want syntax errors to emit command usage help. This makes the
+# stderr output considerably shorter and makes it easier to updates the tests
+# and documentation without having to make pointless changes to the test
+# output files.
+function __fish_print_help
+end