On 27/09/2025 04:51, ionnss via GitGitGadget wrote:

From: ionnss <redacted>

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.

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.

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