From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Tue, 04 Nov 2025 01:48:21 GMT
Subject: Re: [PATCH 06/14] hash: add a function to look up hash algo structs
Message-ID: <aQlbZRAKq9RwYnqe@fruit.crustytoothpaste.net>
In-Reply-To: <xmqqwm4ebxap.fsf@gitster.g>

On 2025-10-28 at 20:12:30, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > +const struct git_hash_algo *hash_algo_ptr_by_offset(uint32_t algo)
> > +{
> > +	return &hash_algos[algo];
> > +}
> 
> Hmph, technically "algo" may be an "offset" into the array, but I'd
> consider it an implementation detail.  We have hash_algo instances
> floating somewhere in-core, and have a way to obtain a pointer to
> one of these instances by "algorithm number".  For the user of the
> API, the fact that these instances are stored in contiguous pieces
> of memory as an array of struct is totally irrelevant.  For that
> reason, I was somewhat repelled by the "by-offset" part of the
> function name.

I fear I don't have a better name.  "by_id" is the format ID.  I could
write "hash_algo_ptr_by_hash_algo" but that seems slightly bizarre and
difficult to type.  I could do "by_index", but you might have the same
objection to that name.  Would you like to propose a nicer alternative?

> The next function ...
> 
> >  uint32_t hash_algo_by_name(const char *name)
> 
> ... calls what it returns "hash_algo", but the "hash_algo" returned
> by this new function is quite different.  One is just the "algorithm
> number", while the other is "algorithm instance".  Perhaps calling
> both with the same name "hash algo" is the true source of confusing
> naming of this new function?

Note that the name is "hash_algo_ptr", not "hash_algo".  That is, we're
explicitly returning a pointer to the structure here.  I realize that's
slightly hard to notice at first glance, but it was intentional.  I had
the same thought about using "hash_algo" as you did and for that reason
decided to not create an ambiguous name.

> I am somewhat surprised that we do not expose "struct git_hash_algo"
> the same way a previous step exposed "struct object_id" in C as
> "struct ObjectID" in Rust, but instead pass its address as a void
> pointer.  Hopefully the reason for doing so may become apparent as I
> read further into the series?

We're going to replace this with a nicer abstraction in Rust.  Since we
don't have bindgen or cbindgen yet, it's going to be kind of tricky to
deal with the complexities of the structure such that we get it
correctly aligned and matching and we only need to use it when working
with C, so we don't bother to write out the details here.

I certainly haven't measured, but I think the Rust compiler will be able
to better optimize a function like `raw_len` with two explicit
possibilities, especially when its `const`[0], than the C compiler will
with reading what could be an arbitrary value out of the `rawsz` member.
Because it's const, the compiler absolutely will be able to evaluate the
size of anything where the hash algorithm is known at compile time and
the fact that `hex_len` is defined in terms of `raw_len` provides a
helpful hint for the compiler as well in that one is always twice the
other.

[0] `const` for a function meaning in this case that it can be evaluated
at compile time.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA