From: Tanay Abhra <tanayabh@gmail.com>
Date: Tue, 24 Jun 2014 12:21:01 GMT
Subject: Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval
Message-ID: <53A96D2D.1020102@gmail.com>
In-Reply-To: <xmqqwqc79q1p.fsf@gitster.dls.corp.google.com>

On 6/24/2014 4:44 AM, Junio C Hamano wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> +static int hashmap_initialized;
>> + ...
>> +static struct hashmap *get_config_cache(void)
>> +{
>> +	static struct hashmap config_cache;
>> +	if (!hashmap_initialized) {
>> +		config_cache_init(&config_cache);
>> +		hashmap_initialized = 1;
>
> I find the arrangement of these two variables somewhat dubious at
> the API design level.
>
> If you are going to keep the singleton "config_cache" as a function
> scope static, shouldn't the corresponding guard also be in the same
> scope?
>
> If you ever need to "uninitialize" to force re-read the file to the
> in-core cache, such an uninitializer will need access to not just
> the "is hashmap initialized?" boolean (which you do by having it as
> a file-scope global like this patch does) but also the thing that
> may need to be uninitialized (i.e. the hashmap that may already be
> populated), but a function scope static variable config_cache does
> not allow access from other places, so you end up calling this
> function to initialize it if necessary only to get the pointer to
> that structure in order to uninitialize it.
>
> Sounds very twisted and ugly, doesn't it?
>

Hmn, You are right. but I couldn't find a way for that without making
"config_cache" file-scope static.

For the config_cache_free(), would this change be enough?

+static void config_cache_free(void)
+{
+	struct hashmap *config_cache;
+	struct config_cache_entry *entry;
+	struct hashmap_iter iter;
+	if (!hashmap_initialized)
+		return;
+	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;
+}
+

The idea was only initialize the hashmap only once,
as it is fed by git_config(). I ended up following
this approach where the map would be lazily generated whenever
a function asked for the map. Is there another way out?