Hi 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);
}
/*
