Argparser variable arg option parsing fix/improvement.

doc/appendices/command-line/traffic_ctl.en.rst

@@ -456,11 +456,17 @@ Display the current value of a configuration record.
 
       .. note::
 
-         ``-D`` uses variable-argument parsing and must appear as the **last option**
-         on the command line. Any flags placed after ``-D`` will be consumed as directive
-         values. ``-D`` and ``-d`` cannot be combined in the same invocation due to this
-         same constraint. Use ``-d`` with full YAML when you need both directives and
-         inline content in a single reload request.
+         ``-D`` accepts values until the next option or the end of the command line, so it
+         may appear anywhere among the options and can be combined with ``-d`` — directives
+         and inline content merge under the same config key:
+
+         .. code-block:: bash
+
+            $ traffic_ctl config reload -D myconfig.id=foo --monitor
+            $ traffic_ctl config reload -D myconfig.id=foo -d 'myconfig: {rules: [a]}'
+
+         To pass a directive value that begins with ``-``, place ``--`` before it; every
+         token after ``--`` is taken as a value rather than an option.
 
       .. note::
 
@@ -563,6 +569,25 @@ Display the current value of a configuration record.
    Specifying the file name is not needed as `traffic_ctl` will try to use the build(or the runroot if used) information to figure
    out the path to the `records.yaml`.
 
+   ``-c`` accepts at most one file name, so it may be written before or after the record
+   names:
+
+   .. code-block:: bash
+
+      $ traffic_ctl config get -c records.yaml proxy.config.diags.debug.enabled
+      $ traffic_ctl config get proxy.config.diags.debug.enabled -c records.yaml
+      $ traffic_ctl config get --cold=records.yaml proxy.config.diags.debug.enabled
+
+   When no file name is given, write ``-c`` last, or use the ``--cold=`` form for the
+   explicit file. A bare ``-c`` followed by a record name is ambiguous, because the record
+   name is taken as the file name:
+
+   .. code-block:: bash
+
+      $ traffic_ctl config get proxy.config.diags.debug.enabled -c    # default records.yaml
+      $ traffic_ctl config get -c proxy.config.diags.debug.enabled    # wrong, reads a file
+                                                                      # named for the record
+
    If the file exists and is empty a new document will be created. If a file does not exist, an attempt to create a new file will be done.
 
    This option(only for the config file changes) lets you use the prefix `proxy.config.` or `ts.` for variable names, either would work.

doc/developer-guide/internal-libraries/ArgParser.en.rst

@@ -104,6 +104,33 @@ To add options to the parser or current command:
 
 This function call returns the new :class:`Option` instance. (0 is also number of arguments expected)
 
+.. Note::
+
+   For options, the number of arguments may also be one of the following, which mirror the
+   ``nargs`` values of Python's ``argparse``:
+
+   ================================ =======================================================
+   Value                            Meaning
+   ================================ =======================================================
+   ``AT_MOST_ONE_ARG_N``            Zero or one value (``argparse`` ``nargs='?'``)
+   ``MORE_THAN_ZERO_ARG_N``         Zero or more values (``argparse`` ``nargs='*'``)
+   ``MORE_THAN_ONE_ARG_N``          One or more values (``argparse`` ``nargs='+'``)
+   ================================ =======================================================
+
+   An option taking a variable number of values stops collecting when it reaches a token
+   naming another option of the same command, so options written afterwards keep their own
+   arguments. Use ``AT_MOST_ONE_ARG_N`` rather than ``MORE_THAN_ZERO_ARG_N`` for an option
+   whose value is optional, otherwise it also consumes the positional arguments of its
+   command.
+
+   A token naming another option is not a value for a fixed number of arguments either. An
+   option written where a value is expected leaves the value missing, which is reported as a
+   usage error rather than the option being consumed and applied as the value.
+
+   A ``--`` token stops option recognition for the values being collected, which is how a
+   value beginning with ``-`` is passed. Note this differs from the POSIX ``--``: it does
+   not end the value list nor force the remainder to be positional arguments.
+
 We can also use the following chained way to add subcommand or option:
 
 .. code-block:: cpp

include/tscore/ArgParser.h

@@ -34,10 +34,23 @@
 constexpr unsigned MORE_THAN_ZERO_ARG_N = ~0;
 // more than one arguments
 constexpr unsigned MORE_THAN_ONE_ARG_N = ~0 - 1;
+// zero or one argument
+constexpr unsigned AT_MOST_ONE_ARG_N = ~0 - 2;
 // customizable indent for help message
 constexpr int INDENT_ONE = 32;
 constexpr int INDENT_TWO = 46;
 
