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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

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");

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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/