Add configuration options for some commonly used command-line options (Was: [RFH] GSoC 2015 application)

Add configuration options for some commonly used command-line options (Was: [RFH] GSoC 2015 application)

[Duy Nguyen]

On Thu, Feb 19, 2015 at 5:32 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> +### Add configuration options for some commonly used command-line options
> +
> +This includes:
> +
> +* git am -3
> +
> +* git am -c
> +
> +Some people always run the command with these options, and would
> +prefer to be able to activate them by default in ~/.gitconfig.

I was reading the .md file to add a new microproject and found this.
Instead of adding new config case by case, should we do something more
generic?

We could have a new group defaultOptions.<command> (or
<command>.options) which contains <option-name> = <value> where option
names are the long name in parse-options? Then we don't have to
manually add more config options any more (mostly, I'm aware of stuff
like diff options that do not use parse-options).

If we want to stop the users from shooting themselves in the foot, we
could extend parse-options to allow/disallow certain options being
used this way. Hmm?
-- 
Duy

Re: Add configuration options for some commonly used command-line options

[Matthieu Moy]

Duy Nguyen <pclouds@gmail.com> writes:

> On Thu, Feb 19, 2015 at 5:32 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> +### Add configuration options for some commonly used command-line options
>> +
>> +This includes:
>> +
>> +* git am -3
>> +
>> +* git am -c
>> +
>> +Some people always run the command with these options, and would
>> +prefer to be able to activate them by default in ~/.gitconfig.
>
> I was reading the .md file to add a new microproject and found this.
> Instead of adding new config case by case, should we do something more
> generic?
>
> We could have a new group defaultOptions.<command> (or
> <command>.options) which contains <option-name> = <value> where option
> names are the long name in parse-options? Then we don't have to
> manually add more config options any more (mostly, I'm aware of stuff
> like diff options that do not use parse-options).
>
> If we want to stop the users from shooting themselves in the foot, we
> could extend parse-options to allow/disallow certain options being
> used this way. Hmm?

I think the main problem is indeed "stop the users from shooting
themselves in the foot". Many command-line options change the behavior
completely so allowing users to enable them by default means allowing
the users to change Git in such a way that scripts calling it are
broken.

This also doesn't help when troublshouting an issue as these options are
typically something set once and for all and which you forget about.
This typically leads to discussion in Q&A forums like:

A: Can you run "git foo"?
B: Here's the result: ...
A: I don't understand, I can't reproduce here.

just because B has a CLI option enabled by default.

This is the same reasoning that leads Git to forbid aliasing an existing
command to something else.

OTOH, we already have almost "enable such or such option by default"
with aliases. People who always run "git am" with "-3" can write

[alias]
        a3 = am -3

and just run "git a3".

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: Add configuration options for some commonly used command-line options

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1723 bytes --]

On Sun, Mar 19, 2017 at 11:15:33AM +0100, Matthieu Moy wrote:
> I think the main problem is indeed "stop the users from shooting
> themselves in the foot". Many command-line options change the behavior
> completely so allowing users to enable them by default means allowing
> the users to change Git in such a way that scripts calling it are
> broken.
> 
> This also doesn't help when troublshouting an issue as these options are
> typically something set once and for all and which you forget about.
> This typically leads to discussion in Q&A forums like:
> 
> A: Can you run "git foo"?
> B: Here's the result: ...
> A: I don't understand, I can't reproduce here.
> 
> just because B has a CLI option enabled by default.
> 
> This is the same reasoning that leads Git to forbid aliasing an existing
> command to something else.
> 
> OTOH, we already have almost "enable such or such option by default"
> with aliases. People who always run "git am" with "-3" can write
> 
> [alias]
>         a3 = am -3
> 
> and just run "git a3".

I tend to agree here.  At work, we have code that wants git status
--porcelain to be empty.  If a user added -b to all of their git status
calls (to make -s output more helpful), that would break a lot of
tooling.  It's much better if they create an alias, since that doesn't
affect automated tools.

I expect developers of things such as fugitive would dislike such a
feature as well.  I get the impression our existing config file options
already make life difficult enough.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: Add configuration options for some commonly used command-line options

[Ævar Arnfjörð Bjarmason]

On Sun, Mar 19, 2017 at 2:18 PM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> On Sun, Mar 19, 2017 at 11:15:33AM +0100, Matthieu Moy wrote:
>> I think the main problem is indeed "stop the users from shooting
>> themselves in the foot". Many command-line options change the behavior
>> completely so allowing users to enable them by default means allowing
>> the users to change Git in such a way that scripts calling it are
>> broken.
>>
>> This also doesn't help when troublshouting an issue as these options are
>> typically something set once and for all and which you forget about.
>> This typically leads to discussion in Q&A forums like:
>>
>> A: Can you run "git foo"?
>> B: Here's the result: ...
>> A: I don't understand, I can't reproduce here.
>>
>> just because B has a CLI option enabled by default.
>>
>> This is the same reasoning that leads Git to forbid aliasing an existing
>> command to something else.
>>
>> OTOH, we already have almost "enable such or such option by default"
>> with aliases. People who always run "git am" with "-3" can write
>>
>> [alias]
>>         a3 = am -3
>>
>> and just run "git a3".

I can't find the E-Mail chain now but this has been discussed on-list
a while ago. I.e. having some getopt support to say for the push
command, that the --rebase option can also come from the config, i.e.
in this case the pull.rebase option.

IIRC the consensus was that such a facility would allow commands or
individual options to say "this command/option is configurable", thus
of course all plumbing utilities would be unconfigurable, but
porcelain scripts would be configurable by default, with some
exceptions.

> I tend to agree here.  At work, we have code that wants git status
> --porcelain to be empty.  If a user added -b to all of their git status
> calls (to make -s output more helpful), that would break a lot of
> tooling.  It's much better if they create an alias, since that doesn't
> affect automated tools.

With the caveat I noted above this would be a complete non-issue, i.e.
we'd just pass some option to the getopt for --porcelain which would
disable any other option slurping up its configuration from a config
file.

> I expect developers of things such as fugitive would dislike such a
> feature as well.  I get the impression our existing config file options
> already make life difficult enough.

I don't know if this is what Duy has in mind, but the facility I've
described is  purely an internal code reorganization issue. I.e. us
not having to write custom code for each bultin every time we want to
take an option from the command line || config.

It does make it *easier* to flag more options as "this is
configurable", but whether that flag is turned on for each
command/option is just something we could discuss on-list on a
case-by-case basis.

Re: Add configuration options for some commonly used command-line options

[Duy Nguyen]

On Sun, Mar 19, 2017 at 8:18 PM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>> OTOH, we already have almost "enable such or such option by default"
>> with aliases. People who always run "git am" with "-3" can write
>>
>> [alias]
>>         a3 = am -3
>>
>> and just run "git a3".
>
> I tend to agree here.

That's exactly what I'm doing too. But I have an impression that the
stream of new configuration for default cmdline options keeps coming
in. And this, as a gsoc microproject, encourages more to come :-/

There's also another reason I suggested this but I don't know if it
will work out yet. If we start to make parse_options() or similar to
handle some configurations, we probably can gradually move away from
the procedural callback-based config parsing to a more declarative
style like 'struct option'. That opens up new opportunities: spotting
config name typos, listing applicable variables of a command ....
-- 
Duy

Re: Add configuration options for some commonly used command-line options

[Duy Nguyen]

On Sun, Mar 19, 2017 at 8:43 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Sun, Mar 19, 2017 at 2:18 PM, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
>> On Sun, Mar 19, 2017 at 11:15:33AM +0100, Matthieu Moy wrote:
>>> I think the main problem is indeed "stop the users from shooting
>>> themselves in the foot". Many command-line options change the behavior
>>> completely so allowing users to enable them by default means allowing
>>> the users to change Git in such a way that scripts calling it are
>>> broken.
>>>
>>> This also doesn't help when troublshouting an issue as these options are
>>> typically something set once and for all and which you forget about.
>>> This typically leads to discussion in Q&A forums like:
>>>
>>> A: Can you run "git foo"?
>>> B: Here's the result: ...
>>> A: I don't understand, I can't reproduce here.
>>>
>>> just because B has a CLI option enabled by default.
>>>
>>> This is the same reasoning that leads Git to forbid aliasing an existing
>>> command to something else.
>>>
>>> OTOH, we already have almost "enable such or such option by default"
>>> with aliases. People who always run "git am" with "-3" can write
>>>
>>> [alias]
>>>         a3 = am -3
>>>
>>> and just run "git a3".
>
> I can't find the E-Mail chain now but this has been discussed on-list
> a while ago. I.e. having some getopt support to say for the push
> command, that the --rebase option can also come from the config, i.e.
> in this case the pull.rebase option.
>
> IIRC the consensus was that such a facility would allow commands or
> individual options to say "this command/option is configurable", thus
> of course all plumbing utilities would be unconfigurable, but
> porcelain scripts would be configurable by default, with some
> exceptions.

This is exactly it! It's much better than adding individual config
variables (less work for sure, but messier). Maybe we should promote
the microproject "Add configuration options for commonly used cmdline
options" to project. If it's too short (I'm guessing the core code
could be done in a month), the gsoc student can always convert more
config to the new way.
-- 
Duy

