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

From: Tanay Abhra <tanayabh@gmail.com>
Date: Jul 30, 2014, 14:40 UTC
Message-ID: <53D903E0.4080209@gmail.com>
In-Reply-To: <vpq1tt3q6ic.fsf@anie.imag.fr>

On 7/30/2014 7:43 PM, Matthieu Moy wrote:

Show 26 quoted lines

> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> -	git_config(notes_display_config, &load_config_refs);
>> +	if (load_config_refs) {
>> +		values = git_config_get_value_multi("notes.displayref");
>> +		if (values) {
>> +			for (i = 0; i < values->nr; i++) {
>> +				if (!values->items[i].string)
>> +					config_error_nonbool("notes.displayref");
>> +				else
>> +					string_list_add_refs_by_glob(&display_notes_refs,
>> +								     values->items[i].string);
>> +			}
>> +		}
>> +	}
> 
> It seems to me that you're doing a lot here that should have been done
> once in the config API:
> 
> * if (values) {
>           for (i = 0; i < values->nr
> 
>   => We could avoid the "if" statement if git_config_get_value_multi was
>   always returning a string_list, possibly empty (values->nr == 0
>   instead of values == NULL).
>

or we can do something like,

	if (!git_config_get_value_multi("notes.displayref", &values)) {
		/* return 0 if there is a value_list for the key */

Show 9 quoted lines

>   Not as obvious as it seems, because you normally return a pointer to
>   the string_list that is already in the hashmap, so you can't just
>   malloc() an empty one if you don't want to leak it.
> 
>   Another option would be to provide an iterator that would call a
>   function on each value of the list, and do nothing when there's no
>   list at all (back to the callback-style API, but you would iterate
>   only through the values for the right key).
>

This is also a good idea, but still we are back to the callback API, and what we are gaining is fewer loop iterations than git_config().

Which way do you prefer, a reroll is easy but Junio might have been sick of replacing the patches in pu by now. :)

Show 18 quoted lines

> * 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.

Show 12 quoted lines

> const struct string_list *git_config_get_value_multi_nonbool(const char *key)
> {
> 	git_config_check_init();
> 	return git_configset_get_value_multi_nonbool(&the_config_set, key);
> }
> 
> 
>   (totally untested)
> 
>   BTW, is it intentional that you call config_error_nonbool() without
>   die-ing?
>

Yup, it's intentional, original code didn't die for empty values, and it seemed logical to me to emulate that over to the rewrite.