Re: [RFC/PATCH 1/2] config: Add cache for config value querying
From: Torsten Bögershausen <hidden>
Date: 2016-06-15 23:01:23
On 2014-05-26 19.33, Tanay Abhra wrote: I like the idea. Please allow some minor comments (and read them as questions rather then answers)
Add an internal cache with the all variable value pairs read from the usual
"cache": The word "cache" is in Git often used for "index" "variable value" can be written as "key value" "usual": I don't think we handle "unusual" config files, (so can we drop the word usual ?) I think the (important) first line can be written like this:
Add a hash table with the all key-value pairs read from the
or
Add a hash table to cache all key-value pairs read from the
config files(repo specific .git/config, user wide ~/.gitconfig and the global /etc/gitconfig). Also, add two external functions `git_config_get_string` and
Can we drop "Also" ?
quoted hunk ↗ jump to hunk
@@ -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;
Do we need a whole strbuf for the key? Or could a "char *key" work as well? (and/or could it be "const char *key" ?
+ struct string_list *value_list ;
quoted hunk ↗ jump to hunk
+static struct string_list *config_cache_get_value(const char *key) +{ + struct config_cache_node *e = config_cache_find_entry(key);
why "e" ? Will "node" be easier to read ? Or entry ?
quoted hunk ↗ jump to hunk
+static void config_cache_set_value(const char *key, const char *value) +{ + struct config_cache_node *e; + + if (!value) + return;
Hm, either NULL could mean "unset==remove" the value, (but we don't do that, do we? Or it could mean a programming or runtime error?, Should there be a warning ?
quoted hunk ↗ jump to hunk
+ + e = config_cache_find_entry(key); + if (!e) { + e = malloc(sizeof(*e)); + hashmap_entry_init(e, strihash(key)); + strbuf_init(&(e->key), 1024); + strbuf_addstr(&(e->key),key); + 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;
When malloc() is replaced by xcalloc() the x = NULL and y = 0 can go away, and the code is shorter and easier to read.
quoted hunk ↗ jump to hunk
+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 ;
We could get rid of the variable "int num_values" by simply writing result = values->items[values->nr-1].string;