On Sat, Mar 25, 2017 at 12:10 AM, Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> 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.