| Message ID | 5ade145352f44b431c16a2ec29cd87de489e8032.1718130288.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mktree: support more flexible usage | expand |
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
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 --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; }