Re: [PATCH v3 1/2] builtin/config.c: treat type specifiers singularly
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Apr 5, 2018, 01:53 UTC
- Message-ID
- <20180405015304.GB4671@syl.local>
- In-Reply-To
- <CAPig+cRNSYz_guBS9yNEXdAbfG+FWtvnzwsWRu0DRe0N_VkiOQ@mail.gmail.com>
On Wed, Apr 04, 2018 at 03:57:12AM -0400, Eric Sunshine wrote:
Show 38 quoted lines
> On Wed, Apr 4, 2018 at 2:07 AM, Taylor Blau <me@ttaylorr.com> wrote: > > [...] > > In fact, `git config` will not accept multiple type specifiers at a > > time, as indicated by: > > > > $ git config --int --bool some.section > > error: only one type at a time. > > > > This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type > > specifier, so that the above command would instead be valid, and a > > synonym of: > > > > $ git config --bool some.section > > > > This change is motivated by two urges: (1) it does not make sense to > > represent a singular type specifier internally as a bitset, only to > > complain when there are multiple bits in the set. `OPT_SET_INT` is more > > well-suited to this task than `OPT_BIT` is. (2) a future patch will > > introduce `--type=<type>`, and we would like not to complain in the > > following situation: > > > > $ git config --int --type=int > > I can understand "last wins" for related options such as "--verbose > --quiet" or "--verbose=1 --verbose=2", but I have a lot of trouble > buying the "last wins" argument in this context where there is no > semantic relationship between, say, --bool and --expiry-date. > Specifying conflicting options together is likely to be unintentional, > thus reporting it as an error seems sensible, so I'm not convinced > that patch's removal of that error is a good idea. > > I understand that you're doing this to avoid complaining about "--int > --type=int", but exactly how that case is supported should be an > implementation detail; it doesn't need to bleed into the UI as an > unnecessary and not-well-justified loosening of an age-old > restriction. There are other ways to support "--int --type=int" > without "last wins". Also, do we really need to support "--int > --type=int"? Is that an expected use-case?
This is a fair concern, though I view the problem slightly differently. I see this change as being motivated by the fact that we are adding "--type", not removing an age-old restriction.
Peff's motivation for this--as I understand it--is that "--type=int" should be a _true_ synonym for "--int". Adopting the old-style "OPT_SET_BIT" for this purpose makes "--type=int" and "--int" _mostly_ a synonym for one another, except that "--type=bool --type=int" will not complain, whereas "--bool --int" would.
Loosening this restriction, in my view, brings us closer (1) to the new semantics of multiple "--type"'s, and (2) to the existing semantics of "--verbose=1 --verbose=2" as you mentioned above.
I would like to hear Peff's take on this as well, since he suggested the patch originally, and would likely provide the clearest insight into this.
Thanks, Taylor