Re: [PATCH 06/14] hash: add a function to look up hash algo structs
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 28, 2025, 20:12 UTC
- Message-ID
- <xmqqwm4ebxap.fsf@gitster.g>
- In-Reply-To
- <20251027004404.2152927-7-sandals@crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 27 quoted lines
> In C, it's easy for us to look up a hash algorithm structure by its
> offset by simply indexing the hash_algos array. However, in Rust, we
> sometimes need a pointer to pass to a C function, but we have our own
> hash algorithm abstraction.
>
> To get one from the other, let's provide a simple function that looks up
> the C structure from the offset and expose it in Rust.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> hash.c | 5 +++++
> hash.h | 1 +
> src/hash.rs | 15 +++++++++++++++
> 3 files changed, 21 insertions(+)
>
> diff --git a/hash.c b/hash.c
> index 81b4f87027..2f4e88e501 100644
> --- a/hash.c
> +++ b/hash.c
> @@ -241,6 +241,11 @@ const char *empty_tree_oid_hex(const struct git_hash_algo *algop)
> return oid_to_hex_r(buf, algop->empty_tree);
> }
>
> +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.
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?
Show 23 quoted lines
> +use std::os::raw::c_void;
> +
> pub const GIT_MAX_RAWSZ: usize = 32;
>
> /// A binary object ID.
> @@ -160,4 +162,17 @@ impl HashAlgorithm {
> HashAlgorithm::SHA256 => &Self::SHA256_NULL_OID,
> }
> }
> +
> + /// A pointer to the C `struct git_hash_algo` for interoperability with C.
> + pub fn hash_algo_ptr(self) -> *const c_void {
> + unsafe { c::hash_algo_ptr_by_offset(self as u32) }
> + }
> +}
> +
> +pub mod c {
> + use std::os::raw::c_void;
> +
> + extern "C" {
> + pub fn hash_algo_ptr_by_offset(n: u32) -> *const c_void;
> + }
> }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?