Merge branch 'sk/oidmap-clear-with-custom-free-func' into jch

A bit of OIDmap API enhancement and cleanup.

Comments?

* sk/oidmap-clear-with-custom-free-func:
  sequencer: use oidmap_clear_with_free() for string_entry cleanup
  odb: use oidmap_clear_with_free() to release replace_map entries
  list-objects-filter: use oidmap_clear_with_free() for cleanup
  builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free()
  oidmap: make entry cleanup explicit in oidmap_clear

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

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

odb.c

@@ -13,6 +13,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"
@@ -977,6 +978,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) {
@@ -997,7 +1005,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);

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)

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.

sequencer.c

@@ -5705,6 +5705,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;
@@ -6095,8 +6101,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);
 

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