[PATCH 1/2] oidmap: make oidmap_free independent of struct layout

[PATCH 1/2] oidmap: make oidmap_free independent of struct layout

[Abhishek Kumar]

c8e424c introduced hashmap_free_entries, which can free any struct
pointer, regardless of the hashmap_entry field offset.

oidmap does not make use of this flexibilty, hardcoding the offset to
zero instead. Let's fix this by passing struct type and member to
hashmap_free_entries.

Additionally, removes an erroneous semi-colon at the end of
hashmap_free_entries macro.

Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
---
 hashmap.h | 2 +-
 oidmap.c  | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hashmap.h b/hashmap.h
index 79ae9f80de..6d0a65a39f 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -245,7 +245,7 @@ void hashmap_free_(struct hashmap *map, ssize_t offset);
  * where @member is the hashmap_entry struct used to associate with @map
  */
 #define hashmap_free_entries(map, type, member) \
-	hashmap_free_(map, offsetof(type, member));
+	hashmap_free_(map, offsetof(type, member))
 
 /* hashmap_entry functions */
 
diff --git a/oidmap.c b/oidmap.c
index 423aa014a3..65d63787a8 100644
--- a/oidmap.c
+++ b/oidmap.c
@@ -26,8 +26,10 @@ void oidmap_free(struct oidmap *map, int free_entries)
 	if (!map)
 		return;
 
-	/* TODO: make oidmap itself not depend on struct layouts */
-	hashmap_free_(&map->map, free_entries ? 0 : -1);
+	if (free_entries)
+		hashmap_free_entries(&map->map, struct oidmap_entry, internal_entry);
+	else
+		hashmap_free(&map->map);
 }
 
 void *oidmap_get(const struct oidmap *map, const struct object_id *key)
-- 
2.26.0

[PATCH 2/2] oidmap: rework iterators to return typed pointer

[Abhishek Kumar]

87571c3 modified hashmap_iter_next to return a hashmap_entry pointer
instead of void pointer.

However, oidmap_iter_next is unaware of the struct type containing
oidmap_entry and explicilty returns a void pointer.

Rework oidmap_iter_* to include struct type and return appropriate
pointer. This allows for compile-time type checks.

Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
---
 oidmap.h               | 27 +++++++++++++++------------
 t/helper/test-oidmap.c |  2 +-
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/oidmap.h b/oidmap.h
index c66a83ab1d..5d6b34a7ce 100644
--- a/oidmap.h
+++ b/oidmap.h
@@ -76,18 +76,21 @@ static inline void oidmap_iter_init(struct oidmap *map, struct oidmap_iter *iter
 	hashmap_iter_init(&map->map, &iter->h_iter);
 }
 
-static inline void *oidmap_iter_next(struct oidmap_iter *iter)
-{
-	/* TODO: this API could be reworked to do compile-time type checks */
-	return (void *)hashmap_iter_next(&iter->h_iter);
-}
+/*
+ * Returns the next entry, or NULL if there are no more entries.
+ *
+ * The entry is of @type (e.g. "struct foo") and has a member of type struct
+ * oidmap_entry.
+ */
+#define oidmap_iter_next(iter, type) \
+	(type *) hashmap_iter_next(&(iter)->h_iter)
 
-static inline void *oidmap_iter_first(struct oidmap *map,
-				      struct oidmap_iter *iter)
-{
-	oidmap_iter_init(map, iter);
-	/* TODO: this API could be reworked to do compile-time type checks */
-	return (void *)oidmap_iter_next(iter);
-}
+/*
+ * Returns the first entry in @map using @iter, where the entry is of @type
+ * (e.g. "struct foo") and has a member of type struct oidmap_entry.
+ */
+#define oidmap_iter_first(map, iter, type) \
+	({oidmap_iter_init(map, iter); \
+	 oidmap_iter_next(iter, type); })
 
 #endif
diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c
index 0acf99931e..a28bf007a8 100644
--- a/t/helper/test-oidmap.c
+++ b/t/helper/test-oidmap.c
@@ -96,7 +96,7 @@ int cmd__oidmap(int argc, const char **argv)
 
 			struct oidmap_iter iter;
 			oidmap_iter_init(&map, &iter);