+/** Whether @a arg_num asks for a variable rather than a fixed number of values.
+
+    Use this in preference to comparing against the sentinels, so that adding another
+    variable arity does not silently leave a sentinel being treated as a literal count.
+ */
+constexpr bool
+is_variable_arg_num(unsigned arg_num)
+{
+  return arg_num == MORE_THAN_ZERO_ARG_N || arg_num == MORE_THAN_ONE_ARG_N || arg_num == AT_MOST_ONE_ARG_N;
+}
+
 namespace ts
 {
 using AP_StrVec = std::vector<std::string>;
@@ -222,6 +235,10 @@ class ArgParser
     void version_message() const;
     // Helper method for parse()
     void append_option_data(Arguments &ret, AP_StrVec &args, int index);
+    // Helper method to collect the values of an option or command into @a ret
+    std::string handle_args(Arguments &ret, AP_StrVec &args, std::string const &name, unsigned arg_num, unsigned &index) const;
+    // Whether @a token names an option registered on this command
+    bool is_registered_option(std::string const &token) const;
     // Helper method to validate mutually exclusive groups
     void validate_mutex_groups(Arguments &ret) const;
     // Helper method to validate option dependencies

src/traffic_ctl/CtrlCommands.cc

@@ -555,6 +555,14 @@ ConfigCommand::config_reload()
     _printer->write_output("");
   }
 
+  // Without content the request would silently degrade to a full reload of every handler,
+  // which is the opposite of the scoped reload the operator asked for.
+  if (data_args && data_args.size() == 0) {
+    _printer->write_output("Error: --data (-d) requires content: @file, @- or a YAML string");
+    App_Exit_Status_Code = CTRL_EX_ERROR;
+    return;
+  }
+
   // Parse inline config data if provided (supports multiple -d arguments)
   YAML::Node configs;
   for (auto const &data_arg : data_args) {
@@ -587,17 +595,15 @@ ConfigCommand::config_reload()
 
   // Parse --directive (-D) arguments into configs[key]["_reload"][directive] = value
   auto dir_args = get_parsed_arguments()->get("directive");
+  if (dir_args && dir_args.size() == 0) {
+    _printer->write_output("Error: --directive (-D) requires at least one config_key.directive_key=value");
+    App_Exit_Status_Code = CTRL_EX_ERROR;
+    return;
+  }
   for (auto const &dir : dir_args) {
     if (dir.empty()) {
       continue;
     }
-    if (dir[0] == '-') {
-      _printer->write_output("Error: '" + dir +
-                             "' looks like a flag, not a directive. "
-                             "Place -D as the last option on the command line.");
-      App_Exit_Status_Code = CTRL_EX_ERROR;
-      return;
-    }
     std::string err;
     if (!parse_directive(dir, configs, err)) {
       _printer->write_output("Error: " + err);

src/traffic_ctl/traffic_ctl.cc

@@ -118,7 +118,7 @@ main([[maybe_unused]] int argc, const char **argv)
     .add_example_usage("traffic_ctl config get [OPTIONS] RECORD [RECORD ...]")
     .add_option("--cold", "-c",
                 "Save the value in a configuration file. This does not save the value in TS. Local file change only",
-                "TS_RECORD_YAML", MORE_THAN_ZERO_ARG_N)
+                "TS_RECORD_YAML", AT_MOST_ONE_ARG_N)
     .add_option("--records", "", "Emit output in YAML format")
     .add_option("--default", "", "Include default value");
   config_command.add_command("match", "Get configuration matching a regular expression", "", MORE_THAN_ONE_ARG_N, Command_Execute)
@@ -186,7 +186,7 @@ main([[maybe_unused]] int argc, const char **argv)
   config_command.add_command("set", "Set a configuration value", "", 2, Command_Execute)
     .add_option("--cold", "-c",
                 "Save the value in a configuration file. This does not save the value in TS. Local file change only",
-                "TS_RECORD_YAML", MORE_THAN_ZERO_ARG_N)
+                "TS_RECORD_YAML", AT_MOST_ONE_ARG_N)
     .add_option("--update", "-u", "Update a configuration value. [only relevant if --cold set]")
     .add_option(
       "--type", "-t",

src/tscore/ArgParser.cc

@@ -418,6 +418,8 @@ ArgParser::Command::output_option() const
       return {" [<arg> ...]"};
     } else if (num == MORE_THAN_ONE_ARG_N) {
       return {" <arg> ..."};
+    } else if (num == AT_MOST_ONE_ARG_N) {
+      return {" [<arg>]"};
     } else {
       return " <arg1> ... <arg" + std::to_string(num) + ">";
     }
@@ -518,34 +520,106 @@ ArgParser::Command::output_option() const
   }
 }
 
+bool
+ArgParser::Command::is_registered_option(std::string const &token) const
+{
+  if (_option_list.find(token) != _option_list.end() || _option_map.find(token) != _option_map.end()) {
+    return true;
+  }
+  // The --option=value form.
+  if (token.size() > 2 && token[0] == '-' && token[1] == '-') {
+    if (auto const pos = token.find_first_of('='); pos != std::string::npos) {
+      return _option_list.find(token.substr(0, pos)) != _option_list.end();
+    }
+  }
+  return false;
+}
+
 // helper method to handle the arguments and put them nicely in arguments
 // can be switched to ts::errata
