[PATCH v2 3/3] libgit-rs: address review feedback for get_bool()

From

IGionnss via GitGitGadget <gitgitgadget@gmail.com>

Date

Sep 27, 2025, 00:07 UTC

Message-ID

<43784e3ff991929b5c314ad5b9ab6573e6ca48c4.1758931659.git.gitgitgadget@gmail.com>

In-Reply-To

<pull.1977.v2.git.1758931659.gitgitgadget@gmail.com>

From: ionnss <zara.leonardo@gmail.com>

- Use git_configset_get_bool() C function instead of reimplementing parsing
- Fix libgit_configset_get_bool() function signature in bindings
- Improve .expect() error messages to be more descriptive
- Add comprehensive boolean tests including edge cases (00, 100, 007)

Signed-off-by: ionnss <zara.leonardo@gmail.com>
---
 contrib/libgit-rs/src/config.rs    | 33 ++++++++++++++++--------------
 contrib/libgit-rs/testdata/config3 |  4 +---
 contrib/libgit-rs/testdata/config4 | 10 +++++++++
 contrib/libgit-sys/src/lib.rs      |  6 ++++++
 4 files changed, 35 insertions(+), 18 deletions(-)
 create mode 100644 contrib/libgit-rs/testdata/config4

diff --git a/contrib/libgit-rs/src/config.rs b/contrib/libgit-rs/src/config.rs
index 3f4a32c72d..b280b952b2 100644
--- a/contrib/libgit-rs/src/config.rs
+++ b/contrib/libgit-rs/src/config.rs
@@ -69,24 +69,18 @@ impl ConfigSet {
         }
     }
 
+    /// Load the value for the given key and attempt to parse it as a boolean. Dies with a fatal error
+    /// if the value cannot be parsed. Returns None if the key is not present.
     pub fn get_bool(&mut self, key: &str) -> Option<bool> {
-        let key = CString::new(key).expect("Couldn't convert key to CString");
-        let mut val: *mut c_char = std::ptr::null_mut();
+        let key = CString::new(key).expect("config key should be valid CString");
+        let mut val: c_int = 0;
         unsafe {
-            if libgit_configset_get_string(self.0, key.as_ptr(), &mut val as *mut *mut c_char) != 0
-            {
+            if libgit_configset_get_bool(self.0, key.as_ptr(), &mut val as *mut c_int) != 0 {
                 return None;
             }
-            let borrowed_str = CStr::from_ptr(val);
-            let owned_str =
-                String::from(borrowed_str.to_str().expect("Couldn't convert val to str"));
-            free(val as *mut c_void); // Free the xstrdup()ed pointer from the C side
-            match owned_str.to_lowercase().as_str() {
-                "true" | "yes" | "on" | "1" => Some(true),
-                "false" | "no" | "off" | "0" => Some(false),
-                _ => None,
-            }
         }
+
+        Some(val != 0)
     }
 }
 
@@ -115,6 +109,7 @@ mod tests {
             Path::new("testdata/config1"),
             Path::new("testdata/config2"),
             Path::new("testdata/config3"),
+            Path::new("testdata/config4"),
         ]);
         // ConfigSet retrieves correct value
         assert_eq!(cs.get_int("trace2.eventTarget"), Some(1));
@@ -122,8 +117,16 @@ mod tests {
         assert_eq!(cs.get_int("trace2.eventNesting"), Some(3));
         // ConfigSet returns None for missing key
         assert_eq!(cs.get_string("foo.bar"), None);
-        // Test boolean parsing
-        assert_eq!(cs.get_bool("test.booleanValue"), Some(true));
+        // Test boolean parsing - comprehensive tests
+        assert_eq!(cs.get_bool("test.boolTrue"), Some(true));
+        assert_eq!(cs.get_bool("test.boolFalse"), Some(false));
+        assert_eq!(cs.get_bool("test.boolYes"), Some(true));
+        assert_eq!(cs.get_bool("test.boolNo"), Some(false));
+        assert_eq!(cs.get_bool("test.boolOne"), Some(true));
+        assert_eq!(cs.get_bool("test.boolZero"), Some(false));
+        assert_eq!(cs.get_bool("test.boolZeroZero"), Some(false)); // "00" → false
+        assert_eq!(cs.get_bool("test.boolHundred"), Some(true)); // "100" → true
+        assert_eq!(cs.get_bool("test.boolSeven"), Some(true)); // "007" → true
         // Test missing boolean key
         assert_eq!(cs.get_bool("missing.boolean"), None);
     }
diff --git a/contrib/libgit-rs/testdata/config3 b/contrib/libgit-rs/testdata/config3
index 83a474ccef..3ea5b96f12 100644
--- a/contrib/libgit-rs/testdata/config3
+++ b/contrib/libgit-rs/testdata/config3
@@ -1,4 +1,2 @@
 [trace2]
-	eventNesting = 3
-[test]
-	booleanValue = true
+	eventNesting = 3
\ No newline at end of file
diff --git a/contrib/libgit-rs/testdata/config4 b/contrib/libgit-rs/testdata/config4
new file mode 100644
index 0000000000..5b75385c38
--- /dev/null
+++ b/contrib/libgit-rs/testdata/config4
@@ -0,0 +1,10 @@
+[test]
+	boolTrue = true
+	boolFalse = false
+	boolYes = yes
+	boolNo = no
+	boolOne = 1
+	boolZero = 0
+	boolZeroZero = 00
+	boolHundred = 100
+	boolSeven = 007
diff --git a/contrib/libgit-sys/src/lib.rs b/contrib/libgit-sys/src/lib.rs
index 4bfc650450..b104fda8f6 100644
--- a/contrib/libgit-sys/src/lib.rs
+++ b/contrib/libgit-sys/src/lib.rs
@@ -43,6 +43,12 @@ extern "C" {
         dest: *mut *mut c_char,
     ) -> c_int;
 
+    pub fn libgit_configset_get_bool(
+        cs: *mut libgit_config_set,
+        key: *const c_char,
+        dest: *mut c_int,
+    ) -> c_int;
+
 }
 
 #[cfg(test)]

-- 
gitgitgadget