git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

Previous: Seyi KuforijiNext: Seyi Kuforiji
Message 4 of 30 in “oidmap: migrate cleanup to oidmap_clear_with_free()”
  1. 0/5 oidmap: migrate cleanup to oidmap_clear_with_free()Seyi Kuforiji, Feb 27, 2026
  2. 1/5 oidmap: make entry cleanup explicit in oidmap_clearSeyi Kuforiji, Feb 27, 2026
  3. 2/5 builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free()Seyi Kuforiji, Feb 27, 2026
  4. Junio C HamanoFeb 28, 2026
  5. 3/5 list-objects-filter: use oidmap_clear_with_free() for cleanupSeyi Kuforiji, Feb 27, 2026
  6. 4/5 odb: use oidmap_clear_with_free() to release replace_map entriesSeyi Kuforiji, Feb 27, 2026
  7. 5/5 sequencer: use oidmap_clear_with_free() for string_entry cleanupSeyi Kuforiji, Feb 27, 2026
  8. 0/5 oidmap: migrate cleanup to oidmap_clear_with_free()Seyi Kuforiji, Mar 2, 2026
  9. 1/5 oidmap: make entry cleanup explicit in oidmap_clearSeyi Kuforiji, Mar 2, 2026
  10. Junio C HamanoMar 2, 2026
  11. 2/5 builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free()Seyi Kuforiji, Mar 2, 2026
  12. Junio C HamanoMar 2, 2026
  13. Patrick SteinhardtMar 4, 2026
  14. 3/5 list-objects-filter: use oidmap_clear_with_free() for cleanupSeyi Kuforiji, Mar 2, 2026
  15. Junio C HamanoMar 2, 2026
  16. Patrick SteinhardtMar 4, 2026
  17. Junio C HamanoMar 4, 2026
  18. Seyi KuforijiMar 4, 2026
  19. Junio C HamanoMar 4, 2026
  20. Junio C HamanoMar 4, 2026
  21. 4/5 odb: use oidmap_clear_with_free() to release replace_map entriesSeyi Kuforiji, Mar 2, 2026
  22. Junio C HamanoMar 2, 2026
  23. 5/5 sequencer: use oidmap_clear_with_free() for string_entry cleanupSeyi Kuforiji, Mar 2, 2026
  24. Junio C HamanoMar 2, 2026
  25. Patrick SteinhardtMar 4, 2026
  26. 0/2 oidmap: migrate cleanup to oidmap_clear_with_free()Seyi Kuforiji, Mar 5, 2026
  27. 1/2 oidmap: make entry cleanup explicit in oidmap_clearSeyi Kuforiji, Mar 5, 2026
  28. 2/2 builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free()Seyi Kuforiji, Mar 5, 2026
  29. Patrick SteinhardtMar 5, 2026
  30. Junio C HamanoMar 5, 2026

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.