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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Aug 22, 2024, 09:12 UTC
Message-ID: <9c6d8fea-997e-49d5-9195-e1721a750a36@gmail.com>
In-Reply-To: <20240821184605.341205-1-calvinwan@google.com>

Hi Calvin

On 21/08/2024 19:46, Calvin Wan wrote:

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

Show 8 quoted lines

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

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.

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