git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Git Mailing List <git@vger.kernel.org>
Cc: "Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
	"brian m . carlson" <sandals@crustytoothpaste.net>,
	"Matthieu Moy" <Matthieu.Moy@grenoble-inp.fr>,
	"Duy Nguyen" <pclouds@gmail.com>,
	"Brandon Williams" <bmwill@google.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH/RFC] parse-options: add facility to make options configurable
Date: Sat, 25 Mar 2017 17:47:49 +0100	[thread overview]
Message-ID: <CACBZZX6iz5QpfpOO6s9c-GY7+ZZ2uXBxqgKfSRhU+__P0VLC5g@mail.gmail.com> (raw)
In-Reply-To: <20170324231013.23346-1-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.

  reply	other threads:[~2017-03-25 16:48 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-19  9:57 Add configuration options for some commonly used command-line options (Was: [RFH] GSoC 2015 application) Duy Nguyen
2017-03-19 10:15 ` Add configuration options for some commonly used command-line options Matthieu Moy
2017-03-19 13:18   ` brian m. carlson
2017-03-19 13:43     ` Ævar Arnfjörð Bjarmason
2017-03-20 10:56       ` Duy Nguyen
2017-03-20 17:32         ` Brandon Williams
2017-03-20 18:18           ` Jeff King
2017-03-31 19:44             ` Brandon McCaig
2017-03-20 18:56           ` Junio C Hamano
2017-03-20 19:14             ` Jeff King
2017-03-20 21:57             ` Ævar Arnfjörð Bjarmason
2017-03-24 23:10       ` [PATCH/RFC] parse-options: add facility to make options configurable Ævar Arnfjörð Bjarmason
2017-03-25 16:47         ` Ævar Arnfjörð Bjarmason [this message]
2017-03-25 21:31           ` Jeff King
2017-03-25 22:32             ` Ævar Arnfjörð Bjarmason
2017-03-28  5:17               ` Jeff King
2017-03-28 13:13                 ` [PATCH/RFC v2] WIP configurable options facility Ævar Arnfjörð Bjarmason
2017-03-25 21:28         ` [PATCH/RFC] parse-options: add facility to make options configurable brian m. carlson
2017-03-20 10:42     ` Add configuration options for some commonly used command-line options Duy Nguyen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CACBZZX6iz5QpfpOO6s9c-GY7+ZZ2uXBxqgKfSRhU+__P0VLC5g@mail.gmail.com \
    --to=avarab@gmail.com \
    --cc=Matthieu.Moy@grenoble-inp.fr \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    --cc=sandals@crustytoothpaste.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).