Tanay Abhra <tanayabh@xxxxxxxxx> writes:
> 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.
> Signed-off-by: Tanay Abhra <tanayabh@xxxxxxxxx>
> ---
> 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.
> +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 :-).
> +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.
> +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.
> @@ -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/
--
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