From 274464683462d04363d2107822b0f9d2d5a27623 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 May 2025 14:50:28 -0400 Subject: [PATCH 1/3] oidmap: rename oidmap_free() to oidmap_clear() 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 Signed-off-by: Junio C Hamano --- 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) From 596184786c1b1998573df4c130eadb1668d8c304 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 May 2025 14:51:30 -0400 Subject: [PATCH 2/3] oidmap: add size function 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 Signed-off-by: Junio C Hamano --- 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); } From 4b63963f5d729cb9eb997c8912b7d500ffc53297 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 May 2025 14:52:33 -0400 Subject: [PATCH 3/3] raw_object_store: drop extra pointer to replace_map 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 Signed-off-by: Junio C Hamano --- 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); }