Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Feb 9, 2017, 17:40 UTC
Message-ID: <4a21dba7-76ef-6aec-b326-c1046f3daad2@alum.mit.edu>
In-Reply-To: <CAGZ79kau2bYs7zegEiacAdbhn1LyOfAH9__rePfbQkX2iLgmMQ@mail.gmail.com>

On 02/09/2017 05:58 PM, Stefan Beller wrote:

Show 13 quoted lines

>> @@ -1402,17 +1435,17 @@ struct ref_store *ref_store_init(const char *submodule)
>>
>>  struct ref_store *lookup_ref_store(const char *submodule)
>>  {
> 
>> +       if (!submodule_ref_stores.tablesize)
>> +               hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);
> 
> 
> So we can lookup a submodule even before we initialized the subsystem?
> Does that actually happen? (It sounds like a bug to me.)
>
> Instead of initializing, you could return NULL directly here.

The lines you quoted are only concerned with bringing the (empty) hashmap into existence if it hasn't been initialized already. (There's no HASHMAP_INIT.) I don't know what you mean by "initialize the subsystem". The only way to bring a ref_store *object* into existence is currently to call get_ref_store(submodule), which calls lookup_ref_store(submodule) to see if it already exists, and if not calls ref_store_init(submodule) to instantiate it and register it in the hashmap. There's nothing else that has to be initialize before that (except maybe the usual startup config reading etc.)

I suppose this code path could be changed to return NULL without initializing the hashmap, but the hashmap will be initialized a moment later by ref_store_init(), so I don't see much difference either way.

Thanks for your review! Michael