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

[Junio C Hamano <gitster@pobox.com>]

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?