git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* Add configuration options for some commonly used command-line options (Was: [RFH] GSoC 2015 application)
@ 2017-03-19  9:57 Duy Nguyen
  2017-03-19 10:15 ` Re: Add configuration options for some commonly used command-line options Matthieu Moy
  0 siblings, 1 reply; 16+ messages in thread
From: Duy Nguyen @ 2017-03-19  9:57 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: Jeff King, Git Mailing List

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

^ permalink raw reply	[flat|threaded] 16+ messages in thread

* Re: Add configuration options for some commonly used command-line options
  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 ` Matthieu Moy
  2017-03-19 13:18   ` brian m. carlson
  0 siblings, 1 reply; 16+ messages in thread
From: Matthieu Moy @ 2017-03-19 10:15 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Jeff King, Git Mailing List

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/

^ permalink raw reply	[flat|threaded] 16+ messages in thread

* Re: Add configuration options for some commonly used command-line options
  2017-03-19 10:15 ` Re: 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:42     ` Duy Nguyen
  0 siblings, 2 replies; 16+ messages in thread
From: brian m. carlson @ 2017-03-19 13:18 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: Duy Nguyen, Jeff King, Git Mailing List

[-- 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 --]

^ permalink raw reply	[flat|threaded] 16+ messages in thread

* Re: Add configuration options for some commonly used command-line options
  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-24 23:10       ` [PATCH/RFC] parse-options: add facility to make options configurable Ævar Arnfjörð Bjarmason
  2017-03-20 10:42     ` Duy Nguyen
  1 sibling, 2 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-19 13:43 UTC (permalink / raw)
  To: brian m. carlson, Matthieu Moy, Duy Nguyen, Jeff King, Git Mailing List

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.

^ permalink raw reply	[flat|threaded] 16+ messages in thread

* Re: Add configuration options for some commonly used command-line options
  2017-03-19 13:18   ` brian m. carlson
  2017-03-19 13:43     ` Ævar Arnfjörð Bjarmason
@ 2017-03-20 10:42     ` Duy Nguyen
  1 sibling, 0 replies; 16+ messages in thread
From: Duy Nguyen @ 2017-03-20 10:42 UTC (permalink / raw)
  To: brian m. carlson, Matthieu Moy, Duy Nguyen, Jeff King, Git Mailing List

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

^ permalink raw reply	[flat|threaded] 16+ messages in thread

* Re: Add configuration options for some commonly used command-line options
  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-24 23:10       ` [PATCH/RFC] parse-options: add facility to make options configurable Ævar Arnfjörð Bjarmason
  1 sibling, 1 reply; 16+ messages in thread
From: Duy Nguyen @ 2017-03-20 10:56 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: brian m. carlson, Matthieu Moy, Jeff King, Git Mailing List

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

^ permalink raw reply	[flat|threaded] 16+ messages in thread

* Re: Add configuration options for some commonly used command-line options
  2017-03-20 10:56       ` Duy Nguyen
@ 2017-03-20 17:32         ` Brandon Williams
  2017-03-20 18:18           ` Jeff King
  2017-03-20 18:56           ` Junio C Hamano
  0 siblings, 2 replies; 16+ messages in thread
From: Brandon Williams @ 2017-03-20 17:32 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Ævar Arnfjörð Bjarmason, brian m. carlson, Matthieu Moy, Jeff King, Git Mailing List

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

^ permalink raw reply	[flat|threaded] 16+ messages in thread

* Re: Add configuration options for some commonly used command-line options
  2017-03-20 17:32         ` Brandon Williams
@ 2017-03-20 18:18           ` Jeff King
  2017-03-20 18:56           ` Junio C Hamano
  1 sibling, 0 replies; 16+ messages in thread
From: Jeff King @ 2017-03-20 18:18 UTC (permalink / raw)
  To: Brandon Williams; +Cc: Duy Nguyen, Ævar Arnfjörð Bjarmason, brian m. carlson, Matthieu Moy, Git Mailing List

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

^ permalink raw reply	[flat|threaded] 16+ messages in thread

* Re: Add configuration options for some commonly used command-line options
  2017-03-20 17:32         ` Brandon Williams
  2017-03-20 18:18           ` Jeff King
