Re: [PATCH v2 1/4] builtin/config: introduce `--default`

[Taylor Blau <me@ttaylorr.com>]

On Mon, Mar 26, 2018 at 04:34:42AM -0400, Jeff King wrote:
> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> > index e09ed5d7d..d9e389a33 100644
> > --- a/Documentation/git-config.txt
> > +++ b/Documentation/git-config.txt
> > @@ -233,8 +233,10 @@ See also <<FILES>>.
> >  	using `--file`, `--global`, etc) and `on` when searching all
> >  	config files.
> >
> > -CONFIGURATION
> > --------------
> > +--default value::
> > +  When using `--get`, behave as if value were the value assigned to the given
> > +  slot.
> > +
> >  `pager.config` is only respected when listing configuration, i.e., when
> >  using `--list` or any of the `--get-*` which may return multiple results.
> >  The default is to use a pager.
>
> Hmm, what's going on with the CONFIGURATION header here?

My mistake, I have correct this erroneous diff in v3. Thanks for
pointing it out!

> For the description of "--default" itself, do we want to say something
> like:
>
>   When using `--get` and the requested slot is not found, behave as
>   if...
>
> That is hopefully a given from the name "--default", but it seems like
> an important point to mention.

Ditto.

> > +test_expect_success 'marshals default value as int' '
> > +	echo 810 >expect &&
> > +	git config --default 810 --int core.foo >actual &&
> > +	test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'marshals default value as int (storage unit)' '
> > +	echo 1048576 >expect &&
> > +	git config --default 1M --int core.foo >actual &&
> > +	test_cmp expect actual
> > +'
>
> I'm not sure what we're really testing in the first one. The storage
> unit conversion is the interesting bit.
>
> TBH, I'm not sure we need to separately test each possible type
> conversion here. If we know that the type conversions are tested
> elsewhere (and I would not be surprised if they're not, but let's assume
> for a moment...) and we check that type conversions are applied to
> "--default" values for some types, I don't think there's any reason to
> assume that the others would not work.

Agreed. I don't think that this test (and the ones that your comment
below concerns) are testing anything meaningful, or that would catch any
interesting future regressions. I have removed them from this series in
v3.


Thanks,
Taylor