Re: [PATCH v4 04/17] chunk-format.h: extract oid_version()

On Wed, May 18 2022, Taylor Blau wrote:

There are three definitions of an identical function which converts
`the_hash_algo` into either 1 (for SHA-1) or 2 (for SHA-256). There is a
copy of this function for writing both the commit-graph and
multi-pack-index file, and another inline definition used to write the
.rev header.

Consolidate these into a single definition in chunk-format.h. It's not
clear that this is the best header to define this function in, but it
should do for now.

Maybe hash.h? :)
https://lore.kernel.org/git/RFC-patch-2.2-051f0612ab9-20220519T113538Z-avarab@gmail.com/ (local)

(Worth noting, the .rev caller expects a 4-byte unsigned, but the other
two callers work with a single unsigned byte. The consolidated version
uses the latter type, and lets the compiler widen it when required).

I just went for "int" and had the compiler similarly cast that, which
seems simpler & more obvious, no?

I.e. it seems to me that we really only need these more narrow types at
the time that we write this data, which we alredy have casts for.

+uint8_t oid_version(const struct git_hash_algo *algop)
+{
+	switch (hash_algo_by_ptr(algop)) {
+	case GIT_HASH_SHA1:
+		return 1;
+	case GIT_HASH_SHA256:
+		return 2;
+	default:
+		die(_("invalid hash version"));
+	}

As noted in the 2/2 I posted above we have some cases where we really
should have BUG here, and others (reading) which are arguably die(). I
think just going for BUG() makes sense in this case.

But if you're just unifying existing code we can also just keep it
as-is.

FWIW I struggled to come up with a name for this, and ended up with
hash_short_id_by_algo(). Somewhat bikesheddy, but I'd prefer if we fixed
that "oid_version" name while at it, since this really has nothing do do
with an "OID version" (whatever that is).

We only refer to hash versions elsewhere, which collectively describe
the versions of all OIDs we need to handle.