Re: [PATCH v2 2/2] config: Add hashtable for config parsing & retrieval
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- Jun 16, 2014, 17:11 UTC
- Message-ID
- <vpqy4ww230l.fsf@anie.imag.fr>
- In-Reply-To
- <1402907232-24629-3-git-send-email-tanayabh@gmail.com>
Tanay Abhra <tanayabh@gmail.com> writes:
Show 5 quoted lines
> Add a hash table to cache all key-value pairs read from config files > (repo specific .git/config, user wide ~/.gitconfig and the global > /etc/gitconfig). Add two external functions `git_config_get_string` and > `git_config_get_string_multi` for querying in a non-callback manner from the > hash table.
This describes rather well _what_ your patch does, but the most important part of a commit message is to justify _why_ the change is good, and why the way you implemented it is good.
Think of it as an way to convince reviewers to accept your patch.
Show 6 quoted lines
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com> > --- > Documentation/technical/api-config.txt | 17 +++++ > cache.h | 2 + > config.c | 123 +++++++++++++++++++++++++++++++++ > 3 files changed, 142 insertions(+)
I'm still concerned about the fact that there is no test. At this point, git_config_get_string_multi and git_config_get_string are basically dead code.
I'm sure you did experiments to test these functions, run them through valgrind, ... Now, you need to let other people reproduce these experiments. "other people" can be reviewers, now, or anyone looking for bugs or regressions in the future.
Show 16 quoted lines
> +static int config_cache_callback(const char *key, const char *value, void *unused)
> +{
> + config_cache_set_value(key, value);
> + return 0;
> +}
> +
> +static struct hashmap *get_config_cache(void)
> +{
> + static struct hashmap config_cache;
> + if (!hashmap_initialized) {
> + config_cache_init(&config_cache);
> + hashmap_initialized = 1;
> + git_config(config_cache_callback, NULL);
> + }
> + return &config_cache;
> +}Good, just enough code for the job :-).
Show 15 quoted lines
> +static struct config_cache_entry *config_cache_find_entry(const char *key)
> +{
> + struct hashmap *config_cache;
> + struct config_cache_entry k;
> + struct config_cache_entry *found_entry;
> + char *normalized_key;
> + int ret;
> + config_cache = get_config_cache();
> + ret = git_config_parse_key(key, &normalized_key, NULL);
> +
> + if (ret)
> + return NULL;
> +
> + hashmap_entry_init(&k, strhash(normalized_key));
> + k.key = (char *)normalized_key;No need to cast.
Show 19 quoted lines
> +static int config_cache_set_value(const char *key, const char *value)
> +{
> + struct hashmap *config_cache;
> + struct config_cache_entry *e;
> +
> + config_cache = get_config_cache();
> + e = config_cache_find_entry(key);
> + if (!e) {
> + e = xmalloc(sizeof(*e));
> + hashmap_entry_init(e, strhash(key));
> + e->key = xstrdup(key);
> + string_list_init_dup(&e->value_list);
> + string_list_append(&e->value_list, value);
> + hashmap_add(config_cache, e);
> + } else {
> + string_list_append(&e->value_list, value);
> + }
> + return 0;
> +}I find the function name a bit confusing, as it does not "set" in the sense "override any previous value". Wouldn't this be better named config_cache_add_value? Or perhaps a comment would help.
Show 8 quoted lines
> @@ -1714,6 +1830,13 @@ int git_config_set_multivar_in_file(const char *config_filename, > lock = NULL; > ret = 0; > > + /* > + *contents of config file has changed, so invalidate the > + *config cache used by non-callback based query functions. > + */
Spaces after stars:
/* * Contents of config file has changed, so invalidate the * config cache used by non-callback based query functions. */
(I think the "s" of "contents" should be dropped too).
-- Matthieu Moy http://www-verimag.imag.fr/~moy/