Re: [PATCH v2 2/3] parse-options: add early_scan_options()

>
> On 9/23/26 13:39, Christian Couder wrote:
> >
>  > [ snip ]
>  >
> > One consequence of staying simple is that abbreviated options are
> > still not matched, even though the scan is now given the command's
> > full option array. Resolving them the way parse_options() does would
> > mean duplicating the ambiguity detection that parse_long_opt()
> > performs. So the scan can fail to see an option that parse_options()
> > would accept, and its callers have to cope with that, typically by
> > erring on the safe side. This and the other differences with
> > parse_options() are documented in "parse-options.h".
> >
>
> I think not handling abbreviations could also have another potential
> problem. Consider a command as follows:
>
>   $ git fast-import --quiet --export-pack --allow-unsafe-features
>
> Here `--export-pack` is an abbreviation of `--export-pack-edges`. So,
> the arg next to it should ideally be considered as a value for it but
> given the correct "ignore" logic, we will happily interpret is an
> argument which misaligns with parse_options()'s behaviour.

> In the ideal world, we could say such weird names for files is unlikely
> and this isn't such a big concern. But given it is the scope of this
> series to make early scan more reliable, I think we should consider how
> to handle this better.
>
> Would it make sense to actually err on the safe side and just stop
> walking the args as soon as we notice an unrecognized argument? This
> will the ensure the walk never misinterpret a value for an argument.