-static std::string
-handle_args(Arguments &ret, AP_StrVec &args, std::string const &name, unsigned arg_num, unsigned &index)
+std::string
+ArgParser::Command::handle_args(Arguments &ret, AP_StrVec &args, std::string const &name, unsigned arg_num, unsigned &index) const
 {
   ArgumentData data;
   ret.append(name, data);
   // handle the args
-  if (arg_num == MORE_THAN_ZERO_ARG_N || arg_num == MORE_THAN_ONE_ARG_N) {
-    // infinite arguments
-    if (arg_num == MORE_THAN_ONE_ARG_N && args.size() <= index + 1) {
-      return "at least one argument expected by " + name;
+  if (arg_num == AT_MOST_ONE_ARG_N) {
+    // Zero or one value. A value is taken only when the following token does not name
+    // another option of this command, which leaves this command's positional arguments
+    // in place. A "--" token makes whatever follows it a value rather than an option.
+    unsigned j{index + 1};
+    bool     takes_value{false};
+
+    if (j < args.size()) {
+      if (args[j] == "--") {
+        ++j;
+        takes_value = j < args.size();
+      } else {
+        takes_value = !is_registered_option(args[j]);
+      }
+    }
+    if (takes_value) {
+      ret.append_arg(name, args[j]);
+      ++j;
     }
-    for (unsigned j = index + 1; j < args.size(); j++) {
+    args.erase(args.begin() + index, args.begin() + j);
+    index -= 1;
+    return "";
+  }
+  if (arg_num == MORE_THAN_ZERO_ARG_N || arg_num == MORE_THAN_ONE_ARG_N) {
+    // Variable number of arguments. Stop collecting at a token that names another option of this
+    // command, so options written afterwards keep their own values. Every other token is taken as
+    // a value, including a positional argument of the command, which is why an option whose value
+    // is optional wants AT_MOST_ONE_ARG_N rather than MORE_THAN_ZERO_ARG_N. A "--" token ends
+    // option recognition, which is how a value that starts with '-' can be passed.
+    unsigned j{index + 1};
+    unsigned collected{0};
+    bool     recognize_options{true};
+
+    for (; j < args.size(); j++) {
+      if (recognize_options) {
+        if (args[j] == "--") {
+          recognize_options = false;
+          continue;
+        }
+        if (is_registered_option(args[j])) {
+          break;
+        }
+      }
       ret.append_arg(name, args[j]);
+      ++collected;
+    }
+    if (arg_num == MORE_THAN_ONE_ARG_N && collected == 0) {
+      return "at least one argument expected by " + name;
     }
-    args.erase(args.begin() + index, args.end());
+    args.erase(args.begin() + index, args.begin() + j);
+    index -= 1;
     return "";
   }
-  // finite number of argument handling
-  for (unsigned j = 0; j < arg_num; j++) {
-    if (args.size() < index + j + 2 || args[index + j + 1].empty()) {
+  // Fixed number of arguments. A token naming another option of this command is not a value, so
+  // the missing value is reported rather than the following option being consumed as one. A "--"
+  // token ends option recognition, which is how a value that starts with '-' is passed.
+  unsigned j{index + 1};
+  bool     recognize_options{true};
+
+  for (unsigned collected{0}; collected < arg_num; ++j) {
+    if (j >= args.size() || args[j].empty()) {
       return std::to_string(arg_num) + " argument(s) expected by " + name;
     }
-    ret.append_arg(name, args[index + j + 1]);
+    if (recognize_options) {
+      if (args[j] == "--") {
+        recognize_options = false;
+        continue;
+      }
+      if (is_registered_option(args[j])) {
+        return std::to_string(arg_num) + " argument(s) expected by " + name;
+      }
+    }
+    ret.append_arg(name, args[j]);
+    ++collected;
   }
   // erase the used arguments and append the data to the return structure
-  args.erase(args.begin() + index, args.begin() + index + arg_num + 1);
+  args.erase(args.begin() + index, args.begin() + j);
   index -= 1;
   return "";
 }
@@ -658,7 +732,7 @@ ArgParser::Command::append_option_data(Arguments &ret, AP_StrVec &args, int inde
     if (args[i][0] == '-' && args[i][1] == '-' && args[i].find('=') != std::string::npos) {
       // deal with --args=
       std::string option_name = args[i].substr(0, args[i].find_first_of('='));
-      std::string value       = args[i].substr(args[i].find_last_of('=') + 1);
+      std::string value       = args[i].substr(args[i].find_first_of('=') + 1);
       if (value.empty()) {
         help_message("missing argument for '" + option_name + "'");
       }
@@ -721,7 +795,7 @@ ArgParser::Command::append_option_data(Arguments &ret, AP_StrVec &args, int inde
   // check for wrong number of arguments for --arg=...
   for (const auto &it : check_map) {
     unsigned num = _option_list.at(it.first).arg_num;
-    if (num != it.second && num < MORE_THAN_ONE_ARG_N) {
+    if (num != it.second && !is_variable_arg_num(num)) {
       help_message(std::to_string(_option_list.at(it.first).arg_num) + " arguments expected by " + it.first);
     }
   }