Re: [PATCH v2 1/2] builtin/config.c: treat type specifiers singularly
- From
Jeff King <peff@peff.net>
- Date
- Mar 30, 2018, 13:41 UTC
- Message-ID
- <20180330134148.GC29568@sigill.intra.peff.net>
- In-Reply-To
- <20180330052830.57251-1-me@ttaylorr.com>
On Fri, Mar 30, 2018 at 01:28:29AM -0400, Taylor Blau wrote:
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
I think you could just end here, since...
Show 9 quoted lines
> 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.
Show 5 quoted lines
> Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > 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.
Show 18 quoted lines
> 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