-
Notifications
You must be signed in to change notification settings - Fork 157
libgit-rs: add get_bool(), get_ulong(), and get_pathname() methods to ConfigSet #1977
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
|
||
use libgit_sys::*; | ||
|
||
/// A ConfigSet is an in-memory cache for config-like files such as `.gitmodules` or `.gitconfig`. | ||
|
@@ -68,6 +72,55 @@ impl ConfigSet { | |
Some(owned_str) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the Git mailing list, Chris Torek wrote (reply to this): A bit minor, and I'm not a real Rust programmer, but:
On Thu, Sep 25, 2025 at 4:44 AM ionnss via GitGitGadget
<[email protected]> wrote:
>
> From: ionnss <[email protected]>
>
> Add support for parsing boolean configuration values in the Rust
> ConfigSet API. The method follows Git's standard boolean parsing
> rules, accepting true/yes/on/1 as true and false/no/off/0 as false.
>
> The implementation reuses the existing get_string() infrastructure
> and adds case-insensitive boolean parsing logic.
>
> Signed-off-by: ionnss <[email protected]>
> ---
> contrib/libgit-rs/src/config.rs | 24 ++++++++++++++++++++++++
> contrib/libgit-rs/testdata/config3 | 2 ++
> 2 files changed, 26 insertions(+)
>
> diff --git a/contrib/libgit-rs/src/config.rs b/contrib/libgit-rs/src/config.rs
> index 6bf04845c8..3f4a32c72d 100644
> --- a/contrib/libgit-rs/src/config.rs
> +++ b/contrib/libgit-rs/src/config.rs
> @@ -68,6 +68,26 @@ impl ConfigSet {
> Some(owned_str)
> }
> }
> +
> + pub fn get_bool(&mut self, key: &str) -> Option<bool> {
> + let key = CString::new(key).expect("Couldn't convert key to CString");
The string argument for `.expect` should be phrased in
a more positive manner in terms of what is expected,
since failure will cause a panic. So, something like:
let key = CString::new(key).expect("boolean key should be valid CString");
which would produce, e.g.,
panic: boolean key should be valid CString: ... details of key ...
A similar rule applies to the later `.expect`.
Chris There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the Git mailing list, Phillip Wood wrote (reply to this): On 25/09/2025 12:44, ionnss via GitGitGadget wrote:
> From: ionnss <[email protected]>
> > Add support for parsing boolean configuration values in the Rust
> ConfigSet API. The method follows Git's standard boolean parsing
> rules, accepting true/yes/on/1 as true and false/no/off/0 as false.
> > The implementation reuses the existing get_string() infrastructure
> and adds case-insensitive boolean parsing logic.
It's nice to know that someone is using the rust bindings. The code in contrib/libgit-rs is intended to be safe wrappers around the unsafe functions in contrib/libgit-sys which wrap git's C code. I think what we need to do here is add a binding for git_configset_get_bool() to libgit-sys and then wrap that in libgit-rs. We don't want to start implementing the parsing separately as they'll inevitably end up behaving differently to git. For example what you have here parses "00" or "100" differently to git.
Thanks
Phillip
> Signed-off-by: ionnss <[email protected]>
> ---
> contrib/libgit-rs/src/config.rs | 24 ++++++++++++++++++++++++
> contrib/libgit-rs/testdata/config3 | 2 ++
> 2 files changed, 26 insertions(+)
> > diff --git a/contrib/libgit-rs/src/config.rs b/contrib/libgit-rs/src/config.rs
> index 6bf04845c8..3f4a32c72d 100644
> --- a/contrib/libgit-rs/src/config.rs
> +++ b/contrib/libgit-rs/src/config.rs
> @@ -68,6 +68,26 @@ impl ConfigSet {
> Some(owned_str)
> }
> }
> +
> + 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();
> + unsafe {
> + if libgit_configset_get_string(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("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,
> + }
> + }
> + }
> }
> > impl Default for ConfigSet {
> @@ -102,5 +122,9 @@ 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 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 ca7b9a7c38..83a474ccef 100644
> --- a/contrib/libgit-rs/testdata/config3
> +++ b/contrib/libgit-rs/testdata/config3
> @@ -1,2 +1,4 @@
> [trace2]
> eventNesting = 3
> +[test]
> + booleanValue = true There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. Will work on that! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the Git mailing list, Junio C Hamano wrote (reply to this): Phillip Wood <[email protected]> writes:
> On 25/09/2025 12:44, ionnss via GitGitGadget wrote:
>> From: ionnss <[email protected]>
>> Add support for parsing boolean configuration values in the Rust
>> ConfigSet API. The method follows Git's standard boolean parsing
>> rules, accepting true/yes/on/1 as true and false/no/off/0 as false.
>> The implementation reuses the existing get_string() infrastructure
>> and adds case-insensitive boolean parsing logic.
>
> It's nice to know that someone is using the rust bindings. The code in
> contrib/libgit-rs is intended to be safe wrappers around the unsafe
> functions in contrib/libgit-sys which wrap git's C code. I think what
> we need to do here is add a binding for git_configset_get_bool() to
> libgit-sys and then wrap that in libgit-rs. We don't want to start
> implementing the parsing separately as they'll inevitably end up
> behaving differently to git. For example what you have here parses
> "00" or "100" differently to git.
>
> Thanks
Thanks for paying attention to an important detail on the design
criteria.
As this is a binding to allow Rust programs to access the feature
implemented and offered by Git, I fully agree that reimplementing
what Git does in an incompatible way in Rust would not help the
targetted intended audiences at all (that is better off done in a
project that aims to reimplement what Git does, like gitoxide,
targetting native Rust solution).
If I were not paying attention, I would very likely have missed the
misparses of number-as-bool, distracted and blinded by the prettier
and nicer syntax the language offers over the original in C X-<.
Thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the Git mailing list, Phillip Wood wrote (reply to this): On 27/09/2025 04:51, ionnss via GitGitGadget wrote:
> From: ionnss <[email protected]>
> > Add support for parsing boolean configuration values using Git's
> git_configset_get_bool() C function. This ensures consistent behavior
> with Git's native boolean parsing logic.
> > The method handles all Git boolean formats (true/false, yes/no, on/off,
> 1/0) and edge cases like "00" and "100" correctly.
> > Includes comprehensive tests for various boolean formats and edge cases.
Unfortunately when I try to run those tests with
make INCLUDE_LIBGIT_RS=1 && cd contrib/libgit-rs && cargo test
I see
= note: /usr/bin/ld: /home/phil/src/git/update-ref-date/contrib/libgit-rs/target/debug/deps/libgit-8c021564b9d1ec26.9j3smxv8a5f6u8czhaxq401mn.rcgu.o: in function `libgit::config::ConfigSet::get_bool':
/home/phil/src/git/update-ref-date/contrib/libgit-rs/src/config.rs:78: undefined reference to `libgit_configset_get_bool'
collect2: error: ld returned 1 exit status
This happens because libgit_configset_get_bool() is not defined in contrib/libgit-sys/public_symbol_export.[ch]. You need to wrap configset_get_bool() in the some way that configset_get_int() is.
Thanks
Phillip
> Signed-off-by: ionnss <[email protected]>
> ---
> contrib/libgit-rs/src/config.rs | 26 ++++++++++++++++++++++++++
> contrib/libgit-rs/testdata/config3 | 2 +-
> contrib/libgit-rs/testdata/config4 | 9 +++++++++
> contrib/libgit-sys/src/lib.rs | 6 ++++++
> 4 files changed, 42 insertions(+), 1 deletion(-)
> 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 6bf04845c8..72ee88801b 100644
> --- a/contrib/libgit-rs/src/config.rs
> +++ b/contrib/libgit-rs/src/config.rs
> @@ -68,6 +68,20 @@ impl ConfigSet {
> Some(owned_str)
> }
> }
> +
> + /// 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("config key should be valid CString");
> + let mut val: c_int = 0;
> + unsafe {
> + if libgit_configset_get_bool(self.0, key.as_ptr(), &mut val as *mut c_int) != 0 {
> + return None;
> + }
> + }
> +
> + Some(val != 0)
> + }
> }
> > impl Default for ConfigSet {
> @@ -95,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));
> @@ -102,5 +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 - 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
> + // 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 ca7b9a7c38..3ea5b96f12 100644
> --- a/contrib/libgit-rs/testdata/config3
> +++ b/contrib/libgit-rs/testdata/config3
> @@ -1,2 +1,2 @@
> [trace2]
> - eventNesting = 3
> + 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..c0755a32be
> --- /dev/null
> +++ b/contrib/libgit-rs/testdata/config4
> @@ -0,0 +1,9 @@
> +[test]
> + boolTrue = true
> + boolFalse = false
> + boolYes = yes
> + boolNo = no
> + boolOne = 1
> + boolZero = 0
> + boolZeroZero = 00
> + boolHundred = 100
> 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)] |
||
} | ||
} | ||
|
||
/// 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("config key should be valid CString"); | ||
let mut val: c_int = 0; | ||
unsafe { | ||
if libgit_configset_get_bool(self.0, key.as_ptr(), &mut val as *mut c_int) != 0 { | ||
return None; | ||
} | ||
} | ||
|
||
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. | ||
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) | ||
} | ||
|
||
/// 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"), | ||
); | ||
free(val as *mut c_void); // Free the xstrdup()ed pointer from the C side | ||
Some(PathBuf::from(owned_str)) | ||
} | ||
} | ||
} | ||
|
||
impl Default for ConfigSet { | ||
|
@@ -95,12 +148,38 @@ 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)); | ||
// ConfigSet respects last config value set | ||
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 - 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 | ||
// Test missing boolean key | ||
assert_eq!(cs.get_bool("missing.boolean"), None); | ||
// Test ulong parsing | ||
assert_eq!(cs.get_ulong("test.ulongSmall"), Some(42)); | ||
assert_eq!(cs.get_ulong("test.ulongBig"), Some(4294967296)); // > 32-bit int | ||
assert_eq!(cs.get_ulong("missing.ulong"), None); | ||
// Test pathname parsing | ||
assert_eq!( | ||
cs.get_pathname("test.pathRelative"), | ||
Some(PathBuf::from("./some/path")) | ||
); | ||
assert_eq!( | ||
cs.get_pathname("test.pathAbsolute"), | ||
Some(PathBuf::from("/usr/bin/git")) | ||
); | ||
assert_eq!(cs.get_pathname("missing.path"), None); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,2 @@ | ||
[trace2] | ||
eventNesting = 3 | ||
eventNesting = 3 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
[test] | ||
boolTrue = true | ||
boolFalse = false | ||
boolYes = yes | ||
boolNo = no | ||
boolOne = 1 | ||
boolZero = 0 | ||
boolZeroZero = 00 | ||
boolHundred = 100 | ||
ulongSmall = 42 | ||
ulongBig = 4294967296 | ||
pathRelative = ./some/path | ||
pathAbsolute = /usr/bin/git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Phillip Wood wrote (reply to this):