Re: [PATCH 03/16] mktree: use non-static tree_entry array

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2612 bytes --]

On Tue, Jun 11, 2024 at 06:24:35PM +0000, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
> 
> Replace the static 'struct tree_entry **entries' with a non-static 'struct
> tree_entry_array' instance. In later commits, we'll want to be able to
> create additional 'struct tree_entry_array' instances utilizing common
> functionality (create, push, clear, free). To avoid code duplication, create
> the 'struct tree_entry_array' type and add functions that perform those
> basic operations.

Thanks for getting rid of more global state, I really appreciate this.

> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  builtin/mktree.c | 67 +++++++++++++++++++++++++++++++++---------------
>  1 file changed, 47 insertions(+), 20 deletions(-)
> 
> diff --git a/builtin/mktree.c b/builtin/mktree.c
> index c02feb06aff..15bd908702a 100644
> --- a/builtin/mktree.c
> +++ b/builtin/mktree.c
> @@ -12,15 +12,39 @@
>  #include "parse-options.h"
>  #include "object-store-ll.h"
>  
> -static struct tree_entry {
> +struct tree_entry {
>  	unsigned mode;
>  	struct object_id oid;
>  	int len;
>  	char name[FLEX_ARRAY];
> -} **entries;
> -static int alloc, used;
> +};
> +
> +struct tree_entry_array {
> +	size_t nr, alloc;
> +	struct tree_entry **entries;
> +};
>  
> -static void append_to_tree(unsigned mode, struct object_id *oid, char *path)
> +static void tree_entry_array_push(struct tree_entry_array *arr, struct tree_entry *ent)
> +{
> +	ALLOC_GROW(arr->entries, arr->nr + 1, arr->alloc);
> +	arr->entries[arr->nr++] = ent;
> +}
> +
> +static void clear_tree_entry_array(struct tree_entry_array *arr)
> +{
> +	for (size_t i = 0; i < arr->nr; i++)
> +		FREE_AND_NULL(arr->entries[i]);
> +	arr->nr = 0;
> +}
> +
> +static void release_tree_entry_array(struct tree_entry_array *arr)
> +{
> +	FREE_AND_NULL(arr->entries);
> +	arr->nr = arr->alloc = 0;
> +}

Nit: should these be called `tree_entry_array_clear()` and
`tree_entry_array_release()`? This is one of the areas where our coding
guidelines aren't sufficiently clear in my opinion. I personally
strongly prefer `<noun>_<action>` syntax because it groups together
related functionality much better. And while I personally do not use
code completion, it does help others that do because they can simply
type in the noun as prefix and then, via completion, learn about all
related functions.

Most of our general-purpose interfaces follow this naming schema
(strbuf, string_list, strvec, oidset, ...). I think we should document
this accordingly.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]