From: Christian Couder Date: Mon, 12 Oct 2026 08:10:08 GMT Subject: Re: [PATCH v2 2/3] parse-options: add early_scan_options() Message-ID: In-Reply-To: <84f9d1c9-30b7-4d8b-82d6-9afd16d0ab08@gmail.com> On Wed, Sep 30, 2026 at 9:13 AM Kaartic Sivaraam wrote: > > 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. Right, thanks for pointing this out. In the v3 I have just sent, abbreviations are handled properly, using the same code that parse_options() uses. > 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. This is another improvement in v3. We err on the safe side and just stop the early scan as soon as something is not recognized. Thanks for this suggestion too. > > 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? You were right to ask because it was actually worth worrying about a bit. The early scan reported an option that parse_options() would reject. But now in v3, the scan stops at such an option instead of reporting it, except when an option with PARSE_OPT_LASTARG_DEFAULT is the last argument. In that special case, parse_options() uses the option's default value, so the scan similarly still reports the option, with a NULL value meaning "the default". Both cases are documented in "parse-options.h" and tested. > > + 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? Right. The index returned in that case was the index of the last argument consumed, while in all the other cases it was the index of the first argument not consumed (the "--", the non-option, etc), which was not consistent. In v3, early_scan_options() always returns the index of the first argument not consumed by the scan. So if it found an option it was looking for, this is the index of the next argument after the option and its value if any. A "--stop-at-found" option has been added to the test helper to test this. Thanks. > > + * 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-") 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? No. It was a leftover from v1. The `--number --wanted` test was still interesting to check that the value of an OPTION_INTEGER option is skipped, so it was moved into the "skips the value of other options" test, and this one was removed. > > +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 Yes, thanks. I added it to the "does not skip an optional value" test.