On Thu, Feb 09, 2017 at 06:40:29PM +0100, Michael Haggerty 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.
> [...]
> 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.
I faced a similar issue when adding the oidset API recently:
http://public-inbox.org/git/20170208205307.uvgj3saf3cyrvtan@xxxxxxxxxxxxxxxxxxxxx/
I came to the same conclusion that it doesn't matter much in practice. A
nice thing about "return NULL" is that you do not have to duplicate the
initialization which happens on the "write" side (so if you ever changed
submodule_hash_cmp, for example, it needs changed in both places).
I also used the "cmpfn" member to check whether the table had been
initialized, which matches existing uses elsewhere. But I do notice that
the documentation explicitly says "tablesize" is good for this purpose,
so it's probably a better choice.
-Peff