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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Aug 4, 2014, 20:04 UTC
Message-ID: <vpqegwwauo2.fsf@anie.imag.fr>
In-Reply-To: <53DFD729.8090307@gmail.com>

Tanay Abhra <tanayabh@gmail.com> writes:

Show 13 quoted lines

> 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/