header signatures for hash transition?

Commit Message

Jeff King Sept. 8, 2024, 11:36 p.m. UTC

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

Comments

brian m. carlson Sept. 9, 2024, 9:44 a.m. UTC | #1

On 2024-09-08 at 23:36:36, Jeff King wrote:
> 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?

I had not intended to add more functionality here.  I don't recall what
I was doing, but I think we either needed to parse this for some set of
atoms somewhere or I had intended to replace all callers with
`parse_signature` instead of `parse_signed_buffer`.

I remember briefly working on this code and that it was a bear to get
working, but not anything more than that.

> 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.

It was probably that this code was to fix one or more of the
signature-related atoms in the tests, but I can't speak to it more than
that.  I trust that you'll make a suitable change that fixes the issue,
but I'm afraid I can't be more help than that.

Jeff King Sept. 9, 2024, 8:20 p.m. UTC | #2

On Mon, Sep 09, 2024 at 09:44:26AM +0000, brian m. carlson wrote:

> > 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.
> 
> It was probably that this code was to fix one or more of the
> signature-related atoms in the tests, but I can't speak to it more than
> that.  I trust that you'll make a suitable change that fixes the issue,
> but I'm afraid I can't be more help than that.

OK, thanks. I'll proceed with something like the patch that I showed
earlier. It shouldn't change any existing behavior, but I wanted to make
sure I wasn't going to make life harder for you in the future.

-Peff

Patch

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