From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 28 Jan 2025 15:08:29 GMT
Subject: Re: [PATCH v7 3/4] libgit-sys: also export some config_set functions
Message-ID: <8ed463cb-1191-4106-bf30-87e9e8bcc95a@gmail.com>
In-Reply-To: <d67d3648d1bdb7dde5e475f3a8eba834cc0ea891.1738023208.git.steadmon@google.com>

Hi Josh

On 28/01/2025 00:19, Josh Steadmon wrote:
> In preparation for implementing a higher-level Rust API for accessing
> Git configs, export some of the upstream configset API via libgitpub and
> libgit-sys. Since this will be exercised as part of the higher-level API
> in the next commit, no tests have been added for libgit-sys.
> 
> While we're at it, add git_configset_alloc() and git_configset_free()
> functions in libgitpub so that callers can manage config_set structs on
> the heap. This also allows non-C external consumers to treat config_sets
> as opaque structs.

This interface is looks nice, I've left a couple of comments below

> diff --git a/contrib/libgit-sys/public_symbol_export.c b/contrib/libgit-sys/public_symbol_export.c
> index cd1602206e..a0297cb1a5 100644
> --- a/contrib/libgit-sys/public_symbol_export.c
> +++ b/contrib/libgit-sys/public_symbol_export.c
> @@ -4,11 +4,40 @@
>    */
>   
>   #include "git-compat-util.h"
> +#include "config.h"
>   #include "contrib/libgit-sys/public_symbol_export.h"
>   #include "version.h"
>   
>   #pragma GCC visibility push(default)

Personally I'd prefer it if we actually defined struct libgit_config_set 
here

struct libgit_config_set {
	struct config_set cs;
}

Then we could avoid all the casts below. For example

struct libgit_config_set *libgit_configset_alloc(void)
{
	struct libget_config_set *cs =
		xmalloc(sizeof(struct libgit_config_set));
	git_configset_init(&cs->cs);
	return cs;
}

> +struct libgit_config_set *libgit_configset_alloc(void)
> +{
> +	struct config_set *cs = xmalloc(sizeof(struct config_set));
> +	git_configset_init(cs);
> +	return (struct libgit_config_set *) cs;
> +}
> +
> +void libgit_configset_free(struct libgit_config_set *cs)
> +{
> +	git_configset_clear((struct config_set *) cs);
> +	free((struct config_set *) cs);
> +}
> +
> +int libgit_configset_add_file(struct libgit_config_set *cs, const char *filename)
> +{
> +	return git_configset_add_file((struct config_set *) cs, filename);
> +}
> +
> +int libgit_configset_get_int(struct libgit_config_set *cs, const char *key, int *dest)

Style: this and the one below could do with being wrapped at 80 characters

This whole series looks pretty good to me

Best Wishes

Phillip

> +{
> +	return git_configset_get_int((struct config_set *) cs, key, dest);
> +}
> +
> +int libgit_configset_get_string(struct libgit_config_set *cs, const char *key, char **dest)
> +{
> +	return git_configset_get_string((struct config_set *) cs, key, dest);
> +}
> +
>   const char *libgit_user_agent(void)
>   {
>   	return git_user_agent();
> diff --git a/contrib/libgit-sys/public_symbol_export.h b/contrib/libgit-sys/public_symbol_export.h
> index a3372f93fa..701db92d53 100644
> --- a/contrib/libgit-sys/public_symbol_export.h
> +++ b/contrib/libgit-sys/public_symbol_export.h
> @@ -1,6 +1,16 @@
>   #ifndef PUBLIC_SYMBOL_EXPORT_H
>   #define PUBLIC_SYMBOL_EXPORT_H
>   
> +struct libgit_config_set *libgit_configset_alloc(void);
> +
> +void libgit_configset_free(struct libgit_config_set *cs);
> +
> +int libgit_configset_add_file(struct libgit_config_set *cs, const char *filename);
> +
> +int libgit_configset_get_int(struct libgit_config_set *cs, const char *key, int *dest);
> +
> +int libgit_configset_get_string(struct libgit_config_set *cs, const char *key, char **dest);
> +
>   const char *libgit_user_agent(void);
>   
>   const char *libgit_user_agent_sanitized(void);
> diff --git a/contrib/libgit-sys/src/lib.rs b/contrib/libgit-sys/src/lib.rs
> index d4853f3074..dadb4e5f40 100644
> --- a/contrib/libgit-sys/src/lib.rs
> +++ b/contrib/libgit-sys/src/lib.rs
> @@ -1,15 +1,44 @@
>   #[cfg(has_std__ffi__c_char)]
> -use std::ffi::c_char;
> +use std::ffi::{c_char, c_int};
>   
>   #[cfg(not(has_std__ffi__c_char))]
>   #[allow(non_camel_case_types)]
>   pub type c_char = i8;
>   
> +#[cfg(not(has_std__ffi__c_char))]
> +#[allow(non_camel_case_types)]
> +pub type c_int = i32;
> +
>   extern crate libz_sys;
>   
> +#[allow(non_camel_case_types)]
> +#[repr(C)]
> +pub struct libgit_config_set {
> +    _data: [u8; 0],
> +    _marker: core::marker::PhantomData<(*mut u8, core::marker::PhantomPinned)>,
> +}
> +
>   extern "C" {
>       pub fn libgit_user_agent() -> *const c_char;
>       pub fn libgit_user_agent_sanitized() -> *const c_char;
> +
> +    pub fn libgit_configset_alloc() -> *mut libgit_config_set;
> +    pub fn libgit_configset_free(cs: *mut libgit_config_set);
> +
> +    pub fn libgit_configset_add_file(cs: *mut libgit_config_set, filename: *const c_char) -> c_int;
> +
> +    pub fn libgit_configset_get_int(
> +        cs: *mut libgit_config_set,
> +        key: *const c_char,
> +        int: *mut c_int,
> +    ) -> c_int;
> +
> +    pub fn libgit_configset_get_string(
> +        cs: *mut libgit_config_set,
> +        key: *const c_char,
> +        dest: *mut *mut c_char,
> +    ) -> c_int;
> +
>   }
>   
>   #[cfg(test)]