Re: Add configuration options for some commonly used command-line options

[Brandon Williams]

On 03/20, Duy Nguyen wrote:
> On Sun, Mar 19, 2017 at 8:43 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> > On Sun, Mar 19, 2017 at 2:18 PM, brian m. carlson
> > <sandals@crustytoothpaste.net> wrote:
> >> On Sun, Mar 19, 2017 at 11:15:33AM +0100, Matthieu Moy wrote:
> >>> I think the main problem is indeed "stop the users from shooting
> >>> themselves in the foot". Many command-line options change the behavior
> >>> completely so allowing users to enable them by default means allowing
> >>> the users to change Git in such a way that scripts calling it are
> >>> broken.
> >>>
> >>> This also doesn't help when troublshouting an issue as these options are
> >>> typically something set once and for all and which you forget about.
> >>> This typically leads to discussion in Q&A forums like:
> >>>
> >>> A: Can you run "git foo"?
> >>> B: Here's the result: ...
> >>> A: I don't understand, I can't reproduce here.
> >>>
> >>> just because B has a CLI option enabled by default.
> >>>
> >>> This is the same reasoning that leads Git to forbid aliasing an existing
> >>> command to something else.
> >>>
> >>> OTOH, we already have almost "enable such or such option by default"
> >>> with aliases. People who always run "git am" with "-3" can write
> >>>
> >>> [alias]
> >>>         a3 = am -3
> >>>
> >>> and just run "git a3".
> >
> > I can't find the E-Mail chain now but this has been discussed on-list
> > a while ago. I.e. having some getopt support to say for the push
> > command, that the --rebase option can also come from the config, i.e.
> > in this case the pull.rebase option.
> >
> > IIRC the consensus was that such a facility would allow commands or
> > individual options to say "this command/option is configurable", thus
> > of course all plumbing utilities would be unconfigurable, but
> > porcelain scripts would be configurable by default, with some
> > exceptions.
> 
> This is exactly it! It's much better than adding individual config
> variables (less work for sure, but messier). Maybe we should promote
> the microproject "Add configuration options for commonly used cmdline
> options" to project. If it's too short (I'm guessing the core code
> could be done in a month), the gsoc student can always convert more
> config to the new way.

If in the future we did want better support for making user defaults
(apart from aliases) for commands we could entertain creating a command
like bash's 'command' which ignores any user defaults and executes a
particular command in a vanilla mode.

So if the user configured 'git am' to always use the -3 option then
running `git command am` (or something akin to that) would just run the
vanilla 'am' command with no options.  Probably not the best idea since
tooling would need to become aware of such a paradigm change, but its
just a thought.

-- 
Brandon Williams

Re: Add configuration options for some commonly used command-line options

[Jeff King]

On Mon, Mar 20, 2017 at 10:32:37AM -0700, Brandon Williams wrote:

> > > IIRC the consensus was that such a facility would allow commands or
> > > individual options to say "this command/option is configurable", thus
> > > of course all plumbing utilities would be unconfigurable, but
> > > porcelain scripts would be configurable by default, with some
> > > exceptions.
> > 
> > This is exactly it! It's much better than adding individual config
> > variables (less work for sure, but messier). Maybe we should promote
> > the microproject "Add configuration options for commonly used cmdline
> > options" to project. If it's too short (I'm guessing the core code
> > could be done in a month), the gsoc student can always convert more
> > config to the new way.
> 
> If in the future we did want better support for making user defaults
> (apart from aliases) for commands we could entertain creating a command
> like bash's 'command' which ignores any user defaults and executes a
> particular command in a vanilla mode.
> 
> So if the user configured 'git am' to always use the -3 option then
> running `git command am` (or something akin to that) would just run the
> vanilla 'am' command with no options.  Probably not the best idea since
> tooling would need to become aware of such a paradigm change, but its
> just a thought.

I think we've had similar proposals in the form of an environment
variable like "GIT_PLUMBING" (and your "command", which I do like
syntactically, would probably just end up setting such an environment
variable anyway).

But yeah, the sticking point is that we'd have to wait for scripts to
adopt it. Manually marking options as "safe" is tedious, but gives more
control.

If we want to follow the GIT_PLUMBING route, I think the first step
would be to introduce it now as a noop (or even a mostly-noop that just
turns off eve the "safe" options). And then we wait for N time units so
that scripts can start using it, and only then start introducing the
breaking changes.

-Peff

Re: Add configuration options for some commonly used command-line options

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> If in the future we did want better support for making user defaults
> (apart from aliases) for commands we could entertain creating a command
> like bash's 'command' which ignores any user defaults and executes a
> particular command in a vanilla mode.
>
> So if the user configured 'git am' to always use the -3 option then
> running `git command am` (or something akin to that) would just run the
> vanilla 'am' command with no options.  Probably not the best idea since
> tooling would need to become aware of such a paradigm change, but its
> just a thought.

I do not think "command" is a good analogy.  In practice it is used
by those who want to create a wrapper that overrides a command for
her own use, e.g. "ls () { command ls -AF "$@" }", in her .bashrc.

I suspect that it is too cumbersome for script writers to use for
the purpose of protecting their scripts from random aliases that may
change the behaviour of the commands their scripts want to rely
on---they'll be forced to sprinkle practically each and every
invocations of the basic UNIX building blocks with "command".

The saving grace for shell scripts is that the shell has good way to
tell interactive and scripted use apart, by disabling aliases and
not reading some rc files for non-interactive shells.  Unfortunately
I do not think "git" has the luxury of using that "hint" as a command 
invoked by these shells.

One thing we may want to consider is why we have to even worry about
scripts getting broken.  It is because people script around
Porcelain, and that is because we have been too eager to improve
Porcelain while neglecting plumbing for too long, to the point that
some things are only doable with Porcelain (or doing the same with
plumbing while possible are made too cumbersome).  I find it quite
disturbing that nobody brought that up as an issue that needs to be
addressed in this entire thread.

Re: Add configuration options for some commonly used command-line options

[Jeff King]

On Mon, Mar 20, 2017 at 11:56:35AM -0700, Junio C Hamano wrote:

> One thing we may want to consider is why we have to even worry about
> scripts getting broken.  It is because people script around
> Porcelain, and that is because we have been too eager to improve
> Porcelain while neglecting plumbing for too long, to the point that
> some things are only doable with Porcelain (or doing the same with
> plumbing while possible are made too cumbersome).  I find it quite
> disturbing that nobody brought that up as an issue that needs to be
> addressed in this entire thread.

I think there is a chicken-and-egg problem there. The plumbing _wasn't_
there, so people started using porcelain in their scripts, which made us
hesitant to change it. That fact that it doesn't break makes script
writers think it's OK. And now we're stuck with things like "log" and
"diff" as pseudo-plumbing, unless we want to take a strong stand and say
"you're doing it wrong, even though there was no other way to do it
until now".

Unless you want to follow the usual deprecation schedule by introducing
new plumbing commands to fill in the gaps, waiting, and then proceeding
to change the porcelain. But I think that's isomorphic with the other
solutions. I.e., out of:

  1. git rev-list
  2. GIT_PLUMBING=1 git log
  3. git command log

They are all doing the exact same thing: running a log-like command
without any config or cross-version changes. It's just a matter of
syntax. One of the nice things about (2) and (3) is that you don't have
to invent a new plumbing-ish name for each command.

-Peff

Re: Add configuration options for some commonly used command-line options

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 20, 2017 at 7:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
> One thing we may want to consider is why we have to even worry about
> scripts getting broken.  It is because people script around
> Porcelain, and that is because we have been too eager to improve
> Porcelain while neglecting plumbing for too long, to the point that
> some things are only doable with Porcelain (or doing the same with
> plumbing while possible are made too cumbersome).  I find it quite
> disturbing that nobody brought that up as an issue that needs to be
> addressed in this entire thread.

I very much doubt this describes anything but a tiny number of cases
where people are using the porcelain as an API.

People aren't going through the process of trying to find out how to
do something with a plumbing command, and then failing and falling
back to a porcelain command because the plumbing isn't complete
enough. They just use the porcelain because they're familiar with it
and scripting it works for them.

E.g. I just looked at both major Emacs modes for git now, magit &
vc-git, neither use "mktag", they just shell out to "git tag" for
making tags. I just went to the git-scm.com website and looked at one
open source GUI client I could "git clone", Giggle. It just shells out
to e.g. "git commit" to make commits, not "git commit-tree". The other
commands they're using are porcelain too. If they've used some
plumbing it's probably by sheer accident. E.g. they use ls-tree which
is plumbing, but don't use for-each-ref.

What's that Google SRE-ism again? Something like "People use the
reliability you provide them with in practice, not what you
advertise". Our porcelain is very stable, and so people use it as a
stable API, and not just for trivial scripts.

Which I think has some big implications for how we maintain the
porcelain & plumbing. Since people *will* use the porcelain, probably
no matter what we advertise to them or how good the plumbing is.

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

[Ævar Arnfjörð Bjarmason]

Add a nascent WIP facility to specify via the options parsing that
we'd e.g. like to grab the --status option for commit.status from the
commit.status config key.

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.

