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

[Stefan Beller <sbeller@google.com>]

On Thu, Feb 9, 2017 at 9:40 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 02/09/2017 05:58 PM, Stefan Beller wrote:
>>> @@ -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.

Oh, I did not see that.

Thanks,
Stefan

>
> Thanks for your review!
> Michael
>