Re: [PATCH 5/6] hash_pos(): convert to oid_pos()

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 28, 2021, 22:48 UTC
Message-ID: <YBM/NpLeS0uFzVb3@nand.local>
In-Reply-To: <YBJXfl86Juv5kn16@coredump.intra.peff.net>

On Thu, Jan 28, 2021 at 01:19:42AM -0500, Jeff King wrote:

Show 20 quoted lines

> All of our callers are actually looking up an object_id, not a bare
> hash. Likewise, the arrays they are looking in are actual arrays of
> object_id (not just raw bytes of hashes, as we might find in a pack
> .idx; those are handled by bsearch_hash()).
>
> Using an object_id gives us more type safety, and makes the callers
> slightly shorter. It also gets rid of the word "sha1" from several
> access functions, though we could obviously also rename those with
> s/sha1/hash/.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> If we don't want to make this change, I think it's still worth sweeping
> through the callers and changing the names of their access functions.
>
>  builtin/name-rev.c  |  8 ++++----
>  commit-graph.c      | 28 ++++++++++++++--------------
>  commit.c            | 10 +++++-----
>  hash-lookup.c       | 18 +++++++++---------
>  hash-lookup.h       | 10 +++++-----

I wondered briefly if we should rename this to oid-lookup.{c,h}, but I think the answer is "no", since we still have bsearch_hash() in this header.

Probably a single header with "hash" in the name is better than two headers each containing a single function (and one containing an additional typedef).

So, I think that what you did here is good.

Thanks, Taylor