Re: [PATCH v1] config: Add hashtable for config parsing & retrival
On Mon, Jun 9, 2014 at 8:49 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
Show 25 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.
>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
> diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
> index 230b3a0..5b6e376 100644
> --- a/Documentation/technical/api-config.txt
> +++ b/Documentation/technical/api-config.txt
> @@ -77,6 +77,24 @@ To read a specific file in git-config format, use
> `git_config_from_file`. This takes the same callback and data parameters
> as `git_config`.
>
> +Querying For Specific Variables
> +-------------------------------
> +
> +For programs wanting to query for specific variables in a non-callback
> +manner, the config API provides two functions `git_config_get_string`
> +and `git_config_get_string_multi`. They both take a single parameter,
> +
> +- a key string in canonical flat form for which the corresponding values
> + will be retrieved and returned.
It's strange to have a bulleted list for a single item. It can be stated more naturally as a full sentence, without the list.
More below.
Show 32 quoted lines
> +They both read values from an internal cache generated previously from
> +reading the config files. `git_config_get_string` returns the value with
> +the highest priority(i.e. for the same variable value in the repo config
> +will be preferred over value in user wide config).
> +
> +`git_config_get_string_multi` returns a `string_list` containing all the
> +values for that particular key, sorted in order of increasing priority.
> +
> Value Parsing Helpers
> ---------------------
>
> diff --git a/config.c b/config.c
> index a30cb5c..6f6b04e 100644
> --- a/config.c
> +++ b/config.c
> @@ -9,6 +9,8 @@
> #include "exec_cmd.h"
> #include "strbuf.h"
> #include "quote.h"
> +#include "hashmap.h"
> +#include "string-list.h"
>
> struct config_source {
> struct config_source *prev;
> @@ -37,6 +39,120 @@ static struct config_source *cf;
>
> static int zlib_compression_seen;
>
> +struct config_cache_entry {
> + struct hashmap_entry ent;
> + char *key;
> + struct string_list *value_list;Same question as in my v1 and v2 reviews [1][2], and reiterated by Matthieu [3]: Why is 'value_list' a pointer to a structure rather than just a structure?
This is a genuine question. As a reviewer, I'm not sure how to interpret your lack of response. I can't tell if you merely overlook or forget the question multiple times; if you don't understand the inquiry; or if you have a strong reason to prefer making this a pointer. If you don't understand the question, it's okay to ask for clarification. If there is a strong reason for this to be a pointer, it should be explained. Without feedback from you, reviewers will continue asking the same question(s) upon each submission.
Show 10 quoted lines
> +};
> +
> +static int hashmap_is_init;
> +
> +static int config_cache_set_value(const char *key, const char *value);
> +
> +static int config_cache_entry_cmp_case(const struct config_cache_entry *e1,
> + const struct config_cache_entry *e2, const void *unused)
> +{
> + return strcmp(e1->key, e2->key);As suggested by Peff [4], this comparison is now case-sensitive, instead of case-insensitive as in the previous version. Rather than changing the appended '_icase' to '_case', a more typical function name would be simply config_cache_entry_cmp().
Show 5 quoted lines
> +}
> +
> +static void config_cache_init(struct hashmap *config_cache)
> +{
> + hashmap_init(config_cache, (hashmap_cmp_fn) config_cache_entry_cmp_case, 0);In his review [4], Peff suggested giving config_cache_entry_cmp_case() the correct hashmap_cmp_fn signature so that this cast can be dropped. It's not clear whether you disagree with his advice or overlooked or forgot about it. If you disagree with his suggestion, it's okay to say so and explain why you feel the way you do, but without feedback from you, he or another reviewer is going to continue suggesting the same change.
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_is_init) {
> + config_cache_init(&config_cache);
> + hashmap_is_init = 1;
> + git_config(config_cache_callback, NULL);
> + return &config_cache;Why do you 'return' here rather than just falling through to the 'return' outside the conditional?
Show 27 quoted lines
> + }
> + return &config_cache;
> +}
> +
> +static void config_cache_free(void)
> +{
> + struct hashmap *config_cache;
> + struct config_cache_entry *entry;
> + struct hashmap_iter iter;
> + config_cache = get_config_cache();
> + hashmap_iter_init(config_cache, &iter);
> + while ((entry = hashmap_iter_next(&iter))) {
> + free(entry->key);
> + string_list_clear(entry->value_list, 0);
> + }
> + hashmap_free(config_cache, 1);
> + hashmap_is_init = 0;
> +}
> +
> +static struct config_cache_entry *config_cache_find_entry(const char *key)
> +{
> + struct hashmap *config_cache;
> + struct config_cache_entry k;
> + char *normalized_key;
> + int baselength = 0, ret;
> + config_cache = get_config_cache();
> + ret = git_config_parse_key(key, &normalized_key, &baselength);Since you neither care about nor ever reference 'baselength', you should just pass NULL as the third argument.
Show 5 quoted lines
> + if (ret)
> + return NULL;
> +
> + hashmap_entry_init(&k, strhash(normalized_key));
> + k.key = (char*) key;
This looks fishy. You're hashing based upon 'normalized_key' but then comparing against the unnormalized 'key'. Peff's suggestion [4] was to store the normalized key in the hash, which means that you should use 'normalized_key' for both hashing and comparing. (See additional commentary about this issue below in config_cache_set_value().)
Style: (char *)key
> + return hashmap_get(config_cache, &k, NULL);
You're leaking 'normalized_key', which git_config_parse_key() allocated on your behalf.
Show 19 quoted lines
> +}
> +
> +static struct string_list *config_cache_get_value(const char *key)
> +{
> + struct config_cache_entry *e = config_cache_find_entry(key);
> + return e ? e->value_list : NULL;
> +}
> +
> +
> +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));The hash computed to store the key should be the same as the hash to look it up. In config_cache_find_entry(), you're correctly computing the hash based upon the normalized key, but here you're doing so from the unnormalized key.
> + e->key = xstrdup(key);
Likewise. Normalized keys should be stored in the hash, not unnormalized.
You should be invoking git_config_parse_key() to normalize the key both for hashing and storing.
Note, also, that call git_config_parse_key() allocates the normalize key on your behalf, so you do *not* want to xstrdup() it here.
> + e->value_list = xcalloc(sizeof(struct string_list), 1);
> + e->value_list->strdup_strings = 1;
This code is perhaps too intimate with the implementation of string_list. It may work today (because it is safe to initialize all string_list fields to 0 via xcalloc()), but a future change to the string_list implementation may invalidate that assumption. The initialization macros in string-list.h exist to preserve the abstraction, so you don't have to know the details of initialization. The macros are not suitable for your use-case, but an initialization function, such as string_list_init(), would be suitable, and it would be appropriate to add such a function in a preparatory patch (as already suggested by [1]).
Show 9 quoted lines
> + string_list_append(e->value_list, value);
> + hashmap_add(config_cache, e);
> + } else {
> + string_list_append(e->value_list, value);
> + }
> + return 0;
> +}
> +
> +extern const char *git_config_get_string(const char *key)Drop the 'extern'. The header already declares it such.
Show 10 quoted lines
> +{
> + struct string_list *values;
> + values = config_cache_get_value(key);
> + if (!values)
> + return NULL;
> + assert(values->nr > 0);
> + return values->items[values->nr - 1].string;
> +}
> +
> +extern const struct string_list *git_config_get_string_multi(const char *key)Drop the 'extern'. The header already declares it such.
Show 22 quoted lines
> +{
> + return config_cache_get_value(key);
> +}
> +
> static int config_file_fgetc(struct config_source *conf)
> {
> return fgetc(conf->u.file);
> @@ -1700,6 +1816,12 @@ 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.
> + */
> + if (hashmap_is_init)
> + config_cache_free();
> +
> out_free:
> if (lock)
> rollback_lock_file(lock);
> --
> 1.9.0.GIT[1]: http://git.661346.n2.nabble.com/RFC-PATCH-0-2-Git-config-cache-amp-special-querying-api-utilizing-the-cache-td7611691.html#a7611860 [2]: http://thread.gmane.org/gmane.comp.version-control.git/250566/focus=251058 [3]: http://thread.gmane.org/gmane.comp.version-control.git/251073/focus=251079 [4]: http://thread.gmane.org/gmane.comp.version-control.git/250566/focus=250618
Tanay Abhra
