From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 29 Sep 2025 13:23:59 GMT
Subject: Re: [PATCH v3 3/3] libgit-rs: add get_ulong() and get_pathname() methods
Message-ID: <9e7da34f-8d1d-4223-a160-e0223984aeaf@gmail.com>
In-Reply-To: <1ac8d768194b15eaf536000ed5f76f36dd0a39b2.1758945111.git.gitgitgadget@gmail.com>

On 27/09/2025 04:51, ionnss via GitGitGadget wrote:
> From: ionnss <zara.leonardo@gmail.com>
> 
> Expand the ConfigSet API with additional configuration value types:
> 
> - get_ulong(): Parse unsigned long integers for large numeric values

I'm torn as to whether we should keep the same name as the C function as 
you have done, or call it get_u64 to avoid the name depending on a 
platform dependent type. It is very welcome that this function returns u64.

> - get_pathname(): Parse file paths, returning PathBuf for type safety

I'm not sure why these two functions are in the same commit, it would 
make more sense to separate them out I think like you have done for 
get_bool().

> Both functions follow the same pattern as existing get_* methods,
> using Git's C functions for consistent parsing behavior.
> 
> Add comprehensive tests covering normal cases, edge cases, and
> error handling for all new functionality.

We can debate what constituets "comprehensive" but it is good to see 
that the new functions are tested.

> diff --git a/contrib/libgit-rs/src/config.rs b/contrib/libgit-rs/src/config.rs
> index 72ee88801b..ffd9f311b6 100644
> --- a/contrib/libgit-rs/src/config.rs
> +++ b/contrib/libgit-rs/src/config.rs
> @@ -1,8 +1,8 @@
>   use std::ffi::{c_void, CStr, CString};
> -use std::path::Path;
> +use std::path::{Path, PathBuf};
>   
>   #[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)]
> @@ -12,6 +12,10 @@ type c_char = i8;
>   #[allow(non_camel_case_types)]
>   type c_int = i32;
>   
> +#[cfg(not(has_std__ffi__c_char))]
> +#[allow(non_camel_case_types)]
> +type c_ulong = u64;
This is a bit problematic as the type depends on the platform. I'm not 
entirely clear why the current code doesn't just rely on having std::ffi 
define these types.

> +
>   use libgit_sys::*;
>   
>   /// A ConfigSet is an in-memory cache for config-like files such as `.gitmodules` or `.gitconfig`.
> @@ -82,6 +86,41 @@ impl ConfigSet {
>   
>           Some(val != 0)
>       }
> +
> +    /// Load the value for the given key and attempt to parse it as an unsigned long. Dies with a fatal error
> +    /// if the value cannot be parsed. Returns None if the key is not present.

Please wrap the comments to 80 columns

> +    pub fn get_ulong(&mut self, key: &str) -> Option<u64> {
> +        let key = CString::new(key).expect("config key should be valid CString");
> +        let mut val: c_ulong = 0;
> +        unsafe {
> +            if libgit_configset_get_ulong(self.0, key.as_ptr(), &mut val as *mut c_ulong) != 0 {
> +                return None;
> +            }
> +        }
> +        Some(val as u64)
> +    }

This looks good

> +    /// Load the value for the given key and attempt to parse it as a file path. Dies with a fatal error
> +    /// if the value cannot be converted to a PathBuf. Returns None if the key is not present.
> +    pub fn get_pathname(&mut self, key: &str) -> Option<PathBuf> {
> +        let key = CString::new(key).expect("config key should be valid CString");
> +        let mut val: *mut c_char = std::ptr::null_mut();
> +        unsafe {
> +            if libgit_configset_get_pathname(self.0, key.as_ptr(), &mut val as *mut *mut c_char)
> +                != 0
> +            {
> +                return None;
> +            }
> +            let borrowed_str = CStr::from_ptr(val);
> +            let owned_str = String::from(
> +                borrowed_str
> +                    .to_str()
> +                    .expect("config path should be valid UTF-8"),

As we're returning a PathBuf it is a shame that we're restricted to 
UTF-8 encoded paths. Unfortunately rust's standard library does not seem 
to provide an easy way to convert a CStr to an OsString on windows as 
one needs to convert it to a UTF-16 encoded string first.

> +    pub fn libgit_configset_get_ulong(
> +        cs: *mut libgit_config_set,
> +        key: *const c_char,
> +        dest: *mut c_ulong,
> +    ) -> c_int;
> +
> +    pub fn libgit_configset_get_pathname(
> +        cs: *mut libgit_config_set,
> +        key: *const c_char,
> +        dest: *mut *mut c_char,
> +    ) -> c_int;

As with the previous patch you need to define these functions in 
public_symbol_export.[ch]

Thanks

Phillip