* [PATCH v2 1/5] oidmap: make entry cleanup explicit in oidmap_clear
2026-03-02 20:00 ` [PATCH v2 0/5] oidmap: migrate cleanup to oidmap_clear_with_free() Seyi Kuforiji
@ 2026-03-02 20:00 ` Seyi Kuforiji
2026-03-02 22:23 ` Junio C Hamano
2026-03-02 20:00 ` [PATCH v2 2/5] builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free() Seyi Kuforiji
` (4 subsequent siblings)
5 siblings, 1 reply; 30+ messages in thread
From: Seyi Kuforiji @ 2026-03-02 20:00 UTC (permalink / raw)
To: git; +Cc: ps, Seyi Kufoiji
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v2 1/5] oidmap: make entry cleanup explicit in oidmap_clear
2026-03-02 20:00 ` [PATCH v2 1/5] oidmap: make entry cleanup explicit in oidmap_clear Seyi Kuforiji
@ 2026-03-02 22:23 ` Junio C Hamano
0 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2026-03-02 22:23 UTC (permalink / raw)
To: Seyi Kuforiji; +Cc: git, ps
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);
> }
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 2/5] builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free()
2026-03-02 20:00 ` [PATCH v2 0/5] oidmap: migrate cleanup to oidmap_clear_with_free() Seyi Kuforiji
2026-03-02 20:00 ` [PATCH v2 1/5] oidmap: make entry cleanup explicit in oidmap_clear Seyi Kuforiji
@ 2026-03-02 20:00 ` Seyi Kuforiji
2026-03-02 22:26 ` Junio C Hamano
2026-03-04 6:57 ` Patrick Steinhardt
2026-03-02 20:00 ` [PATCH v2 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup Seyi Kuforiji
` (3 subsequent siblings)
5 siblings, 2 replies; 30+ messages in thread
From: Seyi Kuforiji @ 2026-03-02 20:00 UTC (permalink / raw)
To: git; +Cc: ps, Seyi Kufoiji
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v2 2/5] builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free()
2026-03-02 20:00 ` [PATCH v2 2/5] builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free() Seyi Kuforiji
@ 2026-03-02 22:26 ` Junio C Hamano
2026-03-04 6:57 ` Patrick Steinhardt
1 sibling, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2026-03-02 22:26 UTC (permalink / raw)
To: Seyi Kuforiji; +Cc: git, ps
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);
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 2/5] builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free()
2026-03-02 20:00 ` [PATCH v2 2/5] builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free() Seyi Kuforiji
2026-03-02 22:26 ` Junio C Hamano
@ 2026-03-04 6:57 ` Patrick Steinhardt
1 sibling, 0 replies; 30+ messages in thread
From: Patrick Steinhardt @ 2026-03-04 6:57 UTC (permalink / raw)
To: Seyi Kuforiji; +Cc: git
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup
2026-03-02 20:00 ` [PATCH v2 0/5] oidmap: migrate cleanup to oidmap_clear_with_free() Seyi Kuforiji
2026-03-02 20:00 ` [PATCH v2 1/5] oidmap: make entry cleanup explicit in oidmap_clear Seyi Kuforiji
2026-03-02 20:00 ` [PATCH v2 2/5] builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free() Seyi Kuforiji
@ 2026-03-02 20:00 ` Seyi Kuforiji
2026-03-02 22:30 ` Junio C Hamano
2026-03-02 20:00 ` [PATCH v2 4/5] odb: use oidmap_clear_with_free() to release replace_map entries Seyi Kuforiji
` (2 subsequent siblings)
5 siblings, 1 reply; 30+ messages in thread
From: Seyi Kuforiji @ 2026-03-02 20:00 UTC (permalink / raw)
To: git; +Cc: ps, Seyi Kufoiji
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v2 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup
2026-03-02 20:00 ` [PATCH v2 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup Seyi Kuforiji
@ 2026-03-02 22:30 ` Junio C Hamano
2026-03-04 6:57 ` Patrick Steinhardt
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2026-03-02 22:30 UTC (permalink / raw)
To: Seyi Kuforiji; +Cc: git, ps
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);
> }
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup
2026-03-02 22:30 ` Junio C Hamano
@ 2026-03-04 6:57 ` Patrick Steinhardt
2026-03-04 15:31 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: Patrick Steinhardt @ 2026-03-04 6:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Seyi Kuforiji, git
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup
2026-03-04 6:57 ` Patrick Steinhardt
@ 2026-03-04 15:31 ` Junio C Hamano
2026-03-04 19:29 ` Seyi Kuforiji
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2026-03-04 15:31 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Seyi Kuforiji, git
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup
2026-03-04 15:31 ` Junio C Hamano
@ 2026-03-04 19:29 ` Seyi Kuforiji
2026-03-04 20:30 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: Seyi Kuforiji @ 2026-03-04 19:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, git
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup
2026-03-04 19:29 ` Seyi Kuforiji
@ 2026-03-04 20:30 ` Junio C Hamano
2026-03-04 21:43 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2026-03-04 20:30 UTC (permalink / raw)
To: Seyi Kuforiji; +Cc: Patrick Steinhardt, git
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup
2026-03-04 20:30 ` Junio C Hamano
@ 2026-03-04 21:43 ` Junio C Hamano
0 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2026-03-04 21:43 UTC (permalink / raw)
To: Seyi Kuforiji; +Cc: Patrick Steinhardt, git
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 4/5] odb: use oidmap_clear_with_free() to release replace_map entries
2026-03-02 20:00 ` [PATCH v2 0/5] oidmap: migrate cleanup to oidmap_clear_with_free() Seyi Kuforiji
` (2 preceding siblings ...)
2026-03-02 20:00 ` [PATCH v2 3/5] list-objects-filter: use oidmap_clear_with_free() for cleanup Seyi Kuforiji
@ 2026-03-02 20:00 ` Seyi Kuforiji
2026-03-02 22:35 ` Junio C Hamano
2026-03-02 20:00 ` [PATCH v2 5/5] sequencer: use oidmap_clear_with_free() for string_entry cleanup Seyi Kuforiji
2026-03-05 10:05 ` [PATCH v3 0/2] oidmap: migrate cleanup to oidmap_clear_with_free() Seyi Kuforiji
5 siblings, 1 reply; 30+ messages in thread
From: Seyi Kuforiji @ 2026-03-02 20:00 UTC (permalink / raw)
To: git; +Cc: ps, Seyi Kufoiji
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v2 4/5] odb: use oidmap_clear_with_free() to release replace_map entries
2026-03-02 20:00 ` [PATCH v2 4/5] odb: use oidmap_clear_with_free() to release replace_map entries Seyi Kuforiji
@ 2026-03-02 22:35 ` Junio C Hamano
0 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2026-03-02 22:35 UTC (permalink / raw)
To: Seyi Kuforiji; +Cc: git, ps
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);
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 5/5] sequencer: use oidmap_clear_with_free() for string_entry cleanup
2026-03-02 20:00 ` [PATCH v2 0/5] oidmap: migrate cleanup to oidmap_clear_with_free() Seyi Kuforiji
` (3 preceding siblings ...)
2026-03-02 20:00 ` [PATCH v2 4/5] odb: use oidmap_clear_with_free() to release replace_map entries Seyi Kuforiji
@ 2026-03-02 20:00 ` Seyi Kuforiji
2026-03-02 22:38 ` Junio C Hamano
2026-03-05 10:05 ` [PATCH v3 0/2] oidmap: migrate cleanup to oidmap_clear_with_free() Seyi Kuforiji
5 siblings, 1 reply; 30+ messages in thread
From: Seyi Kuforiji @ 2026-03-02 20:00 UTC (permalink / raw)
To: git; +Cc: ps, Seyi Kufoiji
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v2 5/5] sequencer: use oidmap_clear_with_free() for string_entry cleanup
2026-03-02 20:00 ` [PATCH v2 5/5] sequencer: use oidmap_clear_with_free() for string_entry cleanup Seyi Kuforiji
@ 2026-03-02 22:38 ` Junio C Hamano
2026-03-04 6:57 ` Patrick Steinhardt
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2026-03-02 22:38 UTC (permalink / raw)
To: Seyi Kuforiji; +Cc: git, ps
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);
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 5/5] sequencer: use oidmap_clear_with_free() for string_entry cleanup
2026-03-02 22:38 ` Junio C Hamano
@ 2026-03-04 6:57 ` Patrick Steinhardt
0 siblings, 0 replies; 30+ messages in thread
From: Patrick Steinhardt @ 2026-03-04 6:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Seyi Kuforiji, git
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 0/2] oidmap: migrate cleanup to oidmap_clear_with_free()
2026-03-02 20:00 ` [PATCH v2 0/5] oidmap: migrate cleanup to oidmap_clear_with_free() Seyi Kuforiji
` (4 preceding siblings ...)
2026-03-02 20:00 ` [PATCH v2 5/5] sequencer: use oidmap_clear_with_free() for string_entry cleanup Seyi Kuforiji
@ 2026-03-05 10:05 ` Seyi Kuforiji
2026-03-05 10:05 ` [PATCH v3 1/2] oidmap: make entry cleanup explicit in oidmap_clear Seyi Kuforiji
` (3 more replies)
5 siblings, 4 replies; 30+ messages in thread
From: Seyi Kuforiji @ 2026-03-05 10:05 UTC (permalink / raw)
To: git; +Cc: ps, 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
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH v3 1/2] oidmap: make entry cleanup explicit in oidmap_clear
2026-03-05 10:05 ` [PATCH v3 0/2] oidmap: migrate cleanup to oidmap_clear_with_free() Seyi Kuforiji
@ 2026-03-05 10:05 ` Seyi Kuforiji
2026-03-05 10:05 ` [PATCH v3 2/2] builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free() Seyi Kuforiji
` (2 subsequent siblings)
3 siblings, 0 replies; 30+ messages in thread
From: Seyi Kuforiji @ 2026-03-05 10:05 UTC (permalink / raw)
To: git; +Cc: ps, Seyi Kufoiji
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v3 2/2] builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free()
2026-03-05 10:05 ` [PATCH v3 0/2] oidmap: migrate cleanup to oidmap_clear_with_free() Seyi Kuforiji
2026-03-05 10:05 ` [PATCH v3 1/2] oidmap: make entry cleanup explicit in oidmap_clear Seyi Kuforiji
@ 2026-03-05 10:05 ` Seyi Kuforiji
2026-03-05 14:27 ` [PATCH v3 0/2] oidmap: migrate " Patrick Steinhardt
2026-03-05 19:17 ` Junio C Hamano
3 siblings, 0 replies; 30+ messages in thread
From: Seyi Kuforiji @ 2026-03-05 10:05 UTC (permalink / raw)
To: git; +Cc: ps, Seyi Kufoiji
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v3 0/2] oidmap: migrate cleanup to oidmap_clear_with_free()
2026-03-05 10:05 ` [PATCH v3 0/2] oidmap: migrate cleanup to oidmap_clear_with_free() Seyi Kuforiji
2026-03-05 10:05 ` [PATCH v3 1/2] oidmap: make entry cleanup explicit in oidmap_clear Seyi Kuforiji
2026-03-05 10:05 ` [PATCH v3 2/2] builtin/rev-list: migrate missing_objects cleanup to oidmap_clear_with_free() Seyi Kuforiji
@ 2026-03-05 14:27 ` Patrick Steinhardt
2026-03-05 19:17 ` Junio C Hamano
3 siblings, 0 replies; 30+ messages in thread
From: Patrick Steinhardt @ 2026-03-05 14:27 UTC (permalink / raw)
To: Seyi Kuforiji; +Cc: git
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 0/2] oidmap: migrate cleanup to oidmap_clear_with_free()
2026-03-05 10:05 ` [PATCH v3 0/2] oidmap: migrate cleanup to oidmap_clear_with_free() Seyi Kuforiji
` (2 preceding siblings ...)
2026-03-05 14:27 ` [PATCH v3 0/2] oidmap: migrate " Patrick Steinhardt
@ 2026-03-05 19:17 ` Junio C Hamano
3 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2026-03-05 19:17 UTC (permalink / raw)
To: Seyi Kuforiji; +Cc: git, ps
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.
^ permalink raw reply [flat|nested] 30+ messages in thread