From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 12 Aug 2024 09:26:29 GMT
Subject: Re: [PATCH v2 5/5] cgit: add higher-level cgit crate
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:
> 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.

> 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