[PATCH 0/5] oidmap: migrate cleanup to oidmap_clear_with_free()

[PATCH 0/5] oidmap: migrate cleanup to oidmap_clear_with_free()

[Seyi Kuforiji]

Hi,

This series replaces oidmap_clear(map, 1) with
oidmap_clear_with_free() and introduces explicit free callbacks
at the remaining call sites.

The old boolean-based API implicitly assumed plain free(),
which obscures ownership semantics and does not work well
when oidmap_entry is embedded inside larger structures.
The callback-based API makes cleanup explicit and type-safe,
and avoids relying on hidden assumptions about allocation.

This improves readability, maintainability, and correctness,
and makes future refactoring of oidmap users more robust.

This is used in subsequent commits to adequately cleanup all
usage site.

Thanks,
Seyi Kuforiji

Seyi Kufoiji (5):
  oidmap: make entry cleanup explicit in oidmap_clear
  builtin/rev-list: migrate missing_objects cleanup to
    oidmap_clear_with_free()
  list-objects-filter: use oidmap_clear_with_free() for cleanup
  odb: use oidmap_clear_with_free() to release replace_map entries
  sequencer: use oidmap_clear_with_free() for string_entry cleanup

 builtin/rev-list.c      | 13 ++++++++++---
 list-objects-filter.c   |  9 ++++++++-
 odb.c                   | 11 ++++++++++-
 oidmap.c                | 23 ++++++++++++++++++++---
 oidmap.h                | 15 +++++++++++++++
 sequencer.c             | 10 ++++++++--
 t/unit-tests/u-oidmap.c | 41 +++++++++++++++++++++++++++++++++++++++++
 7 files changed, 112 insertions(+), 10 deletions(-)

-- 
2.43.0

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

[Seyi Kuforiji]

From: Seyi Kufoiji <kuforiji98@gmail.com>

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 <kuforiji98@gmail.com>
---
 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);
+}
-- 
2.43.0

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

[Seyi Kuforiji]

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);
 	}
 
 	stop_progress(&progress);
-- 
2.43.0

