Jeff KingHi brian,
I'm trying to make sense of one of your commits from a few years ago. I'm looking at ref-filter's find_subpos(), which parses a tag into subject, body, and signature. Originally that was done by finding the pointer location for each within the original buffer.
In 482c119186 (gpg-interface: improve interface for parsing tags, 2021-02-11), you replaced the parsing with parse_signature(), which stuffs the payload and signature into separate strbufs. The rationale there seems to be that in a multi-hash world, we'd store the signatures for non-default hashes in the header, so we have to extra them from there into a separate buffer.
So that makes sense, but...it doesn't look like parse_signature() parses anything out of the headers. It still relies on parse_signed_buffer(), which just loops over each line looking for get_format_by_sig(). And that's just looking for "-----BEGIN" type messages, like we'd see in the actual tag body.
Is this a direction that we were going to go in, but ultimately didn't? Or is it something that just hasn't yet been fully implemented?
As an additional curiosity, this also won't find anything for signed commits, which really are in the header. The "%(signature)" placeholder handles this correctly, because it actually verified the signature. This "subpos" stuff is all about "%(contents:signature)", etc, where you're picking out the individual parts. I'm not sure if it is a bug that "%(contents:signature)" doesn't show anything for a signed commit (it is not part of the body contents in the same way, but AFAICT there is no other way to format the literal signature).
The reason I stumbled upon all of this is that I need to use the subject and buffer from find_subpos(), and the obvious way to do that is to use "subpos" as a pointer, and "sigpos - subpos" as the length. But that doesn't work after 482c119186, since "sigpos" is not pointing into the same buffer anymore.
We still separately find the start of the in-body signature and return a "size_t nonsiglen", though it's a bit awkward (it's counting from the body start, and I am coming from the subject start, but if we assume they're contiguous, it's just a little pointer math). So if this approach is still useful in the long run, I can work around it. But my initial approach (before digging in the history) was to drop the separate buffer, something like the patch below, since it also drops some useless extra copying of the tag contents.
And so I wanted to check with you on what the future plans are here.
-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); } /*
