@matheusbsbs / matheus-bagliano / commits / e595b016fc

ref-filter: store ref_trailer_buf data per-atom

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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Sep 9, 2024 at 19:16 UTC e595b016fc4ab20b87b935d29cf689fd956d8588

2 files changed +49 -6

ref-filter.c

+30 -6

@@ -75,11 +75,11 @@ struct refname_atom {

75 int lstrip, rstrip;

76 };

78 -static struct ref_trailer_buf {

78 +struct ref_trailer_buf {

79 struct string_list filter_list;

80 struct strbuf sepbuf;

81 struct strbuf kvsepbuf;

82 -} ref_trailer_buf = {STRING_LIST_INIT_NODUP, STRBUF_INIT, STRBUF_INIT};

82 +};

84 static struct expand_data {

85 struct object_id oid;

@@ -201,6 +201,7 @@ static struct used_atom {

201 enum { C_BARE, C_BODY, C_BODY_DEP, C_LENGTH, C_LINES,

202 C_SIG, C_SUB, C_SUB_SANITIZE, C_TRAILERS } option;

203 struct process_trailer_options trailer_opts;

204 + struct ref_trailer_buf *trailer_buf;

205 unsigned int nlines;

206 } contents;

207 struct {

@@ -568,12 +569,24 @@ static int trailers_atom_parser(struct ref_format *format UNUSED,

569 if (arg) {

570 const char *argbuf = xstrfmt("%s)", arg);

571 char *invalid_arg = NULL;

572 + struct ref_trailer_buf *tb;

573 +

574 + /*

575 + * Do not inline these directly into the used_atom struct!

576 + * When we parse them in format_set_trailers_options(),

577 + * we will make pointer references directly to them,

578 + * which will not survive a realloc() of the used_atom list.

579 + * They must be allocated in a separate, stable struct.

580 + */

581 + atom->u.contents.trailer_buf = tb = xmalloc(sizeof(*tb));

582 + string_list_init_nodup(&tb->filter_list);

583 + strbuf_init(&tb->sepbuf, 0);

584 + strbuf_init(&tb->kvsepbuf, 0);

585

586 if (format_set_trailers_options(&atom->u.contents.trailer_opts,

573 - &ref_trailer_buf.filter_list,

574 - &ref_trailer_buf.sepbuf,

575 - &ref_trailer_buf.kvsepbuf,

576 - &argbuf, &invalid_arg)) {

587 + &tb->filter_list,

588 + &tb->sepbuf, &tb->kvsepbuf,

589 + &argbuf, &invalid_arg)) {

590 if (!invalid_arg)

591 strbuf_addf(err, _("expected %%(trailers:key=<value>)"));

592 else

@@ -2988,6 +3001,17 @@ void ref_array_clear(struct ref_array *array)

3001 struct used_atom *atom = &used_atom[i];

3002 if (atom->atom_type == ATOM_HEAD)

3003 free(atom->u.head);

3004 + else if (atom->atom_type == ATOM_TRAILERS ||

3005 + (atom->atom_type == ATOM_CONTENTS &&

3006 + atom->u.contents.option == C_TRAILERS)) {

3007 + struct ref_trailer_buf *tb = atom->u.contents.trailer_buf;

3008 + if (tb) {

3009 + string_list_clear(&tb->filter_list, 0);

3010 + strbuf_release(&tb->sepbuf);

3011 + strbuf_release(&tb->kvsepbuf);

3012 + free(tb);

3013 + }

3014 + }

3015 free((char *)atom->name);

3016 }

3017 FREE_AND_NULL(used_atom);

t/t6300-for-each-ref.sh

+19

@@ -1560,6 +1560,25 @@ test_trailer_option '%(trailers:separator,key_value_separator) changes both sepa

1560 Reviewed-by,A U Thor <author@example.com>,Signed-off-by,A U Thor <author@example.com>

1561 EOF

1562

1563 +test_expect_success 'multiple %(trailers) use their own options' '

1564 + git tag -F - tag-with-trailers <<-\EOF &&

1565 + body

1566 +

1567 + one: foo

1568 + one: bar

1569 + two: baz

1570 + two: qux

1571 + EOF

1572 + t1="%(trailers:key=one,key_value_separator=W,separator=X)" &&

1573 + t2="%(trailers:key=two,key_value_separator=Y,separator=Z)" &&

1574 + git for-each-ref --format="$t1%0a$t2" refs/tags/tag-with-trailers >actual &&

1575 + cat >expect <<-\EOF &&

1576 + oneWfooXoneWbar

1577 + twoYbazZtwoYqux

1578 + EOF

1579 + test_cmp expect actual

1580 +'

1581 +

1582 test_failing_trailer_option () {

1583 title=$1 option=$2

1584 cat >expect