Re: [PATCH v3 3/4] parse-options: add early_scan_options()

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

On 10/12/26 13:14, Christian Couder wrote:
> diff --git a/parse-options.c b/parse-options.c
> index a98b958423..c03c396aa6 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -726,6 +726,8 @@ static void parse_options_check(const struct option *opts)
>   		     opts->long_name))
>   			optbug(opts, "uses feature "
>   			       "not supported for dashless options");
> +		if ((opts->flags & PARSE_OPT_EARLY) && !opts->long_name)
> +			optbug(opts, "uses PARSE_OPT_EARLY, which needs a long name");
>   		if (opts->type == OPTION_SET_INT && !opts->defval &&
>   		    opts->long_name && !(opts->flags & PARSE_OPT_NONEG))
>   			optbug(opts, "OPTION_SET_INT 0 should not be negatable");
> @@ -763,6 +765,8 @@ static void parse_options_check(const struct option *opts)
>   		case OPTION_SUBCOMMAND:
>   			if (!opts->value || !opts->subcommand_fn)
>   				optbug(opts, "OPTION_SUBCOMMAND needs a value and a subcommand function");
> +			if (opts->flags & PARSE_OPT_EARLY)
> +				optbug(opts, "OPTION_SUBCOMMAND does not support PARSE_OPT_EARLY");
>   			if (!subcommand_value)
>   				subcommand_value = opts->value;
>   			else if (subcommand_value != opts->value)

Would it make sense to also have these validations within 
early_scan_options() just in case it is called before 
parse_options_start() is invoked?
   > diff --git a/parse-options.h b/parse-options.h
> index 589f8d9008..f39b9a779a 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -51,6 +51,7 @@ enum parse_opt_option_flags {
>   	PARSE_OPT_NODASH = 1 << 5,
>   	PARSE_OPT_LITERAL_ARGHELP = 1 << 6,
>   	PARSE_OPT_FROM_ALIAS = 1 << 7,
> +	PARSE_OPT_EARLY = 1 << 8,	/* only for early_scan_options() */
>   	PARSE_OPT_NOCOMPLETE = 1 << 9,
>   	PARSE_OPT_COMP_ARG = 1 << 10,
>   	PARSE_OPT_CMDMODE = 1 << 11,
> @@ -502,6 +503,96 @@ static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name,
>   		BUG("option callback expects an argument"); \
>   } while(0)
>   
> +/*----- Early scan: scanning argv before the actual option parsing -----*/
> +
> +/*
> + * Some commands need to look at a few options before they can parse
> + * their command line for real, for example because the result decides
> + * whether a repository is needed at all.
> + *
> + * Such an early scan has to know which options take their value as a
> + * separate argument, or it could mistake such a value for an
> + * option. The functions below allow performing such early scans
> + * without being fooled by option values.
> + */
> +
> +/*
> + * Called by early_scan_options() for each argument matching a
> + * `struct option` with PARSE_OPT_EARLY set.
> + *
> + * - `option` is the matching option,

Would it be helpful to also convey here that the caller is not expected 
to retain the pointer as the memory pointing to it would be freed once 
the scan completes?

Rest of the patch looks fine to me.

-- 
Sivaraam