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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 2, 2026, 22:23 UTC
Message-ID: <xmqqfr6hyior.fsf@gitster.g>
In-Reply-To: <20260302200018.75731-2-kuforiji98@gmail.com>

Seyi Kuforiji <kuforiji98@gmail.com> writes:

Show 9 quoted lines

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

Show 6 quoted lines

> +{
> +	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.

Show 10 quoted lines

> +	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);
>  }