Re: [RFC/WIP PATCH] implement reading of submodule .gitmodules configuration into cache

[Junio C Hamano <gitster@pobox.com>]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> This submodule configuration cache allows us to lazily read .gitmodules
> configurations by commit into a runtime cache which can then be used to
> easily lookup values from it. Currently only the values for path or name
> are stored but it can be extended for any value needed.
> ...

Thanks.

> diff --git a/submodule-config-cache.c b/submodule-config-cache.c
> new file mode 100644
> index 0000000..7253fad
> --- /dev/null
> +++ b/submodule-config-cache.c
> @@ -0,0 +1,96 @@
> +#include "cache.h"
> +#include "submodule-config-cache.h"
> +#include "strbuf.h"
> +#include "hash.h"
> +
> +void submodule_config_cache_init(struct submodule_config_cache *cache)
> +{
> +	init_hash(&cache->for_name);
> +	init_hash(&cache->for_path);
> +}
> +
> +static int free_one_submodule_config(void *ptr, void *data)
> +{
> +	struct submodule_config *entry = ptr;
> +
> +	strbuf_release(&entry->path);
> +	strbuf_release(&entry->name);
> +	free(entry);
> +
> +	return 0;
> +}
> +
> +void submodule_config_cache_free(struct submodule_config_cache *cache)
> +{
> +	/* NOTE: its important to iterate over the name hash here
> +	 * since paths might have multiple entries */

Style (multi-line comments).

This is interesting.  I wonder what the practical consequence is to
have a single submodule bound to the top-level tree more than once.
Updating from one of the working tree will make the other working
tree out of sync because the ultimate location of the submodule
directory pointed at by the two .git gitdirs can only have a single
HEAD, be it detached or on a branch, and a single index.

Not that the decision to enforce that names are unique in the
top-level .gitmodules, and follow that decision in this part of the
code to be defensive (not rely on the "one submodule can be bound
only once to a top-level tree"), but shouldn't such a configuration
to have a single submodule bound to more than one place in the
top-level tree be forbidden?

> +	for_each_hash(&cache->for_name, free_one_submodule_config, NULL);
> +	free_hash(&cache->for_path);
> +	free_hash(&cache->for_name);
> +}
> +
> +static unsigned int hash_sha1_string(const unsigned char *sha1, const char *string)
> +{
> +	int c;
> +	unsigned int hash, string_hash = 5381;
> +	memcpy(&hash, sha1, sizeof(hash));
> +
> +	/* djb2 hash */
> +	while ((c = *string++))
> +		string_hash = ((string_hash << 5) + hash) + c; /* hash * 33 + c */

Hmm, the comment and the code does not seem to match in math here...