Re: [PATCH v6 6/7] config: add `git_die_config()` to the config-set API

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

Tanay Abhra <tanayabh@gmail.com> writes:

> On 7/31/2014 11:39 PM, Matthieu Moy wrote:
>
>> This is the part I find weird. You're calling git_die_config_exact() on
>> the first boggus value, and git_die_config_exact() will notify an error
>> at the line of the last boggus value.
>
> Hmn, we may have some confusion here. I meant the implementation of
> git_config_exact() to look like this,
>
> void git_die_config_exact(const char *key, const char *value)
> {
> 	int i;
> 	const struct string_list *values;
> 	struct key_value_info *kv_info;
> 	values = git_config_get_value_multi(key);
> 	for (i = 0; i < values.nr; i++) {
> 		kv_info = values->items[i].util;
> 		/* A null check will be here also */
> 		if (!strcmp(value, values->items[i].string)) {
> 			if (!kv_info->linenr)
> 				die(_("unable to parse '%s' from command-line config"), key);
> 			else
> 				die(_("bad config variable '%s' at file line %d in %s"),
> 					key,
> 					kv_info->linenr,
> 					kv_info->filename);
> 		}
> 	}
> }
>
> The above code would print the info on first bogus value.

OK, I got confused because git_die_config() errors out at the first
error. So, your version works, but I do not see any added value in this
extra complexity.

To be cleary, my version would be

NORETURN static /* not sure about the "static" */
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);
	else
		die(_("bad config variable '%s' at file line %d in %s"),
			key,
			linenr,
			filename);
}

(hmm, I actually do not need the value, it's not printed)

NORETURN
void git_die_config(const char *key)
{
	const struct string_list *values;
	struct key_value_info *kv_info;
	values = git_config_get_value_multi(key);
	kv_info = values->items[values->nr - 1].util;
	git_die_config_linenr(key, kv_info->linenr, kv_info->filename);
}

(we forgot the NORETURN in previous version BTW. It should be there in
both functions here and in the .h file)

In my version, git_die_config uses git_die_config_linenr, and there's no
issue with first/last boggus value. Callers of git_die_config_linenr
have all the information to call it directly.

There are 3 code path that leads to the final die() calls, and having
this little helper allows making sure the messages are the same for
every callers, by construction (as opposed to cut-and-pasting).

Really, I don't see any reason to do anything more complex.

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