Tanay Abhra [off-list ref] 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/
On 8/5/2014 1:34 AM, Matthieu Moy wrote:
Tanay Abhra [off-list ref] writes:
quoted
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.