Re: [PATCH v2 1/5] oidmap: make entry cleanup explicit in oidmap_clear

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

Seyi Kuforiji <kuforiji98@gmail.com> writes:

>  void oidmap_clear(struct oidmap *map, int free_entries)
>  {
> -	if (!map)
> +	oidmap_clear_with_free(map,
> +		free_entries ? free : NULL);
> +}
> +
> +void oidmap_clear_with_free(struct oidmap *map,
> +			    oidmap_free_fn free_fn)

Made me briefly wonder if passing "void free(void *)" or NULL as
oidmap_free_fn would be flagged by the compilers as suspicious, but 
"typedef void (*oidmap_free_fn)(void *);" is obviously compatible
with both, so it is good.

> +{
> +	struct hashmap_iter iter;
> +	struct hashmap_entry *e;
> +
> +	if (!map || !map->map.cmpfn)
>  		return;

The first half prepares our oidmap_clear() to be fed a NULL map, but
what about the new condition?  Where did it come from?  What makes
it suddenly necessary that in order to "clear" an oidmap you already
have to have defined cmpfn?

> -	/* TODO: make oidmap itself not depend on struct layouts */
> -	hashmap_clear_(&map->map, free_entries ? 0 : -1);

This is now achieved by the use of container_of(), right?  Nice.

> +	hashmap_iter_init(&map->map, &iter);
> +	while ((e = hashmap_iter_next(&iter))) {
> +		struct oidmap_entry *entry =
> +			container_of(e, struct oidmap_entry, internal_entry);
> +		if (free_fn)
> +			free_fn(entry);
> +	}
> +
> +	hashmap_clear(&map->map);
>  }