Re: [PATCH 3/6] hash.h: introduce `unsafe_hash_algo()`

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 10, 2025, 21:38 UTC
Message-ID: <Z4GTcKTVFOFmSsgO@nand.local>
In-Reply-To: <20241121093731.GD602681@coredump.intra.peff.net>

On Thu, Nov 21, 2024 at 04:37:31AM -0500, Jeff King wrote:

Show 15 quoted lines

> If we don't care about the speed of this function, then an
> implementation like:
>
>   for (i = 0; i < GIT_HASH_NALGOS; i++) {
> 	if (p == &hash_algos[i] || p == hash_algos[i]->unsafe)
> 		return i;
>   }
>   return GIT_HASH_UNKNOWN;
>
> would work. I'm not sure if that would be measurable. I was surprised at
> the number of places that hash_algo_by_ptr() is called. Many low-level
> oid functions need it because we store the integer id there rather than
> a direct pointer (so oidread(), oidclr(), oid_object_info_extended(),
> and so on). But I'd also expect the loop above to be pretty fast. So I
> dunno.

Concerns about the speed aside (I agree that the for loop is likely to be very fast, and will probably get unrolled by modern compilers), this looks good to me with one small tweak.

We can't use `hash_algos[i]->unsafe` directly it might be NULL, so the function as above would change the behavior of hash_algo_by_ptr() slightly when provided NULL (it would return SHA-256 instead of UNKNOWN).

So I think you'd want to write the loop like:

    size_t i;

    for (i = 0; i < GIT_HASH_NALGOS) {
        const struct git_hash_algo *algop = &hash_algos[i];

        if (p == algop || (algop->unsafe && p == algop->unsafe))
            return i;
    }

    return GIT_HASH_UNKNOWN;

Thanks, Taylor