{"thread":{"id":"44067","subject":"git-am includes escape characters from 'From' field","startedAt":"2016-09-12T20:10:38Z","lastAt":"2016-09-15T07:18:31Z","messageCount":16,"participants":["Swift Geek","Jeff King","Kevin Daudt","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"301756","messageId":"op.yno0zli7ig5ot5@localhost.localdomain","threadId":"44067","inReplyTo":null,"subject":"git-am includes escape characters from 'From' field","fromName":"Swift Geek","fromEmail":"swiftgeek@gmail.com","sentAt":"2016-09-12T20:10:23Z","receivedAt":"2016-09-12T20:10:38Z","isPatch":false,"sender":{"key":"swiftgeek@gmail.com","avatar":null},"body":"git-am seems to add backslash that escapes double quote character, example \ngit format-patch\n\n From 63da989a5295214f9bd06cd7b409a86a65241eea Mon Sep 17 00:00:00 2001\nFrom: \"Sebastian \\\"Swift Geek\\\" Grzywna\" \nDate: Mon, 12 Sep 2016 21:27:32 +0200\nSubject: [PATCH] Example showing git-am bug that includes escape characters\n\n---\n testfile | 1 +\n 1 file changed, 1 insertion(+)\n create mode 100644 testfile\n\ndiff --git a/testfile b/testfile\nnew file mode 100644\nindex 0000000..b48e119\n--- /dev/null\n+++ b/testfile\n@@ -0,0 +1 @@\n+test file contents\n-- \n2.9.3\n\nIn .gitconfig I have\nname = Sebastian \\\"Swift Geek\\\" Grzywna\nwhich appears to work fine with regular git commit - backslashes are not \nincluded in 'From' field.\n\nRegards,\nSwift Geek\n"},{"id":"301847","messageId":"20160913152622.2xtyn6mki6p6afsg@sigill.intra.peff.net","threadId":"44067","inReplyTo":"op.yno0zli7ig5ot5@localhost.localdomain","subject":"Re: git-am includes escape characters from 'From' field","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-13T15:26:23Z","receivedAt":"2016-09-13T15:26:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 12, 2016 at 10:10:23PM +0200, Swift Geek wrote:\n\n> git-am seems to add backslash that escapes double quote character, example\n> git format-patch\n> \n> From 63da989a5295214f9bd06cd7b409a86a65241eea Mon Sep 17 00:00:00 2001\n> From: \"Sebastian \\\"Swift Geek\\\" Grzywna\" \n\nThis looks correct; the output of format-patch is an rfc2822 message,\nand it requires this quoting.\n\nThe part you don't show, and that I think is wrong, is that if you then\n\"git am\" this patch, it pulls the backslashes into the commit object.\nThe culprit looks like \"parse_mail()\" in builtin/am.c (or possibly\nmailinfo() that it calls), which blindly picks up the name portion\nwithout doing any rfc2822 de-quoting.\n\nI don't think we have any existing de-quoting routines to plug in, so\nfixing it would probably start with writing one.\n\n-Peff\n"},{"id":"301892","messageId":"20160913234612.22806-1-me@ikke.info","threadId":"44067","inReplyTo":"20160913152622.2xtyn6mki6p6afsg@sigill.intra.peff.net","subject":"[RFC 0/1] de-quote quoted-strings in mailinfo","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-13T23:46:11Z","receivedAt":"2016-09-13T23:46:25Z","isPatch":false,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"This is my first 'big' C patch, so first an RFC. \n\nThis patch implements RFC2822 dequoting of quoted-pairs in quoted\nstrings, which was not done yet. This means removing the \"\\\" as escape\ncharacter from header fields, but only quoted strings, and comments\n(text between braces).\n\nAccording to the RFC, comments can also appear in square brackets in the\ne-mail domain, but that has not been implemented. In fact, just like\nother functions, it just looks at the whole header line.\n\nPlease let me know what you think.\n\nKevin Daudt (1):\n mailinfo: de-quote quoted-pair in header fields\n\n mailinfo.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++\n t/t5100-mailinfo.sh | 5 +++++\n t/t5100/quoted-pair.expect | 5 +++++\n t/t5100/quoted-pair.in | 9 +++++++++\n t/t5100/quoted-pair.info | 5 +++++\n 5 files changed, 70 insertions(+)\n create mode 100644 t/t5100/quoted-pair.expect\n create mode 100644 t/t5100/quoted-pair.in\n create mode 100644 t/t5100/quoted-pair.info\n\n-- \n2.10.0.rc2\n\n"},{"id":"301893","messageId":"20160913234612.22806-2-me@ikke.info","threadId":"44067","inReplyTo":"20160913234612.22806-1-me@ikke.info","subject":"[RFC 0/1] mailinfo: de-quote quoted-pair in header fields","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-13T23:46:12Z","receivedAt":"2016-09-13T23:46:33Z","isPatch":false,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"rfc2822 has provisions for quoted strings in structured header fields,\nbut also allows for escaping these with so-called quoted-pairs.\n\ngit currently does not do anything with this at all, and verbatim takes\nover the field body.\n\nMake sure to properly dequote these quoted-strings and comments.\n\nSigned-off-by: Kevin Daudt \n---\n mailinfo.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++\n t/t5100-mailinfo.sh | 5 +++++\n t/t5100/quoted-pair.expect | 5 +++++\n t/t5100/quoted-pair.in | 9 +++++++++\n t/t5100/quoted-pair.info | 5 +++++\n 5 files changed, 70 insertions(+)\n create mode 100644 t/t5100/quoted-pair.expect\n create mode 100644 t/t5100/quoted-pair.in\n create mode 100644 t/t5100/quoted-pair.info\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex e19abe3..3b7ae8a 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -445,6 +445,51 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it)\n \t\tmi->input_error = -1;\n }\n \n+static int unescape_quoted_pair(struct mailinfo *mi, struct strbuf *line)\n+{\n+\tstruct strbuf outbuf = STRBUF_INIT;\n+\tconst char *in = line->buf;\n+\tint c, skip=0;\n+\tchar escape_context=0;\n+\n+\twhile ((c = *in++) != 0) {\n+\t\tif (!skip) {\n+\t\t\tswitch (c) {\n+\t\t\t\tcase '\"':\n+\t\t\t\t\tif (!escape_context)\n+\t\t\t\t\t\tescape_context = '\"';\n+\t\t\t\t\telse if (escape_context == '\"')\n+\t\t\t\t\t\tescape_context = 0;\n+\t\t\t\t\tbreak;\n+\t\t\t\tcase '\\\\':\n+\t\t\t\t\tif (escape_context) {\n+\t\t\t\t\t\tskip = 1;\n+\t\t\t\t\t\tcontinue;\n+\t\t\t\t\t}\n+\t\t\t\t\tbreak;\n+\t\t\t\tcase '(':\n+\t\t\t\t\tif (!escape_context)\n+\t\t\t\t\t\tescape_context = '(';\n+\t\t\t\t\tbreak;\n+\t\t\t\tcase ')':\n+\t\t\t\t\tif (escape_context == '(')\n+\t\t\t\t\t\tescape_context = 0;\n+\t\t\t\t\tbreak;\n+\t\t\t}\n+\t\t} else {\n+\t\t\tskip = 0;\n+\t\t}\n+\n+\t\tstrbuf_addch(&outbuf, c);\n+\t}\n+\n+\tstrbuf_reset(line);\n+\tstrbuf_addbuf(line, &outbuf);\n+\n+\treturn 0;\n+\n+}\n+\n static int check_header(struct mailinfo *mi,\n \t\t\tconst struct strbuf *line,\n \t\t\tstruct strbuf *hdr_data[], int overwrite)\n@@ -461,6 +506,7 @@ static int check_header(struct mailinfo *mi,\n \t\t\t */\n \t\t\tstrbuf_add(&sb, line->buf + len + 2, line->len - len - 2);\n \t\t\tdecode_header(mi, &sb);\n+\t\t\tunescape_quoted_pair(mi, &sb);\n \t\t\thandle_header(&hdr_data[i], &sb);\n \t\t\tret = 1;\n \t\t\tgoto check_header_out;\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex 1a5a546..2be61bf 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -142,4 +142,9 @@ test_expect_success 'mailinfo unescapes with --mboxrd' '\n \ttest_cmp expect mboxrd/msg\n '\n \n+test_expect_success 'mailinfo unescapes rfc2822 quoted-pair' '\n+ git mailinfo /dev/null /dev/null <\"$TEST_DIRECTORY\"/t5100/quoted-pair.in >\"$TEST_DIRECTORY\"/t5100/quoted-pair.info &&\n+ test_cmp \"$TEST_DIRECTORY\"/t5100/quoted-pair.expect \"$TEST_DIRECTORY\"/t5100/quoted-pair.info\n+'\n+\n test_done\ndiff --git a/t/t5100/quoted-pair.expect b/t/t5100/quoted-pair.expect\nnew file mode 100644\nindex 0000000..9fe72e9\n--- /dev/null\n+++ b/t/t5100/quoted-pair.expect\n@@ -0,0 +1,5 @@\n+Author: \"Author \"The Author\" Name\"\n+Email: somebody@example.com\n+Subject: testing quoted-pair\n+Date: Sun, 25 May 2008 00:38:18 -0700\n+\ndiff --git a/t/t5100/quoted-pair.in b/t/t5100/quoted-pair.in\nnew file mode 100644\nindex 0000000..e2e627a\n--- /dev/null\n+++ b/t/t5100/quoted-pair.in\n@@ -0,0 +1,9 @@\n+From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001\n+From: \"Author \\\"The Author\\\" Name\" \n+Date: Sun, 25 May 2008 00:38:18 -0700\n+Subject: [PATCH] testing quoted-pair\n+\n+\n+\n+---\n+patch\ndiff --git a/t/t5100/quoted-pair.info b/t/t5100/quoted-pair.info\nnew file mode 100644\nindex 0000000..9fe72e9\n--- /dev/null\n+++ b/t/t5100/quoted-pair.info\n@@ -0,0 +1,5 @@\n+Author: \"Author \"The Author\" Name\"\n+Email: somebody@example.com\n+Subject: testing quoted-pair\n+Date: Sun, 25 May 2008 00:38:18 -0700\n+\n-- \n2.10.0.rc2\n\n"},{"id":"301894","messageId":"xmqqr38ns5wi.fsf@gitster.mtv.corp.google.com","threadId":"44067","inReplyTo":"20160913234612.22806-2-me@ikke.info","subject":"Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-14T00:04:45Z","receivedAt":"2016-09-14T00:04:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Daudt writes:\n\n> +static int unescape_quoted_pair(struct mailinfo *mi, struct strbuf *line)\n> +{\n> +\tstruct strbuf outbuf = STRBUF_INIT;\n> +\tconst char *in = line->buf;\n> +\tint c, skip=0;\n> +\tchar escape_context=0;\n\nHave SP around '=', i.e.\n\n\tint c, skip = 0;\n\tchar escape_context = 0;\n\n> +\twhile ((c = *in++) != 0) {\n> +\t\tif (!skip) {\n> +\t\t\tswitch (c) {\n> +\t\t\t\tcase '\"':\n> +...\n> +\t\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t} else {\n> +\t\t\tskip = 0;\n> +\t\t}\n> +\n> +\t\tstrbuf_addch(&outbuf, c);\n> +\t}\n\nIt often is easier to read if smaller of the two are in the if part\nand the larger in else part. Also your switch/case is indented one\nlevel too deep. I.e.\n\n while (...) {\n if (skip) {\n skip = 0;\n } else {\n switch (c) {\n case '\"':\n do this;\n ...\n }\n }\n strbuf_addch(...);\n }\n\nI found the variable name \"skip\" a bit hard to reason about. What\nit does is to signal the next round of the processing that we have\nseen a single-byte quote and it should keep the byte it will get, no\nmatter what its value is. It is \"skipping\" the conditional\nprocessing, but I'd imagine most people would consider it is\n\"keeping the byte\".\n\n> @@ -461,6 +506,7 @@ static int check_header(struct mailinfo *mi,\n> \t\t\t */\n> \t\t\tstrbuf_add(&sb, line->buf + len + 2, line->len - len - 2);\n> \t\t\tdecode_header(mi, &sb);\n> +\t\t\tunescape_quoted_pair(mi, &sb);\n> \t\t\thandle_header(&hdr_data[i], &sb);\n> \t\t\tret = 1;\n> \t\t\tgoto check_header_out;\n\nI wonder why this call is only in here, not on other headers that\nall call decode_header(). For that matter, I wonder if the call (or\nthe logic of the helper function itself) should go at the end of\ndecode_header(). After all, this is different kind of decoding; the\ncurrent one knows how to do b/q encoding but forgot about the more\ntraditional quoting done with backslash, and you are teaching the\ncode that the current decoding it does is insufficient and how to\nhandle the one that the original implementors forgot about.\n\nThanks.\n"},{"id":"301895","messageId":"20160914045841.GA26893@ikke.info","threadId":"44067","inReplyTo":"xmqqr38ns5wi.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-14T04:58:41Z","receivedAt":"2016-09-14T04:58:48Z","isPatch":false,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Tue, Sep 13, 2016 at 05:04:45PM -0700, Junio C Hamano wrote:\n> Kevin Daudt writes:\n> \n> \n> It often is easier to read if smaller of the two are in the if part\n> and the larger in else part. Also your switch/case is indented one\n> level too deep. I.e.\n> \n\nThanks, I've switched the order and fixed indentation.\n\n> \n> I found the variable name \"skip\" a bit hard to reason about. What\n> it does is to signal the next round of the processing that we have\n> seen a single-byte quote and it should keep the byte it will get, no\n> matter what its value is. It is \"skipping\" the conditional\n> processing, but I'd imagine most people would consider it is\n> \"keeping the byte\".\n\nYes, I agree and was trying to find a better name. I have renamed it to\n\"take_next_literally\", which indicates better what it means.\n\n> \n> > @@ -461,6 +506,7 @@ static int check_header(struct mailinfo *mi,\n> > \t\t\t */\n> > \t\t\tstrbuf_add(&sb, line->buf + len + 2, line->len - len - 2);\n> > \t\t\tdecode_header(mi, &sb);\n> > +\t\t\tunescape_quoted_pair(mi, &sb);\n> > \t\t\thandle_header(&hdr_data[i], &sb);\n> > \t\t\tret = 1;\n> > \t\t\tgoto check_header_out;\n> \n> I wonder why this call is only in here, not on other headers that\n> all call decode_header(). For that matter, I wonder if the call (or\n> the logic of the helper function itself) should go at the end of\n> decode_header(). After all, this is different kind of decoding; the\n> current one knows how to do b/q encoding but forgot about the more\n> traditional quoting done with backslash, and you are teaching the\n> code that the current decoding it does is insufficient and how to\n> handle the one that the original implementors forgot about.\n\nMakes sense, it should be applied to all headers (I missed the other\ndecode_header calls).\n\nI will send a new version later.\n"},{"id":"301896","messageId":"20160914050919.qhv2gxzjyj5ydpub@sigill.intra.peff.net","threadId":"44067","inReplyTo":"xmqqr38ns5wi.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-14T05:09:20Z","receivedAt":"2016-09-14T05:09:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 13, 2016 at 05:04:45PM -0700, Junio C Hamano wrote:\n\n> > @@ -461,6 +506,7 @@ static int check_header(struct mailinfo *mi,\n> > \t\t\t */\n> > \t\t\tstrbuf_add(&sb, line->buf + len + 2, line->len - len - 2);\n> > \t\t\tdecode_header(mi, &sb);\n> > +\t\t\tunescape_quoted_pair(mi, &sb);\n> > \t\t\thandle_header(&hdr_data[i], &sb);\n> > \t\t\tret = 1;\n> > \t\t\tgoto check_header_out;\n> \n> I wonder why this call is only in here, not on other headers that\n> all call decode_header(). For that matter, I wonder if the call (or\n> the logic of the helper function itself) should go at the end of\n> decode_header(). After all, this is different kind of decoding; the\n> current one knows how to do b/q encoding but forgot about the more\n> traditional quoting done with backslash, and you are teaching the\n> code that the current decoding it does is insufficient and how to\n> handle the one that the original implementors forgot about.\n\nIt has been a while since I looked at rfc2822, but aren't the quoting\nand syntax rules different for addresses versus other headers? We would\nnot want to dequote a Subject header, I think.\n\n-Peff\n"},{"id":"301897","messageId":"20160914051305.vphknpsikyxi3hg3@sigill.intra.peff.net","threadId":"44067","inReplyTo":"20160913234612.22806-2-me@ikke.info","subject":"Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-14T05:13:06Z","receivedAt":"2016-09-14T05:13:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 14, 2016 at 01:46:12AM +0200, Kevin Daudt wrote:\n\n> diff --git a/t/t5100/quoted-pair.expect b/t/t5100/quoted-pair.expect\n> new file mode 100644\n> index 0000000..9fe72e9\n> --- /dev/null\n> +++ b/t/t5100/quoted-pair.expect\n> @@ -0,0 +1,5 @@\n> +Author: \"Author \"The Author\" Name\"\n> +Email: somebody@example.com\n> +Subject: testing quoted-pair\n> +Date: Sun, 25 May 2008 00:38:18 -0700\n\nSo obviously this is much better than including the backslashed quotes.\nBut I have to wonder why the first line is not:\n\n Author: Author \"The Author\" Name\n\nWho is responsible for stripping out the other quotes? I know that they\n_do_ get stripped out even in the current code, but it is not clear to\nme if that is intentional or an accident.\n\nIn Git's world-view (e.g., in commit headers), an ident name continues\nuntil we get to the \"<\" of the email (or a \"\\n\" terminates the header\nline completely). So if mailinfo is converting rfc2822 headers into Git\nident, I'd expect it to fully remove any quotes that are not intended to\nbe in the name, and everything after \"Author: \" up to the newline would\nbecome the name.\n\nIt's entirely possible I'm missing something subtle about the design of\nmailinfo, though.\n\n-Peff\n"},{"id":"301898","messageId":"xmqqmvjbrpp4.fsf@gitster.mtv.corp.google.com","threadId":"44067","inReplyTo":"20160914050919.qhv2gxzjyj5ydpub@sigill.intra.peff.net","subject":"Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-14T05:54:47Z","receivedAt":"2016-09-14T05:55:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It has been a while since I looked at rfc2822, but aren't the quoting\n> and syntax rules different for addresses versus other headers? We would\n> not want to dequote a Subject header, I think.\n\nYou're absolutely right. RFC2822 does not quite _want_ to dequote\nanything. As you pointed out in a separate message, we are the one\nwho want to strip out \"\" quoting when mailinfo says\n\n\tAuthor: \"Jeff King\"\n\nto its standard output (aka \"info\"), and turn it into\n\n\tGIT_AUTHOR_NAME='Jeff King'\n\nand do so ONLY for the author name.\n\nSo I would think it is the responsibility of the one that reads the\n\"info\" file that is produced by mailinfo to dequote the backslash\nthing if the mailinfo gave us\n\n\tAuthor: \"Jeff \\\"Peff\\\" King\"\n\n"},{"id":"301910","messageId":"20160914160308.GB26893@ikke.info","threadId":"44067","inReplyTo":"xmqqmvjbrpp4.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-14T16:03:08Z","receivedAt":"2016-09-14T16:03:25Z","isPatch":false,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Tue, Sep 13, 2016 at 10:54:47PM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > It has been a while since I looked at rfc2822, but aren't the quoting\n> > and syntax rules different for addresses versus other headers? We would\n> > not want to dequote a Subject header, I think.\n> \n> You're absolutely right. RFC2822 does not quite _want_ to dequote\n> anything. As you pointed out in a separate message, we are the one\n> who want to strip out \"\" quoting when mailinfo says\n> \n> \tAuthor: \"Jeff King\"\n> \n> to its standard output (aka \"info\"), and turn it into\n> \n> \tGIT_AUTHOR_NAME='Jeff King'\n> \n> and do so ONLY for the author name.\n> \n> So I would think it is the responsibility of the one that reads the\n> \"info\" file that is produced by mailinfo to dequote the backslash\n> thing if the mailinfo gave us\n> \n> \tAuthor: \"Jeff \\\"Peff\\\" King\"\n> \n\nThe RFC makes a distinction between structured fields and unstructured\nfields. Quoting would not even be necessary for unstructured fields\n(like Subject), so yes, that those fields should be left alone.\n\nUnstructures fields are subject, comments, keywords and optional fields,\nthe rest is considered structured.\n\nBecause the only field where this is a problem is the From field, I\nthink it would be safe to limit the unquoting just to that field.\n\nMy reasoning to do the unquoting here is because it's the RFC requires\nthe quoting in the first place.\n\nI already noticed a bug in the current unquoting of the author when\nadding a comment to the From: field.\n\n From: \"A U Thor\" (test)\n\nWhen applied the the author of this patch shows up as:\n\n Author: A U Thor\" (test) \n\nSo I agree with Jeff[1] where he states that the surrounding quotes\nshould be removed, if that's not a problem for git.\n\n[1]:https://public-inbox.org/git/20160914051305.vphknpsikyxi3hg3@sigill.intra.peff.net/\n"},{"id":"301915","messageId":"xmqqoa3qqsw9.fsf@gitster.mtv.corp.google.com","threadId":"44067","inReplyTo":"20160914160308.GB26893@ikke.info","subject":"Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-14T17:43:18Z","receivedAt":"2016-09-14T17:43:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Daudt writes:\n\n> When applied the the author of this patch shows up as:\n>\n> Author: A U Thor\" (test) \n>\n> So I agree with Jeff[1] where he states that the surrounding quotes\n> should be removed, if that's not a problem for git.\n>\n> [1]:https://public-inbox.org/git/20160914051305.vphknpsikyxi3hg3@sigill.intra.peff.net/\n\nI think we can go either way and it does not matter all that much if\n\"mailinfo\" changes its output or the reader of \"mailinfo\" output\nchanges its input--we will either be munging data read from \"From:\"\nwhen producing the \"Author:\" line, or taking the \"Author:\" output by\nmailinfo and removing the quotes.\n\nAs an output from mailinfo that looks like this:\n\n\tAuthor: \"A U Thor\"\n Email: au@thor.com\n\nis made into a commit object that has this:\n\n\tauthor A U Thor \n\nwe know that the reader of mailinfo output _already_ has some logic\nto strip the surrounding double quotes. That is the only reason why\nI think it is a better approach to not dequote in the \"mailinfo\" but\nin the reader to turn\n\n\tAuthor: \"A \\\"U\\\" Thor\"\n Email: au@thor.com\n\ninto a commit object that has this:\n\n\tauthor A \"U\" Thor \n\nthan updating mailinfo to produce\n\n\tAuthor: A \"U\" Thor\n Email: au@thor.com\n\nand then create the same result.\n"},{"id":"301918","messageId":"20160914191759.5unwaq2eequ4pifr@sigill.intra.peff.net","threadId":"44067","inReplyTo":"xmqqoa3qqsw9.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-14T19:17:59Z","receivedAt":"2016-09-14T19:18:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 14, 2016 at 10:43:18AM -0700, Junio C Hamano wrote:\n\n> I think we can go either way and it does not matter all that much if\n> \"mailinfo\" changes its output or the reader of \"mailinfo\" output\n> changes its input--we will either be munging data read from \"From:\"\n> when producing the \"Author:\" line, or taking the \"Author:\" output by\n> mailinfo and removing the quotes.\n\nYeah, that was the part I was wondering about in my original response.\nWhat is the output of mailinfo _supposed_ to be, and do we consider that\nat all public (i.e., are there are other tools besides \"git am\" that\nbuild on mailinfo)?\n\nAt least \"am\" already does some quote-stripping, so any de-quoting added\nin mailinfo is potentially a regression (if we indeed care about keeping\nthe output stable).\n\nBut if we are OK with that, it seems to me that mailinfo is the best\nplace to do the de-quoting, because then its output is well-defined:\neverything after \"Author:\" up to the newline is the name. Whereas if the\ncleanup of the value is split across mailinfo and its reader, then it is\nhard to know which side is responsible for which part. mailinfo handles\nwhitespace unfolding, certainly. What other rfc2822 things does it\nhandle? What are the rules for dequoting its output?\n\nI'll admit I don't care _too_ much. This is a remote corner of the code\nthat I hope never to have to look at. I'm mostly just describing how the\nproblem space makes sense to _me_, and how I would write it if starting\nfrom scratch.\n\n-Peff\n"},{"id":"301920","messageId":"xmqqfup2qny9.fsf@gitster.mtv.corp.google.com","threadId":"44067","inReplyTo":"20160914191759.5unwaq2eequ4pifr@sigill.intra.peff.net","subject":"Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-14T19:30:06Z","receivedAt":"2016-09-14T19:30:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Sep 14, 2016 at 10:43:18AM -0700, Junio C Hamano wrote:\n>\n>> I think we can go either way and it does not matter all that much if\n>> \"mailinfo\" changes its output or the reader of \"mailinfo\" output\n>> changes its input--we will either be munging data read from \"From:\"\n>> when producing the \"Author:\" line, or taking the \"Author:\" output by\n>> mailinfo and removing the quotes.\n>\n> Yeah, that was the part I was wondering about in my original response.\n> What is the output of mailinfo _supposed_ to be, and do we consider that\n> at all public (i.e., are there are other tools besides \"git am\" that\n> build on mailinfo)?\n>\n> At least \"am\" already does some quote-stripping, so any de-quoting added\n> in mailinfo is potentially a regression (if we indeed care about keeping\n> the output stable).\n\nAnother small thing I am not sure about is if the \\ quoting can hide\nan embedded newline in the author name. Would we end up turning\n\n\tFrom: \"Jeff \\\n King\" \n\nor somesuch into\n\n\tAuthor: Jeff\n King\n Email: peff@peff.net\n\n;-)\n\n> But if we are OK with that, it seems to me that mailinfo is the best\n> place to do the de-quoting, because then its output is well-defined:\n> everything after \"Author:\" up to the newline is the name.\n\nThere are other things mailinfo does, like turning this\n\n\tFrom: peff@peff.net (Jeff King)\n\ninto\n\n\tAuthor: Jeff King\n Email: peff@peff.net\n\nand\n\n\tFrom: Uh \"foo\" Bar peff@peff.net (Jeff King)\n\ninto\n\n\tAuthor: Uh \"foo\" Bar (Jeff King)\n Email: peff@peff.net\n\nSo let's roll the \\\" -> \" into mailinfo.\n\nI am not sure if we also should remove the surrounding \"\", i.e. we\ncurrently do not turn this\n\n\tFrom: \"Jeff King\" \n\ninto this:\n\n\tAuthor: Jeff King\n Email: peff@peff.net\n\nI think we probably should, and remove the one that does so from the\nreader.\n\n"},{"id":"301923","messageId":"20160914193819.ua5ubvbf5wz7tvuj@sigill.intra.peff.net","threadId":"44067","inReplyTo":"xmqqfup2qny9.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-14T19:38:20Z","receivedAt":"2016-09-14T19:38:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 14, 2016 at 12:30:06PM -0700, Junio C Hamano wrote:\n\n> Another small thing I am not sure about is if the \\ quoting can hide\n> an embedded newline in the author name. Would we end up turning\n> \n> \tFrom: \"Jeff \\\n> King\" \n> \n> or somesuch into\n> \n> \tAuthor: Jeff\n> King\n> Email: peff@peff.net\n> \n> ;-)\n\nHeh, yeah. That is another reason to clean up and sanitize as much as\npossible before stuffing it into another text format that will be\nparsed.\n\n> So let's roll the \\\" -> \" into mailinfo.\n> \n> I am not sure if we also should remove the surrounding \"\", i.e. we\n> currently do not turn this\n> \n> \tFrom: \"Jeff King\" \n> \n> into this:\n> \n> \tAuthor: Jeff King\n> Email: peff@peff.net\n> \n> I think we probably should, and remove the one that does so from the\n> reader.\n\nI think you have to, or else you cannot tell the difference between\nsurrounding quotes that need to be stripped, and ones that were\nbackslash-escaped. Like:\n\n From: \"Jeff King\" \n From: \\\"Jeff King\\\" \n\nwhich would both become:\n\n Author: \"Jeff King\"\n Email: peff@peff.net\n\nthough I am not sure the latter one is actually valid; you might need to\nbe inside syntactic quotes in order to include backslashed quotes. I\nhaven't read rfc2822 carefully recently enough to know.\n\nAnyway, I think that:\n\n From: One \"Two \\\"Three\\\" Four\" Five\n\nmay also be valid. So the quote-stripping in the reader is not just \"at\nthe outside\", but may need to handle interior syntactic quotes, too. So\nit really makes sense for me to clean and sanitize as much as possible\nin one step, and then make the parser of mailinfo as dumb as possible.\n\n-Peff\n"},{"id":"301953","messageId":"20160915051533.GC26893@ikke.info","threadId":"44067","inReplyTo":"20160914193819.ua5ubvbf5wz7tvuj@sigill.intra.peff.net","subject":"Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-15T05:15:33Z","receivedAt":"2016-09-15T05:15:41Z","isPatch":false,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Wed, Sep 14, 2016 at 12:38:20PM -0700, Jeff King wrote:\n> On Wed, Sep 14, 2016 at 12:30:06PM -0700, Junio C Hamano wrote:\n> \n> > Another small thing I am not sure about is if the \\ quoting can hide\n> > an embedded newline in the author name. Would we end up turning\n> > \n> > \tFrom: \"Jeff \\\n> > King\" \n> > \n> > or somesuch into\n> > \n> > \tAuthor: Jeff\n> > King\n> > Email: peff@peff.net\n> > \n> > ;-)\n> \n> Heh, yeah. That is another reason to clean up and sanitize as much as\n> possible before stuffing it into another text format that will be\n> parsed.\n\nA quoted string cannot contain newlines according to the RFC, so I think\nwe don't need to care about that.\n\n> \n> > So let's roll the \\\" -> \" into mailinfo.\n> > \n> > I am not sure if we also should remove the surrounding \"\", i.e. we\n> > currently do not turn this\n> > \n> > \tFrom: \"Jeff King\" \n> > \n> > into this:\n> > \n> > \tAuthor: Jeff King\n> > Email: peff@peff.net\n> > \n> > I think we probably should, and remove the one that does so from the\n> > reader.\n> \n> I think you have to, or else you cannot tell the difference between\n> surrounding quotes that need to be stripped, and ones that were\n> backslash-escaped. Like:\n> \n> From: \"Jeff King\" \n> From: \\\"Jeff King\\\" \n> \n> which would both become:\n> \n> Author: \"Jeff King\"\n> Email: peff@peff.net\n> \n> though I am not sure the latter one is actually valid; you might need to\n> be inside syntactic quotes in order to include backslashed quotes. I\n> haven't read rfc2822 carefully recently enough to know.\n> \n> Anyway, I think that:\n> \n> From: One \"Two \\\"Three\\\" Four\" Five\n> \n> may also be valid. So the quote-stripping in the reader is not just \"at\n> the outside\", but may need to handle interior syntactic quotes, too. So\n> it really makes sense for me to clean and sanitize as much as possible\n> in one step, and then make the parser of mailinfo as dumb as possible.\n> \n\nMakes sense, the current itteration of my patch already strips exterior\nquotes, no matter where they happen.\n\nI will send a patch soon.\n"},{"id":"301957","messageId":"20160915071822.dpfgg7yldtzar4b2@sigill.intra.peff.net","threadId":"44067","inReplyTo":"20160915051533.GC26893@ikke.info","subject":"Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-15T07:18:23Z","receivedAt":"2016-09-15T07:18:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 15, 2016 at 07:15:33AM +0200, Kevin Daudt wrote:\n\n> > > Another small thing I am not sure about is if the \\ quoting can hide\n> > > an embedded newline in the author name. Would we end up turning\n> > > \n> > > \tFrom: \"Jeff \\\n> > > King\" \n> > > \n> > > or somesuch into\n> > > \n> > > \tAuthor: Jeff\n> > > King\n> > > Email: peff@peff.net\n> > > \n> > > ;-)\n> > \n> > Heh, yeah. That is another reason to clean up and sanitize as much as\n> > possible before stuffing it into another text format that will be\n> > parsed.\n> \n> A quoted string cannot contain newlines according to the RFC, so I think\n> we don't need to care about that.\n\nI wondered how we handled something like:\n\n From: =?UTF-8?q?J=0Aff=20King?= \n\nwhich sticks a newline into the middle of the buffer. We do decode it\nthat way, but eventually call cleanup_space() which converts a run of 1\nor more isspace() characters into a single space (0x20). So you end up\nwith:\n\n Author: J ff King\n\nwhich is probably reasonable.\n\n> Makes sense, the current itteration of my patch already strips exterior\n> quotes, no matter where they happen.\n> \n> I will send a patch soon.\n\nGreat. Thanks for working on this.\n\n-Peff\n"}]}