Copilot commented on code in PR #13861:
URL: https://github.com/apache/trafficserver/pull/13861#discussion_r4334798972
##########
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:
Grammar issue: 'any of those options ... is swallowed' should be plural
('are swallowed') or rephrased ('will be swallowed'), since the subject is
'options'.
##########
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:
This introduces a new public API entry point that is explicitly 'Test-only'.
To keep the production API surface clean, consider making this `protected`
(your unit tests already use a derived `Access` shim) and/or compiling it under
a test-only build define so it isn't available to non-test consumers.
##########
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:
Passing an empty string as the short option can be interpreted differently
than 'no short option' (e.g., some implementations test for nullptr, or assume
a non-empty string starts with '-'). Prefer using a null pointer (if the
overload supports it) or a dedicated long-only `add_option` overload to avoid
malformed help text or unexpected parsing behavior.
##########
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:
Incrementing `i` inside a `for` loop that also increments `i` makes the
control flow harder to reason about (and results in `i` advancing by 2 total).
Consider rewriting this to advance explicitly (e.g., `i += 2` with a bounds
check or using a `while` loop) so it's clearer that you're skipping exactly
`--` and the next token and not relying on the loop increment side-effect.
##########
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:
Correct 'worse case' to 'worst case' in the comment.
--
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]
