* [PATCH 0/2] libgit-rs: add get_bool() method to ConfigSet
@ 2025-09-25 11:44 ions via GitGitGadget
2025-09-25 11:44 ` [PATCH 1/2] po: fix escaped underscores in README.md ionnss via GitGitGadget
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: ions via GitGitGadget @ 2025-09-25 11:44 UTC (permalink / raw)
To: git; +Cc: ions
Purpose
This pull request introduces a get_bool() method to the ConfigSet module in
the libgit-rs library. The goal is to enhance the functionality of ConfigSet
by providing a way to fetch and handle boolean configuration values more
easily and consistently.
Implementation Details
• Added a get_bool() method to the ConfigSet module.
• The method retrieves configuration values as boolean values, ensuring
proper parsing and error handling.
• This addition simplifies the process of working with boolean
configurations for developers using the ConfigSet module.
Testing
• Added unit tests to verify the correctness of the get_bool() method.
• Tested edge cases to ensure robustness.
ionnss (2):
po: fix escaped underscores in README.md
libgit-rs: add get_bool() method to ConfigSet
contrib/libgit-rs/src/config.rs | 24 ++++++++++++++++++++++++
contrib/libgit-rs/testdata/config3 | 2 ++
po/README.md | 6 +++---
3 files changed, 29 insertions(+), 3 deletions(-)
base-commit: bb69721404348ea2db0a081c41ab6ebfe75bdec8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1977%2Fionnss%2Fadd-rust-configset-get-bool-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1977/ionnss/add-rust-configset-get-bool-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1977
--
gitgitgadget
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] po: fix escaped underscores in README.md
2025-09-25 11:44 [PATCH 0/2] libgit-rs: add get_bool() method to ConfigSet ions via GitGitGadget
@ 2025-09-25 11:44 ` ionnss via GitGitGadget
2025-09-25 11:44 ` [PATCH 2/2] libgit-rs: add get_bool() method to ConfigSet ionnss via GitGitGadget
2025-09-27 0:07 ` [PATCH v2 0/3] " ions via GitGitGadget
2 siblings, 0 replies; 22+ messages in thread
From: ionnss via GitGitGadget @ 2025-09-25 11:44 UTC (permalink / raw)
To: git; +Cc: ions, ionnss
From: ionnss <zara.leonardo@gmail.com>
Remove unnecessary backslashes from language code examples.
The underscores in "ll\_CC" and "zh\_CN" don't need escaping
in Markdown.
Signed-off-by: ionnss <zara.leonardo@gmail.com>
---
po/README.md | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/po/README.md b/po/README.md
index ec08aa24ad..d7757bed4e 100644
--- a/po/README.md
+++ b/po/README.md
@@ -13,9 +13,9 @@ We will use XX as an alias to refer to the language translation code in
the following paragraphs, for example we use "po/XX.po" to refer to the
translation file for a specific language. But this doesn't mean that
the language code has only two letters. The language code can be in one
-of two forms: "ll" or "ll\_CC". Here "ll" is the ISO 639 two-letter
+of two forms: "ll" or "ll_CC". Here "ll" is the ISO 639 two-letter
language code and "CC" is the ISO 3166 two-letter code for country names
-and subdivisions. For example: "de" for German language code, "zh\_CN"
+and subdivisions. For example: "de" for German language code, "zh_CN"
for Simplified Chinese language code.
@@ -126,7 +126,7 @@ you add a translation for the first time by running:
make po-init PO_FILE=po/XX.po
```
-where XX is the locale, e.g. "de", "is", "pt\_BR", "zh\_CN", etc.
+where XX is the locale, e.g. "de", "is", "pt_BR", "zh_CN", etc.
The newly generated message file "po/XX.po" is based on the core pot
file "po/git-core.pot", so it contains only a minimal set of messages
--
gitgitgadget
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/2] libgit-rs: add get_bool() method to ConfigSet
2025-09-25 11:44 [PATCH 0/2] libgit-rs: add get_bool() method to ConfigSet ions via GitGitGadget
2025-09-25 11:44 ` [PATCH 1/2] po: fix escaped underscores in README.md ionnss via GitGitGadget
@ 2025-09-25 11:44 ` ionnss via GitGitGadget
2025-09-26 6:43 ` Chris Torek
2025-09-26 9:58 ` Phillip Wood
2025-09-27 0:07 ` [PATCH v2 0/3] " ions via GitGitGadget
2 siblings, 2 replies; 22+ messages in thread
From: ionnss via GitGitGadget @ 2025-09-25 11:44 UTC (permalink / raw)
To: git; +Cc: ions, ionnss
From: ionnss <zara.leonardo@gmail.com>
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 <zara.leonardo@gmail.com>
---
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
--
gitgitgadget
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] libgit-rs: add get_bool() method to ConfigSet
2025-09-25 11:44 ` [PATCH 2/2] libgit-rs: add get_bool() method to ConfigSet ionnss via GitGitGadget
@ 2025-09-26 6:43 ` Chris Torek
2025-09-26 9:58 ` Phillip Wood
1 sibling, 0 replies; 22+ messages in thread
From: Chris Torek @ 2025-09-26 6:43 UTC (permalink / raw)
To: ionnss via GitGitGadget; +Cc: git, ionnss
A bit minor, and I'm not a real Rust programmer, but:
On Thu, Sep 25, 2025 at 4:44 AM ionnss via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: ionnss <zara.leonardo@gmail.com>
>
> 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 <zara.leonardo@gmail.com>
> ---
> 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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] libgit-rs: add get_bool() method to ConfigSet
2025-09-25 11:44 ` [PATCH 2/2] libgit-rs: add get_bool() method to ConfigSet ionnss via GitGitGadget
2025-09-26 6:43 ` Chris Torek
@ 2025-09-26 9:58 ` Phillip Wood
2025-09-26 17:15 ` Junio C Hamano
1 sibling, 1 reply; 22+ messages in thread
From: Phillip Wood @ 2025-09-26 9:58 UTC (permalink / raw)
To: ionnss via GitGitGadget, git; +Cc: ions, Josh Steadmon
On 25/09/2025 12:44, ionnss via GitGitGadget wrote:
> From: ionnss <zara.leonardo@gmail.com>
>
> 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 <zara.leonardo@gmail.com>
> ---
> 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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] libgit-rs: add get_bool() method to ConfigSet
2025-09-26 9:58 ` Phillip Wood
@ 2025-09-26 17:15 ` Junio C Hamano
0 siblings, 0 replies; 22+ messages in thread
From: Junio C Hamano @ 2025-09-26 17:15 UTC (permalink / raw)
To: Phillip Wood; +Cc: ionnss via GitGitGadget, git, ions, Josh Steadmon
Phillip Wood <phillip.wood123@gmail.com> writes:
> On 25/09/2025 12:44, ionnss via GitGitGadget wrote:
>> From: ionnss <zara.leonardo@gmail.com>
>> 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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 0/3] libgit-rs: add get_bool() method to ConfigSet
2025-09-25 11:44 [PATCH 0/2] libgit-rs: add get_bool() method to ConfigSet ions via GitGitGadget
2025-09-25 11:44 ` [PATCH 1/2] po: fix escaped underscores in README.md ionnss via GitGitGadget
2025-09-25 11:44 ` [PATCH 2/2] libgit-rs: add get_bool() method to ConfigSet ionnss via GitGitGadget
@ 2025-09-27 0:07 ` ions via GitGitGadget
2025-09-27 0:07 ` [PATCH v2 1/3] po: fix escaped underscores in README.md ionnss via GitGitGadget
` (4 more replies)
2 siblings, 5 replies; 22+ messages in thread
From: ions via GitGitGadget @ 2025-09-27 0:07 UTC (permalink / raw)
To: git; +Cc: Chris Torek, Phillip Wood, ions
Purpose
This pull request introduces a get_bool() method to the ConfigSet module in
the libgit-rs library. The goal is to enhance the functionality of ConfigSet
by providing a way to fetch and handle boolean configuration values more
easily and consistently.
Implementation Details
• Added a get_bool() method to the ConfigSet module.
• The method retrieves configuration values as boolean values, ensuring
proper parsing and error handling.
• This addition simplifies the process of working with boolean
configurations for developers using the ConfigSet module.
Testing
• Added unit tests to verify the correctness of the get_bool() method.
• Tested edge cases to ensure robustness.
ionnss (3):
po: fix escaped underscores in README.md
libgit-rs: add get_bool() method to ConfigSet
libgit-rs: address review feedback for get_bool()
contrib/libgit-rs/src/config.rs | 27 +++++++++++++++++++++++++++
contrib/libgit-rs/testdata/config3 | 2 +-
contrib/libgit-rs/testdata/config4 | 10 ++++++++++
contrib/libgit-sys/src/lib.rs | 6 ++++++
po/README.md | 6 +++---
5 files changed, 47 insertions(+), 4 deletions(-)
create mode 100644 contrib/libgit-rs/testdata/config4
base-commit: bb69721404348ea2db0a081c41ab6ebfe75bdec8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1977%2Fionnss%2Fadd-rust-configset-get-bool-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1977/ionnss/add-rust-configset-get-bool-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1977
Range-diff vs v1:
1: d7810781fc = 1: d7810781fc po: fix escaped underscores in README.md
2: a5904a2ac0 = 2: a5904a2ac0 libgit-rs: add get_bool() method to ConfigSet
-: ---------- > 3: 43784e3ff9 libgit-rs: address review feedback for get_bool()
--
gitgitgadget
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 1/3] po: fix escaped underscores in README.md
2025-09-27 0:07 ` [PATCH v2 0/3] " ions via GitGitGadget
@ 2025-09-27 0:07 ` ionnss via GitGitGadget
2025-09-27 0:07 ` [PATCH v2 2/3] libgit-rs: add get_bool() method to ConfigSet ionnss via GitGitGadget
` (3 subsequent siblings)
4 siblings, 0 replies; 22+ messages in thread
From: ionnss via GitGitGadget @ 2025-09-27 0:07 UTC (permalink / raw)
To: git; +Cc: Chris Torek, Phillip Wood, ions, ionnss
From: ionnss <zara.leonardo@gmail.com>
Remove unnecessary backslashes from language code examples.
The underscores in "ll\_CC" and "zh\_CN" don't need escaping
in Markdown.
Signed-off-by: ionnss <zara.leonardo@gmail.com>
---
po/README.md | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/po/README.md b/po/README.md
index ec08aa24ad..d7757bed4e 100644
--- a/po/README.md
+++ b/po/README.md
@@ -13,9 +13,9 @@ We will use XX as an alias to refer to the language translation code in
the following paragraphs, for example we use "po/XX.po" to refer to the
translation file for a specific language. But this doesn't mean that
the language code has only two letters. The language code can be in one
-of two forms: "ll" or "ll\_CC". Here "ll" is the ISO 639 two-letter
+of two forms: "ll" or "ll_CC". Here "ll" is the ISO 639 two-letter
language code and "CC" is the ISO 3166 two-letter code for country names
-and subdivisions. For example: "de" for German language code, "zh\_CN"
+and subdivisions. For example: "de" for German language code, "zh_CN"
for Simplified Chinese language code.
@@ -126,7 +126,7 @@ you add a translation for the first time by running:
make po-init PO_FILE=po/XX.po
```
-where XX is the locale, e.g. "de", "is", "pt\_BR", "zh\_CN", etc.
+where XX is the locale, e.g. "de", "is", "pt_BR", "zh_CN", etc.
The newly generated message file "po/XX.po" is based on the core pot
file "po/git-core.pot", so it contains only a minimal set of messages
--
gitgitgadget
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 2/3] libgit-rs: add get_bool() method to ConfigSet
2025-09-27 0:07 ` [PATCH v2 0/3] " ions via GitGitGadget
2025-09-27 0:07 ` [PATCH v2 1/3] po: fix escaped underscores in README.md ionnss via GitGitGadget
@ 2025-09-27 0:07 ` ionnss via GitGitGadget
2025-09-27 0:07 ` [PATCH v2 3/3] libgit-rs: address review feedback for get_bool() ionnss via GitGitGadget
` (2 subsequent siblings)
4 siblings, 0 replies; 22+ messages in thread
From: ionnss via GitGitGadget @ 2025-09-27 0:07 UTC (permalink / raw)
To: git; +Cc: Chris Torek, Phillip Wood, ions, ionnss
From: ionnss <zara.leonardo@gmail.com>
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 <zara.leonardo@gmail.com>
---
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
--
gitgitgadget
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 3/3] libgit-rs: address review feedback for get_bool()
2025-09-27 0:07 ` [PATCH v2 0/3] " ions via GitGitGadget
2025-09-27 0:07 ` [PATCH v2 1/3] po: fix escaped underscores in README.md ionnss via GitGitGadget
2025-09-27 0:07 ` [PATCH v2 2/3] libgit-rs: add get_bool() method to ConfigSet ionnss via GitGitGadget
@ 2025-09-27 0:07 ` ionnss via GitGitGadget
2025-09-27 2:01 ` [PATCH v2 0/3] libgit-rs: add get_bool() method to ConfigSet Junio C Hamano
2025-09-27 3:51 ` [PATCH v3 " ions via GitGitGadget
4 siblings, 0 replies; 22+ messages in thread
From: ionnss via GitGitGadget @ 2025-09-27 0:07 UTC (permalink / raw)
To: git; +Cc: Chris Torek, Phillip Wood, ions, ionnss
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)
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 <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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2 0/3] libgit-rs: add get_bool() method to ConfigSet
2025-09-27 0:07 ` [PATCH v2 0/3] " ions via GitGitGadget
` (2 preceding siblings ...)
2025-09-27 0:07 ` [PATCH v2 3/3] libgit-rs: address review feedback for get_bool() ionnss via GitGitGadget
@ 2025-09-27 2:01 ` Junio C Hamano
2025-09-27 3:51 ` [PATCH v3 " ions via GitGitGadget
4 siblings, 0 replies; 22+ messages in thread
From: Junio C Hamano @ 2025-09-27 2:01 UTC (permalink / raw)
To: ions via GitGitGadget; +Cc: git, Chris Torek, Phillip Wood, ions
"ions via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Purpose
>
> This pull request introduces a get_bool() method to the ConfigSet module in
> the libgit-rs library. The goal is to enhance the functionality of ConfigSet
> by providing a way to fetch and handle boolean configuration values more
> easily and consistently.
>
> Implementation Details
>
> • Added a get_bool() method to the ConfigSet module.
>
> • The method retrieves configuration values as boolean values, ensuring
> proper parsing and error handling.
>
> • This addition simplifies the process of working with boolean
> configurations for developers using the ConfigSet module.
>
> Testing
>
> • Added unit tests to verify the correctness of the get_bool() method.
>
> • Tested edge cases to ensure robustness.
Somebody may suggest a better way to organize your cover letter for
second and subsequent iterations, but it is helpful to returning
readers to list what changed since the previous iteration.
> ionnss (3):
> po: fix escaped underscores in README.md
> libgit-rs: add get_bool() method to ConfigSet
> libgit-rs: address review feedback for get_bool()
I am not sure if you really wanted to make this a thre-patch series.
The README.md patch looks pretty much independent and irrelevant to
the Rust topic to me. I am not a GGG user, but there must be a way
to tell it to send out a two-patch series from this branch.
> base-commit: bb69721404348ea2db0a081c41ab6ebfe75bdec8
Perhaps you'd need to rebase the top two commit on top of this
bb697214 (The twelfth batch, 2025-09-23). Currently your two
commits that belong to this libgit-rs series are not built on top of
that commit in the mainline. They are built on top of d7810781fc,
as can be seen below.
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1977%2Fionnss%2Fadd-rust-configset-get-bool-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1977/ionnss/add-rust-configset-get-bool-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1977
>
> Range-diff vs v1:
>
> 1: d7810781fc = 1: d7810781fc po: fix escaped underscores in README.md
> 2: a5904a2ac0 = 2: a5904a2ac0 libgit-rs: add get_bool() method to ConfigSet
> -: ---------- > 3: 43784e3ff9 libgit-rs: address review feedback for get_bool()
Actually, I do not think you want a two-patch series, either.
Review comments (sorry, I do not recall the details) must have
pointed out what was wrong in the initial iteration, but this
iteration is carrying the same badness without correcting anyting in
a5904a2ac0 and resending it with "oops that was wrong in the
previous step, and here is a band-aid on top" commit that is the new
43784e3ff9.
Until there is a concensus that your topic is ready and gets merged
to 'next', you have the luxury to pretend as if you are a perfect
developer who never made such mistakes that can easily be pointed
out on the mailing list by others. In short, anything in 43784e3ff9
that removes lines that were added in a5904a2ac0 and replaces them
with something else are unwanted "I went there, it was a bad idea,
here I am correcting the earlier mistake" changes.
And the way to pretend that you are a perfect developer is to
rewrite (i.e. "commit --amend") your "add get_bool()" with the new
strategy to wrap the C version used by real Git, instead of
reimplementing it manually in Rust. This is a chance you have to
pretend that you weren't even tempted to reimplement the thing in
Rust and went straight to the right solution. So grab that chance.
Once your topic gets merged to 'next', you no longer have that
opportunity to wholesale replace your patches to pretend that you
never made mistakes.
This is because nobody reading "git log -p" later is interested in
your (or anybody's) mistakes that have already been corrected. It
might record your cherished memory to celebrate your growth as a
developer, but that is not something the history of a public project
is used for.
After your topic is merged to 'next', we'd go incremental. This is
because being in 'next' means that enough reviewers agreed that a
particular iteration is good enough. If later it was found that
they are not good enough and need corrections, such a mistake that
nobody managed to spot during the review becomes worth recording and
its correction worth describing as a separate patch on top.
But we are not there yet with this series.
Thanks.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 0/3] libgit-rs: add get_bool() method to ConfigSet
2025-09-27 0:07 ` [PATCH v2 0/3] " ions via GitGitGadget
` (3 preceding siblings ...)
2025-09-27 2:01 ` [PATCH v2 0/3] libgit-rs: add get_bool() method to ConfigSet Junio C Hamano
@ 2025-09-27 3:51 ` ions via GitGitGadget
2025-09-27 3:51 ` [PATCH v3 1/3] po: fix escaped underscores in README.md ionnss via GitGitGadget
` (3 more replies)
4 siblings, 4 replies; 22+ messages in thread
From: ions via GitGitGadget @ 2025-09-27 3:51 UTC (permalink / raw)
To: git; +Cc: Chris Torek, Phillip Wood, ions
Purpose
This pull request introduces a get_bool() method to the ConfigSet module in
the libgit-rs library. The goal is to enhance the functionality of ConfigSet
by providing a way to fetch and handle boolean configuration values more
easily and consistently.
Implementation Details
• Added a get_bool() method to the ConfigSet module.
• The method retrieves configuration values as boolean values, ensuring
proper parsing and error handling.
• This addition simplifies the process of working with boolean
configurations for developers using the ConfigSet module.
Testing
• Added unit tests to verify the correctness of the get_bool() method.
• Tested edge cases to ensure robustness.
ionnss (3):
po: fix escaped underscores in README.md
libgit-rs: add get_bool() method to ConfigSet
libgit-rs: add get_ulong() and get_pathname() methods
contrib/libgit-rs/src/config.rs | 83 +++++++++++++++++++++++++++++-
contrib/libgit-rs/testdata/config3 | 2 +-
contrib/libgit-rs/testdata/config4 | 13 +++++
contrib/libgit-sys/src/lib.rs | 24 ++++++++-
po/README.md | 6 +--
5 files changed, 121 insertions(+), 7 deletions(-)
create mode 100644 contrib/libgit-rs/testdata/config4
base-commit: bb69721404348ea2db0a081c41ab6ebfe75bdec8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1977%2Fionnss%2Fadd-rust-configset-get-bool-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1977/ionnss/add-rust-configset-get-bool-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1977
Range-diff vs v2:
1: d7810781fc = 1: d7810781fc po: fix escaped underscores in README.md
3: 43784e3ff9 ! 2: 479c263bc1 libgit-rs: address review feedback for get_bool()
@@ Metadata
Author: ionnss <zara.leonardo@gmail.com>
## Commit message ##
- libgit-rs: address review feedback for get_bool()
+ libgit-rs: add get_bool() method to ConfigSet
- - 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)
+ 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.
- This addresses feedback from Phillip Wood and Chris Torek about using
- Git's actual boolean parsing logic rather than duplicating it in Rust.
+ 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.
Signed-off-by: ionnss <zara.leonardo@gmail.com>
## contrib/libgit-rs/src/config.rs ##
@@ contrib/libgit-rs/src/config.rs: 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("Couldn't convert key to CString");
-- let mut val: *mut c_char = std::ptr::null_mut();
++ 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_string(self.0, key.as_ptr(), &mut val as *mut *mut c_char) != 0
-- {
++ unsafe {
+ 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,
-- }
- }
++ return None;
++ }
++ }
+
+ Some(val != 0)
- }
++ }
}
+ impl Default for ConfigSet {
@@ contrib/libgit-rs/src/config.rs: mod tests {
Path::new("testdata/config1"),
Path::new("testdata/config2"),
@@ contrib/libgit-rs/src/config.rs: 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));
@@ contrib/libgit-rs/src/config.rs: mod tests {
+ 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);
++ // Test missing boolean key
++ assert_eq!(cs.get_bool("missing.boolean"), None);
}
+ }
## contrib/libgit-rs/testdata/config3 ##
@@
[trace2]
- eventNesting = 3
--[test]
-- booleanValue = true
+ eventNesting = 3
\ No newline at end of file
@@ contrib/libgit-rs/testdata/config4 (new)
+ boolZero = 0
+ boolZeroZero = 00
+ boolHundred = 100
-+ boolSeven = 007
## contrib/libgit-sys/src/lib.rs ##
@@ contrib/libgit-sys/src/lib.rs: extern "C" {
2: a5904a2ac0 ! 3: 1ac8d76819 libgit-rs: add get_bool() method to ConfigSet
@@ Metadata
Author: ionnss <zara.leonardo@gmail.com>
## Commit message ##
- libgit-rs: add get_bool() method to ConfigSet
+ libgit-rs: add get_ulong() and get_pathname() methods
- 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.
+ Expand the ConfigSet API with additional configuration value types:
- The implementation reuses the existing get_string() infrastructure
- and adds case-insensitive boolean parsing logic.
+ - get_ulong(): Parse unsigned long integers for large numeric values
+ - get_pathname(): Parse file paths, returning PathBuf for type safety
+
+ 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.
Signed-off-by: ionnss <zara.leonardo@gmail.com>
## contrib/libgit-rs/src/config.rs ##
+@@
+ 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)]
+@@ contrib/libgit-rs/src/config.rs: 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`.
@@ contrib/libgit-rs/src/config.rs: impl ConfigSet {
- Some(owned_str)
- }
+
+ Some(val != 0)
}
+
-+ pub fn get_bool(&mut self, key: &str) -> Option<bool> {
-+ let key = CString::new(key).expect("Couldn't convert key to CString");
++ /// 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_string(self.0, key.as_ptr(), &mut val as *mut *mut c_char) != 0
++ 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("Couldn't convert val to str"));
++ 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
-+ match owned_str.to_lowercase().as_str() {
-+ "true" | "yes" | "on" | "1" => Some(true),
-+ "false" | "no" | "off" | "0" => Some(false),
-+ _ => None,
-+ }
++ Some(PathBuf::from(owned_str))
+ }
+ }
}
impl Default for ConfigSet {
@@ contrib/libgit-rs/src/config.rs: 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);
+ 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);
}
}
- ## contrib/libgit-rs/testdata/config3 ##
+ ## contrib/libgit-rs/testdata/config4 ##
+@@
+ boolZero = 0
+ boolZeroZero = 00
+ boolHundred = 100
++ ulongSmall = 42
++ ulongBig = 4294967296
++ pathRelative = ./some/path
++ pathAbsolute = /usr/bin/git
+
+ ## contrib/libgit-sys/src/lib.rs ##
@@
- [trace2]
- eventNesting = 3
-+[test]
-+ booleanValue = true
+ use std::ffi::c_void;
+
+ #[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)]
+@@ contrib/libgit-sys/src/lib.rs: pub type c_char = i8;
+ #[allow(non_camel_case_types)]
+ pub type c_int = i32;
+
++#[cfg(not(has_std__ffi__c_char))]
++#[allow(non_camel_case_types)]
++pub type c_ulong = u64;
++
+ extern crate libz_sys;
+
+ #[allow(non_camel_case_types)]
+@@ contrib/libgit-sys/src/lib.rs: extern "C" {
+ dest: *mut c_int,
+ ) -> c_int;
+
++ 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;
++
+ }
+
+ #[cfg(test)]
--
gitgitgadget
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 1/3] po: fix escaped underscores in README.md
2025-09-27 3:51 ` [PATCH v3 " ions via GitGitGadget
@ 2025-09-27 3:51 ` ionnss via GitGitGadget
2025-09-29 13:26 ` Phillip Wood
2025-09-27 3:51 ` [PATCH v3 2/3] libgit-rs: add get_bool() method to ConfigSet ionnss via GitGitGadget
` (2 subsequent siblings)
3 siblings, 1 reply; 22+ messages in thread
From: ionnss via GitGitGadget @ 2025-09-27 3:51 UTC (permalink / raw)
To: git; +Cc: Chris Torek, Phillip Wood, ions, ionnss
From: ionnss <zara.leonardo@gmail.com>
Remove unnecessary backslashes from language code examples.
The underscores in "ll\_CC" and "zh\_CN" don't need escaping
in Markdown.
Signed-off-by: ionnss <zara.leonardo@gmail.com>
---
po/README.md | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/po/README.md b/po/README.md
index ec08aa24ad..d7757bed4e 100644
--- a/po/README.md
+++ b/po/README.md
@@ -13,9 +13,9 @@ We will use XX as an alias to refer to the language translation code in
the following paragraphs, for example we use "po/XX.po" to refer to the
translation file for a specific language. But this doesn't mean that
the language code has only two letters. The language code can be in one
-of two forms: "ll" or "ll\_CC". Here "ll" is the ISO 639 two-letter
+of two forms: "ll" or "ll_CC". Here "ll" is the ISO 639 two-letter
language code and "CC" is the ISO 3166 two-letter code for country names
-and subdivisions. For example: "de" for German language code, "zh\_CN"
+and subdivisions. For example: "de" for German language code, "zh_CN"
for Simplified Chinese language code.
@@ -126,7 +126,7 @@ you add a translation for the first time by running:
make po-init PO_FILE=po/XX.po
```
-where XX is the locale, e.g. "de", "is", "pt\_BR", "zh\_CN", etc.
+where XX is the locale, e.g. "de", "is", "pt_BR", "zh_CN", etc.
The newly generated message file "po/XX.po" is based on the core pot
file "po/git-core.pot", so it contains only a minimal set of messages
--
gitgitgadget
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 2/3] libgit-rs: add get_bool() method to ConfigSet
2025-09-27 3:51 ` [PATCH v3 " ions via GitGitGadget
2025-09-27 3:51 ` [PATCH v3 1/3] po: fix escaped underscores in README.md ionnss via GitGitGadget
@ 2025-09-27 3:51 ` ionnss via GitGitGadget
2025-09-29 13:23 ` Phillip Wood
2025-09-27 3:51 ` [PATCH v3 3/3] libgit-rs: add get_ulong() and get_pathname() methods ionnss via GitGitGadget
2025-09-30 8:46 ` [PATCH v4] libgit-rs: add get_bool(), get_ulong(), " ions via GitGitGadget
3 siblings, 1 reply; 22+ messages in thread
From: ionnss via GitGitGadget @ 2025-09-27 3:51 UTC (permalink / raw)
To: git; +Cc: Chris Torek, Phillip Wood, ions, ionnss
From: ionnss <zara.leonardo@gmail.com>
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.
Signed-off-by: ionnss <zara.leonardo@gmail.com>
---
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)]
--
gitgitgadget
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 3/3] libgit-rs: add get_ulong() and get_pathname() methods
2025-09-27 3:51 ` [PATCH v3 " ions via GitGitGadget
2025-09-27 3:51 ` [PATCH v3 1/3] po: fix escaped underscores in README.md ionnss via GitGitGadget
2025-09-27 3:51 ` [PATCH v3 2/3] libgit-rs: add get_bool() method to ConfigSet ionnss via GitGitGadget
@ 2025-09-27 3:51 ` ionnss via GitGitGadget
2025-09-29 13:23 ` Phillip Wood
2025-09-30 8:46 ` [PATCH v4] libgit-rs: add get_bool(), get_ulong(), " ions via GitGitGadget
3 siblings, 1 reply; 22+ messages in thread
From: ionnss via GitGitGadget @ 2025-09-27 3:51 UTC (permalink / raw)
To: git; +Cc: Chris Torek, Phillip Wood, ions, ionnss
From: ionnss <zara.leonardo@gmail.com>
Expand the ConfigSet API with additional configuration value types:
- get_ulong(): Parse unsigned long integers for large numeric values
- get_pathname(): Parse file paths, returning PathBuf for type safety
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.
Signed-off-by: ionnss <zara.leonardo@gmail.com>
---
contrib/libgit-rs/src/config.rs | 57 ++++++++++++++++++++++++++++--
contrib/libgit-rs/testdata/config4 | 4 +++
contrib/libgit-sys/src/lib.rs | 18 +++++++++-
3 files changed, 76 insertions(+), 3 deletions(-)
diff --git a/contrib/libgit-rs/src/config.rs b/contrib/libgit-rs/src/config.rs
index 72ee88801b..ffd9f311b6 100644
--- a/contrib/libgit-rs/src/config.rs
+++ b/contrib/libgit-rs/src/config.rs
@@ -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`.
@@ -82,6 +86,41 @@ impl ConfigSet {
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 {
@@ -128,5 +167,19 @@ mod tests {
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);
}
}
diff --git a/contrib/libgit-rs/testdata/config4 b/contrib/libgit-rs/testdata/config4
index c0755a32be..bd621ab480 100644
--- a/contrib/libgit-rs/testdata/config4
+++ b/contrib/libgit-rs/testdata/config4
@@ -7,3 +7,7 @@
boolZero = 0
boolZeroZero = 00
boolHundred = 100
+ ulongSmall = 42
+ ulongBig = 4294967296
+ pathRelative = ./some/path
+ pathAbsolute = /usr/bin/git
diff --git a/contrib/libgit-sys/src/lib.rs b/contrib/libgit-sys/src/lib.rs
index b104fda8f6..07386572ec 100644
--- a/contrib/libgit-sys/src/lib.rs
+++ b/contrib/libgit-sys/src/lib.rs
@@ -1,7 +1,7 @@
use std::ffi::c_void;
#[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)]
@@ -11,6 +11,10 @@ pub type c_char = i8;
#[allow(non_camel_case_types)]
pub type c_int = i32;
+#[cfg(not(has_std__ffi__c_char))]
+#[allow(non_camel_case_types)]
+pub type c_ulong = u64;
+
extern crate libz_sys;
#[allow(non_camel_case_types)]
@@ -49,6 +53,18 @@ extern "C" {
dest: *mut c_int,
) -> c_int;
+ 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;
+
}
#[cfg(test)]
--
gitgitgadget
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] libgit-rs: add get_bool() method to ConfigSet
2025-09-27 3:51 ` [PATCH v3 2/3] libgit-rs: add get_bool() method to ConfigSet ionnss via GitGitGadget
@ 2025-09-29 13:23 ` Phillip Wood
0 siblings, 0 replies; 22+ messages in thread
From: Phillip Wood @ 2025-09-29 13:23 UTC (permalink / raw)
To: ionnss via GitGitGadget, git; +Cc: Chris Torek, ions
On 27/09/2025 04:51, ionnss via GitGitGadget wrote:
> From: ionnss <zara.leonardo@gmail.com>
>
> 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 <zara.leonardo@gmail.com>
> ---
> 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)]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/3] libgit-rs: add get_ulong() and get_pathname() methods
2025-09-27 3:51 ` [PATCH v3 3/3] libgit-rs: add get_ulong() and get_pathname() methods ionnss via GitGitGadget
@ 2025-09-29 13:23 ` Phillip Wood
0 siblings, 0 replies; 22+ messages in thread
From: Phillip Wood @ 2025-09-29 13:23 UTC (permalink / raw)
To: ionnss via GitGitGadget, git; +Cc: Chris Torek, ions
On 27/09/2025 04:51, ionnss via GitGitGadget wrote:
> From: ionnss <zara.leonardo@gmail.com>
>
> 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.
> diff --git a/contrib/libgit-rs/src/config.rs b/contrib/libgit-rs/src/config.rs
> index 72ee88801b..ffd9f311b6 100644
> --- a/contrib/libgit-rs/src/config.rs
> +++ b/contrib/libgit-rs/src/config.rs
> @@ -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;
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.
> +
> use libgit_sys::*;
>
> /// A ConfigSet is an in-memory cache for config-like files such as `.gitmodules` or `.gitconfig`.
> @@ -82,6 +86,41 @@ impl ConfigSet {
>
> 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.
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/3] po: fix escaped underscores in README.md
2025-09-27 3:51 ` [PATCH v3 1/3] po: fix escaped underscores in README.md ionnss via GitGitGadget
@ 2025-09-29 13:26 ` Phillip Wood
0 siblings, 0 replies; 22+ messages in thread
From: Phillip Wood @ 2025-09-29 13:26 UTC (permalink / raw)
To: ionnss via GitGitGadget, git; +Cc: Chris Torek, ions
As Junio has already said, you should drop this patch from this series
as it is unrelated to the others and submit it separately (which I think
you may have done already) Running
git rebase --onto HEAD~3 HEAD~2
should do the trick
Thanks
Phillip
On 27/09/2025 04:51, ionnss via GitGitGadget wrote:
> From: ionnss <zara.leonardo@gmail.com>
>
> Remove unnecessary backslashes from language code examples.
> The underscores in "ll\_CC" and "zh\_CN" don't need escaping
> in Markdown.
>
> Signed-off-by: ionnss <zara.leonardo@gmail.com>
> ---
> po/README.md | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/po/README.md b/po/README.md
> index ec08aa24ad..d7757bed4e 100644
> --- a/po/README.md
> +++ b/po/README.md
> @@ -13,9 +13,9 @@ We will use XX as an alias to refer to the language translation code in
> the following paragraphs, for example we use "po/XX.po" to refer to the
> translation file for a specific language. But this doesn't mean that
> the language code has only two letters. The language code can be in one
> -of two forms: "ll" or "ll\_CC". Here "ll" is the ISO 639 two-letter
> +of two forms: "ll" or "ll_CC". Here "ll" is the ISO 639 two-letter
> language code and "CC" is the ISO 3166 two-letter code for country names
> -and subdivisions. For example: "de" for German language code, "zh\_CN"
> +and subdivisions. For example: "de" for German language code, "zh_CN"
> for Simplified Chinese language code.
>
>
> @@ -126,7 +126,7 @@ you add a translation for the first time by running:
> make po-init PO_FILE=po/XX.po
> ```
>
> -where XX is the locale, e.g. "de", "is", "pt\_BR", "zh\_CN", etc.
> +where XX is the locale, e.g. "de", "is", "pt_BR", "zh_CN", etc.
>
> The newly generated message file "po/XX.po" is based on the core pot
> file "po/git-core.pot", so it contains only a minimal set of messages
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v4] libgit-rs: add get_bool(), get_ulong(), and get_pathname() methods
2025-09-27 3:51 ` [PATCH v3 " ions via GitGitGadget
` (2 preceding siblings ...)
2025-09-27 3:51 ` [PATCH v3 3/3] libgit-rs: add get_ulong() and get_pathname() methods ionnss via GitGitGadget
@ 2025-09-30 8:46 ` ions via GitGitGadget
2025-10-01 10:15 ` Phillip Wood
3 siblings, 1 reply; 22+ messages in thread
From: ions via GitGitGadget @ 2025-09-30 8:46 UTC (permalink / raw)
To: git; +Cc: ions, ionnss
From: ionnss <zara.leonardo@gmail.com>
Expand ConfigSet API with three new configuration value parsers:
- get_bool(): Parse boolean values using git_configset_get_bool()
- get_ulong(): Parse unsigned long values
- get_pathname(): Parse file paths, returning PathBuf
All methods use Git's C functions for consistent behavior and
include wrapper functions in public_symbol_export.[ch] for FFI.
Includes comprehensive tests for all new functionality.
Signed-off-by: ionnss <zara.leonardo@gmail.com>
---
libgit-rs: add get_bool(), get_ulong(), and get_pathname() methods to
ConfigSet
libgit-rs: Enhanced ConfigSet API
=================================
This series expands the ConfigSet API with support for additional
configuration value types, following Git's established patterns by
wrapping native C functions.
Implementation
==============
Adds three new ConfigSet methods:
* get_bool(): Parse boolean values using git_configset_get_bool()
* get_ulong(): Parse unsigned long integers using
git_configset_get_ulong()
* get_pathname(): Parse file paths using git_configset_get_pathname(),
returning PathBuf
All methods follow the existing pattern of get_int() and get_string(),
ensuring consistent behavior with Git's native parsing logic.
Changes in v4
=============
* Fixed missing wrapper functions in public_symbol_export.[ch]
(Philip's feedback)
* Rebased to correct base commit bb69721404 (Junio's feedback)
* Squashed into single clean commit as requested
* Removed unrelated README.md commit
* Fixed commit message formatting per GitGitGadget requirements
The ConfigSet API now supports 5 configuration value types: int, string,
bool, ulong, and pathname.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1977%2Fionnss%2Fadd-rust-configset-get-bool-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1977/ionnss/add-rust-configset-get-bool-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1977
Range-diff vs v3:
1: d7810781fc < -: ---------- po: fix escaped underscores in README.md
2: 479c263bc1 < -: ---------- libgit-rs: add get_bool() method to ConfigSet
3: 1ac8d76819 ! 1: 618deca808 libgit-rs: add get_ulong() and get_pathname() methods
@@ Metadata
Author: ionnss <zara.leonardo@gmail.com>
## Commit message ##
- libgit-rs: add get_ulong() and get_pathname() methods
+ libgit-rs: add get_bool(), get_ulong(), and get_pathname() methods
- Expand the ConfigSet API with additional configuration value types:
+ Expand ConfigSet API with three new configuration value parsers:
- - get_ulong(): Parse unsigned long integers for large numeric values
- - get_pathname(): Parse file paths, returning PathBuf for type safety
+ - get_bool(): Parse boolean values using git_configset_get_bool()
+ - get_ulong(): Parse unsigned long values
+ - get_pathname(): Parse file paths, returning PathBuf
- Both functions follow the same pattern as existing get_* methods,
- using Git's C functions for consistent parsing behavior.
+ All methods use Git's C functions for consistent behavior and
+ include wrapper functions in public_symbol_export.[ch] for FFI.
- Add comprehensive tests covering normal cases, edge cases, and
- error handling for all new functionality.
+ Includes comprehensive tests for all new functionality.
Signed-off-by: ionnss <zara.leonardo@gmail.com>
@@ contrib/libgit-rs/src/config.rs: type c_char = i8;
/// A ConfigSet is an in-memory cache for config-like files such as `.gitmodules` or `.gitconfig`.
@@ contrib/libgit-rs/src/config.rs: impl ConfigSet {
-
- Some(val != 0)
+ 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)
++ }
++
+ /// 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> {
@@ contrib/libgit-rs/src/config.rs: impl ConfigSet {
impl Default for ConfigSet {
@@ contrib/libgit-rs/src/config.rs: mod tests {
- assert_eq!(cs.get_bool("test.boolHundred"), Some(true)); // "100" → true
- // Test missing boolean key
- assert_eq!(cs.get_bool("missing.boolean"), None);
+ 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));
+@@ contrib/libgit-rs/src/config.rs: 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);
+ // Test ulong parsing
+ assert_eq!(cs.get_ulong("test.ulongSmall"), Some(42));
+ assert_eq!(cs.get_ulong("test.ulongBig"), Some(4294967296)); // > 32-bit int
@@ contrib/libgit-rs/src/config.rs: mod tests {
}
}
- ## contrib/libgit-rs/testdata/config4 ##
+ ## contrib/libgit-rs/testdata/config3 ##
@@
- boolZero = 0
- boolZeroZero = 00
- boolHundred = 100
+ [trace2]
+- eventNesting = 3
++ eventNesting = 3
+ \ No newline at end of file
+
+ ## contrib/libgit-rs/testdata/config4 (new) ##
+@@
++[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
+ ## contrib/libgit-sys/public_symbol_export.c ##
+@@ contrib/libgit-sys/public_symbol_export.c: int libgit_configset_get_string(struct libgit_config_set *cs, const char *key,
+ return git_configset_get_string(&cs->cs, key, dest);
+ }
+
++int libgit_configset_get_bool(struct libgit_config_set *cs, const char *key,
++ int *dest)
++{
++ return git_configset_get_bool(&cs->cs, key, dest);
++}
++
++int libgit_configset_get_ulong(struct libgit_config_set *cs, const char *key,
++ unsigned long *dest)
++{
++ return git_configset_get_ulong(&cs->cs, key, dest);
++}
++
++int libgit_configset_get_pathname(struct libgit_config_set *cs, const char *key,
++ char **dest)
++{
++ return git_configset_get_pathname(&cs->cs, key, dest);
++}
++
+ const char *libgit_user_agent(void)
+ {
+ return git_user_agent();
+
+ ## contrib/libgit-sys/public_symbol_export.h ##
+@@ contrib/libgit-sys/public_symbol_export.h: int libgit_configset_get_int(struct libgit_config_set *cs, const char *key, int
+
+ int libgit_configset_get_string(struct libgit_config_set *cs, const char *key, char **dest);
+
++int libgit_configset_get_bool(struct libgit_config_set *cs, const char *key, int *dest);
++
++int libgit_configset_get_ulong(struct libgit_config_set *cs, const char *key, unsigned long *dest);
++
++int libgit_configset_get_pathname(struct libgit_config_set *cs, const char *key, char **dest);
++
+ const char *libgit_user_agent(void);
+
+ const char *libgit_user_agent_sanitized(void);
+
## contrib/libgit-sys/src/lib.rs ##
@@
use std::ffi::c_void;
@@ contrib/libgit-sys/src/lib.rs: pub type c_char = i8;
#[allow(non_camel_case_types)]
@@ contrib/libgit-sys/src/lib.rs: extern "C" {
- dest: *mut c_int,
+ 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;
++
+ pub fn libgit_configset_get_ulong(
+ cs: *mut libgit_config_set,
+ key: *const c_char,
contrib/libgit-rs/src/config.rs | 83 ++++++++++++++++++++++-
contrib/libgit-rs/testdata/config3 | 2 +-
contrib/libgit-rs/testdata/config4 | 13 ++++
contrib/libgit-sys/public_symbol_export.c | 18 +++++
contrib/libgit-sys/public_symbol_export.h | 6 ++
contrib/libgit-sys/src/lib.rs | 24 ++++++-
6 files changed, 142 insertions(+), 4 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 6bf04845c8..ffd9f311b6 100644
--- a/contrib/libgit-rs/src/config.rs
+++ b/contrib/libgit-rs/src/config.rs
@@ -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)
}
}
+
+ /// 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,6 +148,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 +156,30 @@ 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);
+ // 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);
}
}
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..bd621ab480
--- /dev/null
+++ b/contrib/libgit-rs/testdata/config4
@@ -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
diff --git a/contrib/libgit-sys/public_symbol_export.c b/contrib/libgit-sys/public_symbol_export.c
index dfbb257115..8d6a0e1ba5 100644
--- a/contrib/libgit-sys/public_symbol_export.c
+++ b/contrib/libgit-sys/public_symbol_export.c
@@ -46,6 +46,24 @@ int libgit_configset_get_string(struct libgit_config_set *cs, const char *key,
return git_configset_get_string(&cs->cs, key, dest);
}
+int libgit_configset_get_bool(struct libgit_config_set *cs, const char *key,
+ int *dest)
+{
+ return git_configset_get_bool(&cs->cs, key, dest);
+}
+
+int libgit_configset_get_ulong(struct libgit_config_set *cs, const char *key,
+ unsigned long *dest)
+{
+ return git_configset_get_ulong(&cs->cs, key, dest);
+}
+
+int libgit_configset_get_pathname(struct libgit_config_set *cs, const char *key,
+ char **dest)
+{
+ return git_configset_get_pathname(&cs->cs, key, dest);
+}
+
const char *libgit_user_agent(void)
{
return git_user_agent();
diff --git a/contrib/libgit-sys/public_symbol_export.h b/contrib/libgit-sys/public_symbol_export.h
index 701db92d53..f9adda6561 100644
--- a/contrib/libgit-sys/public_symbol_export.h
+++ b/contrib/libgit-sys/public_symbol_export.h
@@ -11,6 +11,12 @@ int libgit_configset_get_int(struct libgit_config_set *cs, const char *key, int
int libgit_configset_get_string(struct libgit_config_set *cs, const char *key, char **dest);
+int libgit_configset_get_bool(struct libgit_config_set *cs, const char *key, int *dest);
+
+int libgit_configset_get_ulong(struct libgit_config_set *cs, const char *key, unsigned long *dest);
+
+int libgit_configset_get_pathname(struct libgit_config_set *cs, const char *key, char **dest);
+
const char *libgit_user_agent(void);
const char *libgit_user_agent_sanitized(void);
diff --git a/contrib/libgit-sys/src/lib.rs b/contrib/libgit-sys/src/lib.rs
index 4bfc650450..07386572ec 100644
--- a/contrib/libgit-sys/src/lib.rs
+++ b/contrib/libgit-sys/src/lib.rs
@@ -1,7 +1,7 @@
use std::ffi::c_void;
#[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)]
@@ -11,6 +11,10 @@ pub type c_char = i8;
#[allow(non_camel_case_types)]
pub type c_int = i32;
+#[cfg(not(has_std__ffi__c_char))]
+#[allow(non_camel_case_types)]
+pub type c_ulong = u64;
+
extern crate libz_sys;
#[allow(non_camel_case_types)]
@@ -43,6 +47,24 @@ 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;
+
+ 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;
+
}
#[cfg(test)]
base-commit: bb69721404348ea2db0a081c41ab6ebfe75bdec8
--
gitgitgadget
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v4] libgit-rs: add get_bool(), get_ulong(), and get_pathname() methods
2025-09-30 8:46 ` [PATCH v4] libgit-rs: add get_bool(), get_ulong(), " ions via GitGitGadget
@ 2025-10-01 10:15 ` Phillip Wood
2025-10-06 21:20 ` brian m. carlson
0 siblings, 1 reply; 22+ messages in thread
From: Phillip Wood @ 2025-10-01 10:15 UTC (permalink / raw)
To: ions via GitGitGadget, git; +Cc: ions, brian m . carlson
[I've cc'd brian to sanity check my suggestion for handling c_ulong in
rust 1.63 which lacks std::ffi::c_ulong]
On 30/09/2025 09:46, ions via GitGitGadget wrote:
> From: ionnss <zara.leonardo@gmail.com>
>
> Expand ConfigSet API with three new configuration value parsers:
>
> - get_bool(): Parse boolean values using git_configset_get_bool()
> - get_ulong(): Parse unsigned long values
> - get_pathname(): Parse file paths, returning PathBuf
I would be nice to explain in the commit message why we require paths to
be utf-8 encoded. I've left one comment below, apart from that I think
this looks good.
> --- a/contrib/libgit-sys/src/lib.rs
> +++ b/contrib/libgit-sys/src/lib.rs
> @@ -1,7 +1,7 @@
> use std::ffi::c_void;
>
> #[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)]
> @@ -11,6 +11,10 @@ pub type c_char = i8;
> #[allow(non_camel_case_types)]
> pub type c_int = i32;
>
> +#[cfg(not(has_std__ffi__c_char))]
> +#[allow(non_camel_case_types)]
> +pub type c_ulong = u64;
As I said before this wont work because C's ulong type is platform
dependent so you cannot assume it 64 bits wide. Looking at the previous
discussion[1] the reason we have these fallback definitions is because
std::ffi::c_int etc were only added in rust 1.64 and we want to support
rust 1.63 as that is the version shipped by Debian oldstable. I think it
would be better to have a separate preparatory patch that changes the
existing fallbacks to
#[cfg(not(has_std__ff__c_char))]
use std::os::raw::{c_char, c_int};
and then this patch can add "c_ulong" to the list.
Thanks
Phillip
[1]
https://lore.kernel.org/git/ZtivGeDZ_MZDEDB_@tapette.crustytoothpaste.net/
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4] libgit-rs: add get_bool(), get_ulong(), and get_pathname() methods
2025-10-01 10:15 ` Phillip Wood
@ 2025-10-06 21:20 ` brian m. carlson
2025-10-08 13:36 ` Phillip Wood
0 siblings, 1 reply; 22+ messages in thread
From: brian m. carlson @ 2025-10-06 21:20 UTC (permalink / raw)
To: phillip.wood; +Cc: ions via GitGitGadget, git, ions
[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]
On 2025-10-01 at 10:15:28, Phillip Wood wrote:
> [I've cc'd brian to sanity check my suggestion for handling c_ulong in rust
> 1.63 which lacks std::ffi::c_ulong]
>
> As I said before this wont work because C's ulong type is platform dependent
> so you cannot assume it 64 bits wide. Looking at the previous discussion[1]
> the reason we have these fallback definitions is because std::ffi::c_int etc
> were only added in rust 1.64 and we want to support rust 1.63 as that is the
> version shipped by Debian oldstable. I think it would be better to have a
> separate preparatory patch that changes the existing fallbacks to
>
> #[cfg(not(has_std__ff__c_char))]
> use std::os::raw::{c_char, c_int};
>
> and then this patch can add "c_ulong" to the list.
It's just fine to use `std::os::raw` in general without needing to use
`std::ffi` conditionally. I'd just default to that until we move away
from Rust 1.63. That's what I do myself.
--
brian m. carlson (they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4] libgit-rs: add get_bool(), get_ulong(), and get_pathname() methods
2025-10-06 21:20 ` brian m. carlson
@ 2025-10-08 13:36 ` Phillip Wood
0 siblings, 0 replies; 22+ messages in thread
From: Phillip Wood @ 2025-10-08 13:36 UTC (permalink / raw)
To: brian m. carlson, phillip.wood, ions via GitGitGadget, git, ions
On 06/10/2025 22:20, brian m. carlson wrote:
> On 2025-10-01 at 10:15:28, Phillip Wood wrote:
>> [I've cc'd brian to sanity check my suggestion for handling c_ulong in rust
>> 1.63 which lacks std::ffi::c_ulong]
>>
>> As I said before this wont work because C's ulong type is platform dependent
>> so you cannot assume it 64 bits wide. Looking at the previous discussion[1]
>> the reason we have these fallback definitions is because std::ffi::c_int etc
>> were only added in rust 1.64 and we want to support rust 1.63 as that is the
>> version shipped by Debian oldstable. I think it would be better to have a
>> separate preparatory patch that changes the existing fallbacks to
>>
>> #[cfg(not(has_std__ff__c_char))]
>> use std::os::raw::{c_char, c_int};
>>
>> and then this patch can add "c_ulong" to the list.
>
> It's just fine to use `std::os::raw` in general without needing to use
> `std::ffi` conditionally. I'd just default to that until we move away
> from Rust 1.63. That's what I do myself.
Thanks brian, that sounds much simpler. Lets add a preparatory commit to
use std::os::raw unconditionally.
Thanks
Phillip
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2025-10-08 13:36 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-25 11:44 [PATCH 0/2] libgit-rs: add get_bool() method to ConfigSet ions via GitGitGadget
2025-09-25 11:44 ` [PATCH 1/2] po: fix escaped underscores in README.md ionnss via GitGitGadget
2025-09-25 11:44 ` [PATCH 2/2] libgit-rs: add get_bool() method to ConfigSet ionnss via GitGitGadget
2025-09-26 6:43 ` Chris Torek
2025-09-26 9:58 ` Phillip Wood
2025-09-26 17:15 ` Junio C Hamano
2025-09-27 0:07 ` [PATCH v2 0/3] " ions via GitGitGadget
2025-09-27 0:07 ` [PATCH v2 1/3] po: fix escaped underscores in README.md ionnss via GitGitGadget
2025-09-27 0:07 ` [PATCH v2 2/3] libgit-rs: add get_bool() method to ConfigSet ionnss via GitGitGadget
2025-09-27 0:07 ` [PATCH v2 3/3] libgit-rs: address review feedback for get_bool() ionnss via GitGitGadget
2025-09-27 2:01 ` [PATCH v2 0/3] libgit-rs: add get_bool() method to ConfigSet Junio C Hamano
2025-09-27 3:51 ` [PATCH v3 " ions via GitGitGadget
2025-09-27 3:51 ` [PATCH v3 1/3] po: fix escaped underscores in README.md ionnss via GitGitGadget
2025-09-29 13:26 ` Phillip Wood
2025-09-27 3:51 ` [PATCH v3 2/3] libgit-rs: add get_bool() method to ConfigSet ionnss via GitGitGadget
2025-09-29 13:23 ` Phillip Wood
2025-09-27 3:51 ` [PATCH v3 3/3] libgit-rs: add get_ulong() and get_pathname() methods ionnss via GitGitGadget
2025-09-29 13:23 ` Phillip Wood
2025-09-30 8:46 ` [PATCH v4] libgit-rs: add get_bool(), get_ulong(), " ions via GitGitGadget
2025-10-01 10:15 ` Phillip Wood
2025-10-06 21:20 ` brian m. carlson
2025-10-08 13:36 ` Phillip Wood
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).