Re: [PATCH 1/6] parse-options: add early_scan_options()

On Thu, Sep 3, 2026 at 12:11 AM Junio C Hamano [off-list ref] wrote:

Christian Couder [off-list ref] writes:

quoted

So users must spell these specific options in full. This restriction
could be lifted in the future though, once the scanner is adapted to
accept a command's full option array, as this would give it the
complete context needed for safe abbreviation matching.

It is unfortunate that end-users cannot tell if they are dealing
with a system before of after "once the scanner is adapted"
happened, so they must be trained to always spell the options in
full to make use of the commands that use this feature.  It at least
does not regress relative to the ad-hoc early scanners these selected
commands have that do not even understand what they are parsing, so
it may not be too bad.

Stepping back a bit, the burden on programmers to use this would be
to write in a separate notation what options there are in addition
to what they feed the real parse_options(), which cuts both ways in
the sense that because this does not take parse_options(), commands
that do not use parse_options() can still use it, but those that do
already use parse_options() need additional work to use eary_scan.

And then once the scanner is adapted to accept the full option array,
the programmers only need to discard the struct early_scan_option[]
they wrote and replace it with the struct option[] they already have?
Or would the calling convention to the scanner also change when it
happens (oother than replacing the pointer to struct early_scan_option[]
with another pointer to struct option[])?

I agree that what was implemented in v1 (to be able to accommodate
early scans that do not use parse_options()) didn't bring much
practical value, was a bit complex and required some churn when the
early scan would have been converted to use parse_options(). So, in
the v2 I just sent, it addresses only the early scan where
parse_options() is used, which simplifies a lot of things.

quoted

+static const struct early_scan_option *
+find_early_scan_option(const char *arg,
+                    const struct early_scan_option *options,
+                    const char **value)

Because you return one single element from the incoming array of
options, it is mildly misleading to call the variable/parameter
"options" here and everywhere else.  Let's stick to "arrays are
named singular, so that option[4] names 4th option" convention.

Right, I have changed the argument to `const struct option *option`.

quoted

+{
+     if (!skip_prefix(arg, "--", &arg))
+             return NULL;
+
+     for (; options->name; options++) {
+             const char *rest;
+
+             if (!skip_prefix(arg, options->name, &rest))
+                     continue;

"--option" on the command line, after getting stripped the leading
"--", may begin with "option", and that name may be in the option[]
table, in which case ...

quoted

+             if (!*rest) {
+                     *value = NULL;
+                     return options;
+             }

... we found a hit.  But shouldn't option->takes_value be consulted
before we return to signal the caller that the next arg is an option
value before we return from here?  It looks a bit uneven as we do
that for stuck form "--option=value" here.

Yeah, we found that `arg` exactly matches this option whether or not
it takes a value, but the value is not here.

Whether the next argument has to be skipped is decided by the caller:

  if (parse_options_takes_argument(opt) && !value && i + 1 < argc)
      value = argv[++i];

find_early_scan_option() cannot do that itself, as it has neither
argv, argc nor the current index.

So signalling to the caller would be redundant, because the caller
already holds the matched option and can ask directly.

But maybe I should add a comment on the line before `if (!*rest) {`
saying that skipping a separate value is the caller's job?

quoted

+             /* Only an option taking a value can be stuck to one. */
+             if (*rest == '=' && options->takes_value) {
+                     *value = rest + 1;
+                     return options;
+             }

And if the option[] table had "opt", then "--option" on the command
line may begin with "--opt" but "ion" is an excess that is not a
stuck value, so we do not consider it as a match.  OK.

Now using `takes_value` in the `*rest == '='` case wasn't quite right,
as parse_options_takes_argument() returns 0 for PARSE_OPT_OPTARG and
PARSE_OPT_LASTARG_DEFAULT, but parse_options() does accept a stuck
value for both.

So in v2 we use the same condition parse_options() uses:

  /* Only an option that can take a value may have one stuck to it. */
  if (*rest == '=' && !(opt->flags & PARSE_OPT_NOARG)) {
      *value = rest + 1;
      return opt;
  }

quoted

+     }
+     return NULL;
+}

If we are to write a separate function anyway, I wonder how much
more work to write a early_scan_option() parser that does take a
real "struct option[]" array.  Its elements already know if they
take a value or not.  For expediency, it may be OK to start by
simplified parser that does not handle unique prefix and other
complexities like callback functions of the real parser, but at
least it would reduce the burden on the programmers quite a bit if
we used the real struct option[] array, I suspect.

This is what v2 does, and I agree that it simplifies things.