Re: [PATCH v4 2/5] notes.c: replace `git_config()` with `git_config_get_value_multi()`

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jul 31, 2014, 11:38 UTC
Message-ID: <vpqmwbpdagf.fsf@anie.imag.fr>
In-Reply-To: <vpqsiliol1o.fsf@anie.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Show 27 quoted lines

> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> On 7/30/2014 7:43 PM, Matthieu Moy wrote:
>>> * if (!values->items[i].string)
>>>           config_error_nonbool(
>>> 
>>>   => This check could be done once and for all in a function, say
>>>   git_config_get_value_multi_nonbool, a trivial wrapper around
>>>   git_config_get_value_multi like
>>> 
>>> const struct string_list *git_configset_get_value_multi_nonbool(struct config_set *cs, const char *key)
>>> {
>>> 	struct string_list l = git_configset_get_value_multi(cs, key);
>>>         // possibly if(l) depending on the point above.
>>> 	for (i = 0; i < values->nr; i++) {
>>> 		if (!values->items[i].string)
>>> 			git_config_die(key);
>>> 	}
>>> 	return l;
>>> }
>>>
>>
>> Not worth it, most the multi value calls do not die on a nonbool.
>
> Can you cite some multi-value variables that can be nonbool? I can't
> find many multi-valued variables, and I can't find any which would allow
> bool and nonbool.

Thinking a bit more about it: we actually need more than your patch and my code example above to give accurate error messages. Your code gives no error message, and mine uses git_config_die(key); which gives the line of the _last_ entry, but not necessarily the right line.

The right line number should be extracted from the info field of the string_list. It's not completely trivial, hence I'd rather have a helper doing it well in config.c than letting callers re-do the check and possibly give wrong line in their error message as I did in my first attempt.

I think you can introduce a helper git_config_die_linenr(key, linenr) that displays the right error message. Then git_config_die becomes a wrapper around it that does the lookup to find linenr from the hash.

You already have a duplicate piece of code in your other series:

			if (!kv_info->linenr)
				die("unable to parse '%s' from command-line config", entry->key);
			else
				die("bad config variable '%s' at file line %d in %s",
					entry->key,
					kv_info->linenr,
					kv_info->filename);

That would be the content of git_config_die_linenr().

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