{"thread":{"id":"63450","subject":"[PATCH 0/3] minor oidmap cleanups","startedAt":"2025-05-12T18:50:07Z","lastAt":"2025-05-14T17:59:44Z","messageCount":8,"participants":["Jeff King","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"517896","messageId":"20250512185006.GA1275914@coredump.intra.peff.net","threadId":"63450","inReplyTo":null,"subject":"[PATCH 0/3] minor oidmap cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-12T18:50:06Z","receivedAt":"2025-05-12T18:50:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Just cleaning out old topics from my tree, and I found this one.\n\n  [1/3]: oidmap: rename oidmap_free() to oidmap_clear()\n  [2/3]: oidmap: add size function\n  [3/3]: raw_object_store: drop extra pointer to replace_map\n\n builtin/rev-list.c      | 2 +-\n commit-graph.c          | 2 +-\n list-objects-filter.c   | 2 +-\n object-store.c          | 3 +--\n object-store.h          | 3 ++-\n oidmap.c                | 2 +-\n oidmap.h                | 9 +++++++--\n replace-object.c        | 8 +++-----\n replace-object.h        | 2 +-\n sequencer.c             | 4 ++--\n t/unit-tests/u-oidmap.c | 2 +-\n 11 files changed, 21 insertions(+), 18 deletions(-)\n\n-Peff\n"},{"id":"517897","messageId":"20250512185028.GA1276214@coredump.intra.peff.net","threadId":"63450","inReplyTo":"20250512185006.GA1275914@coredump.intra.peff.net","subject":"[PATCH 1/3] oidmap: rename oidmap_free() to oidmap_clear()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-12T18:50:28Z","receivedAt":"2025-05-12T18:50:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function does not free the oidmap struct itself; it just drops all\nitems from the map (using hashmap_clear_() internally). It should be\ncalled oidmap_clear(), per CodingGuidelines.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/rev-list.c      | 2 +-\n list-objects-filter.c   | 2 +-\n object-store.c          | 2 +-\n oidmap.c                | 2 +-\n oidmap.h                | 5 +++--\n sequencer.c             | 4 ++--\n t/unit-tests/u-oidmap.c | 2 +-\n 7 files changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex c4cd4ed5c8..0984b607bf 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -924,7 +924,7 @@ int cmd_rev_list(int argc,\n \t\t\tfree((void *)entry->path);\n \t\t}\n \n-\t\toidmap_free(&missing_objects, true);\n+\t\toidmap_clear(&missing_objects, true);\n \t}\n \n \tstop_progress(&progress);\ndiff --git a/list-objects-filter.c b/list-objects-filter.c\nindex 7765761b3c..78b397bc19 100644\n--- a/list-objects-filter.c\n+++ b/list-objects-filter.c\n@@ -244,7 +244,7 @@ static void filter_trees_free(void *filter_data) {\n \tstruct filter_trees_depth_data *d = filter_data;\n \tif (!d)\n \t\treturn;\n-\toidmap_free(&d->seen_at_depth, 1);\n+\toidmap_clear(&d->seen_at_depth, 1);\n \tfree(d);\n }\n \ndiff --git a/object-store.c b/object-store.c\nindex 6ab50d25d3..bc24e80829 100644\n--- a/object-store.c\n+++ b/object-store.c\n@@ -1017,7 +1017,7 @@ void raw_object_store_clear(struct raw_object_store *o)\n {\n \tFREE_AND_NULL(o->alternate_db);\n \n-\toidmap_free(o->replace_map, 1);\n+\toidmap_clear(o->replace_map, 1);\n \tFREE_AND_NULL(o->replace_map);\n \tpthread_mutex_destroy(&o->replace_mutex);\n \ndiff --git a/oidmap.c b/oidmap.c\nindex 8b1bc4dec9..508d6c7dec 100644\n--- a/oidmap.c\n+++ b/oidmap.c\n@@ -22,7 +22,7 @@ void oidmap_init(struct oidmap *map, size_t initial_size)\n \thashmap_init(&map->map, oidmap_neq, NULL, initial_size);\n }\n \n-void oidmap_free(struct oidmap *map, int free_entries)\n+void oidmap_clear(struct oidmap *map, int free_entries)\n {\n \tif (!map)\n \t\treturn;\ndiff --git a/oidmap.h b/oidmap.h\nindex fad412827a..603ae1adbc 100644\n--- a/oidmap.h\n+++ b/oidmap.h\n@@ -36,12 +36,13 @@ struct oidmap {\n void oidmap_init(struct oidmap *map, size_t initial_size);\n \n /*\n- * Frees an oidmap structure and allocated memory.\n+ * Clear an oidmap, freeing any allocated memory. The map is empty and\n+ * can be reused without another explicit init.\n  *\n  * If `free_entries` is true, each oidmap_entry in the map is freed as well\n  * using stdlibs free().\n  */\n-void oidmap_free(struct oidmap *map, int free_entries);\n+void oidmap_clear(struct oidmap *map, int free_entries);\n \n /*\n  * Returns the oidmap entry for the specified oid, or NULL if not found.\ndiff --git a/sequencer.c b/sequencer.c\nindex b5c4043757..7fa24db143 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -6053,8 +6053,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \toidset_clear(&interesting);\n \toidset_clear(&child_seen);\n \toidset_clear(&shown);\n-\toidmap_free(&commit2todo, 1);\n-\toidmap_free(&state.commit2label, 1);\n+\toidmap_clear(&commit2todo, 1);\n+\toidmap_clear(&state.commit2label, 1);\n \thashmap_clear_and_free(&state.labels, struct labels_entry, entry);\n \tstrbuf_release(&state.buf);\n \ndiff --git a/t/unit-tests/u-oidmap.c b/t/unit-tests/u-oidmap.c\nindex dc805b7e3c..b23af449f6 100644\n--- a/t/unit-tests/u-oidmap.c\n+++ b/t/unit-tests/u-oidmap.c\n@@ -35,7 +35,7 @@ void test_oidmap__initialize(void)\n \n void test_oidmap__cleanup(void)\n {\n-\toidmap_free(&map, 1);\n+\toidmap_clear(&map, 1);\n }\n \n void test_oidmap__replace(void)\n-- \n2.49.0.821.gd3b3298025\n\n"},{"id":"517898","messageId":"20250512185130.GB1276214@coredump.intra.peff.net","threadId":"63450","inReplyTo":"20250512185006.GA1275914@coredump.intra.peff.net","subject":"[PATCH 2/3] oidmap: add size function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-12T18:51:30Z","receivedAt":"2025-05-12T18:51:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Callers which want to know how many items are in an oidmap have to look\nat the underlying hashmap struct, leaking an implementation detail.\nLet's provide a type-appropriate wrapper and use it.\n\nNote in the call from lookup_replace_object(), the caller was actually\nlooking at the hashmap's tablesize parameter (the allocated size of the\ntable) rather than hashmap_get_size(), the number of items in the table.\nThis probably should have been checking the number of items all along,\nbut the two are functionally equivalent here since we only add to the\nmap and never remove anything. Thus if there was any allocation, it was\nbecause there is at least one item.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit-graph.c   | 2 +-\n oidmap.h         | 4 ++++\n replace-object.h | 2 +-\n 3 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 6394752b0b..1a74e1e1ba 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -222,7 +222,7 @@ static int commit_graph_compatible(struct repository *r)\n \n \tif (replace_refs_enabled(r)) {\n \t\tprepare_replace_object(r);\n-\t\tif (hashmap_get_size(&r->objects->replace_map->map))\n+\t\tif (oidmap_get_size(r->objects->replace_map))\n \t\t\treturn 0;\n \t}\n \ndiff --git a/oidmap.h b/oidmap.h\nindex 603ae1adbc..67fb32290f 100644\n--- a/oidmap.h\n+++ b/oidmap.h\n@@ -67,6 +67,10 @@ void *oidmap_put(struct oidmap *map, void *entry);\n  */\n void *oidmap_remove(struct oidmap *map, const struct object_id *key);\n \n+static inline unsigned int oidmap_get_size(struct oidmap *map)\n+{\n+\treturn hashmap_get_size(&map->map);\n+}\n \n struct oidmap_iter {\n \tstruct hashmap_iter h_iter;\ndiff --git a/replace-object.h b/replace-object.h\nindex ba478eb30c..4226376534 100644\n--- a/replace-object.h\n+++ b/replace-object.h\n@@ -47,7 +47,7 @@ static inline const struct object_id *lookup_replace_object(struct repository *r\n {\n \tif (!replace_refs_enabled(r) ||\n \t    (r->objects->replace_map_initialized &&\n-\t     r->objects->replace_map->map.tablesize == 0))\n+\t     oidmap_get_size(r->objects->replace_map) == 0))\n \t\treturn oid;\n \treturn do_lookup_replace_object(r, oid);\n }\n-- \n2.49.0.821.gd3b3298025\n\n"},{"id":"517899","messageId":"20250512185233.GC1276214@coredump.intra.peff.net","threadId":"63450","inReplyTo":"20250512185006.GA1275914@coredump.intra.peff.net","subject":"[PATCH 3/3] raw_object_store: drop extra pointer to replace_map","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-12T18:52:33Z","receivedAt":"2025-05-12T18:52:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We store the replacement data in an oidmap, which is itself a pointer in\nthe raw_object_store struct. But there's no need for an extra pointer\nindirection here. It is always allocated and initialized along with the\ncontaining struct, and we never check it for NULL-ness.\n\nLet's embed the map directly in the struct, which is simpler and avoids\nextra pointer chasing.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis one may be more subjective, but IMHO it's good to avoid extra\npointers when we can.\n\n commit-graph.c   | 2 +-\n object-store.c   | 3 +--\n object-store.h   | 3 ++-\n replace-object.c | 8 +++-----\n replace-object.h | 2 +-\n 5 files changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 1a74e1e1ba..4a6e34f8a0 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -222,7 +222,7 @@ static int commit_graph_compatible(struct repository *r)\n \n \tif (replace_refs_enabled(r)) {\n \t\tprepare_replace_object(r);\n-\t\tif (oidmap_get_size(r->objects->replace_map))\n+\t\tif (oidmap_get_size(&r->objects->replace_map))\n \t\t\treturn 0;\n \t}\n \ndiff --git a/object-store.c b/object-store.c\nindex bc24e80829..911bc7ff5f 100644\n--- a/object-store.c\n+++ b/object-store.c\n@@ -1017,8 +1017,7 @@ void raw_object_store_clear(struct raw_object_store *o)\n {\n \tFREE_AND_NULL(o->alternate_db);\n \n-\toidmap_clear(o->replace_map, 1);\n-\tFREE_AND_NULL(o->replace_map);\n+\toidmap_clear(&o->replace_map, 1);\n \tpthread_mutex_destroy(&o->replace_mutex);\n \n \tfree_commit_graph(o->commit_graph);\ndiff --git a/object-store.h b/object-store.h\nindex 46961dc954..9f6f27c016 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -5,6 +5,7 @@\n #include \"object.h\"\n #include \"list.h\"\n #include \"oidset.h\"\n+#include \"oidmap.h\"\n #include \"thread-utils.h\"\n \n struct oidmap;\n@@ -176,7 +177,7 @@ struct raw_object_store {\n \t * Objects that should be substituted by other objects\n \t * (see git-replace(1)).\n \t */\n-\tstruct oidmap *replace_map;\n+\tstruct oidmap replace_map;\n \tunsigned replace_map_initialized : 1;\n \tpthread_mutex_t replace_mutex; /* protect object replace functions */\n \ndiff --git a/replace-object.c b/replace-object.c\nindex 7b8a09b5cb..f8c5f68837 100644\n--- a/replace-object.c\n+++ b/replace-object.c\n@@ -31,7 +31,7 @@ static int register_replace_ref(const char *refname,\n \toidcpy(&repl_obj->replacement, oid);\n \n \t/* Register new object */\n-\tif (oidmap_put(r->objects->replace_map, repl_obj))\n+\tif (oidmap_put(&r->objects->replace_map, repl_obj))\n \t\tdie(_(\"duplicate replace ref: %s\"), refname);\n \n \treturn 0;\n@@ -48,9 +48,7 @@ void prepare_replace_object(struct repository *r)\n \t\treturn;\n \t}\n \n-\tr->objects->replace_map =\n-\t\txmalloc(sizeof(*r->objects->replace_map));\n-\toidmap_init(r->objects->replace_map, 0);\n+\toidmap_init(&r->objects->replace_map, 0);\n \n \trefs_for_each_replace_ref(get_main_ref_store(r),\n \t\t\t\t  register_replace_ref, r);\n@@ -80,7 +78,7 @@ const struct object_id *do_lookup_replace_object(struct repository *r,\n \t/* Try to recursively replace the object */\n \twhile (depth-- > 0) {\n \t\tstruct replace_object *repl_obj =\n-\t\t\toidmap_get(r->objects->replace_map, cur);\n+\t\t\toidmap_get(&r->objects->replace_map, cur);\n \t\tif (!repl_obj)\n \t\t\treturn cur;\n \t\tcur = &repl_obj->replacement;\ndiff --git a/replace-object.h b/replace-object.h\nindex 4226376534..3052e96a62 100644\n--- a/replace-object.h\n+++ b/replace-object.h\n@@ -47,7 +47,7 @@ static inline const struct object_id *lookup_replace_object(struct repository *r\n {\n \tif (!replace_refs_enabled(r) ||\n \t    (r->objects->replace_map_initialized &&\n-\t     oidmap_get_size(r->objects->replace_map) == 0))\n+\t     oidmap_get_size(&r->objects->replace_map) == 0))\n \t\treturn oid;\n \treturn do_lookup_replace_object(r, oid);\n }\n-- \n2.49.0.821.gd3b3298025\n"},{"id":"517934","messageId":"aCMVdEg8d6BHo7AA@pks.im","threadId":"63450","inReplyTo":"20250512185130.GB1276214@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] oidmap: add size function","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-13T09:48:36Z","receivedAt":"2025-05-13T09:48:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, May 12, 2025 at 02:51:30PM -0400, Jeff King wrote:\n> Callers which want to know how many items are in an oidmap have to look\n> at the underlying hashmap struct, leaking an implementation detail.\n> Let's provide a type-appropriate wrapper and use it.\n> \n> Note in the call from lookup_replace_object(), the caller was actually\n> looking at the hashmap's tablesize parameter (the allocated size of the\n> table) rather than hashmap_get_size(), the number of items in the table.\n> This probably should have been checking the number of items all along,\n> but the two are functionally equivalent here since we only add to the\n> map and never remove anything. Thus if there was any allocation, it was\n> because there is at least one item.\n\nI was a bit puzzled by this explanation initially. The two sizes aren't\nfunctioally equivalent -- the table size will typically be larger than\nthe number of contained entries. But the thing is that we don't care for\nthe actual size, we only care whether the map is empty or not. And for\nthat those are indeed equivalent in this specific case.\n\nPatrick\n"},{"id":"517935","messageId":"aCMVfHbB7pJ6Mo4j@pks.im","threadId":"63450","inReplyTo":"20250512185233.GC1276214@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] raw_object_store: drop extra pointer to replace_map","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-13T09:48:44Z","receivedAt":"2025-05-13T09:48:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, May 12, 2025 at 02:52:33PM -0400, Jeff King wrote:\n> We store the replacement data in an oidmap, which is itself a pointer in\n> the raw_object_store struct. But there's no need for an extra pointer\n> indirection here. It is always allocated and initialized along with the\n> containing struct, and we never check it for NULL-ness.\n> \n> Let's embed the map directly in the struct, which is simpler and avoids\n> extra pointer chasing.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> This one may be more subjective, but IMHO it's good to avoid extra\n> pointers when we can.\n\nYup, I agree it is a sensible step. There is no good reason why the map\nshould be allocated, so let's just not.\n\nAll of these cleanups in this series look good to me. Thanks!\n\nPatrick\n"},{"id":"518041","messageId":"xmqqplgbb6kf.fsf@gitster.g","threadId":"63450","inReplyTo":"aCMVfHbB7pJ6Mo4j@pks.im","subject":"Re: [PATCH 3/3] raw_object_store: drop extra pointer to replace_map","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-14T14:38:56Z","receivedAt":"2025-05-14T14:38:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Mon, May 12, 2025 at 02:52:33PM -0400, Jeff King wrote:\n>> We store the replacement data in an oidmap, which is itself a pointer in\n>> the raw_object_store struct. But there's no need for an extra pointer\n>> indirection here. It is always allocated and initialized along with the\n>> containing struct, and we never check it for NULL-ness.\n>> \n>> Let's embed the map directly in the struct, which is simpler and avoids\n>> extra pointer chasing.\n>> \n>> Signed-off-by: Jeff King <peff@peff.net>\n>> ---\n>> This one may be more subjective, but IMHO it's good to avoid extra\n>> pointers when we can.\n>\n> Yup, I agree it is a sensible step. There is no good reason why the map\n> should be allocated, so let's just not.\n>\n> All of these cleanups in this series look good to me. Thanks!\n\nYup, looking very good.  Queued.\n"},{"id":"518061","messageId":"20250514175936.GA2196784@coredump.intra.peff.net","threadId":"63450","inReplyTo":"aCMVdEg8d6BHo7AA@pks.im","subject":"Re: [PATCH 2/3] oidmap: add size function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-14T17:59:36Z","receivedAt":"2025-05-14T17:59:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 13, 2025 at 11:48:36AM +0200, Patrick Steinhardt wrote:\n\n> On Mon, May 12, 2025 at 02:51:30PM -0400, Jeff King wrote:\n> > Callers which want to know how many items are in an oidmap have to look\n> > at the underlying hashmap struct, leaking an implementation detail.\n> > Let's provide a type-appropriate wrapper and use it.\n> > \n> > Note in the call from lookup_replace_object(), the caller was actually\n> > looking at the hashmap's tablesize parameter (the allocated size of the\n> > table) rather than hashmap_get_size(), the number of items in the table.\n> > This probably should have been checking the number of items all along,\n> > but the two are functionally equivalent here since we only add to the\n> > map and never remove anything. Thus if there was any allocation, it was\n> > because there is at least one item.\n> \n> I was a bit puzzled by this explanation initially. The two sizes aren't\n> functioally equivalent -- the table size will typically be larger than\n> the number of contained entries. But the thing is that we don't care for\n> the actual size, we only care whether the map is empty or not. And for\n> that those are indeed equivalent in this specific case.\n\nYep, exactly. Probably replacing \"looking at X\" with \"checking whether X\nis empty\" would have been more clear. I of course was looking at the\ndiff while writing this, so took it for granted. ;)\n\nI don't think it's worth a re-roll, though, especially since this is in\nnext already. Thanks for the review.\n\n-Peff\n"}]}