Re: [PATCH v2 4/5] config: add git_configset_alloc() and git_configset_clear_and_free()

> Hi Josh
> 
> On 12/08/2024 22:39, Josh Steadmon wrote:
> > On 2024.08.12 10:10, Phillip Wood wrote:
> > > Hi Josh
> > > 
> > > Do we really need to add this code to config.c rather than handling it in
> > > the wrapper layer in the next patch?
> > > 
> > > Looking ahead I wonder how useful it is to users of the library to separate
> > > out allocation from initialization. A function that allocates and
> > > initializes a configset would be more convenient and harder to misuse.
> > > Calling release functions *_free() rather than *_clear_and_free() would be
> > > more convenient as well. I also noticed that the data types are not
> > > namespaced when they are exported. So perhaps we could drop this patch and
> > > add the following to the next patch.
> > > 
> > > /*
> > >   * Namespace data types as well as functions and ensure consistent
> > >   * naming of data types and the functions that operate on them.
> > >   */
> > > struct libgit_configset {
> > > 	struct config_set set;
> > > };
> > > 
> > > struct libgit_configset *libgit_configset_new() {
> > > 	struct libgit_configset *set = xmalloc(sizeof(*set));
> > > 
> > > 	git_configset_init(&set->set);
> > > 	return set;
> > > }
> > > 
> > > void libgit_configset_free(struct libgit_configset *set) {
> > > 	git_configset_clear(&set->set);
> > > 	free(set);
> > > }
> > 
> > Hmmm I see your point, but I am also hoping to keep the symbol export
> > shim as small as possible, so that we can try to autogenerate it rather
> > than add entries by hand.
> 
> That's a nice aim - how do you plan to address namespacing data types with
> that approach? The autogenerator would need to know "struct config_set"
> wants to be wrapped as "struct libgit_configset" to ensure we end up with
> consistent naming for the data type and its functions. We'd also still want
> to make sure we end up with an ergonomic api which means one function to
> allocate and initialize each data type, not separate functions for
> allocation and initialization.
> 
> > However, if people feel strongly that we don't
> > want to add helper functions like *_alloc() or *_free() for types that don't
> > already have them upstream, perhaps we can just put them in a separate
> > rust-helpers.c file or something.
> 
> If we're not using them upstream they're just clutter as far as git is
> concerned. Having said that if you get the autogeneration working well so
> the output is usable without a lot of manual tweaking I can see an argument
> for having these functions upstream.
> 
> Best Wishes
> 
> Phillip