| Message ID | d67d3648d1bdb7dde5e475f3a8eba834cc0ea891.1738023208.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce libgit-rs, a Rust wrapper around libgit.a | expand |
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)]
On 2025.01.28 15:08, Phillip Wood wrote: > 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; > } Hmm yeah I remember this feedback from (checks Lore) back in V2. I think you're right, we should have gone this way from the beginning. Done in V8. > > +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 Fixed. > This whole series looks pretty good to me > > Best Wishes > > Phillip
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) +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) +{ + 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)]