Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
> Hi Josh
>
> On 09/08/2024 23:41, Josh Steadmon wrote:
> > From: Calvin Wan <calvinwan@xxxxxxxxxx>
> >
> > 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.
>
> > 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.
>
> > [...]
> > + 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.