But even so this works, passes all tests, gets rid of 3 lines in
commit.c, and has the promise of getting rid of a *lot* more manual
option parsing in favor of declaring config keys via OPT_*
everywhere. See my
<CACBZZX4FksU6NujPZ_3GZ45EQ+KdJj5G2sajtRipE1wbaA3URA@mail.gmail.com>
for more details.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Sun, Mar 19, 2017 at 2:43 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> I don't know if this is what Duy has in mind, but the facility I've
> described is  purely an internal code reorganization issue. I.e. us
> not having to write custom code for each bultin every time we want to
> take an option from the command line || config.

Here's an implementation of this I hacked up this evening. This is
very WIP as noted in the commit message / TODO comments, but it works!
I thought I'd send it to the list for comments on the general approach
before taking it much further.

 builtin/commit.c   |  7 ++-----
 parse-options-cb.c | 21 +++++++++++++++++++++
 parse-options.c    | 40 ++++++++++++++++++++++++++++++++++++----
 parse-options.h    | 16 +++++++++++++---
 4 files changed, 72 insertions(+), 12 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 4e288bc513..a7c9e4128f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1500,10 +1500,6 @@ static int git_commit_config(const char *k, const char *v, void *cb)
 
 	if (!strcmp(k, "commit.template"))
 		return git_config_pathname(&template_file, k, v);
-	if (!strcmp(k, "commit.status")) {
-		include_status = git_config_bool(k, v);
-		return 0;
-	}
 	if (!strcmp(k, "commit.cleanup"))
 		return git_config_string(&cleanup_arg, k, v);
 	if (!strcmp(k, "commit.gpgsign")) {
@@ -1596,7 +1592,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
 		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
 		OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip spaces and #comments from message")),
-		OPT_BOOL(0, "status", &include_status, N_("include status in commit message template")),
+		OPT_BOOL_C(0, "status", &include_status, N_("include status in commit message template"),
+		           "commit.status", parse_opt_confkey_bool),
 		{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
 		  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 		/* end commit message options */
diff --git a/parse-options-cb.c b/parse-options-cb.c
index b7d8f7dcb2..2383d9bbe0 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -236,3 +236,24 @@ int parse_opt_passthru_argv(const struct option *opt, const char *arg, int unset
 
 	return 0;
 }
+
+/* Does it suck that I have to use the cache interface to the config
+ * here? Should we somehow unroll this whole thing so
+ * parse_options_step loops over the config values, and maybe
+ * populates opt->conf_key (which we'd need to add) for all the
+ * options that need it?
+ *
+ * I.e. should we make this more complex because this one-shot
+ * interface is expensive, or is it just fine?
+*/ 
+
+int parse_opt_confkey_bool(const struct option *opt, const char *arg, int unset) {
+	const char *value;
+
+	if (git_config_get_value(opt->conf_key, &value))
+		return 0;
+
+	*(int *)opt->value = git_config_bool(opt->conf_key, value);
+
+	return 0;
+}
diff --git a/parse-options.c b/parse-options.c
index 4fbe924a5d..f9aba088d8 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -235,12 +235,13 @@ static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *optio
 }
 
 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;
 	const char *arg_end = strchrnul(arg, '=');
 	const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
 	int abbrev_flags = 0, ambiguous_flags = 0;
+	int ret;
 
 	for (; options->type != OPTION_END; options++) {
 		const char *rest, *long_name = options->long_name;
@@ -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;
 	}
 
 	if (ambiguous_option)
@@ -429,7 +440,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *,
 				       const struct option *, int, int);
 
 int parse_options_step(struct parse_opt_ctx_t *ctx,
-		       const struct option *options,
+		       struct option *options,
 		       const char * const usagestr[])
 {
 	int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);
@@ -514,6 +525,27 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 		ctx->out[ctx->cpidx++] = ctx->argv[0];
 		ctx->opt = NULL;
 	}
+
+	/* 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;
+	}
+
 	return PARSE_OPT_DONE;
 
  show_usage_error:
@@ -530,7 +562,7 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)
 }
 
 int parse_options(int argc, const char **argv, const char *prefix,
-		  const struct option *options, const char * const usagestr[],
+		  struct option *options, const char * const usagestr[],
 		  int flags)
 {
 	struct parse_opt_ctx_t ctx;
diff --git a/parse-options.h b/parse-options.h
index dcd8a0926c..14abf21467 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -38,7 +38,9 @@ enum parse_opt_option_flags {
 	PARSE_OPT_LASTARG_DEFAULT = 16,
 	PARSE_OPT_NODASH = 32,
 	PARSE_OPT_LITERAL_ARGHELP = 64,
-	PARSE_OPT_SHELL_EVAL = 256
+	PARSE_OPT_SHELL_EVAL = 256,
+	PARSE_OPT_CONFIGURABLE = 512,
+	PARSE_OPT_VIA_CLI = 1024
 };
 
 struct option;
@@ -110,6 +112,9 @@ struct option {
 	int flags;
 	parse_opt_cb *callback;
 	intptr_t defval;
+
+	const char *conf_key;
+	parse_opt_cb *conf_callback;
 };
 
 #define OPT_END()                   { OPTION_END }
@@ -124,7 +129,11 @@ struct option {
 				      (h), PARSE_OPT_NOARG }
 #define OPT_SET_INT(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG, NULL, (i) }
+#define OPT_SET_INT_C(s, l, v, h, i, ck, cb) \
+				    { OPTION_SET_INT, (s), (l), (v), NULL, \
+				      (h), PARSE_OPT_NOARG | PARSE_OPT_CONFIGURABLE, NULL, (i), ck, cb }
 #define OPT_BOOL(s, l, v, h)        OPT_SET_INT(s, l, v, h, 1)
+#define OPT_BOOL_C(s, l, v, h, ck, cb) OPT_SET_INT_C(s, l, v, h, 1, ck, cb)
 #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}
 #define OPT_CMDMODE(s, l, v, h, i)  { OPTION_CMDMODE, (s), (l), (v), NULL, \
@@ -166,7 +175,7 @@ struct option {
  * Returns the number of arguments left in argv[].
  */
 extern int parse_options(int argc, const char **argv, const char *prefix,
-                         const struct option *options,
+                         struct option *options,
                          const char * const usagestr[], int flags);
 
 extern NORETURN void usage_with_options(const char * const *usagestr,
@@ -210,7 +219,7 @@ extern void parse_options_start(struct parse_opt_ctx_t *ctx,
 				const struct option *options, int flags);
 
 extern int parse_options_step(struct parse_opt_ctx_t *ctx,
-			      const struct option *options,
+			      struct option *options,
 			      const char * const usagestr[]);
 
 extern int parse_options_end(struct parse_opt_ctx_t *ctx);
@@ -231,6 +240,7 @@ extern int parse_opt_noop_cb(const struct option *, const char *, int);
 extern int parse_opt_unknown_cb(const struct option *, const char *, int);
 extern int parse_opt_passthru(const struct option *, const char *, int);
 extern int parse_opt_passthru_argv(const struct option *, const char *, int);
+extern int parse_opt_confkey_bool(const struct option *, const char *, int);
 
 #define OPT__VERBOSE(var, h)  OPT_COUNTUP('v', "verbose", (var), (h))
 #define OPT__QUIET(var, h)    OPT_COUNTUP('q', "quiet",   (var), (h))
-- 
2.11.0

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

[Ævar Arnfjörð Bjarmason]

 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.

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1152 bytes --]

On Fri, Mar 24, 2017 at 11:10:13PM +0000, Ævar Arnfjörð Bjarmason wrote:
> On Sun, Mar 19, 2017 at 2:43 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> > I don't know if this is what Duy has in mind, but the facility I've
> > described is  purely an internal code reorganization issue. I.e. us
> > not having to write custom code for each bultin every time we want to
> > take an option from the command line || config.
> 
> Here's an implementation of this I hacked up this evening. This is
> very WIP as noted in the commit message / TODO comments, but it works!
> I thought I'd send it to the list for comments on the general approach
> before taking it much further.

For what it's worth, I think this is a good design.  It makes it easy to
add options when needed, but it doesn't override the defaults for the
entire command, which was my concern.

The potential for removing a decent amount of likely duplicative code
also makes me happy.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Jeff King]

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

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

[Ævar Arnfjörð Bjarmason]

On Sat, Mar 25, 2017 at 10:31 PM, Jeff King <peff@peff.net> wrote:
> 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).

