[PATCH 0/3] minor oidmap cleanups

[PATCH 0/3] minor oidmap cleanups

[Jeff King]

Just cleaning out old topics from my tree, and I found this one.

  [1/3]: oidmap: rename oidmap_free() to oidmap_clear()
  [2/3]: oidmap: add size function
  [3/3]: raw_object_store: drop extra pointer to replace_map

 builtin/rev-list.c      | 2 +-
 commit-graph.c          | 2 +-
 list-objects-filter.c   | 2 +-
 object-store.c          | 3 +--
 object-store.h          | 3 ++-
 oidmap.c                | 2 +-
 oidmap.h                | 9 +++++++--
 replace-object.c        | 8 +++-----
 replace-object.h        | 2 +-
 sequencer.c             | 4 ++--
 t/unit-tests/u-oidmap.c | 2 +-
 11 files changed, 21 insertions(+), 18 deletions(-)

-Peff

[PATCH 1/3] oidmap: rename oidmap_free() to oidmap_clear()

[Jeff King]

This function does not free the oidmap struct itself; it just drops all
items from the map (using hashmap_clear_() internally). It should be
called oidmap_clear(), per CodingGuidelines.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/rev-list.c      | 2 +-
 list-objects-filter.c   | 2 +-
 object-store.c          | 2 +-
 oidmap.c                | 2 +-
 oidmap.h                | 5 +++--
 sequencer.c             | 4 ++--
 t/unit-tests/u-oidmap.c | 2 +-
 7 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index c4cd4ed5c8..0984b607bf 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -924,7 +924,7 @@ int cmd_rev_list(int argc,
 			free((void *)entry->path);
 		}
 
-		oidmap_free(&missing_objects, true);
+		oidmap_clear(&missing_objects, true);
 	}
 
 	stop_progress(&progress);
diff --git a/list-objects-filter.c b/list-objects-filter.c
index 7765761b3c..78b397bc19 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -244,7 +244,7 @@ static void filter_trees_free(void *filter_data) {
 	struct filter_trees_depth_data *d = filter_data;
 	if (!d)
 		return;
-	oidmap_free(&d->seen_at_depth, 1);
+	oidmap_clear(&d->seen_at_depth, 1);
 	free(d);
 }
 
diff --git a/object-store.c b/object-store.c
index 6ab50d25d3..bc24e80829 100644
--- a/object-store.c
+++ b/object-store.c
@@ -1017,7 +1017,7 @@ void raw_object_store_clear(struct raw_object_store *o)
 {
 	FREE_AND_NULL(o->alternate_db);
 
-	oidmap_free(o->replace_map, 1);
+	oidmap_clear(o->replace_map, 1);
 	FREE_AND_NULL(o->replace_map);
 	pthread_mutex_destroy(&o->replace_mutex);
 
diff --git a/oidmap.c b/oidmap.c
index 8b1bc4dec9..508d6c7dec 100644
--- a/oidmap.c
+++ b/oidmap.c
@@ -22,7 +22,7 @@ void oidmap_init(struct oidmap *map, size_t initial_size)
 	hashmap_init(&map->map, oidmap_neq, NULL, initial_size);
 }
 
