ref-filter: avoid extra copies of payload/signature

When we know we're going to show the subject or body of a tag or commit,
we call find_subpos(), which returns pointers and lengths for the three
parts: subject, body, signature.

Oddly, the function finds the signature twice: once by calling
parse_signature() at the start, which copies the signature into a
separate strbuf, and then again by calling parse_signed_buffer() after
we've parsed past the subject.

This is due to 482c119186 (gpg-interface: improve interface for parsing
tags, 2021-02-11) and 88bce0e24c (ref-filter: hoist signature parsing,
2021-02-11). The idea is that in a multi-hash world, tag signatures may
appear in the header, rather than at the end of the body, in which case
we need to extract them into a separate buffer.

But parse_signature() would never find such a buffer! It only looks for
signature lines (like "-----BEGIN PGP") at the start of each line,
without any header keyword. So this code will never find anything except
the usual in-body signature.

And the extra code has two downsides:

  1. We spend time copying the payload and signature into strbufs. That
     might even be useful if we ended up with a NUL-terminated copy of
     the payload data, but we throw it away immediately. And the
     signature, since it comes at the end of the message, is already its
     own NUL-terminated buffer.

     The overhead isn't huge, but I measured a pretty consistent 1-2%
     speedup running "git for-each-ref --format='%(subject)'" with this
     patch on a clone of linux.git.

  2. The output of find_subpos() is a set of three ptr/len combinations,
     but only two of them point into the original buffer. This makes the
     interface confusing: you can't do pointer comparisons between them,
     and you have to remember to free the signature buffer. Since
     there's only one caller, it's not too bad in practice, but it did
     bite me while working on the next patch (and simplifying it will
     pave the way for that).

In the long run we might have to go back to something like this
approach, if we do have multi-hash header signatures. But I would argue
that the extra buffer should kick in only for a header signature, and be
passed out of find_subpos() separately.

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 b6c6c101276b63f7f144984888aada0524a3af94..0f5513ba7e0e65279f3dd6d5d3b3429ef8d2bc76 100644 (file)
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1833,16 +1833,10 @@ static void find_subpos(const char *buf,
                        size_t *nonsiglen,
                        const char **sig, size_t *siglen)
 {
-       struct strbuf payload = STRBUF_INIT;
-       struct strbuf signature = STRBUF_INIT;
        const char *eol;
        const char *end = buf + strlen(buf);
        const char *sigstart;
 
-       /* parse signature first; we might not even have a subject line */
-       parse_signature(buf, end - buf, &payload, &signature);
-       strbuf_release(&payload);
-
        /* skip past header until we hit empty line */
        while (*buf && *buf != '\n') {
                eol = strchrnul(buf, '\n');
@@ -1853,8 +1847,10 @@ static void find_subpos(const char *buf,
        /* skip any empty lines */
        while (*buf == '\n')
                buf++;
-       *sig = strbuf_detach(&signature, siglen);
+       /* parse signature first; we might not even have a subject line */
        sigstart = buf + parse_signed_buffer(buf, strlen(buf));
+       *sig = sigstart;
+       *siglen = end - *sig;
 
        /* subject is first non-empty line */
        *sub = buf;
@@ -2021,7 +2017,6 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
                        v->s = xstrdup(subpos);
 
        }
-       free((void *)sigpos);
 }
 
 /*