Re: [PATCH v5 1/2] add `config_set` API for caching config-like files

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

Tanay Abhra <tanayabh@gmail.com> writes:

> Add a default `config_set`, `the_config_set` to cache all key-value pairs
> read from usual config files (repo specific .git/config, user wide
> ~/.gitconfig and the global /etc/gitconfig). `the_config_set` uses a
> single hashmap populated using `git_config()`.

"uses a single hashmap" is no longer relevant. Any config_set use a
single hashmap.

(The "populated using git_config()" part is still relevant OTOH)

> +`void git_configset_init(struct config_set *cs)`::
> +
> +	Initializes the member variables of config_set `cs`.

I'd say just "initializes the config_set `cs`".

> +/*
> + * Default config_set that contains key-value pairs from the usual set of config
> + * config files (i.e repo specific .git/config, user wide ~/.gitconfig and the
> + * global /etc/gitconfig)

To be technically correct, "user wide ~/.gitconfig, XDG config file and
the global ...".

> +static int add_configset_hash(const char *filename, struct config_set *cs)
> +{
> +	int ret = 0;
> +	if (!cs->hash_initialized)
> +		config_hash_init(&cs->config_hash);
> +	cs->hash_initialized = 1;

That would seem more natural to me to have a function config_set_init
instead of config_hash_init, that would set hash_initialized.

config_hash_init is currently a one-liner, so there's no risk of making
it too complex.

> +static int config_hash_add_value(const char *key, const char *value, struct hashmap *config_hash)
> +{
> +	struct config_hash_entry *e;
> +	e = config_hash_find_entry(key, config_hash);
> +
> +	if (!e) {
> +		e = xmalloc(sizeof(*e));
> +		hashmap_entry_init(e, strhash(key));

Nitpick: you're hashing the same string twice. Once for
config_hash_find_entry, and another here. It would be slightly faster to
allow config_hash_find_entry to return the hashcode (as a by-address
parameter).

But you may want to keep the code as it is for simplicity.

It took me some time to understand why you normalize in
config_hash_find_entry and not here. My understanding is that
config_hash_find_entry is used to query the hashmap from the user API
(so you need normalization), but this particular codepath receives
normalized keys (the comment right below states that for values).

It's probably worth a comment here and/or in config_hash_find_entry.

> +	}
> +	/*
> +	 * Since the values are being fed by git_config*() callback mechanism, they
> +	 * are already normalized. So simply add them without any further munging.
> +	 */
> +	string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
> +
> +	return 0;
> +}

[...]

> +int git_configset_add_file(struct config_set *cs, const char *filename)
> +{
> +	return add_configset_hash(filename, cs);
> +}

Why have two functions doing the same, with arguments in different
orders?

I guess it's just historical, and you can keep only one.

> +void git_configset_clear(struct config_set *cs)
> +{
> +	struct config_hash_entry *entry;
> +	struct hashmap_iter iter;
> +	if (!cs->hash_initialized)
> +		return;
> +
> +	hashmap_iter_init( &cs->config_hash, &iter);

No space after (.

> +int git_config_get_string(const char *key, const char **dest)
> +{
> +	git_config_check_init();
> +	return git_configset_get_string(&the_config_set, key, dest);
> +}
> +
> +int git_config_get_int(const char *key, int *dest)
> +{
> +	git_config_check_init();
> +	return git_configset_get_int(&the_config_set, key, dest);
> +}

Reading this list, I initially thought it might be worth factoring this
in a macro. But the list isn't that long, and contains several
special-cases (3 parameters instead of 2). So, don't bother.

OK, we're getting close. v6 should be really good :-).

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