Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap
- From
Stefan Beller <sbeller@google.com>
- Date
- Feb 9, 2017, 17:43 UTC
- Message-ID
- <CAGZ79kb-uNnfrvCUk9PbXcFwOJSmSwccuSaZ2H_u61engE+H3A@mail.gmail.com>
- In-Reply-To
- <4a21dba7-76ef-6aec-b326-c1046f3daad2@alum.mit.edu>
On Thu, Feb 9, 2017 at 9:40 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 28 quoted lines
> 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 >