On 7/30/2014 7:43 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@xxxxxxxxx> 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 */
> 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. :)
> * 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.
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html