Re: [PATCH/RFC] parse-options: add facility to make options configurable

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

 On Sat, Mar 25, 2017 at 12:10 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> [...]
> This is all very proof-of-concept, and uses the ugly hack of s/const
> // for the options struct because I'm now keeping state in it, as
> noted in one of the TODO comments that should be moved.
> [...]
>  static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
> -                          const struct option *options)
> +                          struct option *options)
>  {
>         const struct option *all_opts = options;
> [...]
> @@ -313,7 +314,17 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
>                                 continue;
>                         p->opt = rest + 1;
>                 }
> -               return get_value(p, options, all_opts, flags ^ opt_flags);
> +               if (!(ret = get_value(p, options, all_opts, flags ^ opt_flags))) {
> +                       /* TODO: Keep some different state on the side
> +                        * with info about what options we've
> +                        * retrieved via the CLI for use in the loop
> +                        * in parse_options_step, instead of making
> +                        * the 'options' non-const
> +                        */
> +                       if (options->flags & PARSE_OPT_CONFIGURABLE)
> +                               options->flags |= PARSE_OPT_VIA_CLI;
> +               }
> +               return ret;
>         }
> [...]
> +
> +       /* The loop above is driven by the argument vector, so we need
> +        * to make a second pass and find those options that are
> +        * configurable, and haven't been set via the command-line */
> +       for (; options->type != OPTION_END; options++) {
> +               if (!(options->flags & PARSE_OPT_CONFIGURABLE))
> +                       continue;
> +
> +               if (options->flags & PARSE_OPT_VIA_CLI)
> +                       continue;
> +
> +               /* TODO: Maybe factor the handling of OPTION_CALLBACK
> +                * in get_value() into a function.
> +                *
> +                * Do we also need to save away the state from the
> +                * loop above to handle unset? I think not, I think
> +                * we're always unset here by definition, right?
> +                */
> +               return (*options->conf_callback)(options, NULL, 1) ? (-1) : 0;
> +       }
> +
> [...]

After looking at some of the internal APIs I'm thinking of replacing
this pattern with a hashmap.c hashmap where the keys are a
sprintf("%d:%s", short_name, long_name) to uniquely identify the
option. There's no other obvious way to uniquely address an option. I
guess I could just equivalently and more cheaply use the memory
address of each option to identify them, but that seems a bit hacky.

> @@ -110,6 +112,9 @@ struct option {
>         int flags;
>         parse_opt_cb *callback;
>         intptr_t defval;
> +
> +       const char *conf_key;
> +       parse_opt_cb *conf_callback;
>  };

I've already found that this needs to be a char **conf_key, since
several command-line options have multiple ways to spell the option
name, e.g. add.ignoreerrors & add.ignore-errors, pack.writebitmaps &
repack.writebitmaps etc.