@ 2017-03-20 18:56           ` Junio C Hamano
  2017-03-20 19:14             ` Jeff King
  2017-03-20 21:57             ` Ævar Arnfjörð Bjarmason
  1 sibling, 2 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-03-20 18:56 UTC (permalink / raw)
  To: Brandon Williams; +Cc: Duy Nguyen, Ævar Arnfjörð Bjarmason, brian m. carlson, Matthieu Moy, Jeff King, Git Mailing List

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.


^ permalink raw reply	[flat|threaded] 16+ messages in thread

* Re: Add configuration options for some commonly used command-line options
  2017-03-20 18:56           ` Junio C Hamano
@ 2017-03-20 19:14             ` Jeff King
  2017-03-20 21:57             ` Ævar Arnfjörð Bjarmason
  1 sibling, 0 replies; 16+ messages in thread
From: Jeff King @ 2017-03-20 19:14 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Brandon Williams, Duy Nguyen, Ævar Arnfjörð Bjarmason, brian m. carlson, Matthieu Moy, Git Mailing List

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

^ permalink raw reply	[flat|threaded] 16+ messages in thread

* Re: Add configuration options for some commonly used command-line options
  2017-03-20 18:56           ` Junio C Hamano
  2017-03-20 19:14             ` Jeff King
@ 2017-03-20 21:57             ` Ævar Arnfjörð Bjarmason
  1 sibling, 0 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-20 21:57 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Brandon Williams, Duy Nguyen, brian m. carlson, Matthieu Moy, Jeff King, Git Mailing List

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.

^ permalink raw reply	[flat|threaded] 16+ messages in thread

* [PATCH/RFC] parse-options: add facility to make options configurable
  2017-03-19 13:43     ` Ævar Arnfjörð Bjarmason
  2017-03-20 10:56       ` Duy Nguyen
@ 2017-03-24 23:10       ` Ævar Arnfjörð Bjarmason
  2017-03-25 16:47         ` Ævar Arnfjörð Bjarmason
  2017-03-25 21:28         ` brian m. carlson
  1 sibling, 2 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-24 23:10 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jeff King, brian m . carlson, Matthieu Moy, Duy Nguyen, Brandon Williams, Æ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


^ permalink raw reply	[flat|threaded] 16+ messages in thread

* Re: [PATCH/RFC] parse-options: add facility to make options configurable
  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
  2017-03-25 21:28         ` brian m. carlson
  1 sibling, 1 reply; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-25 16:47 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Jeff King, brian m . carlson, Matthieu Moy, Duy Nguyen, Brandon Williams, Æ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.

^ permalink raw reply	[flat|threaded] 16+ messages in thread

* Re: [PATCH/RFC] parse-options: add facility to make options configurable
  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:28         ` brian m. carlson
  1 sibling, 0 replies; 16+ messages in thread
From: brian m. carlson @ 2017-03-25 21:28 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano, Jeff King, Matthieu Moy, Duy Nguyen, Brandon Williams

[-- 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 --]

^ permalink raw reply	[flat|threaded] 16+ messages in thread

* Re: [PATCH/RFC] parse-options: add facility to make options configurable
  2017-03-25 16:47         ` Ævar Arnfjörð Bjarmason
@ 2017-03-25 21:31           ` Jeff King
  2017-03-25 22:32             ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 16+ messages in thread
From: Jeff King @ 2017-03-25 21:31 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Git Mailing List, Junio C Hamano, brian m . carlson, Matthieu Moy, Duy Nguyen, Brandon Williams

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

^ permalink raw reply	[flat|threaded] 16+ messages in thread

* Re: [PATCH/RFC] parse-options: add facility to make options configurable
  2017-03-25 21:31           ` Jeff King
@ 2017-03-25 22:32             ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-25 22:32 UTC (permalink / raw)
  To: Jeff King; +Cc: Git Mailing List, Junio C Hamano, brian m . carlson, Matthieu Moy, Duy Nguyen, Brandon Williams

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.

^ permalink raw reply	[flat|threaded] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 ` Re: 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-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
2017-03-25 22:32             ` Ævar Arnfjörð Bjarmason
2017-03-25 21:28         ` brian m. carlson
2017-03-20 10:42     ` Duy Nguyen

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