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

On Sat, Mar 25, 2017 at 05:47:49PM +0100, Ævar Arnfjörð Bjarmason wrote:

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

Rather than bolt this onto the parse-options code, what if it were a
separate mechanism that mapped config keys to options. E.g., imagine
something like:

  struct {
	const char *config;
	const char *option;
	enum {
		CONFIG_CMDLINE_BOOL,
		CONFIG_CMDLINE_MAYBE_BOOL,
		CONFIG_CMDLINE_VALUE
	} type;
  } config_cmdline_map[] = {
	{ "foo.one", "one", CONFIG_CMDLINE_BOOL },
	{ "foo.two", "two", CONFIG_CMDLINE_VALUE },
  };

And then you "apply" that mapping by finding each item that's set in the
config, and then pretending that "--one" or "--two=<value>" was set on
the command-line, before anything the user has said. This works as long
as the options use the normal last-one-wins rules, the user can
countermand with "--no-one", etc.

You have to write one line for each config/option mapping, but I think
we would need some amount of per-option anyway (i.e., I think the
decision was that options would have to be manually approved for use in
the system). So rather than a flag in the options struct, it becomes a
line in this mapping.

And you get two extra pieces of flexibility:

  1. The config names can map to option names however makes sense; we're
     not constrained by some programmatic rule (I think we _would_
     follow some general guidelines, but there are probably special
     cases for historic config, etc).

  2. A command can choose to apply one or more mappings, or not. So
     porcelain like git-log would call:

       struct option options[] = {...};
       apply_config_cmdline_map(revision_config_mapping, options);
       apply_config_cmdline_map(diff_config_mapping, options);
       apply_config_cmdline_map(log_mapping, options);

     but plumbing like git-diff-tree wouldn't call any of those.

I had in mind that apply_config_cmdline_map() would just call
parse_options, but I think even that is too constricting. The revision
and diff options don't use parse_options at all. So really, it would
probably be more like:

  struct argv_array fake_args = ARGV_ARRAY_INIT;
  apply_config_cmdline_map(revision_config_mapping, &fake_args);
  apply_config_cmdline_map(diff_config_mapping, &fake_args);
  apply_config_cmdline_map(log_mapping, &fake_args);
  argv_array_pushv(&fake_args, argv); /* add the real ones */

At this point we've recreated internally the related suggestion:

  [options]
  log = --one --two=whatever

which is the same as:

  [log]
  one = true
  two = whatever

So hopefully it's clear that the two are functionally equivalent, and
differ only in syntax (in this case we manually decided which options
are safe to pull from the config, but we'd have to parse the options.log
string, too, and we could make the same decision there).

-Peff

  reply index

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
2017-03-25 21:31           ` Jeff King [this message]
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 publically 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=20170325213107.u2l5eunqgqbxpcbb@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=Matthieu.Moy@grenoble-inp.fr \
    --cc=avarab@gmail.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --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

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox