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

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

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

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

* 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;
}

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?

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