Thread (20 messages) 20 messages, 4 authors, 2025-10-08
STALE290d
Revisions (2)
  1. v2 current
  2. v3 [diff vs current]

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

From: ionnss via GitGitGadget <hidden>
Date: 2025-09-27 00:07:51
Subsystem: the rest · Maintainer: Linus Torvalds

From: ionnss <redacted>

- 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)

This addresses feedback from Phillip Wood and Chris Torek about using
Git's actual boolean parsing logic rather than duplicating it in Rust.

Signed-off-by: ionnss <redacted>
---
 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help