-			while ((entry = oidmap_iter_next(&iter)))
+			while ((entry = oidmap_iter_next(&iter, struct test_entry)))
 				printf("%s %s\n", oid_to_hex(&entry->entry.oid), entry->name);
 
 		} else {
-- 
2.26.0

Re: [PATCH 1/2] oidmap: make oidmap_free independent of struct layout

[Junio C Hamano]

Abhishek Kumar <abhishekkumar8222@gmail.com> writes:

> c8e424c introduced hashmap_free_entries, which can free any struct
> pointer, regardless of the hashmap_entry field offset.

    c8e424c9 (hashmap: introduce hashmap_free_entries, 2019-10-06)
    introduced hashmap_free_entries(), which ...

is how we refer to existing commits and functions.

> oidmap does not make use of this flexibilty, hardcoding the offset to
> zero instead. Let's fix this by passing struct type and member to
> hashmap_free_entries.

Makes sense.

> Additionally, removes an erroneous semi-colon at the end of
> hashmap_free_entries macro.

s/removes/remove/

Good eyes.  Of course, your if/else would have broken without this
fix ;-)

Looking good.  Thanks.

> Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
> ---
>  hashmap.h | 2 +-
>  oidmap.c  | 6 ++++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hashmap.h b/hashmap.h
> index 79ae9f80de..6d0a65a39f 100644
> --- a/hashmap.h
> +++ b/hashmap.h
> @@ -245,7 +245,7 @@ void hashmap_free_(struct hashmap *map, ssize_t offset);
>   * where @member is the hashmap_entry struct used to associate with @map
>   */
>  #define hashmap_free_entries(map, type, member) \
> -	hashmap_free_(map, offsetof(type, member));
> +	hashmap_free_(map, offsetof(type, member))
>  
>  /* hashmap_entry functions */
>  
> diff --git a/oidmap.c b/oidmap.c
> index 423aa014a3..65d63787a8 100644
> --- a/oidmap.c
> +++ b/oidmap.c
> @@ -26,8 +26,10 @@ void oidmap_free(struct oidmap *map, int free_entries)
>  	if (!map)
>  		return;
>  
> -	/* TODO: make oidmap itself not depend on struct layouts */
> -	hashmap_free_(&map->map, free_entries ? 0 : -1);
> +	if (free_entries)
> +		hashmap_free_entries(&map->map, struct oidmap_entry, internal_entry);
> +	else
> +		hashmap_free(&map->map);
>  }
>  
>  void *oidmap_get(const struct oidmap *map, const struct object_id *key)

[PATCH v2 1/2] oidmap: make oidmap_free independent of struct layout

[Abhishek Kumar]

c8e424c9 (hashmap: introduce hashmap_free_entries, 2019-10-06)
introduced hashmap_free_entries(), which can free any struct pointer,
regardless of the hashmap_entry field offset.

oidmap does not make use of this flexibilty, hardcoding the offset to
zero instead. Let's fix this by passing struct type and member to
hashmap_free_entries().

Additionally, remove an erroneous semi-colon at the end of
hashmap_free_entries() macro.

Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
---

Changes in v2:
- Fix references to earlier commits.
- Grammar fixes.

 hashmap.h | 2 +-
 oidmap.c  | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hashmap.h b/hashmap.h
index 79ae9f80de..6d0a65a39f 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -245,7 +245,7 @@ void hashmap_free_(struct hashmap *map, ssize_t offset);
  * where @member is the hashmap_entry struct used to associate with @map
  */
 #define hashmap_free_entries(map, type, member) \
-	hashmap_free_(map, offsetof(type, member));
+	hashmap_free_(map, offsetof(type, member))
 
 /* hashmap_entry functions */
 
diff --git a/oidmap.c b/oidmap.c
index 423aa014a3..65d63787a8 100644
--- a/oidmap.c
+++ b/oidmap.c
@@ -26,8 +26,10 @@ void oidmap_free(struct oidmap *map, int free_entries)
 	if (!map)
 		return;
 
-	/* TODO: make oidmap itself not depend on struct layouts */
-	hashmap_free_(&map->map, free_entries ? 0 : -1);
+	if (free_entries)
+		hashmap_free_entries(&map->map, struct oidmap_entry, internal_entry);
+	else
+		hashmap_free(&map->map);
 }
 
 void *oidmap_get(const struct oidmap *map, const struct object_id *key)
