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
