Skip to content

Commit 43784e3

Browse files
committed
libgit-rs: address review feedback for get_bool()
- 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 <[email protected]>
1 parent a5904a2 commit 43784e3

File tree

4 files changed

+35
-18
lines changed

4 files changed

+35
-18
lines changed

contrib/libgit-rs/src/config.rs

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -69,24 +69,18 @@ impl ConfigSet {
6969
}
7070
}
7171

72+
/// Load the value for the given key and attempt to parse it as a boolean. Dies with a fatal error
73+
/// if the value cannot be parsed. Returns None if the key is not present.
7274
pub fn get_bool(&mut self, key: &str) -> Option<bool> {
73-
let key = CString::new(key).expect("Couldn't convert key to CString");
74-
let mut val: *mut c_char = std::ptr::null_mut();
75+
let key = CString::new(key).expect("config key should be valid CString");
76+
let mut val: c_int = 0;
7577
unsafe {
76-
if libgit_configset_get_string(self.0, key.as_ptr(), &mut val as *mut *mut c_char) != 0
77-
{
78+
if libgit_configset_get_bool(self.0, key.as_ptr(), &mut val as *mut c_int) != 0 {
7879
return None;
7980
}
80-
let borrowed_str = CStr::from_ptr(val);
81-
let owned_str =
82-
String::from(borrowed_str.to_str().expect("Couldn't convert val to str"));
83-
free(val as *mut c_void); // Free the xstrdup()ed pointer from the C side
84-
match owned_str.to_lowercase().as_str() {
85-
"true" | "yes" | "on" | "1" => Some(true),
86-
"false" | "no" | "off" | "0" => Some(false),
87-
_ => None,
88-
}
8981
}
82+
83+
Some(val != 0)
9084
}
9185
}
9286

@@ -115,15 +109,24 @@ mod tests {
115109
Path::new("testdata/config1"),
116110
Path::new("testdata/config2"),
117111
Path::new("testdata/config3"),
112+
Path::new("testdata/config4"),
118113
]);
119114
// ConfigSet retrieves correct value
120115
assert_eq!(cs.get_int("trace2.eventTarget"), Some(1));
121116
// ConfigSet respects last config value set
122117
assert_eq!(cs.get_int("trace2.eventNesting"), Some(3));
123118
// ConfigSet returns None for missing key
124119
assert_eq!(cs.get_string("foo.bar"), None);
125-
// Test boolean parsing
126-
assert_eq!(cs.get_bool("test.booleanValue"), Some(true));
120+
// Test boolean parsing - comprehensive tests
121+
assert_eq!(cs.get_bool("test.boolTrue"), Some(true));
122+
assert_eq!(cs.get_bool("test.boolFalse"), Some(false));
123+
assert_eq!(cs.get_bool("test.boolYes"), Some(true));
124+
assert_eq!(cs.get_bool("test.boolNo"), Some(false));
125+
assert_eq!(cs.get_bool("test.boolOne"), Some(true));
126+
assert_eq!(cs.get_bool("test.boolZero"), Some(false));
127+
assert_eq!(cs.get_bool("test.boolZeroZero"), Some(false)); // "00" → false
128+
assert_eq!(cs.get_bool("test.boolHundred"), Some(true)); // "100" → true
129+
assert_eq!(cs.get_bool("test.boolSeven"), Some(true)); // "007" → true
127130
// Test missing boolean key
128131
assert_eq!(cs.get_bool("missing.boolean"), None);
129132
}

contrib/libgit-rs/testdata/config3

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,2 @@
11
[trace2]
2-
eventNesting = 3
3-
[test]
4-
booleanValue = true
2+
eventNesting = 3

contrib/libgit-rs/testdata/config4

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
[test]
2+
boolTrue = true
3+
boolFalse = false
4+
boolYes = yes
5+
boolNo = no
6+
boolOne = 1
7+
boolZero = 0
8+
boolZeroZero = 00
9+
boolHundred = 100
10+
boolSeven = 007

contrib/libgit-sys/src/lib.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,12 @@ extern "C" {
4343
dest: *mut *mut c_char,
4444
) -> c_int;
4545

46+
pub fn libgit_configset_get_bool(
47+
cs: *mut libgit_config_set,
48+
key: *const c_char,
49+
dest: *mut c_int,
50+
) -> c_int;
51+
4652
}
4753

4854
#[cfg(test)]

0 commit comments

Comments
 (0)