Subject: Re: [PATCH 04/14] rust: add a ObjectID struct

From: Junio C Hamano <[email-masked]>
Date: Tue, 28 Oct 2025 12:40:39 -0700

Patrick Steinhardt <[email-masked]> writes:

> On Mon, Oct 27, 2025 at 12:43:54AM +0000, brian m. carlson wrote:
>> diff --git a/src/hash.rs b/src/hash.rs
>> new file mode 100644
>> index 0000000000..0219391820
>> --- /dev/null
>> +++ b/src/hash.rs
>> @@ -0,0 +1,21 @@
>> +// This program is free software; you can redistribute it and/or modify
>> +// it under the terms of the GNU General Public License as published by
>> +// the Free Software Foundation: version 2 of the License, dated June 1991.
>> +//
>> +// This program is distributed in the hope that it will be useful,
>> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +// GNU General Public License for more details.
>> +//
>> +// You should have received a copy of the GNU General Public License along
>> +// with this program; if not, see <https://www.gnu.org/licenses/>.
>
> We typically don't have these headers for our C code, so why have it
> over here?

Yeah, another thing that puzzles me is if src/ is a good name for
the directory in the longer run (unless we plan to rewrite
everything in Rust, that is) for housing our source code written in
Rust (I am assuming that *.c files are unwelcome in that directory).
But it may be a separate topic, perhaps?

>> +pub const GIT_MAX_RAWSZ: usize = 32;
>> +
>> +/// A binary object ID.
>> +#[repr(C)]
>> +#[derive(Debug, Clone, Ord, PartialOrd, Eq, PartialEq)]
>> +pub struct ObjectID {
>> +    pub hash: [u8; GIT_MAX_RAWSZ],
>> +    pub algo: u32,
>> +}
>
> An alternative to represent this type would be to use an enum:
>
>     pub enum ObjectID {
>         SHA1([u8; GIT_SHA1_RAWSZ]),
>         SHA256([u8; GIT_SHA256_RAWSZ]),
>     }
>
> That would give us some type safety going forward, but it might be
> harder to work with for us?

Can the latter be made interoperate with the C side well, with the
same memory layout?  Perhaps there may be a way, but the way written
in the patch looks more obviously identical to what we have on the C
side, so...