-void oidmap_free(struct oidmap *map, int free_entries)
+void oidmap_clear(struct oidmap *map, int free_entries)
 {
 	if (!map)
 		return;
diff --git a/oidmap.h b/oidmap.h
index fad412827a..603ae1adbc 100644
--- a/oidmap.h
+++ b/oidmap.h
@@ -36,12 +36,13 @@ struct oidmap {
 void oidmap_init(struct oidmap *map, size_t initial_size);
 
 /*
- * Frees an oidmap structure and allocated memory.
+ * Clear an oidmap, freeing any allocated memory. The map is empty and
+ * can be reused without another explicit init.
  *
  * If `free_entries` is true, each oidmap_entry in the map is freed as well
  * using stdlibs free().
  */
-void oidmap_free(struct oidmap *map, int free_entries);
+void oidmap_clear(struct oidmap *map, int free_entries);
 
 /*
  * Returns the oidmap entry for the specified oid, or NULL if not found.
diff --git a/sequencer.c b/sequencer.c
index b5c4043757..7fa24db143 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6053,8 +6053,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 	oidset_clear(&interesting);
 	oidset_clear(&child_seen);
 	oidset_clear(&shown);
-	oidmap_free(&commit2todo, 1);
-	oidmap_free(&state.commit2label, 1);
+	oidmap_clear(&commit2todo, 1);
+	oidmap_clear(&state.commit2label, 1);
 	hashmap_clear_and_free(&state.labels, struct labels_entry, entry);
 	strbuf_release(&state.buf);
 
diff --git a/t/unit-tests/u-oidmap.c b/t/unit-tests/u-oidmap.c
index dc805b7e3c..b23af449f6 100644
--- a/t/unit-tests/u-oidmap.c
+++ b/t/unit-tests/u-oidmap.c
@@ -35,7 +35,7 @@ void test_oidmap__initialize(void)
 
 void test_oidmap__cleanup(void)
 {
-	oidmap_free(&map, 1);
+	oidmap_clear(&map, 1);
 }
 
 void test_oidmap__replace(void)
-- 
2.49.0.821.gd3b3298025

[PATCH 2/3] oidmap: add size function

[Jeff King]

Callers which want to know how many items are in an oidmap have to look
at the underlying hashmap struct, leaking an implementation detail.
Let's provide a type-appropriate wrapper and use it.

Note in the call from lookup_replace_object(), the caller was actually
looking at the hashmap's tablesize parameter (the allocated size of the
table) rather than hashmap_get_size(), the number of items in the table.
This probably should have been checking the number of items all along,
but the two are functionally equivalent here since we only add to the
map and never remove anything. Thus if there was any allocation, it was
because there is at least one item.

Signed-off-by: Jeff King <peff@peff.net>
---
 commit-graph.c   | 2 +-
 oidmap.h         | 4 ++++
 replace-object.h | 2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index 6394752b0b..1a74e1e1ba 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -222,7 +222,7 @@ static int commit_graph_compatible(struct repository *r)
 
 	if (replace_refs_enabled(r)) {
 		prepare_replace_object(r);
-		if (hashmap_get_size(&r->objects->replace_map->map))
+		if (oidmap_get_size(r->objects->replace_map))
 			return 0;
 	}
 
diff --git a/oidmap.h b/oidmap.h
index 603ae1adbc..67fb32290f 100644
--- a/oidmap.h
+++ b/oidmap.h
@@ -67,6 +67,10 @@ void *oidmap_put(struct oidmap *map, void *entry);
  */
 void *oidmap_remove(struct oidmap *map, const struct object_id *key);
 
+static inline unsigned int oidmap_get_size(struct oidmap *map)
+{
+	return hashmap_get_size(&map->map);
+}
 
 struct oidmap_iter {
 	struct hashmap_iter h_iter;
diff --git a/replace-object.h b/replace-object.h
index ba478eb30c..4226376534 100644
--- a/replace-object.h
+++ b/replace-object.h
@@ -47,7 +47,7 @@ static inline const struct object_id *lookup_replace_object(struct repository *r
 {
 	if (!replace_refs_enabled(r) ||
 	    (r->objects->replace_map_initialized &&
-	     r->objects->replace_map->map.tablesize == 0))
+	     oidmap_get_size(r->objects->replace_map) == 0))
 		return oid;
 	return do_lookup_replace_object(r, oid);
 }
-- 
2.49.0.821.gd3b3298025

Re: [PATCH 2/3] oidmap: add size function

[Patrick Steinhardt]

On Mon, May 12, 2025 at 02:51:30PM -0400, Jeff King wrote:
> Callers which want to know how many items are in an oidmap have to look
> at the underlying hashmap struct, leaking an implementation detail.
> Let's provide a type-appropriate wrapper and use it.
> 
> Note in the call from lookup_replace_object(), the caller was actually
> looking at the hashmap's tablesize parameter (the allocated size of the
> table) rather than hashmap_get_size(), the number of items in the table.
> This probably should have been checking the number of items all along,
> but the two are functionally equivalent here since we only add to the
> map and never remove anything. Thus if there was any allocation, it was
> because there is at least one item.

I was a bit puzzled by this explanation initially. The two sizes aren't
functioally equivalent -- the table size will typically be larger than
the number of contained entries. But the thing is that we don't care for
the actual size, we only care whether the map is empty or not. And for
that those are indeed equivalent in this specific case.

Patrick

Re: [PATCH 2/3] oidmap: add size function

[Jeff King]

On Tue, May 13, 2025 at 11:48:36AM +0200, Patrick Steinhardt wrote:

> On Mon, May 12, 2025 at 02:51:30PM -0400, Jeff King wrote:
> > Callers which want to know how many items are in an oidmap have to look
> > at the underlying hashmap struct, leaking an implementation detail.
> > Let's provide a type-appropriate wrapper and use it.
> > 
> > Note in the call from lookup_replace_object(), the caller was actually
> > looking at the hashmap's tablesize parameter (the allocated size of the
> > table) rather than hashmap_get_size(), the number of items in the table.
> > This probably should have been checking the number of items all along,
> > but the two are functionally equivalent here since we only add to the
> > map and never remove anything. Thus if there was any allocation, it was
> > because there is at least one item.
> 
> I was a bit puzzled by this explanation initially. The two sizes aren't
> functioally equivalent -- the table size will typically be larger than
> the number of contained entries. But the thing is that we don't care for
> the actual size, we only care whether the map is empty or not. And for
> that those are indeed equivalent in this specific case.