-- 
2.26.0

[PATCH v2 2/2] oidmap: rework iterators to return typed pointer

[Abhishek Kumar]

87571c3f (hashmap: use *_entry APIs for iteration, 2019-10-06) modified
hashmap_iter_next() to return a hashmap_entry pointer instead of void
pointer.

However, oidmap_iter_next() is unaware of the struct type containing
oidmap_entry and explicitly returns a void pointer.

Rework oidmap_iter_next() to include struct type and return appropriate
pointer. This allows for compile-time type checks.

Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
---
 oidmap.h               | 27 +++++++++++++++------------
 t/helper/test-oidmap.c |  2 +-
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/oidmap.h b/oidmap.h
index c66a83ab1d..5d6b34a7ce 100644
--- a/oidmap.h
+++ b/oidmap.h
@@ -76,18 +76,21 @@ static inline void oidmap_iter_init(struct oidmap *map, struct oidmap_iter *iter
 	hashmap_iter_init(&map->map, &iter->h_iter);
 }
 
-static inline void *oidmap_iter_next(struct oidmap_iter *iter)
-{
-	/* TODO: this API could be reworked to do compile-time type checks */
-	return (void *)hashmap_iter_next(&iter->h_iter);
-}
+/*
+ * Returns the next entry, or NULL if there are no more entries.
+ *
+ * The entry is of @type (e.g. "struct foo") and has a member of type struct
+ * oidmap_entry.
+ */
+#define oidmap_iter_next(iter, type) \
+	(type *) hashmap_iter_next(&(iter)->h_iter)
 
-static inline void *oidmap_iter_first(struct oidmap *map,
-				      struct oidmap_iter *iter)
-{
-	oidmap_iter_init(map, iter);
-	/* TODO: this API could be reworked to do compile-time type checks */
-	return (void *)oidmap_iter_next(iter);
-}
+/*
+ * Returns the first entry in @map using @iter, where the entry is of @type
+ * (e.g. "struct foo") and has a member of type struct oidmap_entry.
+ */
+#define oidmap_iter_first(map, iter, type) \
+	({oidmap_iter_init(map, iter); \
+	 oidmap_iter_next(iter, type); })
 
 #endif
diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c
index 0acf99931e..a28bf007a8 100644
--- a/t/helper/test-oidmap.c
+++ b/t/helper/test-oidmap.c
@@ -96,7 +96,7 @@ int cmd__oidmap(int argc, const char **argv)
 
 			struct oidmap_iter iter;
 			oidmap_iter_init(&map, &iter);
