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

From: Jeff King <peff@peff.net>
Date: Feb 9, 2017, 19:32 UTC
Message-ID: <20170209193213.eamc3swvbefwlhr7@sigill.intra.peff.net>
In-Reply-To: <4a21dba7-76ef-6aec-b326-c1046f3daad2@alum.mit.edu>

On Thu, Feb 09, 2017 at 06:40:29PM +0100, Michael Haggerty wrote:

Show 18 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.
> [...]
> 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@sigill.intra.peff.net/

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