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

From: Torsten Bögershausen <tboegi@web.de>
Date: May 26, 2014, 20:02 UTC
Message-ID: <53839DEE.9090504@web.de>
In-Reply-To: <1401125601-18249-2-git-send-email-tanayabh@gmail.com>

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

Show 9 quoted lines

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

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

Show 6 quoted lines

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

Show 13 quoted lines

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

Show 10 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 ;

We could get rid of the variable "int num_values" by simply writing result = values->items[values->nr-1].string;