Re: [PATCH 2/5] builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 28, 2026, 00:09 UTC
- Message-ID
- <xmqqcy1pohjz.fsf@gitster.g>
- In-Reply-To
- <20260227234213.17633-3-kuforiji98@gmail.com>
Seyi Kuforiji <kuforiji98@gmail.com> writes:
Show 53 quoted lines
> 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...