Re: [PATCH 2/9] xdiff: make xrecord_t.ptr a uint8_t instead of char

From: Ezekiel Newren <ezekielnewren@gmail.com>
Date: Oct 22, 2025, 20:55 UTC
Message-ID: <CAH=ZcbALmH1LRKpLXygUOPiNJeoG2Uqvkb0fuy_i412W=z2oeQ@mail.gmail.com>
In-Reply-To: <d863c518-3246-4752-83f3-469592b1de69@gmail.com>

On Wed, Oct 22, 2025 at 7:27 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 11 quoted lines

> > I 100% agree that being honest about the motivation to sell this
> > change would be a good thing to do here.  I do not think "in this
> > series, I want to match the types used at the interface to be of
> > Rust's" is a position to be ashamed of ;-)
> >
> >> I agree with Patrick's comments on this patch - it would be nice to know
> >> how you decided where to add casts. Given that rust is going to be
> >> optional for at least a year we should take care to leave the C code in
> >> good shape with a minimum number of casts.
> >
> > Thanks.

I'm not arguing that uint8_t should be used everywhere in Git, only that it is used everywhere in xdiff. xrecord_t and xdfile_t are fundamental to how xdiff passes data around and they need to be transparent to both sides. I'm trying to leave the rest of the data structures alone in order to avoid refactor churn. Refactoring C to use unambiguous types, outside of xdiff, is outside the scope of this patch series.

Another problem with using char instead of uint8_t is that tools like cbindgen and bindgen don't translate char to u8. Bindgen will see char and will produce std::ffi::c_char on the Rust side, see [1] for why that's a problem. The other way around is a problem too. When cbindgen sees u8 it will generate uint8_t on the C side and then `make DEVELOPER=1` won't compile because uint8_t and char differer in signedness.

[1] Problems with C types https://lore.kernel.org/git/CAH=ZcbA_8JM1hdUAfFe3ho0ShuniguEpV1308S0nCkCHOCsmmg@mail.gmail.com/