From: Phillip Wood Date: Wed, 01 Oct 2025 10:15:28 GMT Subject: Re: [PATCH v4] libgit-rs: add get_bool(), get_ulong(), and get_pathname() methods Message-ID: <0cb55558-a48e-42ac-a80e-a24ee2be55ee@gmail.com> In-Reply-To: [I've cc'd brian to sanity check my suggestion for handling c_ulong in rust 1.63 which lacks std::ffi::c_ulong] On 30/09/2025 09:46, ions via GitGitGadget wrote: > From: ionnss > > Expand ConfigSet API with three new configuration value parsers: > > - get_bool(): Parse boolean values using git_configset_get_bool() > - get_ulong(): Parse unsigned long values > - get_pathname(): Parse file paths, returning PathBuf I would be nice to explain in the commit message why we require paths to be utf-8 encoded. I've left one comment below, apart from that I think this looks good. > --- a/contrib/libgit-sys/src/lib.rs > +++ b/contrib/libgit-sys/src/lib.rs > @@ -1,7 +1,7 @@ > use std::ffi::c_void; > > #[cfg(has_std__ffi__c_char)] > -use std::ffi::{c_char, c_int}; > +use std::ffi::{c_char, c_int, c_ulong}; > > #[cfg(not(has_std__ffi__c_char))] > #[allow(non_camel_case_types)] > @@ -11,6 +11,10 @@ pub type c_char = i8; > #[allow(non_camel_case_types)] > pub type c_int = i32; > > +#[cfg(not(has_std__ffi__c_char))] > +#[allow(non_camel_case_types)] > +pub type c_ulong = u64; As I said before this wont work because C's ulong type is platform dependent so you cannot assume it 64 bits wide. Looking at the previous discussion[1] the reason we have these fallback definitions is because std::ffi::c_int etc were only added in rust 1.64 and we want to support rust 1.63 as that is the version shipped by Debian oldstable. I think it would be better to have a separate preparatory patch that changes the existing fallbacks to #[cfg(not(has_std__ff__c_char))] use std::os::raw::{c_char, c_int}; and then this patch can add "c_ulong" to the list. Thanks Phillip [1] https://lore.kernel.org/git/ZtivGeDZ_MZDEDB_@tapette.crustytoothpaste.net/