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

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> 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[])?

> +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.

> +{
> +	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 ...

> +		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.

> +		/* 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.

> +	}
> +	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.