Yep, exactly. Probably replacing "looking at X" with "checking whether X
is empty" would have been more clear. I of course was looking at the
diff while writing this, so took it for granted. ;)

I don't think it's worth a re-roll, though, especially since this is in
next already. Thanks for the review.

-Peff

[PATCH 3/3] raw_object_store: drop extra pointer to replace_map

[Jeff King]

We store the replacement data in an oidmap, which is itself a pointer in
the raw_object_store struct. But there's no need for an extra pointer
indirection here. It is always allocated and initialized along with the
containing struct, and we never check it for NULL-ness.

Let's embed the map directly in the struct, which is simpler and avoids
extra pointer chasing.

Signed-off-by: Jeff King <peff@peff.net>
---
This one may be more subjective, but IMHO it's good to avoid extra
pointers when we can.

 commit-graph.c   | 2 +-
 object-store.c   | 3 +--
 object-store.h   | 3 ++-
 replace-object.c | 8 +++-----
 replace-object.h | 2 +-
 5 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index 1a74e1e1ba..4a6e34f8a0 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -222,7 +222,7 @@ static int commit_graph_compatible(struct repository *r)
 
 	if (replace_refs_enabled(r)) {
 		prepare_replace_object(r);
-		if (oidmap_get_size(r->objects->replace_map))
+		if (oidmap_get_size(&r->objects->replace_map))
 			return 0;
 	}
 
