Re: [PATCH v4] libgit-rs: add get_bool(), get_ulong(), and get_pathname() methods

[Phillip Wood <phillip.wood123@gmail.com>]

[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 <zara.leonardo@gmail.com>
> 
> 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/