-			while ((entry = oidmap_iter_next(&iter)))
+			while ((entry = oidmap_iter_next(&iter, struct test_entry)))
 				printf("%s %s\n", oid_to_hex(&entry->entry.oid), entry->name);
 
 		} else {
-- 
2.26.0

Re: [PATCH v2 2/2] oidmap: rework iterators to return typed pointer

[Jeff King]

On Wed, Apr 08, 2020 at 12:33:46PM +0530, Abhishek Kumar wrote:

> 87571c3f (hashmap: use *_entry APIs for iteration, 2019-10-06) modified
> hashmap_iter_next() to return a hashmap_entry pointer instead of void
> pointer.
> 
> However, oidmap_iter_next() is unaware of the struct type containing
> oidmap_entry and explicitly returns a void pointer.
> 
> Rework oidmap_iter_next() to include struct type and return appropriate
> pointer. This allows for compile-time type checks.

Yes, I think returning a pointer to an oidmap_entry makes sense. And
then we get type safety, and anybody who wants embed an oidmap_entry can
use container_of() to get back to their original struct.

But...

> +/*
> + * Returns the next entry, or NULL if there are no more entries.
> + *
> + * The entry is of @type (e.g. "struct foo") and has a member of type struct
> + * oidmap_entry.
> + */
> +#define oidmap_iter_next(iter, type) \
> +	(type *) hashmap_iter_next(&(iter)->h_iter)

This cast is turning a hashmap_entry into whatever type the caller
passed in.  But it's doing it with a straight cast. We know that
hashmap_entry and oidmap_entry pointers are equivalent, but we don't
know where the oidmap_entry is with respect to the user's type.

I think oidmap_iter_next() should continue to be a function that returns
an oidmap_entry pointer (and use container_of_or_null() to get to it
from the hashmap_entry, even though we know the offset is 0).

And then the caller can either use container_of() to get to their
original struct, or we can provide a helper macro. See the difference
between hashmap_iter_first() and hashmap_iter_first_entry().

There's no hashmap_iter_next_entry(). There could be, but instead it
skipped straight to hashmap_for_each_entry(), which uses an offset
within the variable rather than the type. But likewise, we could add
oidmap_for_each_entry() here.

> diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c
> index 0acf99931e..a28bf007a8 100644
> --- a/t/helper/test-oidmap.c
> +++ b/t/helper/test-oidmap.c
> @@ -96,7 +96,7 @@ int cmd__oidmap(int argc, const char **argv)
>  
>  			struct oidmap_iter iter;
>  			oidmap_iter_init(&map, &iter);
> -			while ((entry = oidmap_iter_next(&iter)))
> +			while ((entry = oidmap_iter_next(&iter, struct test_entry)))
>  				printf("%s %s\n", oid_to_hex(&entry->entry.oid), entry->name);

This works because "test_entry" has the oidmap_entry at the start.

But it wouldn't work with a struct where that wasn't the case, nor would
it provide any compile-time safety (because of the cast).

Note that if we do want to support that and get type safety (and I think
it is worth doing), oidmap_get() would need similar treatment (it
returns a void pointer, but it is really a pointer to an oidmap_entry).
And I guess oidmap_put() and oidmap_remove(), which returns pointers to
existing entries.

-Peff

Re: [PATCH 1/2] oidmap: make oidmap_free independent of struct layout

[Jeff King]

On Wed, Apr 08, 2020 at 09:36:58AM +0530, Abhishek Kumar wrote:

> c8e424c introduced hashmap_free_entries, which can free any struct
> pointer, regardless of the hashmap_entry field offset.
> 
> oidmap does not make use of this flexibilty, hardcoding the offset to
> zero instead. Let's fix this by passing struct type and member to
> hashmap_free_entries.

I'm not sure how much this improves anything.

We're now telling the hashmap code how to get to our "struct
oidmap_entry" pointer by using the correct offset there. But we know
that offset will always be 0, because we put our internal hashmap entry
at the start of oidmap_entry.

What you can't do is:

  struct foo {
    int first_member;
    struct oidmap_entry not_the_first_member;
  };

and work directly with "struct foo" pointers. You have to convert
oidmap_entry pointers into foo pointers with container_of() or similar.
And that is true both before and after your patch.

That said, I don't mind doing this as a cleanup; there's a subtle
dependency on the location of internal_entry within object_entry,
and this would move towards getting rid of it.

-Peff

[PATCH v3 1/3] oidmap: make oidmap_free independent of struct layout

[Abhishek Kumar]

c8e424c introduced hashmap_free_entries, which can free any struct
pointer, regardless of the hashmap_entry field offset.

oidmap does not make use of this flexibilty, hardcoding the offset to
zero instead. Let's fix this by passing struct type and member to
hashmap_free_entries.

Additionally, remove an erroneous semi-colon at the end of
hashmap_free_entries macro.

Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
---
 hashmap.h | 2 +-
 oidmap.c  | 7 +++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hashmap.h b/hashmap.h
index 79ae9f80de..6d0a65a39f 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -245,7 +245,7 @@ void hashmap_free_(struct hashmap *map, ssize_t offset);
  * where @member is the hashmap_entry struct used to associate with @map
  */
 #define hashmap_free_entries(map, type, member) \
-	hashmap_free_(map, offsetof(type, member));
+	hashmap_free_(map, offsetof(type, member))
 
 /* hashmap_entry functions */
 
diff --git a/oidmap.c b/oidmap.c
index 423aa014a3..44345a8cf2 100644
--- a/oidmap.c
+++ b/oidmap.c
@@ -26,8 +26,11 @@ void oidmap_free(struct oidmap *map, int free_entries)
 	if (!map)
 		return;
 
-	/* TODO: make oidmap itself not depend on struct layouts */
-	hashmap_free_(&map->map, free_entries ? 0 : -1);
+	if (free_entries)
+		hashmap_free_entries(
+			&map->map, struct oidmap_entry, internal_entry);
+	else
+		hashmap_free(&map->map);
 }
 
 void *oidmap_get(const struct oidmap *map, const struct object_id *key)
