Re: [PATCH] oidmap: ensure map is initialized

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 27, 2017, 18:01 UTC
Message-ID: <xmqqtvwcnj6w.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171222232729.253936-1-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 6 quoted lines

> Ensure that an oidmap is initialized before attempting to add, remove,
> or retrieve an entry by simply performing the initialization step
> before accessing the underlying hashmap.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---

Looks sane.  Thanks for illustrating the idea with actual code.

Essentially, you are using map->map.cmpfn as a boolean to see "have we initialized this thing? if not, we need to initialize it on demand".

By the way, I am somewhat more sympathetic than usual to Dscho's "make oidmap_get() very aware of the internal implementation detail of hashmap_get_from_hash() to micro-optimize by removing the check from _get()". Such a layering violation is disgusting, and making it deliberately shows an even worse design taste, but in this particular case, because the oidmap API is too thin a layer on top of hashmap, it is understandably a very tempting approach.

Show 38 quoted lines

>  oidmap.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/oidmap.c b/oidmap.c
> index 6db4fffcd..d9fb19ba6 100644
> --- a/oidmap.c
> +++ b/oidmap.c
> @@ -33,12 +33,19 @@ void oidmap_free(struct oidmap *map, int free_entries)
>  
>  void *oidmap_get(const struct oidmap *map, const struct object_id *key)
>  {
> +	if (!map->map.cmpfn)
> +		return NULL;
> +
>  	return hashmap_get_from_hash(&map->map, hash(key), key);
>  }
>  
>  void *oidmap_remove(struct oidmap *map, const struct object_id *key)
>  {
>  	struct hashmap_entry entry;
> +
> +	if (!map->map.cmpfn)
> +		oidmap_init(map, 0);
> +
>  	hashmap_entry_init(&entry, hash(key));
>  	return hashmap_remove(&map->map, &entry, key);
>  }
> @@ -46,6 +53,10 @@ void *oidmap_remove(struct oidmap *map, const struct object_id *key)
>  void *oidmap_put(struct oidmap *map, void *entry)
>  {
>  	struct oidmap_entry *to_put = entry;
> +
> +	if (!map->map.cmpfn)
> +		oidmap_init(map, 0);
> +
>  	hashmap_entry_init(&to_put->internal_entry, hash(&to_put->oid));
>  	return hashmap_put(&map->map, to_put);
>  }