Re: [PATCH 2/5] builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free()

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

Seyi Kuforiji <kuforiji98@gmail.com> writes:

> From: Seyi Kufoiji <kuforiji98@gmail.com>
>
> As part of the conversion away from oidmap_clear(), switch the
> missing_objects map to use oidmap_clear_with_free().
>
> missing_objects stores struct missing_objects_map_entry instances,
> which own an xstrdup()'d path string in addition to the container
> struct itself. Previously, rev-list manually freed entry->path
> before calling oidmap_clear(&missing_objects, true).
>
> Introduce a dedicated free callback and pass it to
> oidmap_clear_with_free(), consolidating entry teardown into a
> single place and making cleanup semantics explicit. This improves
> clarity and maintainability.
>
> Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
> ---
>  builtin/rev-list.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index ddea8aa251..567dc5e7f5 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -88,9 +88,17 @@ static int arg_print_omitted; /* print objects omitted by filter */
>  
>  struct missing_objects_map_entry {
>  	struct oidmap_entry entry;
> -	const char *path;
> +	char *path;
>  	unsigned type;
>  };
> +
> +static void free_missing_objects_entry(void *e)
> +{
> +	struct missing_objects_map_entry *entry =
> +		container_of(e, struct missing_objects_map_entry, entry);
> +	free(entry);
> +}
> +
>  static struct oidmap missing_objects;
>  enum missing_action {
>  	MA_ERROR = 0,    /* fail if any missing objects are encountered */
> @@ -935,10 +943,9 @@ int cmd_rev_list(int argc,
>  		while ((entry = oidmap_iter_next(&iter))) {
>  			print_missing_object(entry, arg_missing_action ==
>  							    MA_PRINT_INFO);
> -			free((void *)entry->path);
>  		}
>  
> -		oidmap_clear(&missing_objects, true);
> +		oidmap_clear_with_free(&missing_objects, free_missing_objects_entry);
>  	}

Hmph, maybe I am confused, but the shape and memory ownership of the
structure involved has not changed before or after this patch.  We
have bunch of missing_objects_map_entry instances that embeds
oidmap_entry in each of them, and we iterate over this oidmap,
enumerating all the missing_objects_map_entry instances.

In the original code, we used to not just clear the oidmap used to
hold these missing_objects_map_entry instances, but dropped the
string pointed at and owned by the .path member of them while
iterating.  With the new code, I can see the newer interface
oidmap_clear_with_free() being told how to reclaim resources held by
these missing_objects_map_entry instances, so it is a perfect place
to not just free the "shell" structure, but also the piece of memory
held by the .path member, isn't it?  It appears to me that the
string pointed at by .path is no longer freed, introducing a leak?

The above hardly convinces me of the claim in the proposed log
message that this change improves clarity nor maintainability.  I am
puzzled...