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>

diff --git a/ref-filter.c b/ref-filter.c
index 4d0df546dabe452a11e5c1d4b492460a2599a080..ebddc041c745230533a21a52842898c4fcc4870a 100644 (file)
--- 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 b830b542c42b672e393d959171ec1f4c51439d37..e8db612f956adbdfb526e12c63159c874d6e460c 100755 (executable)
--- 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