Re: Re: [PATCH] implement submodule config cache for lookup of submodule names

From: Heiko Voigt <hvoigt@hvoigt.net>
Date: Mar 12, 2014, 17:07 UTC
Message-ID: <20140312170706.GA3817@sandbox-ub>
In-Reply-To: <20140311215808.GB32129@sigill.intra.peff.net>

On Tue, Mar 11, 2014 at 05:58:08PM -0400, Jeff King wrote:

Show 24 quoted lines

> On Mon, Mar 10, 2014 at 10:24:12PM +0100, Heiko Voigt wrote:
> 
> > I have also moved all functions into the new submodule-config-cache
> > module. I am not completely satisfied with the naming since it is quite
> > long. If someone comes up with some different naming I am open for it.
> > Maybe simply submodule-config (submodule_config prefix for functions)?
> 
> Since the cache is totally internal to the submodule-config code, I do
> not know that you even need to have a nice submodule-config-cache API.
> Can it just be a singleton?
> 
> That is bad design in a sense (it becomes harder later if you ever want
> to pull submodule config from two sources simultaneously), but it
> matches many other subsystems in git which cache behind the scenes.
> 
> I also suspect you could call submodule_config simply "submodule", and
> let it be a stand-in for the submodule (for now, only data from the
> config, but potentially you could keep other data on it, too).
> 
> So with all that, the entry point into your code is just:
> 
>   const struct submodule *submodule_from_path(const char *path);
> 
> and the caching magically happens behind-the-scenes.

Actually since we also need to define the revision from which we request these submodule values that would become:

   const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
   					const char *path);

Since the configuration for submodules for a submodule are identified by a unique commit sha1 and its unique path (or unique name) I think it is feasible to make it a singleton. That would also simplify using it from the existing config parsing code.

To be future proof I can internally keep the object oriented approach always passing on the submodule_config_cache pointer. That way it would be easy to expose to the outside in case we later need multiple caches.

So I currently I do not see any downside of making it a singleton to the outside and would go with that.

Show 11 quoted lines

> > +/* one submodule_config_cache entry */
> > +struct submodule_config {
> > +	struct strbuf path;
> > +	struct strbuf name;
> > +	unsigned char gitmodule_sha1[20];
> > +};
> 
> Do these strings need changed after they are written once? If not, you
> may want to just make these bare pointers (you can still use strbufs to
> create them, and then strbuf_detach at the end). That may just be a matter of
> taste, though.

No they do not need to be changed after parsing, since every path, name mapping should be unique in one .gitmodule file. And I think it actually would make the code more clear in one instance where I directly set the buf pointer which Jonathan mentioned. There it is needed only for the hashmap lookup.

Cheers Heiko