[PATCH 5/9] ref-filter: store ref_trailer_buf data per-atom
- From
Jeff King <peff@peff.net>
- Date
- Sep 9, 2024, 23:16 UTC
- Message-ID
- <20240909231653.GE921834@coredump.intra.peff.net>
- In-Reply-To
- <20240909230758.GA921697@coredump.intra.peff.net>
The trailer API takes options via a trailer_opts struct. Some of those options point to data structures which require extra storage. Those structures aren't actually embedded in the options struct, but rather we pass pointers, and the caller is responsible for managing them. This is a little convoluted, but makes sense since some of them are not even concrete (e.g., you can pass a filter function and a void data pointer, but the trailer code doesn't even know what's in the pointer).
When for-each-ref, etc, parse the %(trailers) placeholder, they stuff the extra data into a ref_trailer_buf struct. But we only hold a single static global instance of this struct. So if a format string has multiple %(trailer) placeholders, they'll stomp on each other: the "key" list will end up with entries for all of them, and the separator buffers will use the values from whichever was parsed last.
Instead, we should have a ref_trailer_buf for each instance of the placeholder, and store it alongside the trailer_opts in the used_atom structure.
And that's what this patch does. Note that we also have to add code to clean them up in ref_array_clear(). The original code did not bother cleaning them up, but it wasn't technically a "leak" since they were still reachable from the static global instance.
Reported-by: Brooke Kuhlmann <brooke@alchemists.io> Signed-off-by: Jeff King <peff@peff.net> --- ref-filter.c | 36 ++++++++++++++++++++++++++++++------ t/t6300-for-each-ref.sh | 19 +++++++++++++++++++ 2 files changed, 49 insertions(+), 6 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c index 4d0df546da..ebddc041c7 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -75,11 +75,11 @@ struct refname_atom { int lstrip, rstrip; }; -static struct ref_trailer_buf { +struct ref_trailer_buf { struct string_list filter_list; struct strbuf sepbuf; struct strbuf kvsepbuf; -} ref_trailer_buf = {STRING_LIST_INIT_NODUP, STRBUF_INIT, STRBUF_INIT}; +}; static struct expand_data { struct object_id oid; @@ -201,6 +201,7 @@ static struct used_atom { enum { C_BARE, C_BODY, C_BODY_DEP, C_LENGTH, C_LINES, C_SIG, C_SUB, C_SUB_SANITIZE, C_TRAILERS } option; struct process_trailer_options trailer_opts; + struct ref_trailer_buf *trailer_buf; unsigned int nlines; } contents; struct { @@ -568,12 +569,24 @@ static int trailers_atom_parser(struct ref_format *format UNUSED, if (arg) { const char *argbuf = xstrfmt("%s)", arg); char *invalid_arg = NULL; + struct ref_trailer_buf *tb; + + /* + * Do not inline these directly into the used_atom struct! + * When we parse them in format_set_trailers_options(), + * we will make pointer references directly to them, + * which will not survive a realloc() of the used_atom list. + * They must be allocated in a separate, stable struct. + */ + atom->u.contents.trailer_buf = tb = xmalloc(sizeof(*tb)); + string_list_init_nodup(&tb->filter_list); + strbuf_init(&tb->sepbuf, 0); + strbuf_init(&tb->kvsepbuf, 0); if (format_set_trailers_options(&atom->u.contents.trailer_opts, - &ref_trailer_buf.filter_list, - &ref_trailer_buf.sepbuf, - &ref_trailer_buf.kvsepbuf, - &argbuf, &invalid_arg)) { + &tb->filter_list, + &tb->sepbuf, &tb->kvsepbuf, + &argbuf, &invalid_arg)) { if (!invalid_arg) strbuf_addf(err, _("expected %%(trailers:key=<value>)")); else @@ -2988,6 +3001,17 @@ void ref_array_clear(struct ref_array *array) struct used_atom *atom = &used_atom[i]; if (atom->atom_type == ATOM_HEAD) free(atom->u.head); + else if (atom->atom_type == ATOM_TRAILERS || + (atom->atom_type == ATOM_CONTENTS && + atom->u.contents.option == C_TRAILERS)) { + struct ref_trailer_buf *tb = atom->u.contents.trailer_buf; + if (tb) { + string_list_clear(&tb->filter_list, 0); + strbuf_release(&tb->sepbuf); + strbuf_release(&tb->kvsepbuf); + free(tb); + } + } free((char *)atom->name); } FREE_AND_NULL(used_atom); diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index b830b542c4..e8db612f95 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -1560,6 +1560,25 @@ test_trailer_option '%(trailers:separator,key_value_separator) changes both sepa Reviewed-by,A U Thor <author@example.com>,Signed-off-by,A U Thor <author@example.com> EOF +test_expect_success 'multiple %(trailers) use their own options' ' + git tag -F - tag-with-trailers <<-\EOF && + body + + one: foo + one: bar + two: baz + two: qux + EOF + t1="%(trailers:key=one,key_value_separator=W,separator=X)" && + t2="%(trailers:key=two,key_value_separator=Y,separator=Z)" && + git for-each-ref --format="$t1%0a$t2" refs/tags/tag-with-trailers >actual && + cat >expect <<-\EOF && + oneWfooXoneWbar + twoYbazZtwoYqux + EOF + test_cmp expect actual +' + test_failing_trailer_option () { title=$1 option=$2 cat >expect
-- 2.46.0.867.gf99b2b8e0f