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

> Hi Calvin
> 
> On 21/08/2024 19:46, Calvin Wan wrote:
> > Phillip Wood <phillip.wood123@gmail.com> writes:
> >
> > > 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.
> 
> I was suggesting[1] adding
> 
>     struct libgit_configset {
> 	    struct config_set set;
>     };
> 
> to public_symbol_export.c and rewriting the wrappers in that file to use
> this struct e.g.
> 
>     int libgit_configset_get_int(struct libgit_configset *cs,
> 				 const char *key, int *dest)
>     {
> 	    return git_configset_get_int(&cs.set, key, dest);
>     }
> 
> In public_symbol_export.h we'd then have
> 
>     struct libgit_configset;
> 
>     int libgit_configset_get_int(struct libgit_configset *,
> 				 const char *, int *);
> 
> If we want the symbol exports to be useful outside of the rust bindings I
> think we need to namespace our types as well as our functions.
> 
> [1]
> https://lore.kernel.org/git/5720d5b9-a850-4024-a1fd-54acc6b15a74@gmail.com
> 
> > > > +    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.
> 
> Just to be clear get_cstr() would return Vec<u8>? I'm far from a rust expert
> but my understanding was that crates that wrap C libraries generally avoid
> using CString in their API.
> 
> Best Wishes
> 
> Phillip