I like the simplicity of this approach a lot. I.e. (to paraphrase it
just to make sure we're on the same page): Skip all the complexity of
reaching into the getopt guts, and just munge argc/argv given a config
we can stick ahead of the getopt (struct options) spec, inject some
options at the beginning if they're in the config, and off we go
without any further changes to the getopt guts.

There's two practical issues with this that are easy to solve with my
current approach, but I can't find an easy solution to using this
method.

The first is that we're replacing the semantics of:

"If you're specifying it on the command-line, we take it from there,
otherwise we use your config, if set, regardless of how the option
works"

with:

"We read your config, inject options implicitly at the start of the
command line, and then append whatever command-line you give us"

These two are not the same. Consider e.g. the commit.verbose config.
With my current patch if have commit.verbose=1 in your config and do
"commit --verbose" you just end up with a result equivalent to not
having it in your config, but since the --verbose option can be
supplied multiple times to increase verbosity with the injection
method you'd end up with the equivalent of commit.verbose=2.

I think the semantics I've implemented are much less confusing for the
user, i.e. you can specify an option that's configurable and know that
you're overriding your config, not potentially joining the
command-line with whatever's in your config. We have a lot of options
without last-one-wins semantics.

I can't think of a good way around that with your proposed approach
that doesn't essentially get us back to something very similar to my
patch, i.e. we'd need to parse the command-line using the options spec
before applying our implicit config.

The second issue is related, i.e. I was going to add some flag an
option could supply to say "if I'm provided none of these other
maybe-from-config options get to read their config". This is needed
for hybrid plumbing/porcelain like "git status --porcelain".

Let's say we wanted to add a status.branch config option, and wanted
status.branch=true along with "status --porcelain" not to mean "status
--porcelain --branch", potentially breaking scripts, but "status
--porcelain". This again needs needs the guts of the getopt parsing to
work as far as I can tell.

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

[Jeff King]

On Sat, Mar 25, 2017 at 11:32:02PM +0100, Ævar Arnfjörð Bjarmason wrote:

> > 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).
> 
> I like the simplicity of this approach a lot. I.e. (to paraphrase it
> just to make sure we're on the same page): Skip all the complexity of
> reaching into the getopt guts, and just munge argc/argv given a config
> we can stick ahead of the getopt (struct options) spec, inject some
> options at the beginning if they're in the config, and off we go
> without any further changes to the getopt guts.

Yep, I think that's an accurate description.

> There's two practical issues with this that are easy to solve with my
> current approach, but I can't find an easy solution to using this
> method.
> 
> The first is that we're replacing the semantics of:
> 
> "If you're specifying it on the command-line, we take it from there,
> otherwise we use your config, if set, regardless of how the option
> works"
> 
> with:
> 
> "We read your config, inject options implicitly at the start of the
> command line, and then append whatever command-line you give us"
> 
> These two are not the same. Consider e.g. the commit.verbose config.
> With my current patch if have commit.verbose=1 in your config and do
> "commit --verbose" you just end up with a result equivalent to not
> having it in your config, but since the --verbose option can be
> supplied multiple times to increase verbosity with the injection
> method you'd end up with the equivalent of commit.verbose=2.

Right, for anything where multiple options are meaningful, they'd have
to give "--no-verbose" to reset the option. In a sense that's less
friendly, because it's more manual. But it's also less magical, because
the semantics are clear: the config option behaves exactly as if you
gave the option on the command line. So for an OPT_STRING_LIST(), you
could append to the list, or reset it to empty, etc, as you see fit.

But I do agree that it's more manual, and probably would cause some
confusion.

> I can't think of a good way around that with your proposed approach
> that doesn't essentially get us back to something very similar to my
> patch, i.e. we'd need to parse the command-line using the options spec
> before applying our implicit config.

Yes, the semantics you gave require parsing the options first. I think
it would be sufficient to just give each "struct option" a "seen" flag
(rather than having it understand the config mechanism), having
parse_options() set the flag, and then feeding the result to a separate
config/cmdline mapping mechanism. That keeps the complexity out of the
options code.

It does tie us back in to requiring parse-options, which not all the
options use.

In a lot of cases that "seen" flag is effectively a sentinel value in
whatever variable the option value is stored in. But some of the options
don't have reasonable sentinel values (as you noticed with the "revert
-m" handling recently).

> The second issue is related, i.e. I was going to add some flag an
> option could supply to say "if I'm provided none of these other
> maybe-from-config options get to read their config". This is needed
> for hybrid plumbing/porcelain like "git status --porcelain".

Yeah, I agree you can't make that decision until you've seen the
command-line options. I think we currently do some hairy stuff where we
speculatively read config into a variable, and then apply the
config-based defaults only when we know we're in non-porcelain mode (see
status_deferred_config in builtin/commit.c).

That came about because we didn't want to parse the config a second
time. These days the deferred config should probably just be read from
the cached configset, after we've read the other options.

But I think this can be done after the full option-parsing is finished
by applying the mapping then.  I.e., something like:

    parse_options(argc, argv, options, ...);
    if (status_format != STATUS_FORMAT_PORCELAIN)
	apply_config_mapping(status_mapping, options);

-Peff

[PATCH/RFC v2] WIP configurable options facility

[Ævar Arnfjörð Bjarmason]

---

FWIW here's the current state of this WIP hack which I worked a bit on
yesterday. I converted it to use a hashmap and got rid of the need to
s/const // the options struct.

I've either converted options that read the config to this already, or
left TODO comments on those that are candidates for migration.

All tests pass with this, but as the various TODO comments note
there's some problems. It's leaking memory currently, and as the
comment in parse_options_step() notes due to how this options code is
called I've had to sprinkle some boilerplate in several parse_*_opt()
functions.

This is getting rid of less code than I'd hoped at first, although
it'll be made up if we make more stuff configurable.

