Re: [PATCH v2 5/5] cgit: add higher-level cgit crate

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Aug 12, 2024, 09:26 UTC
Message-ID: <e3f7d292-ee51-4784-8c85-14b5223c380f@gmail.com>
In-Reply-To: <514c744ba41cf48c4ffd85640b7ef0c5783cc77d.1723242556.git.steadmon@google.com>

Hi Josh

On 09/08/2024 23:41, Josh Steadmon wrote:

Show 5 quoted lines

> From: Calvin Wan <calvinwan@google.com>
> 
> Wrap `struct config_set` and a few of its associated functions in
> cgit-sys. Also introduce a higher-level "cgit" crate which provides a
> more Rust-friendly interface to config_set structs.

Having an ergonamic interface is a really good idea. As far as the naming goes I think the suggestion of "libgit-rs" is a good one.

Show 7 quoted lines

> diff --git a/contrib/cgit-rs/cgit-sys/public_symbol_export.h b/contrib/cgit-rs/cgit-sys/public_symbol_export.h
> index 64332f30de..882c7932e8 100644
> --- a/contrib/cgit-rs/cgit-sys/public_symbol_export.h
> +++ b/contrib/cgit-rs/cgit-sys/public_symbol_export.h
> @@ -9,6 +9,18 @@ void libgit_init_git(const char **argv);
>   
>   int libgit_parse_maybe_bool(const char *val);

I'm suprised the compiler does not complain that 'struct config_set' is not declared in this header - I was expecting to see

	struct config_set;

before the function declarations. As I said in my comments on the last patch I think we'd be better to namespace our types as well as our functions in this library layer so this can be resued by other language bindings.

 > [...]

> +    pub fn get_str(&mut self, key: &str) -> Option<CString> {

If we're adding an ergonomic api then having return CString isn't ideal. I think the equivalent function in libgit2-rs has variants that return a String which is convinent if the caller is expecting utf8 values or Vec<u8> for non-utf8 values.

Best Wishes

Phillip