-- 
2.26.0

[PATCH v3 2/3] oidmap: rework iterators to return typed pointer

[Abhishek Kumar]

87571c3f (hashmap: use *_entry APIs for iteration, 2019-10-06) modified
hashmap_iter_next() to return a hashmap_entry pointer instead of void
pointer.

However, oidmap_iter_next() is unaware of the struct type containing
oidmap_entry and explicitly returns a void pointer.

Rewrite oidmap_iter_next() to return oidmap_entry pointer and add
helper macros to return pointers to container struct.

Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
---
 oidmap.h               | 41 +++++++++++++++++++++++++++++++++++------
 t/helper/test-oidmap.c |  7 ++++---
 2 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/oidmap.h b/oidmap.h
index c66a83ab1d..d79b087ab0 100644
--- a/oidmap.h
+++ b/oidmap.h
@@ -66,6 +66,8 @@ void *oidmap_put(struct oidmap *map, void *entry);
  */
 void *oidmap_remove(struct oidmap *map, const struct object_id *key);
 
+#define oidmap_entry_from_hashmap_entry(entry) \
+	container_of_or_null(entry, struct oidmap_entry, internal_entry)
 
 struct oidmap_iter {
 	struct hashmap_iter h_iter;
@@ -76,18 +78,45 @@ static inline void oidmap_iter_init(struct oidmap *map, struct oidmap_iter *iter
 	hashmap_iter_init(&map->map, &iter->h_iter);
 }
 
-static inline void *oidmap_iter_next(struct oidmap_iter *iter)
+/* Returns the next oidmap_entry, or NULL if there are no more entries. */
+static inline struct oidmap_entry *oidmap_iter_next(struct oidmap_iter *iter)
 {
-	/* TODO: this API could be reworked to do compile-time type checks */
-	return (void *)hashmap_iter_next(&iter->h_iter);
+	return oidmap_entry_from_hashmap_entry(
+			hashmap_iter_next(&iter->h_iter));
 }
 
-static inline void *oidmap_iter_first(struct oidmap *map,
+/* Initializes the iterator and returns the first entry, if any. */
+static inline struct oidmap_entry *oidmap_iter_first(struct oidmap *map,
 				      struct oidmap_iter *iter)
 {
 	oidmap_iter_init(map, iter);
-	/* TODO: this API could be reworked to do compile-time type checks */
-	return (void *)oidmap_iter_next(iter);
+	return oidmap_iter_next(iter);
 }
 
+/*
+ * Returns the first entry in @map using @iter, where the entry if of
+ * @type (e.g. "struct foo") and @member is the name of the
+ * "struct oidmap_entry" in @type
+ */
+#define oidmap_iter_first_entry(map, iter, type, member) \
+	container_of_or_null(oidmap_iter_first(map, iter), type, member)
+
+/* Internal macro for oidmap_for_each_entry */
+#define oidmap_iter_next_entry_offset(iter, offset) \
+	container_of_or_null_offset(oidmap_iter_next(iter), offset)
+
+/* Internal macro for oidmap_for_each_entry */
+#define oidmap_iter_first_entry_offset(map, iter, offset) \
+	container_of_or_null_offset(oidmap_iter_first(map, iter), offset)
+
+/*
+ * Iterate through @map using @iter, @var is a pointer to a type
+ * containing a @member which is a "struct oidmap_entry"
+ */
+#define oidmap_for_each_entry(map, iter, var, member) \
+	for (var = oidmap_iter_first_entry_offset(map, iter, \
+						OFFSETOF_VAR(var, member)); \
+		var; \
+		var = oidmap_iter_next_entry_offset(iter, \
+						OFFSETOF_VAR(var, member)))
 #endif
diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c
index 0acf99931e..3f599b21b8 100644
--- a/t/helper/test-oidmap.c
+++ b/t/helper/test-oidmap.c
@@ -95,9 +95,10 @@ int cmd__oidmap(int argc, const char **argv)
 		} else if (!strcmp("iterate", cmd)) {
 
 			struct oidmap_iter iter;
-			oidmap_iter_init(&map, &iter);
-			while ((entry = oidmap_iter_next(&iter)))
-				printf("%s %s\n", oid_to_hex(&entry->entry.oid), entry->name);
+			struct test_entry *ent;
+
+			oidmap_for_each_entry(&map, &iter, ent, entry)
+				printf("%s %s\n", oid_to_hex(&ent->entry.oid), ent->name);
 
 		} else {
 
-- 
2.26.0

[PATCH v3 3/3] oidmap: return oidmap_entry pointers

[Abhishek Kumar]

oidmap_entry is usually embedded as first member of user data
structures. Since oidmap is unaware of structure, it returns an void
pointer for oidmap_get(), oidmap_put() and oidmap_remove().

This pointer is rather a pointer to a oidmap entry. Teach the functions
to return a oidmap_entry pointer and add helper macros to return
pointers to container struct.

Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
---
 list-objects-filter.c  |  8 +++++---
 oidmap.c               | 21 ++++++++++++---------
 oidmap.h               | 42 ++++++++++++++++++++++++++++++++++++------
 replace-object.c       |  7 ++++---
 sequencer.c            | 28 ++++++++++++++++++----------
 t/helper/test-oidmap.c |  8 +++++---
 6 files changed, 80 insertions(+), 34 deletions(-)

diff --git a/list-objects-filter.c b/list-objects-filter.c
index 1e8d4e763d..16b45d913e 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -183,13 +183,15 @@ static enum list_objects_filter_result filter_trees_depth(
 		return include_it ? LOFR_MARK_SEEN | LOFR_DO_SHOW : LOFR_ZERO;
 
 	case LOFS_BEGIN_TREE:
-		seen_info = oidmap_get(
-			&filter_data->seen_at_depth, &obj->oid);
+		oidmap_get_entry(seen_info, base, &filter_data->seen_at_depth,
+				 &obj->oid);
+
 		if (!seen_info) {
 			seen_info = xcalloc(1, sizeof(*seen_info));
 			oidcpy(&seen_info->base.oid, &obj->oid);
 			seen_info->depth = filter_data->current_depth;
-			oidmap_put(&filter_data->seen_at_depth, seen_info);
+			oidmap_put_entry(seen_info, base,
+					 &filter_data->seen_at_depth);
 			already_seen = 0;
 		} else {
 			already_seen =
diff --git a/oidmap.c b/oidmap.c
index 44345a8cf2..0e64c0e8a2 100644
--- a/oidmap.c
+++ b/oidmap.c
@@ -33,15 +33,18 @@ void oidmap_free(struct oidmap *map, int free_entries)
 		hashmap_free(&map->map);
 }
 
-void *oidmap_get(const struct oidmap *map, const struct object_id *key)
+struct oidmap_entry *oidmap_get(const struct oidmap *map,
+		const struct object_id *key)
 {
 	if (!map->map.cmpfn)
 		return NULL;
 
-	return hashmap_get_from_hash(&map->map, oidhash(key), key);
+	return oidmap_entry_from_hashmap_entry(
+		hashmap_get_from_hash(&map->map, oidhash(key), key));
 }
 
-void *oidmap_remove(struct oidmap *map, const struct object_id *key)
+struct oidmap_entry *oidmap_remove(struct oidmap *map,
+		const struct object_id *key)
 {
 	struct hashmap_entry entry;
 
@@ -49,16 +52,16 @@ void *oidmap_remove(struct oidmap *map, const struct object_id *key)
 		oidmap_init(map, 0);
 
 	hashmap_entry_init(&entry, oidhash(key));
-	return hashmap_remove(&map->map, &entry, key);
+	return oidmap_entry_from_hashmap_entry(
+		hashmap_remove(&map->map, &entry, key));
 }
 
-void *oidmap_put(struct oidmap *map, void *entry)
+struct oidmap_entry *oidmap_put(struct oidmap *map, struct oidmap_entry *entry)
 {
-	struct oidmap_entry *to_put = entry;
-
 	if (!map->map.cmpfn)
 		oidmap_init(map, 0);
 
-	hashmap_entry_init(&to_put->internal_entry, oidhash(&to_put->oid));
-	return hashmap_put(&map->map, &to_put->internal_entry);
+	hashmap_entry_init(&entry->internal_entry, oidhash(&entry->oid));
+	return oidmap_entry_from_hashmap_entry(
+		hashmap_put(&map->map, &entry->internal_entry));
 }
diff --git a/oidmap.h b/oidmap.h
index d79b087ab0..f250f0bf87 100644
--- a/oidmap.h
+++ b/oidmap.h
@@ -46,25 +46,55 @@ void oidmap_free(struct oidmap *map, int free_entries);
 /*
  * Returns the oidmap entry for the specified oid, or NULL if not found.
  */
-void *oidmap_get(const struct oidmap *map,
+struct oidmap_entry *oidmap_get(const struct oidmap *map,
 		 const struct object_id *key);
 
+/*
+ * Returns pointer to container struct of the oidmap entry for the
+ * specified oid, or NULL if not found.
+ */
+#define oidmap_get_entry(var, member, map, key) \
+	var = container_of_or_null_offset( \
+		oidmap_get(map, key), \
+		OFFSETOF_VAR(var, member))
 /*
  * Adds or replaces an oidmap entry.
  *
- * ((struct oidmap_entry *) entry)->internal_entry will be populated by this
- * function.
- *
  * Returns the replaced entry, or NULL if not found (i.e. the entry was added).
  */
-void *oidmap_put(struct oidmap *map, void *entry);
+struct oidmap_entry *oidmap_put(struct oidmap *map, struct oidmap_entry *entry);
+
+/*
+ * Adds or replaces an oidmap entry.
+ *
+ * Returns pointer to container struct of replaced entry, or NULL if
+ * not found (i.e. the entry was added).
+ *
+ * The struct type of @var contains a "struct oidmap_entry" @member.
+ */
+#define oidmap_put_entry(var, member, map) \
+	container_of_or_null_offset( \
+		oidmap_put(map, &var->member), \
+		OFFSETOF_VAR(var, member))
 
 /*
  * Removes an oidmap entry matching the specified oid.
  *
  * Returns the removed entry, or NULL if not found.
  */
-void *oidmap_remove(struct oidmap *map, const struct object_id *key);
+struct oidmap_entry *oidmap_remove(struct oidmap *map,
+		const struct object_id *key);
+
+/*
+ * Removes an oidmap entry matching the specified oid.
+ *
+ * Returns pointer to container struct of the removed entry, or NULL if
+ * not found.
+ */
+#define oidmap_remove_entry(var, member, map, key) \
+	var = container_of_or_null_offset( \
+		oidmap_remove(map, key), \
+		OFFSETOF_VAR(var, member))
 
 #define oidmap_entry_from_hashmap_entry(entry) \
 	container_of_or_null(entry, struct oidmap_entry, internal_entry)
diff --git a/replace-object.c b/replace-object.c
index 7bd9aba6ee..ab0354db7a 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -26,7 +26,7 @@ static int register_replace_ref(struct repository *r,
 	oidcpy(&repl_obj->replacement, oid);
 
 	/* Register new object */
-	if (oidmap_put(r->objects->replace_map, repl_obj))
+	if (oidmap_put_entry(repl_obj, original, r->objects->replace_map))
 		die(_("duplicate replace ref: %s"), refname);
 
 	return 0;
@@ -73,8 +73,9 @@ 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);
+		struct replace_object *repl_obj;
+		oidmap_get_entry(repl_obj, original, r->objects->replace_map,
+				 cur);
 		if (!repl_obj)
 			return cur;
 		cur = &repl_obj->replacement;
diff --git a/sequencer.c b/sequencer.c
index f30bb73c70..012e7865db 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4506,7 +4506,8 @@ static const char *label_oid(struct object_id *oid, const char *label,
 	struct object_id dummy;
 	int i;
 
-	string_entry = oidmap_get(&state->commit2label, oid);
+	oidmap_get_entry(string_entry, entry, &state->commit2label, oid);
+
 	if (string_entry)
 		return string_entry->string;
 
@@ -4606,7 +4607,7 @@ static const char *label_oid(struct object_id *oid, const char *label,
 
 	FLEX_ALLOC_STR(string_entry, string, label);
 	oidcpy(&string_entry->entry.oid, oid);
-	oidmap_put(&state->commit2label, string_entry);
+	oidmap_put_entry(string_entry, entry, &state->commit2label);
 
 	return string_entry->string;
 }
@@ -4645,7 +4646,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 		struct object_id *oid = &revs->cmdline.rev[0].item->oid;
 		FLEX_ALLOC_STR(entry, string, "onto");
 		oidcpy(&entry->entry.oid, oid);
-		oidmap_put(&state.commit2label, entry);
+		oidmap_put_entry(entry, entry, /* member name */
+				 &state.commit2label);
 
 		FLEX_ALLOC_STR(onto_label_entry, label, "onto");
 		hashmap_entry_init(&onto_label_entry->entry, strihash("onto"));
@@ -4689,7 +4691,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 
 			FLEX_ALLOC_STR(entry, string, buf.buf);
 			oidcpy(&entry->entry.oid, &commit->object.oid);
-			oidmap_put(&commit2todo, entry);
+			oidmap_put_entry(entry, entry, /* member name */
+					 &commit2todo);
 
 			continue;
 		}
@@ -4731,7 +4734,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 
 		FLEX_ALLOC_STR(entry, string, buf.buf);
 		oidcpy(&entry->entry.oid, &commit->object.oid);
-		oidmap_put(&commit2todo, entry);
+		oidmap_put_entry(entry, entry, /* member name */
+				 &commit2todo);
 	}
 
 	/*
@@ -4769,7 +4773,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 		commit = iter->item;
 		if (oidset_contains(&shown, &commit->object.oid))
 			continue;
-		entry = oidmap_get(&state.commit2label, &commit->object.oid);
+
+		oidmap_get_entry(entry, entry /* member name */,
+				 &state.commit2label, &commit->object.oid);
 
 		if (entry)
 			strbuf_addf(out, "\n%c Branch %s\n", comment_line_char, entry->string);
@@ -4793,8 +4799,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 		else {
 			const char *to = NULL;
 
-			entry = oidmap_get(&state.commit2label,
-					   &commit->object.oid);
+			oidmap_get_entry(entry, entry /* member name */,
+					 &state.commit2label, &commit->object.oid);
 			if (entry)
 				to = entry->string;
 			else if (!rebase_cousins)
@@ -4813,11 +4819,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 
 		for (iter2 = list; iter2; iter2 = iter2->next) {
 			struct object_id *oid = &iter2->item->object.oid;
-			entry = oidmap_get(&commit2todo, oid);
+			oidmap_get_entry(entry, entry /* member name */,
+					 &commit2todo, oid);
 			/* only show if not already upstream */
 			if (entry)
 				strbuf_addf(out, "%s\n", entry->string);
-			entry = oidmap_get(&state.commit2label, oid);
+			oidmap_get_entry(entry, entry /* member name */,
+					 &state.commit2label, oid);
 			if (entry)
 				strbuf_addf(out, "%s %s\n",
 					    cmd_label, entry->string);
diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c
index 3f599b21b8..fc19c1cb8e 100644
--- a/t/helper/test-oidmap.c
+++ b/t/helper/test-oidmap.c
@@ -59,7 +59,7 @@ int cmd__oidmap(int argc, const char **argv)
 			oidcpy(&entry->entry.oid, &oid);
 
 			/* add / replace entry */
-			entry = oidmap_put(&map, entry);
+			entry = oidmap_put_entry(entry, entry, /* member name */ &map);
 
 			/* print and free replaced entry, if any */
 			puts(entry ? entry->name : "NULL");
@@ -73,7 +73,8 @@ int cmd__oidmap(int argc, const char **argv)
 			}
 
 			/* lookup entry in oidmap */
-			entry = oidmap_get(&map, &oid);
+			oidmap_get_entry(entry, entry /* member name */,
+					&map, &oid);
 
 			/* print result */
 			puts(entry ? entry->name : "NULL");
@@ -86,7 +87,8 @@ int cmd__oidmap(int argc, const char **argv)
 			}
 
 			/* remove entry from oidmap */
-			entry = oidmap_remove(&map, &oid);
+			oidmap_remove_entry(entry, entry, /* member name */
+				      &map, &oid);
 
 			/* print result and free entry*/
 			puts(entry ? entry->name : "NULL");
-- 
2.26.0