Re: [PATCH 1/4] parse-options: allow git commands to invent new option types

[Stephen Boyd <bebarino@gmail.com>]

On 10/24/10 01:15, Jonathan Nieder wrote:
> diff --git a/parse-options.c b/parse-options.c
> index 0fa79bc..7907306 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -8,9 +8,6 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,
>  			       const char * const *usagestr,
>  			       const struct option *opts, int err);
>  
> -#define OPT_SHORT 1
> -#define OPT_UNSET 2
> -
>  static int opterror(const struct option *opt, const char *reason, int flags)
>  {
>  	if (flags & OPT_SHORT)
> @@ -51,6 +48,9 @@ static int get_value(struct parse_opt_ctx_t *p,
>  	const int unset = flags & OPT_UNSET;
>  	int err;
>  
> +	if (opt->type == OPTION_LOWLEVEL_CALLBACK)
> +		return (*(parse_opt_ll_cb *)opt->callback)(p, opt, flags);
> +

(I read patch 4 before this one)

Being able to modify the context within a callback is nice. Having to
know if the option is short or long and and checking for validity seems
like something that should be handled within the parse options library
itself.

Is there an actual use case where someone needs to completely override
get_value()? If you move this into the case statement then we get the
generic error checking of get_value() with the benefits of being able to
modify the context within a callback. We could also probably use the
return value of the low level callback to indicate whether or not to
take some action after parsing the option. Perhaps something like
quiting the option parsing loop when encountering such an option?

This reminds me, we can probably simplify that "takes no value" error
path in get_value() (see below).

> diff --git a/parse-options.h b/parse-options.h
> index d982f0f..fa400da 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -17,6 +17,7 @@ enum parse_opt_type {
>  	OPTION_STRING,
>  	OPTION_INTEGER,
>  	OPTION_CALLBACK,
> +	OPTION_LOWLEVEL_CALLBACK,
>  	OPTION_FILENAME
>  };

Don't forget to document what this option does in the comments of this
file and in api-parse-options.txt

>  
> @@ -40,8 +41,16 @@ enum parse_opt_option_flags {
>  	PARSE_OPT_SHELL_EVAL = 256
>  };
>  
> +enum parse_opt_ll_flags {
> +	OPT_SHORT = 1,
> +	OPT_UNSET = 2
> +};
> +

I hope this isn't necessary.

Hope this pasted ok.
--->8----

diff --git a/parse-options.c b/parse-options.c
index 0fa79bc..7234c11 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -56,22 +56,8 @@ static int get_value(struct parse_opt_ctx_t *p,
        if (unset && (opt->flags & PARSE_OPT_NONEG))
                return opterror(opt, "isn't available", flags);

-       if (!(flags & OPT_SHORT) && p->opt) {
-               switch (opt->type) {
-               case OPTION_CALLBACK:
-                       if (!(opt->flags & PARSE_OPT_NOARG))
-                               break;
-                       /* FALLTHROUGH */
-               case OPTION_BOOLEAN:
-               case OPTION_BIT:
-               case OPTION_NEGBIT:
-               case OPTION_SET_INT:
-               case OPTION_SET_PTR:
+       if (!(flags & OPT_SHORT) && p->opt && (opt->flags &
PARSE_OPT_NOARG))
                        return opterror(opt, "takes no value", flags);
-               default:
-                       break;
-               }
-       }

        switch (opt->type) {
        case OPTION_BIT: