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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jul 31, 2014, 18:41 UTC
Message-ID: <vpqoaw55q16.fsf@anie.imag.fr>
In-Reply-To: <53DA8A54.6060208@gmail.com>

Tanay Abhra <tanayabh@gmail.com> writes:

Show 31 quoted lines

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