[PATCH 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup

[Seyi Kuforiji]

From: Seyi Kufoiji <kuforiji98@gmail.com>

Replace the use of oidmap_clear(&seen_at_depth, 1) in
filter_trees_free() with oidmap_clear_with_free().

The seen_at_depth map stores heap-allocated struct
seen_map_entry objects. Previously, passing 1 relied on
oidmap_clear() internally calling free() on each entry.

Convert this to the explicit oidmap_clear_with_free() API
and provide a typed free_seen_map_entry() helper to free
each container entry.

This makes the ownership and cleanup policy explicit and
removes reliance on the legacy boolean free_entries
parameter.

Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 list-objects-filter.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/list-objects-filter.c b/list-objects-filter.c
index 78316e7f90..0038bfaac5 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -143,6 +143,13 @@ struct seen_map_entry {
 	size_t depth;
 };
 
+static void free_seen_map_entry(void *e)
+{
+	struct seen_map_entry *entry =
+		container_of(e, struct seen_map_entry, base);
+	free(entry);
+}
+
 /* Returns 1 if the oid was in the omits set before it was invoked. */
 static int filter_trees_update_omits(
 	struct object *obj,
@@ -244,7 +251,7 @@ static void filter_trees_free(void *filter_data) {
 	struct filter_trees_depth_data *d = filter_data;
 	if (!d)
 		return;
-	oidmap_clear(&d->seen_at_depth, 1);
+	oidmap_clear_with_free(&d->seen_at_depth, free_seen_map_entry);
 	free(d);
 }
 
-- 
2.43.0

[PATCH 4/5] odb: use oidmap_clear_with_free() to release replace_map entries

[Seyi Kuforiji]

From: Seyi Kufoiji <kuforiji98@gmail.com>

Replace the direct oidmap_clear() call in odb_free() with
oidmap_clear_with_free(), and introduce a free_replace_map_entry()
helper to properly free each struct replace_object stored in the map.

This centralizes cleanup logic and ensures entries are released
correctly via a dedicated callback.

Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 odb.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/odb.c b/odb.c
index 1679cc0465..8ca497203f 100644
--- a/odb.c
+++ b/odb.c
@@ -14,6 +14,7 @@
 #include "object-file-convert.h"
 #include "object-file.h"
 #include "odb.h"
+#include "oidmap.h"
 #include "packfile.h"
 #include "path.h"
 #include "promisor-remote.h"
@@ -1089,6 +1090,13 @@ void odb_close(struct object_database *o)
 	close_commit_graph(o);
 }
 
+static void free_replace_map_entry(void *e)
+{
+	struct replace_object *entry =
+		container_of(e, struct replace_object, original);
+	free(entry);
+}
+
 static void odb_free_sources(struct object_database *o)
 {
 	while (o->sources) {
@@ -1109,7 +1117,8 @@ void odb_free(struct object_database *o)
 
 	free(o->alternate_db);
 
-	oidmap_clear(&o->replace_map, 1);
+	if (o->replace_map_initialized)
+		oidmap_clear_with_free(&o->replace_map, free_replace_map_entry);
 	pthread_mutex_destroy(&o->replace_mutex);
 
 	odb_close(o);
-- 
2.43.0

[PATCH 5/5] sequencer: use oidmap_clear_with_free() for string_entry cleanup

[Seyi Kuforiji]

From: Seyi Kufoiji <kuforiji98@gmail.com>

Switch cleanup of the string_entry oidmap to
oidmap_clear_with_free() and introduce a free_string_entry()
helper to properly free each allocated struct string_entry.

This aligns with the ongoing migration to use the callback-based
oidmap cleanup API.

Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 sequencer.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index a3eb39bb25..75ef2ace4f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5654,6 +5654,12 @@ struct string_entry {
 	char string[FLEX_ARRAY];
 };
 
+static void free_string_entry(void *e)
+{
+	struct string_entry *entry = container_of(e, struct string_entry, entry);
+	free(entry);
+}
+
 struct label_state {
 	struct oidmap commit2label;
 	struct hashmap labels;
@@ -6044,8 +6050,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 	oidset_clear(&interesting);
 	oidset_clear(&child_seen);
 	oidset_clear(&shown);
-	oidmap_clear(&commit2todo, 1);
-	oidmap_clear(&state.commit2label, 1);
+	oidmap_clear_with_free(&commit2todo, free_string_entry);
+	oidmap_clear_with_free(&state.commit2label, free_string_entry);
 	hashmap_clear_and_free(&state.labels, struct labels_entry, entry);
 	strbuf_release(&state.buf);
 
-- 
2.43.0

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

[Junio C Hamano]

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

[PATCH v2 0/5] oidmap: migrate cleanup to oidmap_clear_with_free()

[Seyi Kuforiji]

Hi,

This series replaces oidmap_clear(map, 1) with
oidmap_clear_with_free() and introduces explicit free callbacks
at the remaining call sites.

The old boolean-based API implicitly assumed plain free(),
which obscures ownership semantics and does not work well
when oidmap_entry is embedded inside larger structures.
The callback-based API makes cleanup explicit and type-safe,
and avoids relying on hidden assumptions about allocation.

This is used in subsequent commits to adequately cleanup all
usage site.

Changes in v2:
 - fix missing .path cleanup
 - modified commit message to be more accurate

Thanks
Seyi

Seyi Kufoiji (5):
  oidmap: make entry cleanup explicit in oidmap_clear
  builtin/rev-list: migrate missing_objects cleanup to
    oidmap_clear_with_free()
  list-objects-filter: use oidmap_clear_with_free() for cleanup
  odb: use oidmap_clear_with_free() to release replace_map entries
  sequencer: use oidmap_clear_with_free() for string_entry cleanup

 builtin/rev-list.c      | 15 ++++++++++++---
 list-objects-filter.c   |  9 ++++++++-
 odb.c                   | 11 ++++++++++-
 oidmap.c                | 23 ++++++++++++++++++++---
 oidmap.h                | 15 +++++++++++++++
 sequencer.c             | 10 ++++++++--
 t/unit-tests/u-oidmap.c | 41 +++++++++++++++++++++++++++++++++++++++++
 7 files changed, 114 insertions(+), 10 deletions(-)

Range-diff against v1:
1:  a0ab068630 < -:  ---------- sparse-checkout: use string_list_sort_u
2:  b2c0ee2593 < -:  ---------- The 7th batch
3:  6af69c2c49 = 1:  1d544ef7d2 oidmap: make entry cleanup explicit in oidmap_clear
4:  452f5d8edb ! 2:  f2c3a699bd builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free()
    @@ Metadata
     Author: Seyi Kufoiji <kuforiji98@gmail.com>
     
      ## Commit message ##
    -    builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free()
    +    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().
    @@ Commit message
     
         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.
    +    single place and making cleanup semantics explicit.
     
         Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
     
    @@ builtin/rev-list.c: static int arg_print_omitted; /* print objects omitted by fi
     +{
     +	struct missing_objects_map_entry *entry =
     +		container_of(e, struct missing_objects_map_entry, entry);
    ++
    ++	free(entry->path);
     +	free(entry);
     +}
     +
5:  cbfcc4e9cc = 3:  a4e426bcca list-objects-filter: use oidmap_clear_with_free() for cleanup
6:  bfbdb51d84 = 4:  4116e5491d odb: use oidmap_clear_with_free() to release replace_map entries
7:  4fc6e71f4c = 5:  ad1f776a19 sequencer: use oidmap_clear_with_free() for string_entry cleanup
-- 
2.43.0

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

[Seyi Kuforiji]

From: Seyi Kufoiji <kuforiji98@gmail.com>

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 <kuforiji98@gmail.com>
---
 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);
+}
-- 
2.43.0

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

[Seyi Kuforiji]

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.

Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 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 ddea8aa251..ab5f69826c 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 free_missing_objects_entry(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, free_missing_objects_entry);
 	}
 
 	stop_progress(&progress);
-- 
2.43.0

[PATCH v2 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup

[Seyi Kuforiji]

From: Seyi Kufoiji <kuforiji98@gmail.com>

Replace the use of oidmap_clear(&seen_at_depth, 1) in
filter_trees_free() with oidmap_clear_with_free().

The seen_at_depth map stores heap-allocated struct
seen_map_entry objects. Previously, passing 1 relied on
oidmap_clear() internally calling free() on each entry.

Convert this to the explicit oidmap_clear_with_free() API
and provide a typed free_seen_map_entry() helper to free
each container entry.

This makes the ownership and cleanup policy explicit and
removes reliance on the legacy boolean free_entries
parameter.

Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 list-objects-filter.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/list-objects-filter.c b/list-objects-filter.c
index 78316e7f90..0038bfaac5 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -143,6 +143,13 @@ struct seen_map_entry {
 	size_t depth;
 };
 
+static void free_seen_map_entry(void *e)
+{
+	struct seen_map_entry *entry =
+		container_of(e, struct seen_map_entry, base);
+	free(entry);
+}
+
 /* Returns 1 if the oid was in the omits set before it was invoked. */
 static int filter_trees_update_omits(
 	struct object *obj,
@@ -244,7 +251,7 @@ static void filter_trees_free(void *filter_data) {
 	struct filter_trees_depth_data *d = filter_data;
 	if (!d)
 		return;
-	oidmap_clear(&d->seen_at_depth, 1);
+	oidmap_clear_with_free(&d->seen_at_depth, free_seen_map_entry);
 	free(d);
 }
 
-- 
2.43.0

[PATCH v2 4/5] odb: use oidmap_clear_with_free() to release replace_map entries

[Seyi Kuforiji]

From: Seyi Kufoiji <kuforiji98@gmail.com>

Replace the direct oidmap_clear() call in odb_free() with
oidmap_clear_with_free(), and introduce a free_replace_map_entry()
helper to properly free each struct replace_object stored in the map.

This centralizes cleanup logic and ensures entries are released
correctly via a dedicated callback.

Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 odb.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/odb.c b/odb.c
index 1679cc0465..8ca497203f 100644
--- a/odb.c
+++ b/odb.c
@@ -14,6 +14,7 @@
 #include "object-file-convert.h"
 #include "object-file.h"
 #include "odb.h"
+#include "oidmap.h"
 #include "packfile.h"
 #include "path.h"
 #include "promisor-remote.h"
@@ -1089,6 +1090,13 @@ void odb_close(struct object_database *o)
 	close_commit_graph(o);
 }
 
+static void free_replace_map_entry(void *e)
+{
+	struct replace_object *entry =
+		container_of(e, struct replace_object, original);
+	free(entry);
+}
+
 static void odb_free_sources(struct object_database *o)
 {
 	while (o->sources) {
@@ -1109,7 +1117,8 @@ void odb_free(struct object_database *o)
 
 	free(o->alternate_db);
 
-	oidmap_clear(&o->replace_map, 1);
+	if (o->replace_map_initialized)
+		oidmap_clear_with_free(&o->replace_map, free_replace_map_entry);
 	pthread_mutex_destroy(&o->replace_mutex);
 
 	odb_close(o);
-- 
2.43.0

[PATCH v2 5/5] sequencer: use oidmap_clear_with_free() for string_entry cleanup

[Seyi Kuforiji]

From: Seyi Kufoiji <kuforiji98@gmail.com>

Switch cleanup of the string_entry oidmap to
oidmap_clear_with_free() and introduce a free_string_entry()
helper to properly free each allocated struct string_entry.

This aligns with the ongoing migration to use the callback-based
oidmap cleanup API.

Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 sequencer.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index a3eb39bb25..75ef2ace4f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5654,6 +5654,12 @@ struct string_entry {
 	char string[FLEX_ARRAY];
 };
 
+static void free_string_entry(void *e)
+{
+	struct string_entry *entry = container_of(e, struct string_entry, entry);
+	free(entry);
+}
+
 struct label_state {
 	struct oidmap commit2label;
 	struct hashmap labels;
@@ -6044,8 +6050,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 	oidset_clear(&interesting);
 	oidset_clear(&child_seen);
 	oidset_clear(&shown);
-	oidmap_clear(&commit2todo, 1);
-	oidmap_clear(&state.commit2label, 1);
+	oidmap_clear_with_free(&commit2todo, free_string_entry);
+	oidmap_clear_with_free(&state.commit2label, free_string_entry);
 	hashmap_clear_and_free(&state.labels, struct labels_entry, entry);
 	strbuf_release(&state.buf);
 
-- 
2.43.0

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

[Junio C Hamano]

Seyi Kuforiji <kuforiji98@gmail.com> writes:

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

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

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

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

[Junio C Hamano]

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.
>
> Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
> ---
>  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 ddea8aa251..ab5f69826c 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 free_missing_objects_entry(void *e)
> +{
> +	struct missing_objects_map_entry *entry =
> +		container_of(e, struct missing_objects_map_entry, entry);
> +
> +	free(entry->path);
> +	free(entry);
> +}

Unlike the previous iteration, this makes the claim very believable
that it makes the code easier to follow to have the shell and its
contents released together in a single function.

Looking good.

>  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, free_missing_objects_entry);
>  	}
>  
>  	stop_progress(&progress);

Re: [PATCH v2 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup

[Junio C Hamano]

Seyi Kuforiji <kuforiji98@gmail.com> writes:

> diff --git a/list-objects-filter.c b/list-objects-filter.c
> index 78316e7f90..0038bfaac5 100644
> --- a/list-objects-filter.c
> +++ b/list-objects-filter.c
> @@ -143,6 +143,13 @@ struct seen_map_entry {
>  	size_t depth;
>  };
>  
> +static void free_seen_map_entry(void *e)
> +{
> +	struct seen_map_entry *entry =
> +		container_of(e, struct seen_map_entry, base);
> +	free(entry);
> +}

As there is *no* extra resources held in seen_map_entry other than
the shell itself, this step alone does not make the code any clearer
to follow.  But if we are going to add new members to the structure
in the future, the story will change and we'll leap the same benefit
as we saw in [PATCH v2 2/5].

> @@ -244,7 +251,7 @@ static void filter_trees_free(void *filter_data) {
>  	struct filter_trees_depth_data *d = filter_data;
>  	if (!d)
>  		return;
> -	oidmap_clear(&d->seen_at_depth, 1);
> +	oidmap_clear_with_free(&d->seen_at_depth, free_seen_map_entry);
>  	free(d);
>  }

Re: [PATCH v2 4/5] odb: use oidmap_clear_with_free() to release replace_map entries

[Junio C Hamano]

Seyi Kuforiji <kuforiji98@gmail.com> writes:

> +static void free_replace_map_entry(void *e)
> +{
> +	struct replace_object *entry =
> +		container_of(e, struct replace_object, original);
> +	free(entry);
> +}
> +

The same comment as [PATCH v2 3/5].


> @@ -1109,7 +1117,8 @@ void odb_free(struct object_database *o)
>  
>  	free(o->alternate_db);
>  
> -	oidmap_clear(&o->replace_map, 1);
> +	if (o->replace_map_initialized)
> +		oidmap_clear_with_free(&o->replace_map, free_replace_map_entry);

It is a bit unfortunate that we need to know how o->replace_map is
initialized and maintained.  I wondered if we can do this without
peeking into o->replace_map_initialized, but o->replace_map is
already an instance of the map, not a pointer that points at a
lazily initialized instance of a map, so that cannot be done (and we
would not have replace_map_initialized member in the object_database
struct in the first place, if we can tell if o.replace_map needs
clearing by simply looking at it).

So, all OK, I guess.

>  	pthread_mutex_destroy(&o->replace_mutex);
>  
>  	odb_close(o);

Re: [PATCH v2 5/5] sequencer: use oidmap_clear_with_free() for string_entry cleanup

[Junio C Hamano]

Seyi Kuforiji <kuforiji98@gmail.com> writes:

> From: Seyi Kufoiji <kuforiji98@gmail.com>
>
> Switch cleanup of the string_entry oidmap to
> oidmap_clear_with_free() and introduce a free_string_entry()
> helper to properly free each allocated struct string_entry.
>
> This aligns with the ongoing migration to use the callback-based
> oidmap cleanup API.
>
> Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
> ---
>  sequencer.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index a3eb39bb25..75ef2ace4f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5654,6 +5654,12 @@ struct string_entry {
>  	char string[FLEX_ARRAY];
>  };
>  
> +static void free_string_entry(void *e)
> +{
> +	struct string_entry *entry = container_of(e, struct string_entry, entry);
> +	free(entry);
> +}

Exactly the same comment applies to this step as [PATCH v2 3/5].

In other words, with the current codebase, these three steps in the
context of the current code are uninteresting with little value, but
if we ever add a member to these entries that hold their own
resources, it would become easier to manage the lifetime rules of
them.

> @@ -6044,8 +6050,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>  	oidset_clear(&interesting);
>  	oidset_clear(&child_seen);
>  	oidset_clear(&shown);
> -	oidmap_clear(&commit2todo, 1);
> -	oidmap_clear(&state.commit2label, 1);
> +	oidmap_clear_with_free(&commit2todo, free_string_entry);
> +	oidmap_clear_with_free(&state.commit2label, free_string_entry);
>  	hashmap_clear_and_free(&state.labels, struct labels_entry, entry);
>  	strbuf_release(&state.buf);

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

[Patrick Steinhardt]

On Mon, Mar 02, 2026 at 09:00:14PM +0100, Seyi Kuforiji wrote:
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index ddea8aa251..ab5f69826c 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 free_missing_objects_entry(void *e)

Nit: this should be called `missing_objects_map_entry_free()` according
to our coding guidelines.

Patrick

Re: [PATCH v2 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup

[Patrick Steinhardt]

On Mon, Mar 02, 2026 at 02:30:07PM -0800, Junio C Hamano wrote:
> Seyi Kuforiji <kuforiji98@gmail.com> writes:
> 
> > diff --git a/list-objects-filter.c b/list-objects-filter.c
> > index 78316e7f90..0038bfaac5 100644
> > --- a/list-objects-filter.c
> > +++ b/list-objects-filter.c
> > @@ -143,6 +143,13 @@ struct seen_map_entry {
> >  	size_t depth;
> >  };
> >  
> > +static void free_seen_map_entry(void *e)
> > +{
> > +	struct seen_map_entry *entry =
> > +		container_of(e, struct seen_map_entry, base);
> > +	free(entry);
> > +}
> 
> As there is *no* extra resources held in seen_map_entry other than
> the shell itself, this step alone does not make the code any clearer
> to follow.  But if we are going to add new members to the structure
> in the future, the story will change and we'll leap the same benefit
> as we saw in [PATCH v2 2/5].

Agreed. But I think with the current status quo I'd rather drop this
patch though as it may otherwise make the reader scratch their head why
we do the exercise in the first place.

Patrick

Re: [PATCH v2 5/5] sequencer: use oidmap_clear_with_free() for string_entry cleanup

[Patrick Steinhardt]

On Mon, Mar 02, 2026 at 02:38:36PM -0800, Junio C Hamano wrote:
> Seyi Kuforiji <kuforiji98@gmail.com> writes:
> 
> > From: Seyi Kufoiji <kuforiji98@gmail.com>
> >
> > Switch cleanup of the string_entry oidmap to
> > oidmap_clear_with_free() and introduce a free_string_entry()
> > helper to properly free each allocated struct string_entry.
> >
> > This aligns with the ongoing migration to use the callback-based
> > oidmap cleanup API.
> >
> > Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
> > ---
> >  sequencer.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index a3eb39bb25..75ef2ace4f 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -5654,6 +5654,12 @@ struct string_entry {
> >  	char string[FLEX_ARRAY];
> >  };
> >  
> > +static void free_string_entry(void *e)
> > +{
> > +	struct string_entry *entry = container_of(e, struct string_entry, entry);
> > +	free(entry);
> > +}
> 
> Exactly the same comment applies to this step as [PATCH v2 3/5].
> 
> In other words, with the current codebase, these three steps in the
> context of the current code are uninteresting with little value, but
> if we ever add a member to these entries that hold their own
> resources, it would become easier to manage the lifetime rules of
> them.

Personally I'd lean towards keeping the first two patches and drop the
remaining ones though. I think it makes the code harder to understand to
convert all callsites of `oidmap_clear()`, even if it doesn't actually
provide a benefit.

We can still convert callsites to use `oidmap_clear_with_free()` in case
they grow additional allocations per entry that we'll have to care
about.

Thanks!

Patrick

Re: [PATCH v2 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Agreed. But I think with the current status quo I'd rather drop this
> patch though as it may otherwise make the reader scratch their head why
> we do the exercise in the first place.

I do not think too strongly either way myself, but you may be right.

Unless we are dropping the "we optionally let you free the shell"
traditional interface, it is of questionable value to use the new
interface.

Thanks.

Re: [PATCH v2 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup

[Seyi Kuforiji]

On Wed, 4 Mar 2026 at 16:31, Junio C Hamano <gitster@pobox.com> wrote:
>
> Patrick Steinhardt <ps@pks.im> writes:
>
> > Agreed. But I think with the current status quo I'd rather drop this
> > patch though as it may otherwise make the reader scratch their head why
> > we do the exercise in the first place.
>
> I do not think too strongly either way myself, but you may be right.
>
> Unless we are dropping the "we optionally let you free the shell"
> traditional interface, it is of questionable value to use the new
> interface.
>
> Thanks.
>

Hello

Thank you so much for the reviews.

I'll send a new version dropping the [PATCH 3/5].

Thanks,
Seyi

Re: [PATCH v2 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup

[Junio C Hamano]

Seyi Kuforiji <kuforiji98@gmail.com> writes:

> On Wed, 4 Mar 2026 at 16:31, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>> > Agreed. But I think with the current status quo I'd rather drop this
>> > patch though as it may otherwise make the reader scratch their head why
>> > we do the exercise in the first place.
>>
>> I do not think too strongly either way myself, but you may be right.
>>
>> Unless we are dropping the "we optionally let you free the shell"
>> traditional interface, it is of questionable value to use the new
>> interface.
>>
>> Thanks.
>>
>
> Hello
>
> Thank you so much for the reviews.
>
> I'll send a new version dropping the [PATCH 3/5].

I thought that Patrick wants to see only [1/5] and [2/5], discarding
the rest (i.e. 3/5, 4/5, and 5/5).  If that is the plan, I do not
think we need any resend.

Thanks.

Re: [PATCH v2 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Seyi Kuforiji <kuforiji98@gmail.com> writes:
>
>> On Wed, 4 Mar 2026 at 16:31, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> Patrick Steinhardt <ps@pks.im> writes:
>>>
>>> > Agreed. But I think with the current status quo I'd rather drop this
>>> > patch though as it may otherwise make the reader scratch their head why
>>> > we do the exercise in the first place.
>>>
>>> I do not think too strongly either way myself, but you may be right.
>>>
>>> Unless we are dropping the "we optionally let you free the shell"
>>> traditional interface, it is of questionable value to use the new
>>> interface.
>>>
>>> Thanks.
>>>
>>
>> Hello
>>
>> Thank you so much for the reviews.
>>
>> I'll send a new version dropping the [PATCH 3/5].
>
> I thought that Patrick wants to see only [1/5] and [2/5], discarding
> the rest (i.e. 3/5, 4/5, and 5/5).  If that is the plan, I do not
> think we need any resend.

Ah, in https://lore.kernel.org/git/aafX5CmP82WYFyIb@pks.im/ he wants
the callback to be renamed, so we do need a new iteration (v3).  I
still think that if you are to drop [3/5], then [4/5] and [5/5]
should also be dropped, leaving only the first two patches.

Thanks.

[PATCH v3 0/2] oidmap: migrate cleanup to oidmap_clear_with_free()

[Seyi Kuforiji]

Hi,

This series replaces oidmap_clear(map, 1) with
oidmap_clear_with_free() and introduces explicit free callbacks
at the remaining call sites.

The old boolean-based API implicitly assumed plain free(),
which obscures ownership semantics and does not work well
when oidmap_entry is embedded inside larger structures.
The callback-based API makes cleanup explicit and type-safe,
and avoids relying on hidden assumptions about allocation.

This is used in subsequent commits to adequately cleanup all
usage site.

Changes in v3:
 - rename funtion to meet guidlines
 - drop [PATCH 3/5]

Thanks
Seyi

Seyi Kufoiji (2):
  oidmap: make entry cleanup explicit in oidmap_clear
  builtin/rev-list: migrate missing_objects cleanup to
    oidmap_clear_with_free()

 builtin/rev-list.c      | 15 ++++++++++++---
 oidmap.c                | 23 ++++++++++++++++++++---
 oidmap.h                | 15 +++++++++++++++
 t/unit-tests/u-oidmap.c | 41 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 88 insertions(+), 6 deletions(-)

Range-diff against v2:
1:  1d544ef7d2 = 1:  a050491441 oidmap: make entry cleanup explicit in oidmap_clear
2:  f2c3a699bd ! 2:  b592d765e3 builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free()
    @@ builtin/rev-list.c: static int arg_print_omitted; /* print objects omitted by fi
      	unsigned type;
      };
     +
    -+static void free_missing_objects_entry(void *e)
    ++static void missing_objects_map_entry_free(void *e)
     +{
     +	struct missing_objects_map_entry *entry =
     +		container_of(e, struct missing_objects_map_entry, entry);
    @@ builtin/rev-list.c: int cmd_rev_list(int argc,
      		}
      
     -		oidmap_clear(&missing_objects, true);
    -+		oidmap_clear_with_free(&missing_objects, free_missing_objects_entry);
    ++		oidmap_clear_with_free(&missing_objects, missing_objects_map_entry_free);
      	}
      
      	stop_progress(&progress);
3:  a4e426bcca < -:  ---------- list-objects-filter: use oidmap_clear_with_free() for cleanup
4:  4116e5491d < -:  ---------- odb: use oidmap_clear_with_free() to release replace_map entries
5:  ad1f776a19 < -:  ---------- sequencer: use oidmap_clear_with_free() for string_entry cleanup
-- 
2.43.0

[PATCH v3 1/2] oidmap: make entry cleanup explicit in oidmap_clear

[Seyi Kuforiji]

From: Seyi Kufoiji <kuforiji98@gmail.com>

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 <kuforiji98@gmail.com>
---
 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);
+}
-- 
2.43.0

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

[Seyi Kuforiji]

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.

Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 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 ddea8aa251..854d82ece3 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);
-- 
2.43.0

Re: [PATCH v3 0/2] oidmap: migrate cleanup to oidmap_clear_with_free()

[Patrick Steinhardt]

On Thu, Mar 05, 2026 at 11:05:24AM +0100, Seyi Kuforiji wrote:
> Hi,
> 
> This series replaces oidmap_clear(map, 1) with
> oidmap_clear_with_free() and introduces explicit free callbacks
> at the remaining call sites.
> 
> The old boolean-based API implicitly assumed plain free(),
> which obscures ownership semantics and does not work well
> when oidmap_entry is embedded inside larger structures.
> The callback-based API makes cleanup explicit and type-safe,
> and avoids relying on hidden assumptions about allocation.
> 
> This is used in subsequent commits to adequately cleanup all
> usage site.
> 
> Changes in v3:
>  - rename funtion to meet guidlines
>  - drop [PATCH 3/5]

Thanks, this version looks good to me!

Patrick

Re: [PATCH v3 0/2] oidmap: migrate cleanup to oidmap_clear_with_free()

[Junio C Hamano]

Seyi Kuforiji <kuforiji98@gmail.com> writes:

> Range-diff against v2:
> 1:  1d544ef7d2 = 1:  a050491441 oidmap: make entry cleanup explicit in oidmap_clear
> 2:  f2c3a699bd ! 2:  b592d765e3 builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free()
>     @@ builtin/rev-list.c: static int arg_print_omitted; /* print objects omitted by fi
>       	unsigned type;
>       };
>      +
>     -+static void free_missing_objects_entry(void *e)
>     ++static void missing_objects_map_entry_free(void *e)
>      +{
>      +	struct missing_objects_map_entry *entry =
>      +		container_of(e, struct missing_objects_map_entry, entry);
>     @@ builtin/rev-list.c: int cmd_rev_list(int argc,
>       		}
>       
>      -		oidmap_clear(&missing_objects, true);
>     -+		oidmap_clear_with_free(&missing_objects, free_missing_objects_entry);
>     ++		oidmap_clear_with_free(&missing_objects, missing_objects_map_entry_free);
>       	}
>       
>       	stop_progress(&progress);
> 3:  a4e426bcca < -:  ---------- list-objects-filter: use oidmap_clear_with_free() for cleanup
> 4:  4116e5491d < -:  ---------- odb: use oidmap_clear_with_free() to release replace_map entries
> 5:  ad1f776a19 < -:  ---------- sequencer: use oidmap_clear_with_free() for string_entry cleanup

I think these cover everything Patrick pointed out, and I agree with
what these remaining two patches do.  Will queue.

Let me mark the topic for 'next'.

Thanks.