On Wed, Aug 21, 2024 at 11:46 AM Calvin Wan <calvinwan@xxxxxxxxxx> wrote:
>
> 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.
C doesn't require structs be forward declared separately. You can
change the name to be anything you want, and as long as the forward
declaration of the function and the function definition agree, you're
fine. If they don't agree, well, let's hope you don't encounter that
(it's the same problem as if you have a forward declaration that's
*not* visible from the definition that disagrees with the definition:
`void some_func();` vs. `int some_func(int arg) { ... }` -- if the
forward declaration wasn't made in the same translation unit that
defines `some_func`, nothing detects this misuse in C).
For this reason, you should only ever use forward declarations that
are provided by "the code" that's being forward declared. i.e. if
you're trying to use a function from foo.c, please forward declare it
in foo.h, and only there. This way, assuming foo.c includes foo.h,
you'll detect mismatches.
[apologies if people got multiple copies of this, I sent with HTML
mode enabled the first time]
>
> > 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.
