Re: [PATCH v5 1/2] add `config_set` API for caching config-like files
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- Jul 6, 2014, 17:45 UTC
- Message-ID
- <vpqa98m8jq2.fsf@anie.imag.fr>
- In-Reply-To
- <1404631162-18556-2-git-send-email-tanayabh@gmail.com>
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 ...".
Show 6 quoted lines
> +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.
Show 8 quoted lines
> +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.
Show 9 quoted lines
> + } > + /* > + * 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.
Show 8 quoted lines
> +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 (.
Show 11 quoted lines
> +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/