From: Josh Steadmon <steadmon@google.com>
Date: Wed, 02 Oct 2024 22:31:33 GMT
Subject: Re: [PATCH v2 5/5] cgit: add higher-level cgit crate
Message-ID: <sh6cwo4uzgp5bvlctmdkhwhjum2zc36cpe74ygy725nr3invxe@z6ee2shr4cf3>
In-Reply-To: <9c6d8fea-997e-49d5-9195-e1721a750a36@gmail.com>

On 2024.08.22 10:12, Phillip Wood wrote:
> 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

Yeah let's just go with "get_string()" and return an Option<String>.