Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval

From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date: Jun 23, 2014, 14:57 UTC
Message-ID: <53A84077.4010200@ramsay1.demon.co.uk>
In-Reply-To: <1403518300-23053-3-git-send-email-tanayabh@gmail.com>

On 23/06/14 11:11, Tanay Abhra wrote:

[snip]

Show 54 quoted lines

> diff --git a/config.c b/config.c
> index a1aef1c..6200f36 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,141 @@ static struct config_source *cf;
>  
>  static int zlib_compression_seen;
>  
> +struct config_cache_entry {
> +	struct hashmap_entry ent;
> +	char *key;
> +	struct string_list value_list;
> +};
> +
> +static int hashmap_initialized;
> +
> +static int config_cache_add_value(const char *key, const char *value);
> +
> +static int config_cache_entry_cmp(const struct config_cache_entry *e1,
> +				 const struct config_cache_entry *e2, const void *unused)
> +{
> +	return strcmp(e1->key, e2->key);
> +}
> +
> +static void config_cache_init(struct hashmap *config_cache)
> +{
> +	hashmap_init(config_cache, (hashmap_cmp_fn)config_cache_entry_cmp, 0);
> +}
> +
> +static int config_cache_callback(const char *key, const char *value, void *unused)
> +{
> +	config_cache_add_value(key, value);
> +	return 0;
> +}
> +
> +static struct hashmap *get_config_cache(void)
> +{
> +	static struct hashmap config_cache;
> +	if (!hashmap_initialized) {
> +		config_cache_init(&config_cache);
> +		hashmap_initialized = 1;
> +		git_config(config_cache_callback, NULL);
> +	}
> +	return &config_cache;
> +}

[I have not been following this series at all (sorry I haven't had the time to spare), so take these comments with a very big pinch of salt! ie just ignore me if it's already been discussed etc. ;-) ]

The 'git config' command can be used to read arbitrary files (so long as they conform to the config syntax). For example, see the --file and --blob options to git-config. At present, I think only scripted commands use this facility (eg git-submodule). Noting the singleton config_cache, what happens when git-submodule becomes a C builtin, or indeed any other C builtin wants to take advantage of the new code when processing a non- standard config file?

Show 58 quoted lines

> +
> +static void config_cache_free(void)
> +{
> +	struct hashmap *config_cache;
> +	struct config_cache_entry *entry;
> +	struct hashmap_iter iter;
> +	config_cache = get_config_cache();
> +	hashmap_iter_init(config_cache, &iter);
> +	while ((entry = hashmap_iter_next(&iter))) {
> +		free(entry->key);
> +		string_list_clear(&entry->value_list, 1);
> +	}
> +	hashmap_free(config_cache, 1);
> +	hashmap_initialized = 0;
> +}
> +
> +static struct config_cache_entry *config_cache_find_entry(const char *key)
> +{
> +	struct hashmap *config_cache;
> +	struct config_cache_entry k;
> +	struct config_cache_entry *found_entry;
> +	char *normalized_key;
> +	int ret;
> +	config_cache = get_config_cache();
> +	ret = git_config_parse_key(key, &normalized_key, NULL);
> +
> +	if (ret)
> +		return NULL;
> +
> +	hashmap_entry_init(&k, strhash(normalized_key));
> +	k.key = normalized_key;
> +	found_entry = hashmap_get(config_cache, &k, NULL);
> +	free(normalized_key);
> +	return found_entry;
> +}
> +
> +static struct string_list *config_cache_get_value(const char *key)
> +{
> +	struct config_cache_entry *e = config_cache_find_entry(key);
> +	return e ? &e->value_list : NULL;
> +}
> +
> +static int config_cache_add_value(const char *key, const char *value)
> +{
> +	struct hashmap *config_cache;
> +	struct config_cache_entry *e;
> +	struct string_list_item *item;
> +	int *boolean_null_flag;
> +
> +	config_cache = get_config_cache();
> +	e = config_cache_find_entry(key);
> +
> +	boolean_null_flag = xcalloc(1, sizeof(*boolean_null_flag));
> +
> +	if (!e) {
> +		e = xmalloc(sizeof(*e));
> +		hashmap_entry_init(e, strhash(key));
> +		e->key = xstrdup(key);

config_cache_find_entry() searches for (and hashes the) normalized_key. Should you not be entering the normalized key here?

Show 19 quoted lines

> +		string_list_init_dup(&e->value_list);
> +		hashmap_add(config_cache, e);
> +	}
> +	/*
> +	 * If the variable had no value specified, the value will be NULL
> +	 * (typically this means it should be interpreted as boolean true).
> +	 * For such values, silently convert them to "BOOLEAN_NULL" to
> +	 * store in hashmap and flag that string_list_item as 1.
> +	 */
> +	if (value == NULL) {
> +		value = "BOOLEAN_NULL";
> +		*boolean_null_flag = 1;
> +	}
> +	item = string_list_append(&e->value_list, value);
> +	item->util = boolean_null_flag;
> +
> +	return 0;
> +}
> +

[snip]

ATB, Ramsay Jones