From: Ezekiel Newren Date: Tue, 28 Oct 2025 19:07:36 GMT Subject: Re: [PATCH 04/14] rust: add a ObjectID struct Message-ID: In-Reply-To: On Tue, Oct 28, 2025 at 3:17 AM Patrick Steinhardt wrote: > > 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 . > > We typically don't have these headers for our C code, so why have it > over here? I'm wondering this too even though you gave a reason in your cover letter. I'm against putting licenses in each source file, and don't see how it's better than having a separate license file. > > +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? This would be fine if it was used exclusively in Rust, but since this is a type that has to cross the FFI boundary it should be defined as a struct in C and Rust. If you run size_of::() you'll get 33 (but it could be something else). Without #[repr(C, u8)] the Rust compiler is free to choose how to define the discriminant (its length and values) to distinguish the 2 types. If you do use #[repr(C, u8)] then you have the possible problem of C setting an invalid discriminant value which would result in undefined behavior. It also doesn't make sense as an FFI type since a Rust enum is closer to a C union than a C enum. The point here is that Brian is matching the existing C struct with an equivalent Rust struct.