From: Jeff King <peff@peff.net>
Date: Wed, 14 May 2025 17:59:36 GMT
Subject: Re: [PATCH 2/3] oidmap: add size function
Message-ID: <20250514175936.GA2196784@coredump.intra.peff.net>
In-Reply-To: <aCMVdEg8d6BHo7AA@pks.im>

On Tue, May 13, 2025 at 11:48:36AM +0200, Patrick Steinhardt wrote:

> On Mon, May 12, 2025 at 02:51:30PM -0400, Jeff King wrote:
> > 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.
> 
> I was a bit puzzled by this explanation initially. The two sizes aren't
> functioally equivalent -- the table size will typically be larger than
> the number of contained entries. But the thing is that we don't care for
> the actual size, we only care whether the map is empty or not. And for
> that those are indeed equivalent in this specific case.

Yep, exactly. Probably replacing "looking at X" with "checking whether X
is empty" would have been more clear. I of course was looking at the
diff while writing this, so took it for granted. ;)

I don't think it's worth a re-roll, though, especially since this is in
next already. Thanks for the review.

-Peff