Re: [PATCH v2] oidmap: map with OID as key

[Jonathan Tan <jonathantanmy@google.com>]

On Mon, Oct 2, 2017 at 11:31 PM, Jeff King <peff@peff.net> wrote:
> Right, I kind of wonder if this has fallen into an uncanny value where
> we have this almost-hashmap infrastructure, but the end result is not
> significantly easier to use than a plain-old hashmap.
>
> I.e., it looks like you still have to declare something like:
>
>   struct my_data {
>         struct oidmap_entry oid;
>         int value; /* mapping to an int */
>   };
>
> and handle the allocation of the entry yourself. If we instead just
> adding an oidhash() and oidcmpfn(), then callers could those directly.

I thought of something like that, but it seems that you have to
remember quite a few things:
- your entry must have "struct oidmap_entry" at the start, not "struct
hashmap_entry"
- initialize your hashmap with oidcmpfn()
- when getting, hashmap_get_from_hash(map, oidhash(&oid), &oid) (and
oid might be longer e.g. ref->old_oid)

> The invocations are a _little_ longer with a raw hashmap, but not much
> (as you can see from the actual oidmap implementation, and the changes
> to oidset).

About the invocation of hashmap_get_from_hash(), I felt that it would
get annoying quickly enough that I would want an oidmap_get(const
struct hashmap *, const struct object_id *) but it might be strange
that the "get" method is named differently from the rest. If we
tolerate oidmap_get(), and tolerate the fact that the user must both
declare "struct oidmap_entry" instead of "struct hashmap_entry" and
initialize the hashmap with oidcmpfn() (so that the invocation to
hashmap_get_from_hash() within oidmap_get() sends the correct
keydata), we can avoid the thin wrapper issue where callers can no
longer use other methods of hashmap. At this point I decided that I
prefer the thin wrapper, but the "light touch" (struct oidmap_entry,
oidcmpfn(), oidmap_get() only) still better than the status quo.