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@xxxxxxxxx/ > (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.
