From a06a725c7847840ac56c0e797a829ac13abbe350 Mon Sep 17 00:00:00 2001 From: Seyi Kufoiji Date: Thu, 5 Mar 2026 11:05:25 +0100 Subject: [PATCH 1/2] oidmap: make entry cleanup explicit in oidmap_clear Replace oidmap's use of hashmap_clear_() and layout-dependent freeing with an explicit iteration and optional free callback. This removes reliance on struct layout assumptions while keeping the existing API intact. Add tests for oidmap_clear_with_free behavior. test_oidmap__clear_with_free_callback verifies that entries are freed when a callback is provided, while test_oidmap__clear_without_free_callback verifies that entries are not freed when no callback is given. These tests ensure the new clear implementation behaves correctly and preserves ownership semantics. Signed-off-by: Seyi Kuforiji Signed-off-by: Junio C Hamano --- oidmap.c | 23 ++++++++++++++++++++--- oidmap.h | 15 +++++++++++++++ t/unit-tests/u-oidmap.c | 41 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 3 deletions(-) diff --git a/oidmap.c b/oidmap.c index 508d6c7dec..a1ef53577f 100644 --- a/oidmap.c +++ b/oidmap.c @@ -24,11 +24,28 @@ void oidmap_init(struct oidmap *map, size_t initial_size) 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) +{ + struct hashmap_iter iter; + struct hashmap_entry *e; + + if (!map || !map->map.cmpfn) return; - /* TODO: make oidmap itself not depend on struct layouts */ - hashmap_clear_(&map->map, free_entries ? 0 : -1); + 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); } void *oidmap_get(const struct oidmap *map, const struct object_id *key) diff --git a/oidmap.h b/oidmap.h index 67fb32290f..acddcaecdd 100644 --- a/oidmap.h +++ b/oidmap.h @@ -35,6 +35,21 @@ struct oidmap { */ void oidmap_init(struct oidmap *map, size_t initial_size); +/* + * Function type for functions that free oidmap entries. + */ +typedef void (*oidmap_free_fn)(void *); + +/* + * Clear an oidmap, freeing any allocated memory. The map is empty and + * can be reused without another explicit init. + * + * The `free_fn`, if not NULL, is called for each oidmap entry in the map + * to free any user data associated with the entry. + */ +void oidmap_clear_with_free(struct oidmap *map, + oidmap_free_fn free_fn); + /* * Clear an oidmap, freeing any allocated memory. The map is empty and * can be reused without another explicit init. diff --git a/t/unit-tests/u-oidmap.c b/t/unit-tests/u-oidmap.c index b23af449f6..00481df63f 100644 --- a/t/unit-tests/u-oidmap.c +++ b/t/unit-tests/u-oidmap.c @@ -14,6 +14,13 @@ struct test_entry { char name[FLEX_ARRAY]; }; +static int freed; + +static void test_free_fn(void *p) { + freed++; + free(p); +} + static const char *const key_val[][2] = { { "11", "one" }, { "22", "two" }, { "33", "three" } }; @@ -134,3 +141,37 @@ void test_oidmap__iterate(void) cl_assert_equal_i(count, ARRAY_SIZE(key_val)); cl_assert_equal_i(hashmap_get_size(&map.map), ARRAY_SIZE(key_val)); } + +void test_oidmap__clear_without_free_callback(void) +{ + struct oidmap local_map = OIDMAP_INIT; + struct test_entry *entry; + + freed = 0; + + FLEX_ALLOC_STR(entry, name, "one"); + cl_parse_any_oid("11", &entry->entry.oid); + cl_assert(oidmap_put(&local_map, entry) == NULL); + + oidmap_clear_with_free(&local_map, NULL); + + cl_assert_equal_i(freed, 0); + + free(entry); +} + +void test_oidmap__clear_with_free_callback(void) +{ + struct oidmap local_map = OIDMAP_INIT; + struct test_entry *entry; + + freed = 0; + + FLEX_ALLOC_STR(entry, name, "one"); + cl_parse_any_oid("11", &entry->entry.oid); + cl_assert(oidmap_put(&local_map, entry) == NULL); + + oidmap_clear_with_free(&local_map, test_free_fn); + + cl_assert_equal_i(freed, 1); +} From a98ea50288c9fd39b501710635977478fb1f0a05 Mon Sep 17 00:00:00 2001 From: Seyi Kufoiji Date: Thu, 5 Mar 2026 11:05:26 +0100 Subject: [PATCH 2/2] builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free() 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. Signed-off-by: Seyi Kuforiji Signed-off-by: Junio C Hamano --- builtin/rev-list.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 99f876ba85..3bf30e1467 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -88,9 +88,19 @@ 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 missing_objects_map_entry_free(void *e) +{ + struct missing_objects_map_entry *entry = + container_of(e, struct missing_objects_map_entry, entry); + + free(entry->path); + free(entry); +} + static struct oidmap missing_objects; enum missing_action { MA_ERROR = 0, /* fail if any missing objects are encountered */ @@ -935,10 +945,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, missing_objects_map_entry_free); } stop_progress(&progress);