From: Chris Torek <chris.torek@gmail.com>
Date: Fri, 26 Sep 2025 06:43:27 GMT
Subject: Re: [PATCH 2/2] libgit-rs: add get_bool() method to ConfigSet
Message-ID: <CAPx1Gvcj-mVfXFMpLuvda_s89e_3jwdjMMdywEOya16yStXvFw@mail.gmail.com>
In-Reply-To: <a5904a2ac00ea6de142344272b45f6b4697b4f98.1758800669.git.gitgitgadget@gmail.com>

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