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

[Taylor Blau <me@ttaylorr.com>]

On Tue, Apr 10, 2018 at 10:22:25AM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > Internally, we represent `git config`'s type specifiers as a bitset
> > using OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique
> > allows for the representation of multiple type specifiers in the `int
> > types` field, but this multi-representation is left unused.
> >
> > 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
>
> The above does not exactly argue for adopting the last-one-wins
> semantics, and still leaves it unclear if we want to complain
> against
>
>     $ git config --bool --type=int
>
> Is it intentionally left vague if we want to (or not want to)
> complain when such a conflicting specification is given?
>
> We could keep the traditional behaviour of "only one type at a time"
> error and still move away from the bitset representation that does
> not make sense, if we wanted to.  Initialize the "type" variable to
> an unset value, and use a callback to ensure either the variable is
> set to the unset value, or the value being set is already in the
> variable.  I think if you use OPT_CMDMODE(), it would do all of that
> for you automatically.
>
> 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 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:

diff --git a/builtin/config.c b/builtin/config.c
index 5c8952a17c..d9b73b949a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -111,11 +111,11 @@ static struct option builtin_config_options[] = {
     OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
     OPT_GROUP(N_("Type")),
     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),
     OPT_GROUP(N_("Other")),
     OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
     OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),

The following occurs:

  ~/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'.

I am not sure whether amending the implementation of OPT_CMDMODE is something
that you're interested in here.

I can amend my patch to include this extra reasoning, if you think that
would be helpful.

> > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> > index 4f8e6f5fde..24de37d544 100755
> > --- a/t/t1300-repo-config.sh
> > +++ b/t/t1300-repo-config.sh
> > @@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' '
> >  	test_expect_code 128 nongit git config --local foo.bar
> >  '
> >
> > +cat >.git/config <<-\EOF &&
> > +[core]
> > +number = 10
> > +EOF
> > +
> > +test_expect_success 'later legacy specifiers are given precedence' '
> > +	git config --bool --int core.number >actual &&
> > +	echo 10 >expect &&
> > +	test_cmp expect actual
> > +'
>
> And this expects more than we gave justifications for in the
> proposed log message.  I do not think it is necessarily a bad idea
> to switch to last-one-wins, but if that is where we really want to
> go, the proposed log message is being misleading.  It is true that
> OPT_SET_INT is more suited to complain when two conflicting things
> are given than OPT_BIT, but this example actually tells us that you
> no longer want to catch an error to give conflicting requests.

Yes, since I'd like to be able to insert `--type=` and have it behave
the same way. Since `--type=bool --type=int` will not complain (and
instead adopt `--type=int` given that it comes later in the command
string), neither should `--bool --int`.


Thanks,
Taylor