| Message ID | 20240909231228.GB921834@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 72916999288d7cd40a1f82d7878b31fa15c4fdb2 |
| Headers | show |
| Series | ref-filter %(trailer) fixes | expand |
On Mon, Sep 09, 2024 at 07:12:28PM -0400, Jeff King wrote: > 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. Okay. So in other words the intent was to parse in-header signatures, but the code failed to do so correctly and thus this never worked in the first place? In any case, `parse_signature()` is only a glorified wrapper around `parse_signed_buffer()` in the first place, so in the end they would both parse the buffer in the same way. Nice cleanup, even though it leaves one wondering why the in-header signatures have only been wired up partially. Patrick
On Tue, Sep 10, 2024 at 08:09:22AM +0200, Patrick Steinhardt wrote: > > 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. > > Okay. So in other words the intent was to parse in-header signatures, > but the code failed to do so correctly and thus this never worked in the > first place? > > In any case, `parse_signature()` is only a glorified wrapper around > `parse_signed_buffer()` in the first place, so in the end they would > both parse the buffer in the same way. > > Nice cleanup, even though it leaves one wondering why the in-header > signatures have only been wired up partially. I, too, was confused. See this exchange with brian: https://lore.kernel.org/git/20240908233636.GA4026999@coredump.intra.peff.net/ -Peff
diff --git a/ref-filter.c b/ref-filter.c index b6c6c10127..0f5513ba7e 100644 --- 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); } /*
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> --- This should produce no behavior change. It does seem funny to me that we do this signature parsing for commits as well as tags, even through the former would have an in-header signature. So arguably we are wrongly cutting in-body signatures from commits, and not showing their signatures with %(contents:signature). (But note that %(signature) is its own thing and does handle commits correctly). So I don't know if we'd want to fix that or not, but I left it as-is in this series. And as I said above, if we did want to go that way, I think we'd still want to build it on top of this, so that find_subpos() returns the in-header and in-body signatures separately. ref-filter.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)