Re: [PATCH v4 1/3] builtin/config: introduce `--default`

[Taylor Blau <me@ttaylorr.com>]

On Thu, Apr 05, 2018 at 06:29:49PM -0400, Jeff King wrote:
> On Wed, Apr 04, 2018 at 07:59:12PM -0700, Taylor Blau wrote:
>
> > @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)
> >  	config_with_options(collect_config, &values,
> >  			    &given_config_source, &config_options);
> >
> > +	if (!values.nr && default_value) {
> > +		struct strbuf *item;
> > +		ALLOC_GROW(values.items, values.nr + 1, values.alloc);
> > +		item = &values.items[values.nr++];
> > +		strbuf_init(item, 0);
> > +		if (format_config(item, key_, default_value) < 0) {
> > +			exit(1);
> > +		}
> > +	}
>
> Calling exit() explicitly is unusual for our code. Usually we would
> either die() or propagate the error. Most of the types in
> format_config() would die on bogus input, but a few code paths will
> return an error.
>
> What happens if a non-default value has a bogus format? E.g.:
>
>   $ git config foo.bar '~NoSuchUser'
>   $ git config --path foo.bar
>   fatal: failed to expand user dir in: '~NoSuchUser'
>
> Oops. Despite us checking for an error return from
> git_config_pathname(), it will always either return 0 or die(). So
> that's not a good example. ;)
>
> Let's try expiry-date:
>
>   $ git config foo.bar 'the first of octember'
>   $ git config --expiry-date foo.bar
>   error: 'the first of octember' for 'foo.bar' is not a valid timestamp
>   fatal: bad config line 7 in file .git/config
>
> OK. So we call format_config() there from the actual collect_config()
> callback, and the error gets propagated back to the config parser, which
> then gives us an informative die(). What happens with your new code:
>
>   $ ./git config --default 'the first of octember' --type=expiry-date no.such.key
>   error: 'the first of octember' for 'no.such.key' is not a valid timestamp
>
> It's obvious in this toy example, but that config call may be buried
> deep in a script. It'd probably be nicer for that exit(1) to be
> something like:
>
>   die(_("failed to format default config value"));

Aha. Thanks for the explanation :-). I've removed the exit() and changed
it to the die() that you suggested above. The test in t1310 is updated
to grep for the new message, so we know that it is being reported
appropriately.

> > +test_expect_success 'does not allow --default without --get' '
> > +	test_must_fail git config --default quux --unset a >output 2>&1 &&
> > +	test_i18ngrep "\-\-default is only applicable to" output
> > +'
>
> I think "a" here needs to be "a.section". I get:
>
>   error: key does not contain a section: a

Aha, thanks again. I have updated this in the forthcoming re-roll.


Thanks,
Taylor