> > diff --git a/parse-options.c b/parse-options.c
> > index a132c1ea12..559dad9061 100644
> > --- a/parse-options.c
> > +++ b/parse-options.c
> >
>  > [ snip ]
>  >
> > +int early_scan_options(int argc, const char **argv,
> > +                    const struct option *option,
> > +                    enum early_scan_flags flags,
> > +                    early_scan_fn *fn, void *data)
> > +{
> > +     for (int i = 0; i < argc; i++) {
> > +             const char *arg = argv[i];
> > +             const char *value;
> > +             const struct option *opt;
> > +             int pos = i;
> > +
> > +             /*
> > +              * parse_options() always stops parsing options at these,
> > +              * whatever its flags, so nothing after them is an option.
> > +              */
> > +             if (!strcmp(arg, "--") || !strcmp(arg, "--end-of-options"))
> > +                     return i;
> > +
> > +             opt = find_early_scan_option(arg, option, &value);
> > +             if (!opt) {
> > +                     if ((flags & EARLY_SCAN_STOP_AT_NON_OPTION) &&
> > +                         (*arg != '-' || !arg[1]))
> > +                             return i;
> > +                     continue;
> > +             }
> > +
> > +             /*
> > +              * When an option takes a value, but that value is not
> > +              * stuck to it with '=', then the next argument is the
> > +              * value and it has to be skipped so that it isn't
> > +              * taken for an option itself.
> > +              */
> > +             if (parse_options_takes_argument(opt) && !value && i + 1 < argc)
> > +                     value = argv[++i];
> > +
>
> The 'i +1 < argc' part is an appropriate guard to have. But this means a
> command such as the following:
>
>    test-tool early-scan-options --wanted-value
>
> ... would reult in 'value' being NULL. I suppose this is kind of
> expected for the early scan code and is not something we need to worry
> about?

> > +             if (opt->flags & PARSE_OPT_EARLY && fn(opt, value, pos, data))
> > +                     return i;
> > +     }
> > +
> > +     return argc;
> > +}
> > +
> >   static int usage_argh(const struct option *opts, FILE *outfile)
> >   {
> >       const char *s;
> > diff --git a/parse-options.h b/parse-options.h
> > index f29e73f85c..3ef64744a4 100644
> > --- a/parse-options.h
> > +++ b/parse-options.h
>  >
> > [ snip ]
> >
> > +/*
> > + * Scan `argv` for the options described by `option`, calling `fn` for
> > + * each of those that have PARSE_OPT_EARLY set. `argv` is not
> > + * modified.
> > + *
> > + * `fn` may be NULL when no option has PARSE_OPT_EARLY set, which is
> > + * useful to only find out where the scan stops.
> > + *
> > + * The scan always stops at "--" and at "--end-of-options", as
> > + * parse_options() always stops parsing options there too, whatever its
> > + * flags. PARSE_OPT_KEEP_DASHDASH and PARSE_OPT_KEEP_UNKNOWN_OPT only
> > + * decide if the terminator is left in argv, not if it terminates.
> > + *
> > + * Returns the index at which the scan stopped, which is `argc` when the
> > + * whole array was scanned.
> > + *
>
> As for the return index, when the callback stops the scan the index
> returned is that of the option's value rather than the option itself.
> Would it be better to capture this more clearly?
>
> Also, would it be helpful to also have a test for this?

> > + * This scan is for now deliberately much simpler than
> > + * parse_options(), so it differs from it in the following ways:
> > + *
> > + *  - Only the long form of an option is matched, and it has to be
> > + *    spelled in full: short options and abbreviations are ignored.
> > + *
> > + *  - Negated forms ("--no-<name>") are not matched. This is harmless,
> > + *    as they never take a value to skip.
> > + *
> > + *  - Options with PARSE_OPT_OPTARG or PARSE_OPT_LASTARG_DEFAULT are
> > + *    treated as not taking a separate value.
> > + *
> > + *  - OPTION_SUBCOMMAND entries are skipped.
> > + *
> > + *  - OPTION_ALIAS entries are not resolved to the option they stand
> > + *    for.
> > + *
> > + * So the scan can fail to see an option that parse_options() would
> > + * accept, and callers have to cope with that, typically by erring on
> > + * the safe side.
> > + */
> > +int early_scan_options(int argc, const char **argv,
> > +                    const struct option *option,
> > +                    enum early_scan_flags flags,
> > +                    early_scan_fn *fn, void *data);
>  >
> > [ snip ]>
> > diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> > index 449fff4d34..b796d96b9a 100755
> >
>  > [ snip ]> +
> > +test_expect_success 'early_scan_options() takes values from struct option' '
> > +     test-tool early-scan-options --number --wanted >actual &&
> > +     cat >expect <<-\EOF &&
> > +     stopped at: 2 of 2
> > +     EOF
> > +     test_cmp expect actual &&
> > +     test-tool early-scan-options --number=5 --wanted >actual &&
> > +     cat >expect <<-\EOF &&
> > +     found: wanted at 1
> > +     stopped at: 2 of 2
> > +     EOF
> > +     test_cmp expect actual
> > +'
>
> Compared to others, I'm not quite sure this test is testing something
> special. Do we need it?

> > +test_expect_success 'early_scan_options() does not skip an optional value' '
> > +     test-tool early-scan-options --optarg --wanted >actual &&
> > +     cat >expect <<-\EOF &&
> > +     found: wanted at 1
> > +     stopped at: 2 of 2
> > +     EOF
> > +     test_cmp expect actual &&
> > +     test-tool early-scan-options --lastarg --wanted >actual &&
> > +     cat >expect <<-\EOF &&
> > +     found: wanted at 1
> > +     stopped at: 2 of 2
> > +     EOF
> > +     test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'early_scan_options() matches a stuck optional value' '
> > +     test-tool early-scan-options --early-optarg=one >actual &&
> > +     cat >expect <<-\EOF &&
> > +     found: early-optarg at 0 value: one
> > +     stopped at: 1 of 1
> > +     EOF
> > +     test_cmp expect actual &&
> > +     test-tool early-scan-options --early-lastarg=two >actual &&
> > +     cat >expect <<-\EOF &&
> > +     found: early-lastarg at 0 value: two
> > +     stopped at: 1 of 1
> > +     EOF
> > +     test_cmp expect actual
> > +'
>
> Would the following be a useful part to also add to the above?
>
>           test-tool early-scan-options --optarg=5 --wanted >actual &&
>           cat >expect <<-\EOF &&
>           found: wanted at 1
>           stopped at: 2 of 2
>           EOF
>           test_cmp expect actual