diff mbox series

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

Message ID 5ade145352f44b431c16a2ec29cd87de489e8032.1718130288.git.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series mktree: support more flexible usage | expand

Commit Message

Victoria Dye June 11, 2024, 6:24 p.m. UTC
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.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/mktree.c | 67 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 20 deletions(-)

Comments

Eric Sunshine June 11, 2024, 6:45 p.m. UTC | #1
On Tue, Jun 11, 2024 at 2:25 PM Victoria Dye via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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.
>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> diff --git a/builtin/mktree.c b/builtin/mktree.c
> @@ -12,15 +12,39 @@
> +struct tree_entry_array {
> +       size_t nr, alloc;
> +       struct tree_entry **entries;
> +};
>
> +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;
> +}

For robustness, to make it less likely for future code to leak the
items pointed to by `arr->entries`, it might make sense for
release_tree_entry_array() to call clear_tree_entry_array() before
calling FREE_AND_NULL().

> -       ALLOC_GROW(entries, used + 1, alloc);
> -       entries[used++] = ent;
> +       /* Append the update */
> +       tree_entry_array_push(arr, ent);

Nit: the new comment seems superfluous
Patrick Steinhardt June 12, 2024, 9:40 a.m. UTC | #2
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
diff mbox series

Patch

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;
+}
+
+static void append_to_tree(unsigned mode, struct object_id *oid, const char *path,
+			   struct tree_entry_array *arr)
 {
 	struct tree_entry *ent;
 	size_t len = strlen(path);
@@ -32,8 +56,8 @@  static void append_to_tree(unsigned mode, struct object_id *oid, char *path)
 	ent->len = len;
 	oidcpy(&ent->oid, oid);
 
-	ALLOC_GROW(entries, used + 1, alloc);
-	entries[used++] = ent;
+	/* Append the update */
+	tree_entry_array_push(arr, ent);
 }
 
 static int ent_compare(const void *a_, const void *b_)
@@ -44,19 +68,18 @@  static int ent_compare(const void *a_, const void *b_)
 				 b->name, b->len, b->mode);
 }
 
-static void write_tree(struct object_id *oid)
+static void write_tree(struct tree_entry_array *arr, struct object_id *oid)
 {
 	struct strbuf buf;
-	size_t size;
-	int i;
+	size_t size = 0;
 
-	QSORT(entries, used, ent_compare);
-	for (size = i = 0; i < used; i++)
-		size += 32 + entries[i]->len;
+	QSORT(arr->entries, arr->nr, ent_compare);
+	for (size_t i = 0; i < arr->nr; i++)
+		size += 32 + arr->entries[i]->len;
 
 	strbuf_init(&buf, size);
-	for (i = 0; i < used; i++) {
-		struct tree_entry *ent = entries[i];
+	for (size_t i = 0; i < arr->nr; i++) {
+		struct tree_entry *ent = arr->entries[i];
 		strbuf_addf(&buf, "%o %s%c", ent->mode, ent->name, '\0');
 		strbuf_add(&buf, ent->oid.hash, the_hash_algo->rawsz);
 	}
@@ -70,7 +93,8 @@  static const char *mktree_usage[] = {
 	NULL
 };
 
-static void mktree_line(char *buf, int nul_term_line, int allow_missing)
+static void mktree_line(char *buf, int nul_term_line, int allow_missing,
+			struct tree_entry_array *arr)
 {
 	char *ptr, *ntr;
 	const char *p;
@@ -146,7 +170,7 @@  static void mktree_line(char *buf, int nul_term_line, int allow_missing)
 		}
 	}
 
-	append_to_tree(mode, &oid, path);
+	append_to_tree(mode, &oid, path, arr);
 	free(to_free);
 }
 
@@ -158,6 +182,7 @@  int cmd_mktree(int ac, const char **av, const char *prefix)
 	int allow_missing = 0;
 	int is_batch_mode = 0;
 	int got_eof = 0;
+	struct tree_entry_array arr = { 0 };
 	strbuf_getline_fn getline_fn;
 
 	const struct option option[] = {
@@ -182,9 +207,9 @@  int cmd_mktree(int ac, const char **av, const char *prefix)
 					break;
 				die("input format error: (blank line only valid in batch mode)");
 			}
-			mktree_line(sb.buf, nul_term_line, allow_missing);
+			mktree_line(sb.buf, nul_term_line, allow_missing, &arr);
 		}
-		if (is_batch_mode && got_eof && used < 1) {
+		if (is_batch_mode && got_eof && arr.nr < 1) {
 			/*
 			 * Execution gets here if the last tree entry is terminated with a
 			 * new-line.  The final new-line has been made optional to be
@@ -192,12 +217,14 @@  int cmd_mktree(int ac, const char **av, const char *prefix)
 			 */
 			; /* skip creating an empty tree */
 		} else {
-			write_tree(&oid);
+			write_tree(&arr, &oid);
 			puts(oid_to_hex(&oid));
 			fflush(stdout);
 		}
-		used=0; /* reset tree entry buffer for re-use in batch mode */
+		clear_tree_entry_array(&arr); /* reset tree entry buffer for re-use in batch mode */
 	}
+
+	release_tree_entry_array(&arr);
 	strbuf_release(&sb);
 	return 0;
 }