Re: [RFC/PATCH 1/2] config: Add cache for config value querying
On Mon, May 26, 2014 at 1:33 PM, Tanay Abhra <tanayabh@gmail.com> wrote:
> Add an internal cache with the all variable value pairs read from the usual
> config files(repo specific .git/config, user wide ~/.gitconfig and the global
> /etc/gitconfig). Also, add two external functions `git_config_get_string` and
> `git_config_get_string_multi` for querying in an non callback manner from the
s/an/a/ s/non/non-/
More below.
Show 39 quoted lines
> cache.
>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
> cache.h | 2 ++
> config.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 107 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index 107ac61..2038150 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1271,6 +1271,8 @@ extern int check_repository_format_version(const char *var, const char *value, v
> extern int git_env_bool(const char *, int);
> extern int git_config_system(void);
> extern int config_error_nonbool(const char *);
> +extern const char *git_config_get_string(const char *);
> +extern const struct string_list *git_config_get_string_multi(const char *);
> #if defined(__GNUC__) && ! defined(__clang__)
> #define config_error_nonbool(s) (config_error_nonbool(s), -1)
> #endif
> diff --git a/config.c b/config.c
> index a30cb5c..f6515c2 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,102 @@ static struct config_source *cf;
>
> static int zlib_compression_seen;
>
> +struct hashmap config_cache;> +struct config_cache_node {
> + struct hashmap_entry ent;
> + struct strbuf key;Upon reading this, I had the same question as Torsten: Why isn't 'key' a plain 'char *' allocated when the entry is created? Your answer:
To maintain consistency with config.c. config.c uses strbuf for
both key and value throughout. I found the reason by git-blaming
config.c. Key length is flexible so it would be better to use a
api construct such as strbuf for it.doesn't make sense. The existing callback-based code re-uses the strbuf for each key as it parses the file, however, you are just storing each key once in the hashmap and never altering it, thus a strbuf is overkill.
> + struct string_list *value_list ;
Why isn't this just a plain 'struct string_list'? Why the extra indirection of a pointer?
Drop space before semicolon.
> +};
> +
> +static int hashmap_init_v = 0;
It's not obvious what "_v" is meant to convey.
Show 13 quoted lines
> +static int config_cache_node_cmp(const struct config_cache_node *e1,
> + const struct config_cache_node *e2, const char *key)
> +{
> + return strcmp(e1->key.buf, key ? key : e2->key.buf);
> +}
> +
> +static int config_cache_node_cmp_icase(const struct config_cache_node *e1,
> + const struct config_cache_node *e2, const char* key)
> +{
> + return strcasecmp(e1->key.buf, key ? key : e2->key.buf);
> +}
> +
> +static void config_cache_init(const int icase)'const int' as a function argument is very unusual in this code base. (I found only a single instance of it in log-tree.[ch]:parse_decorate_color_config.)
Show 9 quoted lines
> +{
> + hashmap_init(&config_cache, (hashmap_cmp_fn) (icase ? config_cache_node_cmp_icase
> + : config_cache_node_cmp), 0);
> +}
> +
> +static void config_cache_free(void)
> +{
> + hashmap_free(&config_cache, 1);
> +}Doesn't this leak the strbuf 'key' and string_list 'value_list'?
> +static struct config_cache_node *config_cache_find_entry(const char *key)
> +{
> + struct hashmap_entry k;
> + hashmap_entry_init(&k, strihash(key));How does this unconditional use of case-insensitive strihash() interact with the 'icase' argument of config_cache_init()?
Show 7 quoted lines
> + return hashmap_get(&config_cache, &k, key);
> +}
> +
> +static struct string_list *config_cache_get_value(const char *key)
> +{
> + struct config_cache_node *e = config_cache_find_entry(key);
> + if (e == NULL)> + return NULL;
> + else
> + return (e->value_list);
> +}
A purely subject rewrite of the above:
return e ? e->value_list : NULL;
Show 6 quoted lines
> +static void config_cache_set_value(const char *key, const char *value)
> +{
> + struct config_cache_node *e;
> +
> + if (!value)
> + return;Again, I had the same questions as Torsten. ("Is this supposed to delete the entry?", "Is it a programmer error?", etc.) From a pure reading, it's not obvious why a NULL 'value' is handled this way. The intent might be clearer if the caller instead performs the check, and invokes config_cache_set_value() only if 'value' is non-NULL.
> + e = config_cache_find_entry(key);
> + if (!e) {
> + e = malloc(sizeof(*e));
> + hashmap_entry_init(e, strihash(key));Same question as above regarding unconditional case-insensitive strihash() versus the 'icase' argument of config_cache_init().
> + strbuf_init(&(e->key), 1024);
Why allocate such a large buffer for each key, especially when most keys likely are quite short. Since the key is never altered after being assigned, this seems like considerable waste.
Also, unnecessary parentheses.
> + strbuf_addstr(&(e->key),key);
Unnecessary parentheses. Add space after comma.
Show 6 quoted lines
> + e->value_list = malloc(sizeof(struct string_list));
> + e->value_list->strdup_strings = 1;
> + e->value_list->nr = 0;
> + e->value_list->alloc = 0;
> + e->value_list->items = NULL;
> + e->value_list->cmp = NULL;
This string_list initialization code begs for a preparatory patch which adds a string_list_init() function (akin to strbuf_init()).
> + string_list_append(e->value_list, value);
> + hashmap_add(&config_cache, e);
> + }
> + else {Cuddle the else with the braces: } else {> + if (!unsorted_string_list_has_string((e->value_list), value))
> + string_list_append((e->value_list), value);
Unnecessary parentheses (on both lines).
Since there is only an 'if' statement in this 'else' block, you could turn it into an 'else if', thus eliminating one level of indentation.
Show 13 quoted lines
> + }
> +}
> +
> +extern const char *git_config_get_string(const char *name)
> +{
> + struct string_list *values;
> + int num_values;
> + char *result;
> + values = config_cache_get_value(name);
> + if (!values)
> + return NULL;
> + num_values = values->nr;
> + result = values->items[num_values-1].string ;Add spaces around '-'. Drop space before semicolon.
> + return result;
Why do you need the 'result' variable if you are returning it immediately after it's assigned?
return values->items[num_values - 1].string;
Or, taking Torsten's recommendation into consideration to drop num_values:
return values->items[values->nr - 1].string;
Show 33 quoted lines
> +}
> +
> +extern const struct string_list *git_config_get_string_multi(const char *key)
> +{
> + return config_cache_get_value(key);
> +}
> +
> static int config_file_fgetc(struct config_source *conf)
> {
> return fgetc(conf->u.file);
> @@ -333,6 +431,7 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)
> if (!value)
> return -1;
> }
> + config_cache_set_value(name->buf, value);
> return fn(name->buf, value, data);
> }
>
> @@ -1135,6 +1234,12 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
> char *xdg_config = NULL;
> char *user_config = NULL;
>
> + int icase = 1;
> + if (!hashmap_init_v) {
> + config_cache_init(icase);
> + hashmap_init_v = 1;
> + }
> +
> home_config_paths(&user_config, &xdg_config, "config");
>
> if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {
> --
> 1.9.0.GIT
Tanay Abhra
