Re: [RFC PATCH 0/5] oidmap: handle entries with the same key

From: Jeff King <peff@peff.net>
Date: Jul 12, 2019, 21:58 UTC
Message-ID: <20190712215815.GB3593@sigill.intra.peff.net>
In-Reply-To: <xmqqlfx3p0zo.fsf@gitster-ct.c.googlers.com>

On Fri, Jul 12, 2019 at 11:21:47AM -0700, Junio C Hamano wrote:

Show 17 quoted lines

> > Christian Couder <christian.couder@gmail.com> writes:
> >
> >> This is an RFC patch series that is not intended to be merged for now,
> >> as it looks like we don't need oidmaps that can handle several entries
> >> with the same key yet.
> >
> > What does it even mean for a map to allow multiple entries per key?
> 
> Ah, one thing that I was missing (perhaps it was obvious to
> everybody else but me X-<) was that this is merely to expose what is
> already available in the underlying hashmap API, so let's not bother
> with the "don't people usually do a single key to a value, which
> happens to be a bag of stuff (not just a single stuff)?" question.
> 
> And from that "a generic hashmap can do this, and an upcoming code
> needs to use a hashmap keyed with oid in the same fashion" point of
> view, the new wrappers the patches add all made sense to me.

FWIW, I went through the same thought process. :)

One devil's advocate point against, though: we found recently that khash performs much better than hashmap.[ch] for the oidset data structure. AFAIK nobody has looked at whether the same is true for oidmap. But if it is, then this strategy may make it harder to switch.

(OTOH, we already have kh_oid_map, so the two could probably co-exist and we could just convert particular callers from one to the other).

Show 5 quoted lines

> > I actually think that showing how it is used in the real application
> > (reftable?) is the best way to illustrate why this is useful and to
> > get opinions from others.
> 
> This part still stands, though.

Agreed.

-Peff