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

[Tanay Abhra <tanayabh@gmail.com>]

On 8/5/2014 1:34 AM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> 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
> 
> It's good to do at least as well as the old system, but I agree with
> Junio that it's suboptimal.
> 
> Having a single API call to replace
> 
>         error("'%s' must be between 1 and 3");
>         git_config_die(key);
> 
> with stg like:
> 
>         git_config_die(key, "'%s' must be between 1 and 3");
>

Matthieu, I have finished the new version, but instead of flooding the mailing list with
a series again, I wanted to confirm if the new git_config_die() is alright.

	NORETURN __attribute__((format(printf, 2, 3)))
	void git_die_config(const char *key, const char *err, ...)
	{
		const struct string_list *values;
		struct key_value_info *kv_info;

		if (err) {
			va_list params;
			va_start(params, err);
			vreportf("error: ", err, params);
			va_end(params);
		}
		values = git_config_get_value_multi(key);
		kv_info = values->items[values->nr - 1].util;
		git_die_config_linenr(key, kv_info->filename, kv_info->linenr);
	}

Currently works like the old git_config() error reporting path. If err is set to "NULL",
it would print no error message and just the die message. If given something like,

	 git_config_die(key, "value '%s' is not allowed", value);

it would print,

	error: value '3' is not allowed
	fatal: bad config variable 'core.frotz' at file line 15 in .git/config

Cheers,
Tanay Abhra.

> in Junio's example would allow git_config_die to format the error
> message the way it likes (i.e. it can be the same as before when you
> introduce it, and improve afterwards).
> 
> I've never been disturbed by the quality of Git's error messages wrt
> config files (it's not a compiler!), so having good quality messages is
> not a high priority IMHO, but having a good API that as a side effect
> can produce good error messages is important. If changing the error
> messages requires rewritting all callers later, then we've missed the
> point doing the refactoring now.
>