On Fri, Mar 30, 2018 at 01:28:29AM -0400, Taylor Blau wrote: > 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 I think you could just end here, since... > Where--although the legacy and modern type specifier (`--int`, and > `--type`, respectively) do not conflict--we would store the value of > `--type=` in a `char *` and the `--int` as a bit in the `int` bitset. > > In the above, when error-checking `if (types != && type_str)`, we do not > check additionally whether or not these types are the same, and simply > complain immediately. This change makes `--int` and `--type=int` a true > synonym of each other, and removes the need for additional complexity, > as above in the conditional. None of this type_str stuff exists yet, and you've sufficiently motivated the change. > Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx> > --- > builtin/config.c | 39 +++++++++++++++++---------------------- > t/t1300-repo-config.sh | 11 +++++++++++ > 2 files changed, 28 insertions(+), 22 deletions(-) The patch mostly looks good. We probably want to also rewrite the TYPE_* #defines to be sequential, since somebody may scratch their head wondering why we use one bit per type when we do not treat them as a bitfield. > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh > index 4f8e6f5fd..aa45b9267 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 > +' Minor style nit: we prefer ">expect" with no space. Though t1300 is certainly a cornucopia of style violations already. I could buy it under the guise of matching existing style if there wasn't a counter-example directly above. :) We also prefer to put that "cat >.git/config" inside a test block, though I'm pretty sure that _is_ following existing style in the test. -Peff
