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

From: Calvin Wan <calvinwan@google.com>
Date: Aug 21, 2024, 18:46 UTC
Message-ID: <20240821184605.341205-1-calvinwan@google.com>
In-Reply-To: <e3f7d292-ee51-4784-8c85-14b5223c380f@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 11 quoted lines

> 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.

Agreed -- we plan on renaming it to "libgit-rs" in the next reroll.

Show 13 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;

I'm surprised as well actually. Removing the forward declaration of "struct config_set" in repository.h doesn't result in any complaints from the compiler either. Will add it in the reroll, but am curious if anyone has any ideas why the compiler isn't complaining.

> 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.

Are you suggesting something like "#define libgit_config_set config_set"? I wouldn't be comfortable renaming config_set in git.git just yet until config/config_set can be a standalone library by itself.

Show 8 quoted lines

> 
>  > [...]
> > +    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.

Having both get_cstr() and get_str() makes sense to me.