On Tue, Mar 28, 2017 at 7:17 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Mar 25, 2017 at 11:32:02PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > 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).
>>
>> I like the simplicity of this approach a lot. I.e. (to paraphrase it
>> just to make sure we're on the same page): Skip all the complexity of
>> reaching into the getopt guts, and just munge argc/argv given a config
>> we can stick ahead of the getopt (struct options) spec, inject some
>> options at the beginning if they're in the config, and off we go
>> without any further changes to the getopt guts.
>
> Yep, I think that's an accurate description.
>
>> There's two practical issues with this that are easy to solve with my
>> current approach, but I can't find an easy solution to using this
>> method.
>>
>> The first is that we're replacing the semantics of:
>>
>> "If you're specifying it on the command-line, we take it from there,
>> otherwise we use your config, if set, regardless of how the option
>> works"
>>
>> with:
>>
>> "We read your config, inject options implicitly at the start of the
>> command line, and then append whatever command-line you give us"
>>
>> These two are not the same. Consider e.g. the commit.verbose config.
>> With my current patch if have commit.verbose=1 in your config and do
>> "commit --verbose" you just end up with a result equivalent to not
>> having it in your config, but since the --verbose option can be
>> supplied multiple times to increase verbosity with the injection
>> method you'd end up with the equivalent of commit.verbose=2.
>
> Right, for anything where multiple options are meaningful, they'd have
> to give "--no-verbose" to reset the option. In a sense that's less
> friendly, because it's more manual. But it's also less magical, because
> the semantics are clear: the config option behaves exactly as if you
> gave the option on the command line. So for an OPT_STRING_LIST(), you
> could append to the list, or reset it to empty, etc, as you see fit.
>
> But I do agree that it's more manual, and probably would cause some
> confusion.

And some slight breakage of backwards compatibiilty for things that
supply the option on the CLI now, although it wouldn't be a huge deal.

>> I can't think of a good way around that with your proposed approach
>> that doesn't essentially get us back to something very similar to my
>> patch, i.e. we'd need to parse the command-line using the options spec
>> before applying our implicit config.
>
> Yes, the semantics you gave require parsing the options first. I think
> it would be sufficient to just give each "struct option" a "seen" flag
> (rather than having it understand the config mechanism), having
> parse_options() set the flag, and then feeding the result to a separate
> config/cmdline mapping mechanism. That keeps the complexity out of the
> options code.

Right, would require s/const // on the struct though like in my v1, or
keeping this info in some datastructure on the side.

> It does tie us back in to requiring parse-options, which not all the
> options use.

I think if we do keep something like this patch it's fair enough to
say it won't work for everything. It's just there to make it easier
for us to add configuration for options in the common case, but
there's going to be various special cases (e.g. currently the options
synonyms, and I think I'll leave that) that we won't be able to
handle.

> In a lot of cases that "seen" flag is effectively a sentinel value in
> whatever variable the option value is stored in. But some of the options
> don't have reasonable sentinel values (as you noticed with the "revert
> -m" handling recently).
>
>> The second issue is related, i.e. I was going to add some flag an
>> option could supply to say "if I'm provided none of these other
>> maybe-from-config options get to read their config". This is needed
>> for hybrid plumbing/porcelain like "git status --porcelain".
>
> Yeah, I agree you can't make that decision until you've seen the
> command-line options. I think we currently do some hairy stuff where we
> speculatively read config into a variable, and then apply the
> config-based defaults only when we know we're in non-porcelain mode (see
> status_deferred_config in builtin/commit.c).
>
> That came about because we didn't want to parse the config a second
> time. These days the deferred config should probably just be read from
> the cached configset, after we've read the other options.
>
> But I think this can be done after the full option-parsing is finished
> by applying the mapping then.  I.e., something like:
>
>     parse_options(argc, argv, options, ...);
>     if (status_format != STATUS_FORMAT_PORCELAIN)
>         apply_config_mapping(status_mapping, options);
>
> -Peff

 builtin/add.c      |   1 +
 builtin/am.c       |  16 +++----
 builtin/blame.c    |  16 +++----
 builtin/clean.c    |   8 +---
 builtin/commit.c   |  40 +++++++++-------
 builtin/difftool.c |  12 ++---
 builtin/fetch.c    |   1 +
 builtin/grep.c     |   1 +
 builtin/help.c     |   1 +
 builtin/log.c      |   1 +
 builtin/merge.c    |   2 +
 builtin/notes.c    |   1 +
 builtin/pull.c     |   2 +
 builtin/push.c     |   3 ++
 builtin/repack.c   |   3 ++
 builtin/tag.c      |  19 ++++++--
 parse-options-cb.c |  62 ++++++++++++++++++++++++
 parse-options.c    | 138 +++++++++++++++++++++++++++++++++++++++++++++++------
 parse-options.h    |  26 +++++++++-
 19 files changed, 283 insertions(+), 70 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 9f53f020d0..a1a9d7cd97 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -286,6 +286,7 @@ static struct option builtin_add_options[] = {
 
 static int add_config(const char *var, const char *value, void *cb)
 {
+	/* NOTE: No support for option synonyms. Probably not worth it. */
 	if (!strcmp(var, "add.ignoreerrors") ||
 	    !strcmp(var, "add.ignore-errors")) {
 		ignore_add_errors = git_config_bool(var, value);
diff --git a/builtin/am.c b/builtin/am.c
index f7a7a971fb..b82f81e70a 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -147,13 +147,8 @@ static void am_state_init(struct am_state *state, const char *dir)
 	state->dir = xstrdup(dir);
 
 	state->prec = 4;
-
-	git_config_get_bool("am.threeway", &state->threeway);
-
 	state->utf8 = 1;
 
-	git_config_get_bool("am.messageid", &state->message_id);
-
 	state->scissors = SCISSORS_UNSET;
 
 	argv_array_init(&state->git_apply_opts);
@@ -947,6 +942,7 @@ static int split_mail(struct am_state *state, enum patch_format patch_format,
 {
 	if (keep_cr < 0) {
 		keep_cr = 0;
+		/* TODO: This is --keep-cr */
 		git_config_get_bool("am.keepcr", &keep_cr);
 	}
 
@@ -2240,8 +2236,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 			N_("run interactively")),
 		OPT_HIDDEN_BOOL('b', "binary", &binary,
 			N_("historical option -- no-op")),
-		OPT_BOOL('3', "3way", &state.threeway,
-			N_("allow fall back on 3way merging if needed")),
+		OPT_BOOL_C('3', "3way", &state.threeway,
+			N_("allow fall back on 3way merging if needed"),
+			"am.threeway", parse_opt_confkey_bool),
 		OPT__QUIET(&state.quiet, N_("be quiet")),
 		OPT_SET_INT('s', "signoff", &state.signoff,
 			N_("add a Signed-off-by line to the commit message"),
@@ -2252,8 +2249,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 			N_("pass -k flag to git-mailinfo"), KEEP_TRUE),
 		OPT_SET_INT(0, "keep-non-patch", &state.keep,
 			N_("pass -b flag to git-mailinfo"), KEEP_NON_PATCH),
-		OPT_BOOL('m', "message-id", &state.message_id,
-			N_("pass -m flag to git-mailinfo")),
+		OPT_BOOL_C('m', "message-id", &state.message_id,
+			N_("pass -m flag to git-mailinfo"),
+			"am.messageid", parse_opt_confkey_bool),
 		{ OPTION_SET_INT, 0, "keep-cr", &keep_cr, NULL,
 		  N_("pass --keep-cr flag to git-mailsplit for mbox format"),
 		  PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1},
diff --git a/builtin/blame.c b/builtin/blame.c
index f7aa95f4ba..b7eec51d0a 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2193,14 +2193,7 @@ static const char *add_prefix(const char *prefix, const char *path)
 
 static int git_blame_config(const char *var, const char *value, void *cb)
 {
-	if (!strcmp(var, "blame.showroot")) {
-		show_root = git_config_bool(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "blame.blankboundary")) {
-		blank_boundary = git_config_bool(var, value);
-		return 0;
-	}
+	/* TODO: The --show-email option (bit) */
 	if (!strcmp(var, "blame.showemail")) {
 		int *output_option = cb;
 		if (git_config_bool(var, value))
@@ -2209,6 +2202,7 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 			*output_option &= ~OUTPUT_SHOW_EMAIL;
 		return 0;
 	}
+	/* TODO: The --date option (string, date format) */
 	if (!strcmp(var, "blame.date")) {
 		if (!value)
 			return config_error_nonbool(var);
@@ -2571,8 +2565,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	const char *contents_from = NULL;
 	const struct option options[] = {
 		OPT_BOOL(0, "incremental", &incremental, N_("Show blame entries as we find them, incrementally")),
-		OPT_BOOL('b', NULL, &blank_boundary, N_("Show blank SHA-1 for boundary commits (Default: off)")),
-		OPT_BOOL(0, "root", &show_root, N_("Do not treat root commits as boundaries (Default: off)")),
+		OPT_BOOL_C('b', NULL, &blank_boundary, N_("Show blank SHA-1 for boundary commits (Default: off)"),
+		           "blame.blankboundary", parse_opt_confkey_bool),
+		OPT_BOOL_C(0, "root", &show_root, N_("Do not treat root commits as boundaries (Default: off)"),
+		           "blame.showroot", parse_opt_confkey_bool),
 		OPT_BOOL(0, "show-stats", &show_stats, N_("Show work cost statistics")),
 		OPT_BOOL(0, "progress", &show_progress, N_("Force progress reporting")),
 		OPT_BIT(0, "score-debug", &output_option, N_("Show output score for blame entries"), OUTPUT_SHOW_SCORE),
diff --git a/builtin/clean.c b/builtin/clean.c
index d861f836a2..3922e146ad 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -119,11 +119,6 @@ static int git_clean_config(const char *var, const char *value, void *cb)
 		return color_parse(value, clean_colors[slot]);
 	}
 
-	if (!strcmp(var, "clean.requireforce")) {
-		force = !git_config_bool(var, value);
-		return 0;
-	}
-
 	/* inspect the color.ui config variable and others */
 	return git_color_default_config(var, value, cb);
 }
@@ -874,7 +869,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("do not print names of files removed")),
 		OPT__DRY_RUN(&dry_run, N_("dry run")),
-		OPT__FORCE(&force, N_("force")),
+		OPT__FORCE_C(&force, N_("force"),
+		            "clean.requireForce", parse_opt_confkey_bool_neg),
 		OPT_BOOL('i', "interactive", &interactive, N_("interactive cleaning")),
 		OPT_BOOL('d', NULL, &remove_directories,
 				N_("remove whole directories")),
diff --git a/builtin/commit.c b/builtin/commit.c
index 4e288bc513..9f11f41be3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1493,28 +1493,30 @@ static void print_summary(const char *prefix, const struct object_id *oid,
 	strbuf_release(&format);
 }
 
+int parse_opt_confkey_verbose_bool_or_int(const struct option *opt, const char *arg, int unset) {
+	const char *value;
+	int is_bool;
+
+	if (git_config_get_value(opt->conf_key, &value))
+		return 0;
+
+	config_commit_verbose = git_config_bool_or_int(opt->conf_key, value, &is_bool);
+
+	trace_printf("getopt/parse_opt_confkey_verbose_bool_or_int: Parsed bool_or_int value for %s got %d\n", opt->long_name, config_commit_verbose);
+
+	return 0;
+}
+
 static int git_commit_config(const char *k, const char *v, void *cb)
 {
 	struct wt_status *s = cb;
 	int status;
 
-	if (!strcmp(k, "commit.template"))
-		return git_config_pathname(&template_file, k, v);
-	if (!strcmp(k, "commit.status")) {
-		include_status = git_config_bool(k, v);
-		return 0;
-	}
-	if (!strcmp(k, "commit.cleanup"))
-		return git_config_string(&cleanup_arg, k, v);
+	/* TODO: The --gpg-sign option (string or bool? confusing) */
 	if (!strcmp(k, "commit.gpgsign")) {
 		sign_commit = git_config_bool(k, v) ? "" : NULL;
 		return 0;
 	}
-	if (!strcmp(k, "commit.verbose")) {
-		int is_bool;
-		config_commit_verbose = git_config_bool_or_int(k, v, &is_bool);
-		return 0;
-	}
 
 	status = git_gpg_config(k, v, NULL);
 	if (status)
@@ -1580,7 +1582,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	static struct wt_status s;
 	static struct option builtin_commit_options[] = {
 		OPT__QUIET(&quiet, N_("suppress summary after successful commit")),
-		OPT__VERBOSE(&verbose, N_("show diff in commit message template")),
+		OPT__VERBOSE_C(&verbose, N_("show diff in commit message template"),
+			      "commit.verbose", parse_opt_confkey_verbose_bool_or_int),
 
 		OPT_GROUP(N_("Commit message options")),
 		OPT_FILENAME('F', "file", &logfile, N_("read message from file")),
@@ -1593,10 +1596,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
 		OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
 		OPT_BOOL('s', "signoff", &signoff, N_("add Signed-off-by:")),
-		OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
+		OPT_FILENAME_C('t', "template", &template_file, N_("use specified template file"),
+			       "commit.template", parse_opt_confkey_pathname),
 		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
-		OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip spaces and #comments from message")),
-		OPT_BOOL(0, "status", &include_status, N_("include status in commit message template")),
+		OPT_STRING_C(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip spaces and #comments from message"),
+			     "commit.cleanup", parse_opt_confkey_string),
+		OPT_BOOL_C(0, "status", &include_status, N_("include status in commit message template"),
+		           "commit.status", parse_opt_confkey_bool),
 		{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
 		  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 		/* end commit message options */
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 25e54ad3ed..6ca258eec3 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -36,11 +36,6 @@ static int difftool_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	if (!strcmp(var, "difftool.trustexitcode")) {
-		trust_exit_code = git_config_bool(var, value);
-		return 0;
-	}
-
 	return git_default_config(var, value, cb);
 }
 
@@ -680,9 +675,10 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "tool-help", &tool_help,
 			 N_("print a list of diff tools that may be used with "
 			    "`--tool`")),
-		OPT_BOOL(0, "trust-exit-code", &trust_exit_code,
-			 N_("make 'git-difftool' exit when an invoked diff "
-			    "tool returns a non - zero exit code")),
+		OPT_BOOL_C(0, "trust-exit-code", &trust_exit_code,
+			   N_("make 'git-difftool' exit when an invoked diff "
+			      "tool returns a non - zero exit code"),
+			   "difftool.trustexitcode", parse_opt_confkey_bool),
 		OPT_STRING('x', "extcmd", &extcmd, N_("<command>"),
 			   N_("specify a custom command for viewing diffs")),
 		OPT_END()
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b5ad09d046..084019cf93 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -69,6 +69,7 @@ static int option_parse_recurse_submodules(const struct option *opt,
 
 static int git_fetch_config(const char *k, const char *v, void *cb)
 {
+	/* TODO: The --prune option. Non-trivial to migrate due to tri-state interaction. Debug it */
 	if (!strcmp(k, "fetch.prune")) {
 		fetch_prune_config = git_config_bool(k, v);
 		return 0;
diff --git a/builtin/grep.c b/builtin/grep.c
index 837836fb3e..3fee3ac121 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -284,6 +284,7 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)
 	if (git_color_default_config(var, value, cb) < 0)
 		st = -1;
 
+	/* TODO: The --threads option (int) */
 	if (!strcmp(var, "grep.threads")) {
 		num_threads = git_config_int(var, value);
 		if (num_threads < 0)
diff --git a/builtin/help.c b/builtin/help.c
index 49f7a07f85..790a934a46 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -251,6 +251,7 @@ static int git_help_config(const char *var, const char *value, void *cb)
 {
 	if (starts_with(var, "column."))
 		return git_column_config(var, value, "help", &colopts);
+	/* TODO: Corresponds to multiple options. I.e. --man, --html etc.*/
 	if (!strcmp(var, "help.format")) {
 		if (!value)
 			return config_error_nonbool(var);
diff --git a/builtin/log.c b/builtin/log.c
index 670229cbb4..bf6dbb3c94 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -390,6 +390,7 @@ static int git_log_config(const char *var, const char *value, void *cb)
 {
 	const char *slot_name;
 
+	/* TODO: Hard to untangle the below, but it's all/most overrides for one option or other */
 	if (!strcmp(var, "format.pretty"))
 		return git_config_string(&fmt_pretty, var, value);
 	if (!strcmp(var, "format.subjectprefix"))
diff --git a/builtin/merge.c b/builtin/merge.c
index 7554b8d412..544b616f77 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -557,6 +557,7 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 		return 0;
 	}
 
+	/* TODO: --stat, documented in git-config. This has *no* tests (bool) */
 	if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
 		show_diffstat = git_config_bool(k, v);
 	else if (!strcmp(k, "pull.twohead"))
@@ -565,6 +566,7 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 		return git_config_string(&pull_octopus, k, v);
 	else if (!strcmp(k, "merge.renormalize"))
 		option_renormalize = git_config_bool(k, v);
+	/* TODO: --ff option, bool + custom values (bool + str) */
 	else if (!strcmp(k, "merge.ff")) {
 		int boolval = git_config_maybe_bool(k, v);
 		if (0 <= boolval) {
diff --git a/builtin/notes.c b/builtin/notes.c
index 0513f7455d..248a8cd714 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -834,6 +834,7 @@ static int merge(int argc, const char **argv, const char *prefix)
 		strbuf_addf(&merge_key, "notes.%s.mergeStrategy", short_ref);
 
 		if (git_config_get_notes_strategy(merge_key.buf, &o.strategy))
+			/* TODO: The -s option (str) */
 			git_config_get_notes_strategy("notes.mergeStrategy", &o.strategy);
 
 		strbuf_release(&merge_key);
diff --git a/builtin/pull.c b/builtin/pull.c
index 3ecb881b0b..65ed5c3a47 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -36,6 +36,7 @@ enum rebase_type {
 static enum rebase_type parse_config_rebase(const char *key, const char *value,
 		int fatal)
 {
+	/* TODO: The --rebase[=...] option (bool & str) */
 	int v = git_config_maybe_bool("pull.rebase", value);
 
 	if (!v)
@@ -271,6 +272,7 @@ static const char *config_get_ff(void)
 	if (git_config_get_value("pull.ff", &value))
 		return NULL;
 
+	/* TODO: The --ff option (bool?) */
 	switch (git_config_maybe_bool("pull.ff", value)) {
 	case 0:
 		return "--no-ff";
diff --git a/builtin/push.c b/builtin/push.c
index 5c22e9f2e5..947737c28a 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -471,12 +471,14 @@ static int git_push_config(const char *k, const char *v, void *cb)
 	if (status)
 		return status;
 
+	/* TODO: The --follow-tags option (bool?) */
 	if (!strcmp(k, "push.followtags")) {
 		if (git_config_bool(k, v))
 			*flags |= TRANSPORT_PUSH_FOLLOW_TAGS;
 		else
 			*flags &= ~TRANSPORT_PUSH_FOLLOW_TAGS;
 		return 0;
+	/* TODO: The --sign option? (bool & str) */
 	} else if (!strcmp(k, "push.gpgsign")) {
 		const char *value;
 		if (!git_config_get_value("push.gpgsign", &value)) {
@@ -494,6 +496,7 @@ static int git_push_config(const char *k, const char *v, void *cb)
 					return error("Invalid value for '%s'", k);
 			}
 		}
+	/* TODO: The --recurse-submodules option (bool & str?) */
 	} else if (!strcmp(k, "push.recursesubmodules")) {
 		const char *value;
 		if (!git_config_get_value("push.recursesubmodules", &value))
diff --git a/builtin/repack.c b/builtin/repack.c
index 677bc7c81a..ff3a0be619 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -26,14 +26,17 @@ static const char incremental_bitmap_conflict_error[] = N_(
 
 static int repack_config(const char *var, const char *value, void *cb)
 {
+	/* TODO: The --delta-base-offset option passed to pack-objects (bool) */
 	if (!strcmp(var, "repack.usedeltabaseoffset")) {
 		delta_base_offset = git_config_bool(var, value);
 		return 0;
 	}
+	/* TODO: The --pack-kept-objects option, not documented in the git-repack manpage, just git-config (bool) */
 	if (!strcmp(var, "repack.packkeptobjects")) {
 		pack_kept_objects = git_config_bool(var, value);
 		return 0;
 	}
+	/* TODO: The --write-bitmap-index option (bool). Has synonym */
 	if (!strcmp(var, "repack.writebitmaps") ||
 	    !strcmp(var, "pack.writebitmaps")) {
 		write_bitmaps = git_config_bool(var, value);
diff --git a/builtin/tag.c b/builtin/tag.c
index ad29be6923..8f5621b72f 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -165,6 +165,7 @@ static int git_tag_config(const char *var, const char *value, void *cb)
 	int status;
 	struct ref_sorting **sorting_tail = (struct ref_sorting **)cb;
 
+	/* TODO: The --sort option (bool / str) */
 	if (!strcmp(var, "tag.sort")) {
 		if (!value)
 			return config_error_nonbool(var);
@@ -175,10 +176,6 @@ static int git_tag_config(const char *var, const char *value, void *cb)
 	status = git_gpg_config(var, value, cb);
 	if (status)
 		return status;
-	if (!strcmp(var, "tag.forcesignannotated")) {
-		force_sign_annotate = git_config_bool(var, value);
-		return 0;
-	}
 
 	if (starts_with(var, "column."))
 		return git_column_config(var, value, "tag", &colopts);
@@ -379,6 +376,17 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
 	return check_refname_format(sb->buf, 0);
 }
 
+static int parse_opt_confkey_sign_bool(const struct option *opt, const char *arg, int unset) {
+	const char *value;
+
+	if (git_config_get_value(opt->conf_key, &value))
+		return 0;
+
+	force_sign_annotate = git_config_bool(opt->conf_key, value);
+
+	return 0;
+}
+
 int cmd_tag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -413,7 +421,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK('m', "message", &msg, N_("message"),
 			     N_("tag message"), parse_msg_arg),
 		OPT_FILENAME('F', "file", &msgfile, N_("read message from file")),
-		OPT_BOOL('s', "sign", &opt.sign, N_("annotated and GPG-signed tag")),
+		OPT_BOOL_C('s', "sign", &opt.sign, N_("annotated and GPG-signed tag"),
+		           "tag.forcesignannotated", parse_opt_confkey_sign_bool),
 		OPT_STRING(0, "cleanup", &cleanup_arg, N_("mode"),
 			N_("how to strip spaces and #comments from message")),
 		OPT_STRING('u', "local-user", &keyid, N_("key-id"),
diff --git a/parse-options-cb.c b/parse-options-cb.c
index b7d8f7dcb2..9b6d967cf5 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -236,3 +236,65 @@ int parse_opt_passthru_argv(const struct option *opt, const char *arg, int unset
 
 	return 0;
 }
+
+/* Does it suck that I have to use the cache interface to the config
+ * here? Should we somehow unroll this whole thing so
+ * parse_options_step loops over the config values, and maybe
+ * populates opt->conf_key (which we'd need to add) for all the
+ * options that need it?
+ *
+ * I.e. should we make this more complex because this one-shot
+ * interface is expensive, or is it just fine?
+*/ 
+
+int parse_opt_confkey_bool(const struct option *opt, const char *arg, int unset) {
+	const char *value;
+
+	if (git_config_get_value(opt->conf_key, &value))
+		return 0;
+
+	*(int *)opt->value = git_config_bool(opt->conf_key, value);
+
+	trace_printf("getopt/parse_opt_confkey_bool: Parsed bool value for %s got %d\n", opt->long_name, *(int*)opt->value);
+
+	return 0;
+}
+
+int parse_opt_confkey_bool_neg(const struct option *opt, const char *arg, int unset) {
+	const char *value;
+
+	if (git_config_get_value(opt->conf_key, &value))
+		return 0;
+
+	*(int *)opt->value = !git_config_bool(opt->conf_key, value);
+
+	trace_printf("getopt/parse_opt_confkey_bool_neg: Parsed bool value for %s got %d\n", opt->long_name, *(int*)opt->value);
+
+	return 0;
+}
+
+int parse_opt_confkey_string(const struct option *opt, const char *arg, int unset) {
+	const char *value;
+
+	if (git_config_get_value(opt->conf_key, &value))
+		return 0;
+
+	git_config_string((const char **)opt->value, opt->conf_key, value);
+
+	trace_printf("getopt/parse_opt_confkey_string: Parsed string value for %s got %s\n", opt->long_name, *(char**)opt->value);
+
+	return 0;
+}
+
+int parse_opt_confkey_pathname(const struct option *opt, const char *arg, int unset) {
+	const char *value;
+
+	if (git_config_get_value(opt->conf_key, &value))
+		return 0;
+
+	git_config_pathname((const char **)opt->value, opt->conf_key, value);
+
+	trace_printf("getopt/parse_opt_confkey_pathname: Parsed pathname value for %s got %s\n", opt->long_name, *(char**)opt->value);
+
+	return 0;
+}
diff --git a/parse-options.c b/parse-options.c
index a23a1e67f0..b7bd0b950e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -3,11 +3,40 @@
 #include "cache.h"
 #include "commit.h"
 #include "color.h"
+#include "hashmap.h"
 #include "utf8.h"
 
 #define OPT_SHORT 1
 #define OPT_UNSET 2
 
+struct option_hash_entry
+{
+	struct hashmap_entry ent; /* must be the first member! */
+
+	char option[FLEX_ARRAY]; /* NUL-terminated canonical option name: */
+};
+
+static int option_hash_cmp(const void *entry, const void *entry_or_key,
+			   const void *keydata)
+{
+	const struct option_hash_entry *e1 = entry, *e2 = entry_or_key;
+	const char *option = keydata ? keydata : e2->option;
+
+	return strcmp(e1->option, option);
+}
+
+/* TODO: This is just something I copied from 7d4558c462, but I could
+ * skip this alloc indirection since I'm malloc-ing the key with
+ * xstrfmt */
+static struct option_hash_entry *alloc_option_hash_entry(const char *option)
+{
+	struct option_hash_entry *entry;
+
+	FLEX_ALLOC_STR(entry, option, option);
+	hashmap_entry_init(entry, strhash(option));
+	return entry;
+}
+
 int optbug(const struct option *opt, const char *reason)
 {
 	if (opt->long_name) {
@@ -200,15 +229,24 @@ static int get_value(struct parse_opt_ctx_t *p,
 	}
 }
 
-static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *options)
+static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *options,
+                           struct hashmap *options_map)
 {
 	const struct option *all_opts = options;
 	const struct option *numopt = NULL;
+	int ret;
+	char *hkey = NULL;
 
 	for (; options->type != OPTION_END; options++) {
 		if (options->short_name == *p->opt) {
 			p->opt = p->opt[1] ? p->opt + 1 : NULL;
-			return get_value(p, options, all_opts, OPT_SHORT);
+			ret = get_value(p, options, all_opts, OPT_SHORT);
+
+			if (!ret && options->flags & PARSE_OPT_CONFIGURABLE) {
+				hkey = xstrfmt("%d:%s", options->short_name, options->long_name);
+				hashmap_put(options_map, alloc_option_hash_entry(hkey));
+			}
+			return ret;
 		}
 
 		/*
@@ -228,6 +266,10 @@ static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *optio
 		arg = xmemdupz(p->opt, len);
 		p->opt = p->opt[len] ? p->opt + len : NULL;
 		rc = (*numopt->callback)(numopt, arg, 0) ? (-1) : 0;
+		if (!rc && numopt->flags & PARSE_OPT_CONFIGURABLE) {
+			hkey = xstrfmt("%d:%s", numopt->short_name, numopt->long_name);
+			hashmap_put(options_map, alloc_option_hash_entry(hkey));
+		}
 		free(arg);
 		return rc;
 	}
@@ -235,12 +277,15 @@ static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *optio
 }
 
 static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
-                          const struct option *options)
+                          const struct option *options,
+                          struct hashmap *options_map)
 {
 	const struct option *all_opts = options;
 	const char *arg_end = strchrnul(arg, '=');
 	const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
 	int abbrev_flags = 0, ambiguous_flags = 0;
+	int ret;
+	char *hkey = NULL;
 
 	for (; options->type != OPTION_END; options++) {
 		const char *rest, *long_name = options->long_name;
@@ -313,7 +358,14 @@ 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);
+		ret = get_value(p, options, all_opts, flags ^ opt_flags);
+		if (!ret && options->flags & PARSE_OPT_CONFIGURABLE) {
+			/* TODO: This leaks memory. See:
+			   valgrind --tool=memcheck --leak-check=yes ./git commit --status */
+			hkey = xstrfmt("%d:%s", options->short_name, options->long_name);
+			hashmap_put(options_map, alloc_option_hash_entry(hkey));
+		}
+		return ret;
 	}
 
 	if (ambiguous_option)
@@ -330,15 +382,24 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
 }
 
 static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,
-			    const struct option *options)
+			    const struct option *options,
+			    struct hashmap *options_map)
 {
 	const struct option *all_opts = options;
+	int ret;
+	char *hkey = NULL;
 
 	for (; options->type != OPTION_END; options++) {
 		if (!(options->flags & PARSE_OPT_NODASH))
 			continue;
-		if (options->short_name == arg[0] && arg[1] == '\0')
-			return get_value(p, options, all_opts, OPT_SHORT);
+		if (options->short_name == arg[0] && arg[1] == '\0') {
+			ret = get_value(p, options, all_opts, OPT_SHORT);
+			if (!ret && options->flags & PARSE_OPT_CONFIGURABLE) {
+				hkey = xstrfmt("%d:%s", options->short_name, options->long_name);
+				hashmap_put(options_map, alloc_option_hash_entry(hkey));
+			}
+			return ret;
+		}
 	}
 	return -2;
 }
@@ -434,6 +495,21 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 {
 	int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);
 	int err = 0;
+	struct hashmap options_map;
+	struct option_hash_entry *entry;
+	char *hkey = NULL;
+
+	/* TODO: This hashmap init would be so much less painful in
+	 * parse_options(), but blame/shortlog/update-index call
+	 * parse_options_step() directly. Maybe we should just
+	 * simplify this and say they can't use this config interface
+	 * unless they're refactored to behave like everything else
+	 *
+	 * TODO: Does I think hashmap_free(..., 1) doesn't free my
+	 * xstrfmt'd strings. Do I need to iter over the hashmap to
+	 * properly free it?
+	 */
+	hashmap_init(&options_map, option_hash_cmp, 0);
 
 	/* we must reset ->opt, unknown short option leave it dangling */
 	ctx->opt = NULL;
@@ -442,10 +518,12 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 		const char *arg = ctx->argv[0];
 
 		if (*arg != '-' || !arg[1]) {
-			if (parse_nodash_opt(ctx, arg, options) == 0)
+			if (parse_nodash_opt(ctx, arg, options, &options_map) == 0)
 				continue;
-			if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)
+			if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION) {
+				hashmap_free(&options_map, 1);
 				return PARSE_OPT_NON_OPTION;
+			}
 			ctx->out[ctx->cpidx++] = ctx->argv[0];
 			continue;
 		}
@@ -456,7 +534,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 
 		if (arg[1] != '-') {
 			ctx->opt = arg + 1;
-			switch (parse_short_opt(ctx, options)) {
+			switch (parse_short_opt(ctx, options, &options_map)) {
 			case -1:
 				goto show_usage_error;
 			case -2:
@@ -469,7 +547,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			if (ctx->opt)
 				check_typos(arg + 1, options);
 			while (ctx->opt) {
-				switch (parse_short_opt(ctx, options)) {
+				switch (parse_short_opt(ctx, options, &options_map)) {
 				case -1:
 					goto show_usage_error;
 				case -2:
@@ -497,11 +575,13 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			break;
 		}
 
-		if (internal_help && !strcmp(arg + 2, "help-all"))
+		if (internal_help && !strcmp(arg + 2, "help-all")) {
+			hashmap_free(&options_map, 1);
 			return usage_with_options_internal(ctx, usagestr, options, 1, 0);
+		}
 		if (internal_help && !strcmp(arg + 2, "help"))
 			goto show_usage;
-		switch (parse_long_opt(ctx, arg + 2, options)) {
+		switch (parse_long_opt(ctx, arg + 2, options, &options_map)) {
 		case -1:
 			goto show_usage_error;
 		case -2:
@@ -509,16 +589,46 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 		}
 		continue;
 unknown:
-		if (!(ctx->flags & PARSE_OPT_KEEP_UNKNOWN))
+		if (!(ctx->flags & PARSE_OPT_KEEP_UNKNOWN)) {
+			hashmap_free(&options_map, 1);
 			return PARSE_OPT_UNKNOWN;
+		}
 		ctx->out[ctx->cpidx++] = ctx->argv[0];
 		ctx->opt = NULL;
 	}
+
+	/* 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;
+
+		/* In some cases options->long_name is null, then we
+		 * just use :(null) as part of the key, it's
+		 * harmless
+		 */
+		hkey = xstrfmt("%d:%s", options->short_name, options->long_name);
+		entry = hashmap_get_from_hash(&options_map, strhash(hkey), hkey);
+		if (entry) {
+			free(hkey);
+			continue;
+		}
+
+		trace_printf("getopt/parse_options_step: Calling callback for configurable option %s\n", options->long_name);
+		if ((*options->conf_callback)(options, NULL, 1))
+			trace_printf("getopt/parse_options_step: Callback for configurable option %s gave us a value\n", options->long_name);
+		else
+			trace_printf("getopt/parse_options_step: Callback for configurable option %s gave us NO value\n", options->long_name);
+	}
+
+	hashmap_free(&options_map, 1);
 	return PARSE_OPT_DONE;
 
  show_usage_error:
 	err = 1;
  show_usage:
+	hashmap_free(&options_map, 1);
 	return usage_with_options_internal(ctx, usagestr, options, 0, err);
 }
 
diff --git a/parse-options.h b/parse-options.h
index dcd8a0926c..8d048c74bd 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -38,7 +38,8 @@ enum parse_opt_option_flags {
 	PARSE_OPT_LASTARG_DEFAULT = 16,
 	PARSE_OPT_NODASH = 32,
 	PARSE_OPT_LITERAL_ARGHELP = 64,
-	PARSE_OPT_SHELL_EVAL = 256
+	PARSE_OPT_SHELL_EVAL = 256,
+	PARSE_OPT_CONFIGURABLE = 512
 };
 
 struct option;
@@ -110,6 +111,9 @@ struct option {
 	int flags;
 	parse_opt_cb *callback;
 	intptr_t defval;
+
+	const char *conf_key;
+	parse_opt_cb *conf_callback;
 };
 
 #define OPT_END()                   { OPTION_END }
@@ -122,9 +126,17 @@ struct option {
 				      (h), PARSE_OPT_NOARG, NULL, (b) }
 #define OPT_COUNTUP(s, l, v, h)     { OPTION_COUNTUP, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG }
+#define OPT_COUNTUP_C(s, l, v, h, ck, cb) \
+				      { OPTION_COUNTUP, (s), (l), (v), NULL, \
+				      (h), PARSE_OPT_NOARG | PARSE_OPT_CONFIGURABLE, \
+				      NULL, 0, ck, cb }
 #define OPT_SET_INT(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG, NULL, (i) }
+#define OPT_SET_INT_C(s, l, v, h, i, ck, cb) \
+				    { OPTION_SET_INT, (s), (l), (v), NULL, \
+				      (h), PARSE_OPT_NOARG | PARSE_OPT_CONFIGURABLE, NULL, (i), ck, cb }
 #define OPT_BOOL(s, l, v, h)        OPT_SET_INT(s, l, v, h, 1)
+#define OPT_BOOL_C(s, l, v, h, ck, cb) OPT_SET_INT_C(s, l, v, h, 1, ck, cb)
 #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}
 #define OPT_CMDMODE(s, l, v, h, i)  { OPTION_CMDMODE, (s), (l), (v), NULL, \
@@ -133,6 +145,8 @@ struct option {
 #define OPT_MAGNITUDE(s, l, v, h)   { OPTION_MAGNITUDE, (s), (l), (v), \
 				      N_("n"), (h), PARSE_OPT_NONEG }
 #define OPT_STRING(s, l, v, a, h)   { OPTION_STRING,  (s), (l), (v), (a), (h) }
+#define OPT_STRING_C(s, l, v, a, h, ck, cb)   { OPTION_STRING,  (s), (l), (v), (a), (h), \
+			                        PARSE_OPT_CONFIGURABLE, NULL, 0, ck, cb }
 #define OPT_STRING_LIST(s, l, v, a, h) \
 				    { OPTION_CALLBACK, (s), (l), (v), (a), \
 				      (h), 0, &parse_opt_string_list }
@@ -151,6 +165,9 @@ struct option {
 	  PARSE_OPT_NOARG | PARSE_OPT_NONEG, (f) }
 #define OPT_FILENAME(s, l, v, h)    { OPTION_FILENAME, (s), (l), (v), \
 				       N_("file"), (h) }
+#define OPT_FILENAME_C(s, l, v, h, ck, cb) { OPTION_FILENAME, (s), (l), (v), \
+			                     N_("file"), (h), PARSE_OPT_NOARG | PARSE_OPT_CONFIGURABLE, \
+			                     NULL, 0, ck, cb }
 #define OPT_COLOR_FLAG(s, l, v, h) \
 	{ OPTION_CALLBACK, (s), (l), (v), N_("when"), (h), PARSE_OPT_OPTARG, \
 		parse_opt_color_flag_cb, (intptr_t)"always" }
@@ -231,8 +248,13 @@ extern int parse_opt_noop_cb(const struct option *, const char *, int);
 extern int parse_opt_unknown_cb(const struct option *, const char *, int);
 extern int parse_opt_passthru(const struct option *, const char *, int);
 extern int parse_opt_passthru_argv(const struct option *, const char *, int);
+extern int parse_opt_confkey_bool(const struct option *, const char *, int);
+extern int parse_opt_confkey_bool_neg(const struct option *, const char *, int);
+extern int parse_opt_confkey_string(const struct option *, const char *, int);
+extern int parse_opt_confkey_pathname(const struct option *, const char *, int);
 
 #define OPT__VERBOSE(var, h)  OPT_COUNTUP('v', "verbose", (var), (h))
+#define OPT__VERBOSE_C(var, h, ck, cb)  OPT_COUNTUP_C('v', "verbose", (var), (h), ck, cb)
 #define OPT__QUIET(var, h)    OPT_COUNTUP('q', "quiet",   (var), (h))
 #define OPT__VERBOSITY(var) \
 	{ OPTION_CALLBACK, 'v', "verbose", (var), NULL, N_("be more verbose"), \
@@ -241,6 +263,8 @@ extern int parse_opt_passthru_argv(const struct option *, const char *, int);
 	  PARSE_OPT_NOARG, &parse_opt_verbosity_cb, 0 }
 #define OPT__DRY_RUN(var, h)  OPT_BOOL('n', "dry-run", (var), (h))
 #define OPT__FORCE(var, h)    OPT_COUNTUP('f', "force",   (var), (h))
+#define OPT__FORCE_C(var, h, ck, cb) \
+	OPT_COUNTUP_C('f', "force", (var), (h), ck, cb)
 #define OPT__ABBREV(var)  \
 	{ OPTION_CALLBACK, 0, "abbrev", (var), N_("n"),	\
 	  N_("use <n> digits to display SHA-1s"),	\
-- 
2.11.0

Re: Add configuration options for some commonly used command-line options

[Brandon McCaig]

[-- Attachment #1: Type: text/plain, Size: 1250 bytes --]

On Mon, Mar 20, 2017 at 02:18:01PM -0400, Jeff King wrote:
> I think we've had similar proposals in the form of an
> environment variable like "GIT_PLUMBING" (and your "command",
> which I do like syntactically, would probably just end up
> setting such an environment variable anyway).

For reference, Mercurial has long had HGPLAIN with a similar
purpose. See `hg help scripting'. I think that this is a
generally good idea to adopt. Albeit, I think that Mercurial
considers its command line a scriptable API, and HGPLAIN skips
over any customization of output. That may not be the exact
intention of this, but something related nevertheless.

Instead of a subcommand I think that a command line --option
would make better sense. It would only even save a few characters
in *nix shells where the variable can be inlined, but could be
more practical for MS Windows users that would need separate
commands to manage the variable.

Regards,


-- 
Brandon McCaig <bamccaig@gmail.com> <bambams@castopulence.org>
Castopulence Software <https://www.castopulence.org/>
Blog <http://www.bambams.ca/>
perl -E '$_=q{V zrna gur orfg jvgu jung V fnl. }.
q{Vg qbrfa'\''g nyjnlf fbhaq gung jnl.};
tr/A-Ma-mN-Zn-z/N-Zn-zA-Ma-m/;say'


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]