Re: [RFC/PATCH 1/2] config: Add cache for config value querying

On Mon, May 26, 2014 at 1:33 PM, Tanay Abhra [off-list ref] 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

s/(/ (/

/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.

This should be 'static'.

+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.

+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.)

+{
+       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()?

+       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)

Rephrase: if (!e)

+               return NULL;
+       else
+               return (e->value_list);

Unnecessary parentheses.

+}

A purely subject rewrite of the above:

    return e ? e->value_list : NULL;

+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.

+               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.

+       }
+}
+
+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;