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

From: Stephen Boyd <bebarino@gmail.com>
Date: Oct 25, 2010, 10:31 UTC
Message-ID: <4CC55C67.8080908@gmail.com>
In-Reply-To: <20101024081507.GB29630@burratino>

On 10/24/10 01:15, Jonathan Nieder wrote:

Show 21 quoted lines

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

Show 11 quoted lines

> 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

Show 10 quoted lines

>  
> @@ -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: