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

[Kaartic Sivaraam <kaartic.sivaraam@gmail.com>]

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

-- 
Sivaraam