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

From: Jeff King <peff@peff.net>
Date: Mar 11, 2014, 21:58 UTC
Message-ID: <20140311215808.GB32129@sigill.intra.peff.net>
In-Reply-To: <20140310212412.GA8693@sandbox-ub>

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.

But take all of this with a giant grain of salt, as I am not too familiar with the needs of the callers.

Show 6 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.

-Peff