brbzull0 commented on code in PR #13861: URL: https://github.com/apache/trafficserver/pull/13861#discussion_r4334851823
########## include/tscore/ArgParser.h: ########## @@ -364,6 +364,14 @@ class ArgParser // Enable test mode - makes do_exit() throw instead of calling exit() for unit testing static void set_test_mode(bool test = true); + /** Clear the parser-wide state held at file scope in ArgParser.cc. + + Test-only. A program builds one parser and never reuses the state, but a unit test binary + runs many parsers in sequence, and set_default_command() or Command::set_default() would + otherwise leak default_command into every later test. + */ + static void reset_global_state(); Review Comment: No change, as in https://github.com/apache/trafficserver/pull/13861#discussion_r4314314492: `reset_global_state()` is declared under the `protected:` label of `ArgParser`, and `tscore` headers are not installed; the tests reach it through the derived `Access` shim. ########## src/traffic_ctl/traffic_ctl.cc: ########## @@ -173,8 +173,9 @@ main([[maybe_unused]] int argc, const char **argv) .add_option("--data", "-d", "Inline config data (@file, @- for stdin, or yaml string)", "", MORE_THAN_ZERO_ARG_N, "") .add_option("--directive", "-D", "Pass a reload directive to a config handler (format: config_key.directive_key=value)", "", MORE_THAN_ZERO_ARG_N, "") + // No short form: -w is the global --watch. .add_option( - "--initial-wait", "-w", + "--initial-wait", "", "Initial wait before first poll, giving the server time to schedule all handlers (seconds). Accepts fractional values", "", 1, "2") Review Comment: No change, as in https://github.com/apache/trafficserver/pull/13861#discussion_r4314315542: `add_option` takes the short form as `std::string const &`, an empty string is never added to the short-option map, and `""` is how the other long-only options in this file are declared. ########## doc/appendices/command-line/traffic_ctl.en.rst: ########## @@ -536,10 +536,14 @@ Display the current value of a configuration record. $ 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. Option - recognition then stays off for the rest of the line, so every remaining token becomes - a directive value and any option written afterwards is swallowed. Use - ``--directive=-value`` instead when options still have to follow: + To pass a directive value that begins with ``-``, place ``--`` before it. Recognition of + ``config reload``'s own options then stays off for the rest of the line, so every + remaining token becomes a directive value and any of those options written afterwards + is swallowed. The global options (``--format``, ``--help``, ``--run-root``, ...) are Review Comment: Reworded to "any such option written afterwards is swallowed", in 38c6f4ed9c. ########## src/tscore/ArgParser.cc: ########## @@ -799,6 +816,14 @@ ArgParser::Command::append_option_data(Arguments &ret, AP_StrVec &args, int inde std::map<std::string, unsigned> check_map; for (unsigned i = index; i < args.size(); i++) { + // A "--" escapes the token after it. That token belongs to an option or a command further + // down the line, which honours the escape itself in handle_args(); this scan covers the + // whole remaining command line, so without this it would still match the escaped token + // against this command's options. Leave both tokens in place for the owner. + if (args[i] == "--") { + ++i; + continue; + } Review Comment: Kept the `++i; continue;`, which matches the escape handling in `handle_args()`, and the comment now says that this `++i` steps past the `--` and the loop increment past the escaped token, in 38c6f4ed9c. ########## tests/uranium_tests/records/test_traffic_ctl_cold_config.py: ########## @@ -176,3 +176,28 @@ def test_traffic_ctl_cold_config(ats_factory: ATSFactory) -> None: _ats.environment["TS_RECORD_YAML"] = str(destination) traffic_ctl("config", "set", "proxy.config.cache.limits.http.max_alts", "3", "-c", _ats=_ats) assert yaml.safe_load(destination.read_text()) == {"records": {"cache": {"limits": {"http": {"max_alts": 3}}}}} + + # A "--" is the way to pass a value that starts with '-'. The option scan of every enclosing + # command covers the whole rest of the command line, so the escape has to be honoured there + # too, or a value spelled like a global option (-f, -h) or like an option of config set itself + # (-t) is taken as that option instead of reaching the record. -h is the worse case: it prints Review Comment: Fixed to "worst case", in 38c6f4ed9c. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
