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

Hi,

On 05/26/2014 01:02 PM, Torsten Bögershausen wrote:

quoted

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" 

Okay, point noted. I thought about choosing between "hashmap" and "cache" and chose
the later.

"variable value" can be written as "key value"

I  had used the term "variable" to be consistent with the documentation
(api-config.txt). But I think "key" is much clearer.

"usual": I don't think we handle "unusual" config files,
(so can we drop the word usual ?)

Okay, noted.

I think the (important) first line can be written like this:

quoted

Add a hash table with the all key-value pairs read from the

or

quoted

Add a hash table to cache all key-value pairs read from the

quoted

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

@@ -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" ?

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.

quoted

+	struct string_list *value_list ;

quoted

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

Noted. Entry is much better.

quoted

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

Nope. It is just a check to not save blank values for a key in the hashmap. Removal
functionality will come later. NULL==remove is implemented in
git_config_set_multivar_in_file(). We are not reading key value pairs from that, just
from git_config().

quoted

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

Much better, thanks.

quoted

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

Noted.


Cheers,
Tanay Abhra.