libgit-rs: add get_bool(), get_ulong(), and get_pathname() methods to ConfigSet #1977
Conversation
Welcome to GitGitGadgetHi @ionnss, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join [email protected], where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
|
/allow |
|
User ionnss already allowed to use GitGitGadget. |
|
/preview |
|
Error: Ignoring PR with empty title and/or body |
|
/preview |
|
Preview email sent as [email protected] |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| 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 | ||
| Some(owned_str) |
There was a problem hiding this comment.
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
|
User |
| 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 | ||
| Some(owned_str) |
There was a problem hiding this comment.
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 = trueThere was a problem hiding this comment.
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.
|
User |
|
Updated to address review feedback:
This ensures the Rust API always matches Git's actual boolean parsing behavior. Thank you for the feedback and patience. |
|
/preview |
|
Preview email sent as [email protected] |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
Added two more ConfigSet methods based on the same architectural pattern: get_ulong(): Handles large unsigned integers (> 32-bit) using
get_pathname(): Handles file paths using
Both functions follow the established pattern of wrapping Git's C functions rather than reimplementing parsing logic, ensuring consistency with Git's behavior. The ConfigSet API now supports 5 different value types: int, string, bool, ulong, and pathname. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "ions via GitGitGadget" <[email protected]> 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. |
ionnss
force-pushed
the
add-rust-configset-get-bool
branch
from
fc50677 to
1ac8d76
Compare
|
Rebased and cleaned up commit history as requested: Commit 1:
Commit 2:
Separated the unrelated README.md fix as suggested. The series now presents clean, professional commits focused on the final solutions. |
|
/preview |
|
Preview email sent as [email protected] |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
@ionnss I am afraid that you will need to reply on the mailing list as described here: https://gitgitgadget.github.io/reply-to-this If you want to describe changes between iterations instead, please edit the PR description (which will be sent as cover letter). |
ionnss
changed the title
| 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 | ||
| Some(owned_str) |
There was a problem hiding this comment.
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)]
|
User |
| @@ -1,8 +1,8 @@ | |||
| use std::ffi::{c_void, CStr, CString}; | |||
There was a problem hiding this comment.
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]>
> > 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
| https://github.com/git-l10n/git-po/ | ||
|
|
||
| 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 |
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
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 <[email protected]>
> > 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 <[email protected]>
> ---
> 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
ionnss
force-pushed
the
add-rust-configset-get-bool
branch
from
1ac8d76 to
1a9d2e8
Compare
|
There are issues in commit 1a9d2e8: |
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 <[email protected]>
ionnss
force-pushed
the
add-rust-configset-get-bool
branch
from
1a9d2e8 to
618deca
Compare
|
/submit Changes in v4:
This version includes all three ConfigSet methods (get_bool, get_ulong, get_pathname) with proper FFI wrapper functions and comprehensive tests. |
@ionnss that's not how it works. You need to edit the PR description, i.e. the initial comment of the PR, to add that |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Phillip Wood wrote (reply to this): [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 <[email protected]>
> > 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/[email protected]/
|
|
User |
libgit-rs: Adding new wrappers to ConfigSet API
This series expands the Rust 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 to the Rust bindings:
get_bool(): Parse boolean values usinggit_configset_get_bool()get_ulong(): Parse unsigned long integers usinggit_configset_get_ulong()get_pathname(): Parse file paths usinggit_configset_get_pathname(), returning PathBufAll methods follow the existing pattern of
get_int()andget_string(), ensuring consistent behavior with Git's native parsing logic.Changes
The Rust ConfigSet API now supports 5 configuration value types: int, string, bool, ulong, and pathname.
cc: Phillip Wood [email protected]