Re: [PATCH v7 1/2] builtin/config.c: treat type specifiers singularly

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Apr 9, 2018 at 10:12 PM, Taylor Blau <me@ttaylorr.com> wrote:
> On Tue, Apr 10, 2018 at 10:22:25AM +0900, Junio C Hamano wrote:
>> I suspect that it may be OK to switch to last-one-wins, but then we
>> should give a justification that is a bit stronger than "we want to
>> avoid complaining against --int --type=int" (i.e. "we want to switch
>> to last-one-wins for such and such reasons").
>
> I think that the major justification is to treat --type=int as a _true_
> synonym of --int, such that neither `--type=<t1> --type=<t2>` nor
> `--<t1> --<t2>` will complain. This, as well as the fact that
> OPT_SET_BIT brings us closer to the semantics of `--verbose=1
> --verbose=2`, which is something that Eric had mentioned above.

I'm probably being dense, but even after reading this paragraph
several times, I still don't have a good idea as to what it is trying
to say.

As for my earlier reference to '--verbose=1 --verbose=2', that was
cited merely as a "last wins" which I could buy; it was offered in
contrast to '--type=bool --type=int' "last wins" which this patch
tries to sell, and which I have a tough time buying (though I defer to
Junio's and Peff's judgments).

This patch (or perhaps its commit message) seems to conflate two
independent goals. (1) ridding this code of OPT_BIT() since its use in
this context is not very sensible, and (2) trying to sell "last wins"
(for a not well justified argument) to support '--int --type=int'
without complaint.

Goal #1 makes plenty sense; no objection to that. Goal #2 isn't so
obviously desirable (I already raised objections to the more general
'--bool --type=int' "last wins" it implements).

> I think that OPT_CMDMODE would not work quite in the way we desire,
> since the error messages would not quite line up with the command typed.
> For instance, after applying the following diff:
>
>      OPT_CALLBACK('t', "type", &type, N_("type"), N_("value is given this type"), option_parse_type),
> -    OPT_SET_INT(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL),
> -    OPT_SET_INT(0, "int", &type, N_("value is decimal number"), TYPE_INT),
> -    OPT_SET_INT(0, "bool-or-int", &type, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
> -    OPT_SET_INT(0, "path", &type, N_("value is a path (file or directory name)"), TYPE_PATH),
> -    OPT_SET_INT(0, "expiry-date", &type, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
> +    OPT_CMDMODE(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL),
> +    OPT_CMDMODE(0, "int", &type, N_("value is decimal number"), TYPE_INT),
> +    OPT_CMDMODE(0, "bool-or-int", &type, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
> +    OPT_CMDMODE(0, "path", &type, N_("value is a path (file or directory name)"), TYPE_PATH),
> +    OPT_CMDMODE(0, "expiry-date", &type, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
>
>   ~/g/git (tb/config-type-specifier-option!) $ ./git-config --type=int --bool foo.bar
>   error: option `bool' : incompatible with --int
>
> Whereas I would expect that to say:
>
>   error: option `bool' is incompatible with `--type=int'.

Couldn't you achieve reasonable output (such as "error: conflicting
types requested: %s and %s") by handling all those --<type>s with a
custom callback? You've already coded such a callback for
--type=<type>, and it doesn't look like it would be much more effort
to refactor it a bit to handle --<type> as well.