diff --git a/object-store.c b/object-store.c
index bc24e80829..911bc7ff5f 100644
--- a/object-store.c
+++ b/object-store.c
@@ -1017,8 +1017,7 @@ void raw_object_store_clear(struct raw_object_store *o)
 {
 	FREE_AND_NULL(o->alternate_db);
 
-	oidmap_clear(o->replace_map, 1);
-	FREE_AND_NULL(o->replace_map);
+	oidmap_clear(&o->replace_map, 1);
 	pthread_mutex_destroy(&o->replace_mutex);
 
 	free_commit_graph(o->commit_graph);
diff --git a/object-store.h b/object-store.h
index 46961dc954..9f6f27c016 100644
--- a/object-store.h
+++ b/object-store.h
@@ -5,6 +5,7 @@
 #include "object.h"
 #include "list.h"
 #include "oidset.h"
+#include "oidmap.h"
 #include "thread-utils.h"
 
 struct oidmap;
@@ -176,7 +177,7 @@ struct raw_object_store {
 	 * Objects that should be substituted by other objects
 	 * (see git-replace(1)).
 	 */
-	struct oidmap *replace_map;
+	struct oidmap replace_map;
 	unsigned replace_map_initialized : 1;
 	pthread_mutex_t replace_mutex; /* protect object replace functions */
 
diff --git a/replace-object.c b/replace-object.c
index 7b8a09b5cb..f8c5f68837 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -31,7 +31,7 @@ static int register_replace_ref(const char *refname,
 	oidcpy(&repl_obj->replacement, oid);
 
 	/* Register new object */
-	if (oidmap_put(r->objects->replace_map, repl_obj))
+	if (oidmap_put(&r->objects->replace_map, repl_obj))
 		die(_("duplicate replace ref: %s"), refname);
 
 	return 0;
@@ -48,9 +48,7 @@ void prepare_replace_object(struct repository *r)
 		return;
 	}
 
-	r->objects->replace_map =
-		xmalloc(sizeof(*r->objects->replace_map));
-	oidmap_init(r->objects->replace_map, 0);
+	oidmap_init(&r->objects->replace_map, 0);
 
 	refs_for_each_replace_ref(get_main_ref_store(r),
 				  register_replace_ref, r);
@@ -80,7 +78,7 @@ const struct object_id *do_lookup_replace_object(struct repository *r,
 	/* Try to recursively replace the object */
 	while (depth-- > 0) {
 		struct replace_object *repl_obj =
-			oidmap_get(r->objects->replace_map, cur);
+			oidmap_get(&r->objects->replace_map, cur);
 		if (!repl_obj)
 			return cur;
 		cur = &repl_obj->replacement;
diff --git a/replace-object.h b/replace-object.h
index 4226376534..3052e96a62 100644
--- a/replace-object.h
+++ b/replace-object.h
@@ -47,7 +47,7 @@ static inline const struct object_id *lookup_replace_object(struct repository *r
 {
 	if (!replace_refs_enabled(r) ||
 	    (r->objects->replace_map_initialized &&
-	     oidmap_get_size(r->objects->replace_map) == 0))
+	     oidmap_get_size(&r->objects->replace_map) == 0))
 		return oid;
 	return do_lookup_replace_object(r, oid);
 }
-- 
2.49.0.821.gd3b3298025

Re: [PATCH 3/3] raw_object_store: drop extra pointer to replace_map

[Patrick Steinhardt]

On Mon, May 12, 2025 at 02:52:33PM -0400, Jeff King wrote:
> We store the replacement data in an oidmap, which is itself a pointer in
> the raw_object_store struct. But there's no need for an extra pointer
> indirection here. It is always allocated and initialized along with the
> containing struct, and we never check it for NULL-ness.
> 
> Let's embed the map directly in the struct, which is simpler and avoids
> extra pointer chasing.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This one may be more subjective, but IMHO it's good to avoid extra
> pointers when we can.

Yup, I agree it is a sensible step. There is no good reason why the map
should be allocated, so let's just not.

All of these cleanups in this series look good to me. Thanks!

Patrick

Re: [PATCH 3/3] raw_object_store: drop extra pointer to replace_map

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Mon, May 12, 2025 at 02:52:33PM -0400, Jeff King wrote:
>> We store the replacement data in an oidmap, which is itself a pointer in
>> the raw_object_store struct. But there's no need for an extra pointer
>> indirection here. It is always allocated and initialized along with the
>> containing struct, and we never check it for NULL-ness.
>> 
>> Let's embed the map directly in the struct, which is simpler and avoids
>> extra pointer chasing.
>> 
>> Signed-off-by: Jeff King <peff@peff.net>
>> ---
>> This one may be more subjective, but IMHO it's good to avoid extra
>> pointers when we can.
>
> Yup, I agree it is a sensible step. There is no good reason why the map
> should be allocated, so let's just not.
>
> All of these cleanups in this series look good to me. Thanks!

Yup, looking very good.  Queued.