{"thread":{"id":"62080","subject":"header signatures for hash transition?","startedAt":"2024-09-08T23:36:45Z","lastAt":"2024-09-09T20:21:02Z","messageCount":3,"participants":["Jeff King","brian m. carlson"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"502417","messageId":"20240908233636.GA4026999@coredump.intra.peff.net","threadId":"62080","inReplyTo":null,"subject":"header signatures for hash transition?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-08T23:36:36Z","receivedAt":"2024-09-08T23:36:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Hi brian,\n\nI'm trying to make sense of one of your commits from a few years ago.\nI'm looking at ref-filter's find_subpos(), which parses a tag into\nsubject, body, and signature. Originally that was done by finding the\npointer location for each within the original buffer.\n\nIn 482c119186 (gpg-interface: improve interface for parsing tags,\n2021-02-11), you replaced the parsing with parse_signature(), which\nstuffs the payload and signature into separate strbufs. The rationale\nthere seems to be that in a multi-hash world, we'd store the signatures\nfor non-default hashes in the header, so we have to extra them from\nthere into a separate buffer.\n\nSo that makes sense, but...it doesn't look like parse_signature() parses\nanything out of the headers. It still relies on parse_signed_buffer(),\nwhich just loops over each line looking for get_format_by_sig(). And\nthat's just looking for \"-----BEGIN\" type messages, like we'd see in the\nactual tag body.\n\nIs this a direction that we were going to go in, but ultimately didn't?\nOr is it something that just hasn't yet been fully implemented?\n\nAs an additional curiosity, this also won't find anything for signed\ncommits, which really are in the header. The \"%(signature)\" placeholder\nhandles this correctly, because it actually verified the signature. This\n\"subpos\" stuff is all about \"%(contents:signature)\", etc, where you're\npicking out the individual parts. I'm not sure if it is a bug that\n\"%(contents:signature)\" doesn't show anything for a signed commit (it is\nnot part of the body contents in the same way, but AFAICT there is no\nother way to format the literal signature).\n\n\nThe reason I stumbled upon all of this is that I need to use the subject\nand buffer from find_subpos(), and the obvious way to do that is to use\n\"subpos\" as a pointer, and \"sigpos - subpos\" as the length. But that\ndoesn't work after 482c119186, since \"sigpos\" is not pointing into the\nsame buffer anymore.\n\nWe still separately find the start of the in-body signature and return a\n\"size_t nonsiglen\", though it's a bit awkward (it's counting from the\nbody start, and I am coming from the subject start, but if we assume\nthey're contiguous, it's just a little pointer math). So if this\napproach is still useful in the long run, I can work around it. But my\ninitial approach (before digging in the history) was to drop the\nseparate buffer, something like the patch below, since it also drops\nsome useless extra copying of the tag contents.\n\nAnd so I wanted to check with you on what the future plans are here.\n\n-Peff\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex b6c6c10127..0f5513ba7e 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1833,16 +1833,10 @@ static void find_subpos(const char *buf,\n \t\t\tsize_t *nonsiglen,\n \t\t\tconst char **sig, size_t *siglen)\n {\n-\tstruct strbuf payload = STRBUF_INIT;\n-\tstruct strbuf signature = STRBUF_INIT;\n \tconst char *eol;\n \tconst char *end = buf + strlen(buf);\n \tconst char *sigstart;\n \n-\t/* parse signature first; we might not even have a subject line */\n-\tparse_signature(buf, end - buf, &payload, &signature);\n-\tstrbuf_release(&payload);\n-\n \t/* skip past header until we hit empty line */\n \twhile (*buf && *buf != '\\n') {\n \t\teol = strchrnul(buf, '\\n');\n@@ -1853,8 +1847,10 @@ static void find_subpos(const char *buf,\n \t/* skip any empty lines */\n \twhile (*buf == '\\n')\n \t\tbuf++;\n-\t*sig = strbuf_detach(&signature, siglen);\n+\t/* parse signature first; we might not even have a subject line */\n \tsigstart = buf + parse_signed_buffer(buf, strlen(buf));\n+\t*sig = sigstart;\n+\t*siglen = end - *sig;\n \n \t/* subject is first non-empty line */\n \t*sub = buf;\n@@ -2021,7 +2017,6 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp\n \t\t\tv->s = xstrdup(subpos);\n \n \t}\n-\tfree((void *)sigpos);\n }\n \n /*\n"},{"id":"502429","messageId":"Zt7Deq2vzZKSTbfU@tapette.crustytoothpaste.net","threadId":"62080","inReplyTo":"20240908233636.GA4026999@coredump.intra.peff.net","subject":"Re: header signatures for hash transition?","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-09-09T09:44:26Z","receivedAt":"2024-09-09T09:44:33Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-09-08 at 23:36:36, Jeff King wrote:\n> Is this a direction that we were going to go in, but ultimately didn't?\n> Or is it something that just hasn't yet been fully implemented?\n\nI had not intended to add more functionality here.  I don't recall what\nI was doing, but I think we either needed to parse this for some set of\natoms somewhere or I had intended to replace all callers with\n`parse_signature` instead of `parse_signed_buffer`.\n\nI remember briefly working on this code and that it was a bear to get\nworking, but not anything more than that.\n\n> We still separately find the start of the in-body signature and return a\n> \"size_t nonsiglen\", though it's a bit awkward (it's counting from the\n> body start, and I am coming from the subject start, but if we assume\n> they're contiguous, it's just a little pointer math). So if this\n> approach is still useful in the long run, I can work around it. But my\n> initial approach (before digging in the history) was to drop the\n> separate buffer, something like the patch below, since it also drops\n> some useless extra copying of the tag contents.\n\nIt was probably that this code was to fix one or more of the\nsignature-related atoms in the tests, but I can't speak to it more than\nthat.  I trust that you'll make a suitable change that fixes the issue,\nbut I'm afraid I can't be more help than that.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"502473","messageId":"20240909202058.GA348398@coredump.intra.peff.net","threadId":"62080","inReplyTo":"Zt7Deq2vzZKSTbfU@tapette.crustytoothpaste.net","subject":"Re: header signatures for hash transition?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-09T20:20:58Z","receivedAt":"2024-09-09T20:21:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 09, 2024 at 09:44:26AM +0000, brian m. carlson wrote:\n\n> > We still separately find the start of the in-body signature and return a\n> > \"size_t nonsiglen\", though it's a bit awkward (it's counting from the\n> > body start, and I am coming from the subject start, but if we assume\n> > they're contiguous, it's just a little pointer math). So if this\n> > approach is still useful in the long run, I can work around it. But my\n> > initial approach (before digging in the history) was to drop the\n> > separate buffer, something like the patch below, since it also drops\n> > some useless extra copying of the tag contents.\n> \n> It was probably that this code was to fix one or more of the\n> signature-related atoms in the tests, but I can't speak to it more than\n> that.  I trust that you'll make a suitable change that fixes the issue,\n> but I'm afraid I can't be more help than that.\n\nOK, thanks. I'll proceed with something like the patch that I showed\nearlier. It shouldn't change any existing behavior, but I wanted to make\nsure I wasn't going to make life harder for you in the future.\n\n-Peff\n"}]}