From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 12 Aug 2024 09:10:48 GMT
Subject: Re: [PATCH v2 4/5] config: add git_configset_alloc() and git_configset_clear_and_free()
Message-ID: <47b18fa4-f01b-4f42-8d04-9e145515ccc1@gmail.com>
In-Reply-To: <908ad0b82fa084fc4e56d7f6dff49e4f255af6ec.1723242556.git.steadmon@google.com>

Hi Josh

On 09/08/2024 23:41, Josh Steadmon wrote:
> Add git_configset_alloc() and git_configset_clear_and_free() functions
> 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.

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);
}

Best Wishes

Phillip

> Co-authored-by: Calvin Wan <calvinwan@google.com>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>   config.c | 11 +++++++++++
>   config.h | 10 ++++++++++
>   2 files changed, 21 insertions(+)
> 
> diff --git a/config.c b/config.c
> index 6421894614..444db8de79 100644
> --- a/config.c
> +++ b/config.c
> @@ -2324,6 +2324,11 @@ static int config_set_element_cmp(const void *cmp_data UNUSED,
>   	return strcmp(e1->key, e2->key);
>   }
>   
> +struct config_set *git_configset_alloc(void)
> +{
> +	return xmalloc(sizeof(struct config_set));
> +}
> +
>   void git_configset_init(struct config_set *set)
>   {
>   	hashmap_init(&set->config_hash, config_set_element_cmp, NULL, 0);
> @@ -2353,6 +2358,12 @@ void git_configset_clear(struct config_set *set)
>   	set->list.items = NULL;
>   }
>   
> +void git_configset_clear_and_free(struct config_set *set)
> +{
> +	git_configset_clear(set);
> +	free(set);
> +}
> +
>   static int config_set_callback(const char *key, const char *value,
>   			       const struct config_context *ctx,
>   			       void *cb)
> diff --git a/config.h b/config.h
> index 54b47dec9e..074c85a788 100644
> --- a/config.h
> +++ b/config.h
> @@ -472,6 +472,11 @@ struct config_set {
>   	struct configset_list list;
>   };
>   
> +/**
> + * Alloc a config_set
> + */
> +struct config_set *git_configset_alloc(void);
> +
>   /**
>    * Initializes the config_set `cs`.
>    */
> @@ -520,6 +525,11 @@ int git_configset_get_string_multi(struct config_set *cs, const char *key,
>    */
>   void git_configset_clear(struct config_set *cs);
>   
> +/**
> + * Clears and frees a heap-allocated `config_set` structure.
> + */
> +void git_configset_clear_and_free(struct config_set *cs);
> +
>   /*
>    * These functions return 1 if not found, and 0 if found, leaving the found
>    * value in the 'dest' pointer.