Re: [PATCH v7 5/8] config: add `git_die_config()` to the config-set API

From: Tanay Abhra <tanayabh@gmail.com>
Date: Aug 4, 2014, 18:55 UTC
Message-ID: <53DFD729.8090307@gmail.com>
In-Reply-To: <xmqqoaw0ruwf.fsf@gitster.dls.corp.google.com>

On 8/4/2014 11:37 PM, Junio C Hamano wrote:

Show 22 quoted lines

> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> Add `git_die_config` that dies printing the line number and the file name
>> of the highest priority value for the configuration variable `key`.
>>
>> It has usage in non-callback based config value retrieval where we can
>> raise an error and die if there is a semantic error.
>> For example,
>>
>> 	if (!git_config_get_value(key, &value)) {
>> 		/* NULL values not allowed */
>> 		if (!value)
>> 			git_config_die(key);
>> 		else
>> 			/* do work */
>> 	}
> 
> It feels a bit unnatural at the API level that this does not take
> 'value'; I do understand that it is not a big deal in the error code
> path to locate again the value from the configuration using the key,
> but still.
>

But, we don't have a use for "value" as it is not denoted in the error string, that is why I left it out.

Show 23 quoted lines

> It feels even more unnatural that the caller cannot say _how_ it
> finds the value offending by not taking any message.  For one
> particular callchain, e.g. git_config_get_string() that eventually
> calls git_config_string() which will show an error message via
> config_error_nonbool(), you may not want any extra message, but for
> new callers that wants to make sure value falls within a supported
> range, this forces it to write
> 
> 	if (!git_config_get_int(key, &num)) {
>         	if (!(0 < num && num < 4)) {
> 			error("'%s' must be between 1 and 3");
>                         git_config_die(key);
> 		}
> 		/* otherwise work */
> 	}
> 
> and then the error message would say something like:
> 
> 	error: 'core.frotz' must be between 1 and 3
> 	fatal: bad config variable 'core.frotz' at file line 15 in .git/config
> 
> which sounds somewhat backwards, at least to me.
>

I was aping the old git_config() system, it also does exactly what you described above. for example, builtin/gc.c line 91,

		if (!strcmp(var, "gc.pruneexpire")) {
		if (value && strcmp(value, "now")) {
			unsigned long now = approxidate("now");
			if (approxidate(value) >= now)
				return error(_("Invalid %s: '%s'"), var, value);
		}

would print,
 	error: Invalid gc.pruneexpire: 'value'
	fatal: bad config variable 'gc.pruneexpire' at file line 15 in .git/config

or imap-send.c line 1340,

	if (!strcmp("sslverify", key))
		server.ssl_verify = git_config_bool(key, val);
	else if (!strcmp("preformattedhtml", key))
		server.use_html = git_config_bool(key, val);
	else if (!val)
		return config_error_nonbool(key);
again would cause a error & die, message combo as above. There are many examples like that.
We can easily take a custom error message but again I was just aping the old system.

Show 12 quoted lines

>> +NORETURN
>> +void git_die_config_linenr(const char *key, const char *filename, int linenr)
>> +{
>> +	if (!linenr)
>> +		die(_("unable to parse '%s' from command-line config"), key);
> 
> Do we have existing code that says "we signal that it is from the
> command line by setting linenr to zero" already?  Otherwise I would
> have thought filename == NULL would be a more sensible convention.
> 
> Otherwise OK.
>

Noted. Next reroll will have filename as the convention.

Show 5 quoted lines

>> +	else
>> +		die(_("bad config variable '%s' at file line %d in %s"),
> 
> At least, quote the last '%s'.
>

Noted. Thanks.