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

From: Ezekiel Newren <ezekielnewren@gmail.com>
Date: Oct 28, 2025, 19:07 UTC
Message-ID: <CAH=ZcbBnTAWe=2SihD5G63e6T__wWj870u3eRE+rueH51gpqnA@mail.gmail.com>
In-Reply-To: <aQCKD--ZmKnwBWs9@pks.im>

On Tue, Oct 28, 2025 at 3:17 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 22 quoted lines

>
> 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?

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.

Show 19 quoted lines

> > +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::<ObjectId>() 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.