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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 9, 2017, 20:34 UTC
Message-ID: <xmqqh943p0hv.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <a944446c4c374125082f5ad8b79e731704b66196.1486629195.git.mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Aside from scaling better, this means that the submodule name needn't be
> stored in the ref_store instance anymore (which will be changed in a
> moment).

Nice. I like the latter reason very much (this is not a suggestion to change the description).

Show 17 quoted lines

> +struct submodule_hash_entry
> +{
> +	struct hashmap_entry ent; /* must be the first member! */
> +
> +	struct ref_store *refs;
> +
> +	/* NUL-terminated name of submodule: */
> +	char submodule[FLEX_ARRAY];
> +};
> +
> +static int submodule_hash_cmp(const void *entry, const void *entry_or_key,
> +			      const void *keydata)
> +{
> +	const struct submodule_hash_entry *e1 = entry, *e2 = entry_or_key;
> +	const char *submodule = keydata;
> +
> +	return strcmp(e1->submodule, submodule ? submodule : e2->submodule);

I would have found it more readable if it were like so:

	const char *submodule = keydata ? keydata : e2->submodule;

	return strcmp(e1->submodule, submodule);

but I suspect the difference is not that huge.

Show 7 quoted lines

> +}
> +
> +static struct submodule_hash_entry *alloc_submodule_hash_entry(
> +		const char *submodule, struct ref_store *refs)
> +{
> +	size_t len = strlen(submodule);
> +	struct submodule_hash_entry *entry = malloc(sizeof(*entry) + len + 1);

I think this (and the later memcpy) is what FLEX_ALLOC_MEM() was invented for.

Show 18 quoted lines

> +	hashmap_entry_init(entry, strhash(submodule));
> +	entry->refs = refs;
> +	memcpy(entry->submodule, submodule, len + 1);
> +	return entry;
> +}
> ...
> @@ -1373,16 +1405,17 @@ void base_ref_store_init(struct ref_store *refs,
>  			die("BUG: main_ref_store initialized twice");
>  
>  		refs->submodule = "";
> -		refs->next = NULL;
>  		main_ref_store = refs;
>  	} else {
> -		if (lookup_ref_store(submodule))
> +		refs->submodule = xstrdup(submodule);
> +
> +		if (!submodule_ref_stores.tablesize)
> +			hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);

Makes me wonder what "20" stands for. Perhaps the caller should be allowed to say "I do not quite care what initial size is" by passing 0 or some equally but more clealy meaningless value (which of course would be outside the scope of this series).