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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 10, 2018, 01:22 UTC
Message-ID: <xmqqy3hv4zzi.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180409224654.GB34547@syl.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 25 quoted lines

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

Show 18 quoted lines

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