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

[Junio C Hamano <gitster@pobox.com>]

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").

> 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.