{"thread":{"id":"44099","subject":"[PATCH] mailinfo: unescape quoted-pair in header fields","startedAt":"2016-09-16T21:02:57Z","lastAt":"2016-09-28T20:27:45Z","messageCount":32,"participants":["Kevin Daudt","Jeff King","Junio C Hamano","Jakub Narębski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"302076","messageId":"20160916210204.31282-1-me@ikke.info","threadId":"44099","inReplyTo":null,"subject":"[PATCH] mailinfo: unescape quoted-pair in header fields","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-16T21:02:04Z","receivedAt":"2016-09-16T21:02:57Z","isPatch":true,"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\nThe only thing git currently does is removing exterior quotes, but\nquotes within are left alone.\n\nTell mailinfo to remove exterior quotes and remove escape characters from the\nauthor so that they don't show up in the commits author field.\n\nSigned-off-by: Kevin Daudt <me@ikke.info>\n---\nThe only thing I could not easily fix is the prevent git am from removing any quotes around the author. This is done in fmt_ident, which calls `strbuf_addstr_without_crud`. \n\n mailinfo.c                 | 54 ++++++++++++++++++++++++++++++++++++++++++++++\n t/t5100-mailinfo.sh        |  6 ++++++\n t/t5100/quoted-pair.expect |  5 +++++\n t/t5100/quoted-pair.in     |  9 ++++++++\n 4 files changed, 74 insertions(+)\n create mode 100644 t/t5100/quoted-pair.expect\n create mode 100644 t/t5100/quoted-pair.in\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex e19abe3..04036f3 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -54,15 +54,69 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line)\n \tget_sane_name(&mi->name, &mi->name, &mi->email);\n }\n \n+static int unquote_quoted_string(struct strbuf *line)\n+{\n+\tstruct strbuf outbuf;\n+\tconst char *in = line->buf;\n+\tint c, take_next_literally = 0;\n+\tint found_error = 0;\n+\tchar escape_context=0;\n+\n+\tstrbuf_init(&outbuf, line->len);\n+\n+\twhile ((c = *in++) != 0) {\n+\t\tif (take_next_literally) {\n+\t\t\ttake_next_literally = 0;\n+\t\t} else {\n+\t\t\tswitch (c) {\n+\t\t\tcase '\"':\n+\t\t\t\tif (!escape_context)\n+\t\t\t\t\tescape_context = '\"';\n+\t\t\t\telse if (escape_context == '\"')\n+\t\t\t\t\tescape_context = 0;\n+\t\t\t\tcontinue;\n+\t\t\tcase '\\\\':\n+\t\t\t\tif (escape_context) {\n+\t\t\t\t\ttake_next_literally = 1;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\t\t\t\tbreak;\n+\t\t\tcase '(':\n+\t\t\t\tif (!escape_context)\n+\t\t\t\t\tescape_context = '(';\n+\t\t\t\telse if (escape_context == '(')\n+\t\t\t\t\tfound_error = 1;\n+\t\t\t\tbreak;\n+\t\t\tcase ')':\n+\t\t\t\tif (escape_context == '(')\n+\t\t\t\t\tescape_context = 0;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tstrbuf_addch(&outbuf, c);\n+\t}\n+\n+\tstrbuf_reset(line);\n+\tstrbuf_addbuf(line, &outbuf);\n+\tstrbuf_release(&outbuf);\n+\n+\treturn found_error;\n+\n+}\n+\n static void handle_from(struct mailinfo *mi, const struct strbuf *from)\n {\n \tchar *at;\n \tsize_t el;\n \tstruct strbuf f;\n \n+\n \tstrbuf_init(&f, from->len);\n \tstrbuf_addbuf(&f, from);\n \n+\tunquote_quoted_string(&f);\n+\n \tat = strchr(f.buf, '@');\n \tif (!at) {\n \t\tparse_bogus_from(mi, from);\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex 1a5a546..d0c21fc 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -142,4 +142,10 @@ test_expect_success 'mailinfo unescapes with --mboxrd' '\n \ttest_cmp expect mboxrd/msg\n '\n \n+test_expect_success 'mailinfo unescapes rfc2822 quoted-string' '\n+    mkdir quoted-pair &&\n+    git mailinfo /dev/null /dev/null <\"$TEST_DIRECTORY\"/t5100/quoted-pair.in >quoted-pair/info &&\n+    test_cmp \"$TEST_DIRECTORY\"/t5100/quoted-pair.expect 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..cab1bce\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\" <somebody@example.com>\n+Date: Sun, 25 May 2008 00:38:18 -0700\n+Subject: [PATCH] testing quoted-pair\n+\n+\n+\n+---\n+patch\n-- \n2.10.0.86.g6ffa4f1.dirty\n\n"},{"id":"302080","messageId":"20160916222206.jz2d4gpaxxccia5p@sigill.intra.peff.net","threadId":"44099","inReplyTo":"20160916210204.31282-1-me@ikke.info","subject":"Re: [PATCH] mailinfo: unescape quoted-pair in header fields","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-16T22:22:06Z","receivedAt":"2016-09-16T22:22:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 16, 2016 at 11:02:04PM +0200, Kevin Daudt wrote:\n\n> rfc2822 has provisions for quoted strings in structured header fields,\n> but also allows for escaping these with so-called quoted-pairs.\n> \n> The only thing git currently does is removing exterior quotes, but\n> quotes within are left alone.\n> \n> Tell mailinfo to remove exterior quotes and remove escape characters from the\n> author so that they don't show up in the commits author field.\n> \n> Signed-off-by: Kevin Daudt <me@ikke.info>\n> ---\n> The only thing I could not easily fix is the prevent git am from\n> removing any quotes around the author. This is done in fmt_ident,\n> which calls `strbuf_addstr_without_crud`. \n\nAh, OK. I was wondering where that stripping was being done. That makes\nsense, and makes me doubly confident this is the right place to be doing\nit, since the other quote-stripping was not even intentional, but just a\nside effect of the low-level routines.\n\nI think it is OK to leave it in place. If you really want your name to\nbe:\n\n  \"My Name is Always in Quotes\"\n\nthen tough luck. Git does not support it via git-am, but nor does it via\ngit-commit, etc.\n\n>  mailinfo.c                 | 54 ++++++++++++++++++++++++++++++++++++++++++++++\n>  t/t5100-mailinfo.sh        |  6 ++++++\n>  t/t5100/quoted-pair.expect |  5 +++++\n>  t/t5100/quoted-pair.in     |  9 ++++++++\n>  4 files changed, 74 insertions(+)\n>  create mode 100644 t/t5100/quoted-pair.expect\n>  create mode 100644 t/t5100/quoted-pair.in\n> \n> diff --git a/mailinfo.c b/mailinfo.c\n> index e19abe3..04036f3 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -54,15 +54,69 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line)\n>  \tget_sane_name(&mi->name, &mi->name, &mi->email);\n>  }\n>  \n> +static int unquote_quoted_string(struct strbuf *line)\n> +{\n> +\tstruct strbuf outbuf;\n> +\tconst char *in = line->buf;\n> +\tint c, take_next_literally = 0;\n> +\tint found_error = 0;\n> +\tchar escape_context=0;\n\nStyle: whitespace around \"=\".\n\nI had to wonder why we needed both escape_context and\ntake_next_literally; shouldn't we just need a single state bit. But\nescape_context is not \"escape the next character\", it is \"we are\ncurrently in a mode where we should be escaping\".\n\nCould we give it a more descriptive name? I guess it is more than just\n\"we are in a mode\", but rather \"here is the character that will end the\nescaped mode\". Maybe a comment would be more appropriate.\n\n> +\twhile ((c = *in++) != 0) {\n> +\t\tif (take_next_literally) {\n> +\t\t\ttake_next_literally = 0;\n> +\t\t} else {\n\nOK, so that means the previous one was backslash-quoted, and we don't do\nany other cleverness. Good.\n\n> +\t\t\tswitch (c) {\n> +\t\t\tcase '\"':\n> +\t\t\t\tif (!escape_context)\n> +\t\t\t\t\tescape_context = '\"';\n> +\t\t\t\telse if (escape_context == '\"')\n> +\t\t\t\t\tescape_context = 0;\n> +\t\t\t\tcontinue;\n\nAnd here we open or close the quoted portion, depending. Makes sense.\n\n> +\t\t\tcase '\\\\':\n> +\t\t\t\tif (escape_context) {\n> +\t\t\t\t\ttake_next_literally = 1;\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\t}\n> +\t\t\t\tbreak;\n\nI didn't look in the RFC. Is:\n\n  From: my \\\"name\\\" <foo@example.com>\n\nreally the same as:\n\n  From: \"my \\\\\\\"name\\\\\\\"\" <foo@example.com>\n\n? That seems weird, but I think it may be that the former is simply\nbogus (you are not supposed to use backslashes outside of the quoted\nsection at all).\n\n> +\t\t\tcase '(':\n> +\t\t\t\tif (!escape_context)\n> +\t\t\t\t\tescape_context = '(';\n> +\t\t\t\telse if (escape_context == '(')\n> +\t\t\t\t\tfound_error = 1;\n> +\t\t\t\tbreak;\n\nHmm. Is:\n\n  From: Name (Comment with (another comment))\n\nreally disallowed? RFC2822 seems to say that \"comment\" can contain\n\"ccontent\", which can itself be a comment.\n\nThis is obviously getting pretty silly, but if we are going to follow\nthe RFC, I think you actually have to do a recursive parse, and keep\ntrack of an arbitrary depth of context.\n\nI dunno. This method probably covers most cases in practice, and it's\neasy to reason about.\n\n> +\t\t\tcase ')':\n> +\t\t\t\tif (escape_context == '(')\n> +\t\t\t\t\tescape_context = 0;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tstrbuf_addch(&outbuf, c);\n> +\t}\n> +\n> +\tstrbuf_reset(line);\n> +\tstrbuf_addbuf(line, &outbuf);\n> +\tstrbuf_release(&outbuf);\n\nI think you can use strbuf_swap() here to avoid copying the line an\nextra time, like:\n\n  strbuf_swap(line, &outbuf);\n  strbuf_release(&outbuf);\n\nAnother option would be to just:\n\n  in = strbuf_detach(&line);\n\nat the beginning, and then output back into \"line\".\n\n> +\treturn found_error;\n\nWhat happens when we get here and take_next_literally is set? I.e., a\nbackslash at the end of the string. We'll silently print nothing, which\nseems reasonable to me (the other option is to print a literal\nbackslash).\n\nDitto, what if escape_context is non-zero? We're in the middle of an\nunterminated quoted string (or comment).\n\nI'm fine with silently continuing, but it seems weird that we notice\nembedded comments (and return an error), but not these other conditions.\n\n>  static void handle_from(struct mailinfo *mi, const struct strbuf *from)\n>  {\n>  \tchar *at;\n>  \tsize_t el;\n>  \tstruct strbuf f;\n>  \n> +\n>  \tstrbuf_init(&f, from->len);\n>  \tstrbuf_addbuf(&f, from);\n\nFunny extra line?\n\n> +test_expect_success 'mailinfo unescapes rfc2822 quoted-string' '\n> +    mkdir quoted-pair &&\n> +    git mailinfo /dev/null /dev/null <\"$TEST_DIRECTORY\"/t5100/quoted-pair.in >quoted-pair/info &&\n> +    test_cmp \"$TEST_DIRECTORY\"/t5100/quoted-pair.expect quoted-pair/info\n> +'\n\nWe usually break long lines with backslash-escapes. Like:\n\n  git mailinfo /dev/null /dev/null \\\n\t<\"$TEST_DIRECTORY\"/t5100/quoted-pair.in \\\n\t>quoted-pair/info\n\nI'd also wonder if things might be made much more readable by putting\n\"$TEST_DIRECTORY/t5100\" into a shorter variable like $data or something.\nThat would be best done as a preparatory patch which updates all of the\ntests.\n\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\" <somebody@example.com>\n> +Date: Sun, 25 May 2008 00:38:18 -0700\n> +Subject: [PATCH] testing quoted-pair\n\nI do not care that much about the \"()\" comment behavior myself, but if\nwe are going to implement it, it probably makes sense to protect it from\nregression with a test.\n\n-Peff\n"},{"id":"302124","messageId":"20160919105133.GA10901@ikke.info","threadId":"44099","inReplyTo":"20160916222206.jz2d4gpaxxccia5p@sigill.intra.peff.net","subject":"Re: [PATCH] mailinfo: unescape quoted-pair in header fields","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-19T10:51:33Z","receivedAt":"2016-09-19T10:51:41Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"Thanks for the review\n\nOn Fri, Sep 16, 2016 at 03:22:06PM -0700, Jeff King wrote:\n> On Fri, Sep 16, 2016 at 11:02:04PM +0200, Kevin Daudt wrote:\n> \n> >  mailinfo.c                 | 54 ++++++++++++++++++++++++++++++++++++++++++++++\n> >  t/t5100-mailinfo.sh        |  6 ++++++\n> >  t/t5100/quoted-pair.expect |  5 +++++\n> >  t/t5100/quoted-pair.in     |  9 ++++++++\n> >  4 files changed, 74 insertions(+)\n> >  create mode 100644 t/t5100/quoted-pair.expect\n> >  create mode 100644 t/t5100/quoted-pair.in\n> > \n> > diff --git a/mailinfo.c b/mailinfo.c\n> > index e19abe3..04036f3 100644\n> > --- a/mailinfo.c\n> > +++ b/mailinfo.c\n> > @@ -54,15 +54,69 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line)\n> >  \tget_sane_name(&mi->name, &mi->name, &mi->email);\n> >  }\n> >  \n> > +static int unquote_quoted_string(struct strbuf *line)\n> > +{\n> > +\tstruct strbuf outbuf;\n> > +\tconst char *in = line->buf;\n> > +\tint c, take_next_literally = 0;\n> > +\tint found_error = 0;\n> > +\tchar escape_context=0;\n> \n> Style: whitespace around \"=\".\n> \n> I had to wonder why we needed both escape_context and\n> take_next_literally; shouldn't we just need a single state bit. But\n> escape_context is not \"escape the next character\", it is \"we are\n> currently in a mode where we should be escaping\".\n> \n> Could we give it a more descriptive name? I guess it is more than just\n> \"we are in a mode\", but rather \"here is the character that will end the\n> escaped mode\". Maybe a comment would be more appropriate.\n> \n\nYes, your analysis is right, we need to know what character would end\nthe 'escape context'. I'll add a comment.\n\n> > +\twhile ((c = *in++) != 0) {\n> > +\t\tif (take_next_literally) {\n> > +\t\t\ttake_next_literally = 0;\n> > +\t\t} else {\n> \n> OK, so that means the previous one was backslash-quoted, and we don't do\n> any other cleverness. Good.\n> \n> > +\t\t\tswitch (c) {\n> > +\t\t\tcase '\"':\n> > +\t\t\t\tif (!escape_context)\n> > +\t\t\t\t\tescape_context = '\"';\n> > +\t\t\t\telse if (escape_context == '\"')\n> > +\t\t\t\t\tescape_context = 0;\n> > +\t\t\t\tcontinue;\n> \n> And here we open or close the quoted portion, depending. Makes sense.\n> \n> > +\t\t\tcase '\\\\':\n> > +\t\t\t\tif (escape_context) {\n> > +\t\t\t\t\ttake_next_literally = 1;\n> > +\t\t\t\t\tcontinue;\n> > +\t\t\t\t}\n> > +\t\t\t\tbreak;\n> \n> I didn't look in the RFC. Is:\n> \n>   From: my \\\"name\\\" <foo@example.com>\n> \n> really the same as:\n> \n>   From: \"my \\\\\\\"name\\\\\\\"\" <foo@example.com>\n> \n> ? That seems weird, but I think it may be that the former is simply\n> bogus (you are not supposed to use backslashes outside of the quoted\n> section at all).\n\nCorrect, the quoted-pair (escape sequence) can only occur in a quoted\nstring or a comment. Even more so, the display name *needs* to be quoted\nwhen consisting of more then one word according to the RFC.\n\n> \n> > +\t\t\tcase '(':\n> > +\t\t\t\tif (!escape_context)\n> > +\t\t\t\t\tescape_context = '(';\n> > +\t\t\t\telse if (escape_context == '(')\n> > +\t\t\t\t\tfound_error = 1;\n> > +\t\t\t\tbreak;\n> \n> Hmm. Is:\n> \n>   From: Name (Comment with (another comment))\n> \n> really disallowed? RFC2822 seems to say that \"comment\" can contain\n> \"ccontent\", which can itself be a comment.\n\nYes, you are right, it is allowed, I was just looking at the ctext when\nadding this, but failed to see that comments can be nested at that time.\n\n> \n> This is obviously getting pretty silly, but if we are going to follow\n> the RFC, I think you actually have to do a recursive parse, and keep\n> track of an arbitrary depth of context.\n> \n> I dunno. This method probably covers most cases in practice, and it's\n> easy to reason about.\n\nThe problem is, how do you differentiate between nested comments, and\nescaped braces within a comment after one run?\n> \n> > +\t\t\tcase ')':\n> > +\t\t\t\tif (escape_context == '(')\n> > +\t\t\t\t\tescape_context = 0;\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\tstrbuf_addch(&outbuf, c);\n> > +\t}\n> > +\n> > +\tstrbuf_reset(line);\n> > +\tstrbuf_addbuf(line, &outbuf);\n> > +\tstrbuf_release(&outbuf);\n> \n> I think you can use strbuf_swap() here to avoid copying the line an\n> extra time, like:\n> \n>   strbuf_swap(line, &outbuf);\n>   strbuf_release(&outbuf);\n> \n> Another option would be to just:\n> \n>   in = strbuf_detach(&line);\n> \n> at the beginning, and then output back into \"line\".\n> \n\nThanks, I just looked at what other functions were doing, but this is\nmuch better indeed.\n\n> > +\treturn found_error;\n> \n> What happens when we get here and take_next_literally is set? I.e., a\n> backslash at the end of the string. We'll silently print nothing, which\n> seems reasonable to me (the other option is to print a literal\n> backslash).\n> \n> Ditto, what if escape_context is non-zero? We're in the middle of an\n> unterminated quoted string (or comment).\n> \n> I'm fine with silently continuing, but it seems weird that we notice\n> embedded comments (and return an error), but not these other conditions.\n> \n\nI agree. I'm thinking it's better to just be lenient in this method. If\na quote wasn't properly closed, there would be no e-mail adress for\nexample. I think it would do little harm, and I'd remove the checking\nfor the opening brace too.\n\n> >  static void handle_from(struct mailinfo *mi, const struct strbuf *from)\n> >  {\n> >  \tchar *at;\n> >  \tsize_t el;\n> >  \tstruct strbuf f;\n> >  \n> > +\n> >  \tstrbuf_init(&f, from->len);\n> >  \tstrbuf_addbuf(&f, from);\n> \n> Funny extra line?\n\nUgh\n\n> \n> > +test_expect_success 'mailinfo unescapes rfc2822 quoted-string' '\n> > +    mkdir quoted-pair &&\n> > +    git mailinfo /dev/null /dev/null <\"$TEST_DIRECTORY\"/t5100/quoted-pair.in >quoted-pair/info &&\n> > +    test_cmp \"$TEST_DIRECTORY\"/t5100/quoted-pair.expect quoted-pair/info\n> > +'\n> \n> We usually break long lines with backslash-escapes. Like:\n> \n>   git mailinfo /dev/null /dev/null \\\n> \t<\"$TEST_DIRECTORY\"/t5100/quoted-pair.in \\\n> \t>quoted-pair/info\n> \n> I'd also wonder if things might be made much more readable by putting\n> \"$TEST_DIRECTORY/t5100\" into a shorter variable like $data or something.\n> That would be best done as a preparatory patch which updates all of the\n> tests.\n> \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\" <somebody@example.com>\n> > +Date: Sun, 25 May 2008 00:38:18 -0700\n> > +Subject: [PATCH] testing quoted-pair\n> \n> I do not care that much about the \"()\" comment behavior myself, but if\n> we are going to implement it, it probably makes sense to protect it from\n> regression with a test.\n\nYeah, good idea.\n> \n> -Peff\n"},{"id":"302168","messageId":"20160919185440.18234-1-me@ikke.info","threadId":"44099","inReplyTo":"20160916210204.31282-1-me@ikke.info","subject":"[PATCH v2 0/2] Handle escape characters in From field.","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-19T18:54:38Z","receivedAt":"2016-09-19T18:55:04Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"Changes since v2:\n- detach from input parameter to reuse it as an output buffer\n- don't return error when encountering another open bracket in a comment\n- test escaping in comments\n\nKevin Daudt (2):\n  t5100-mailinfo: replace common path prefix with variable\n  mailinfo: unescape quoted-pair in header fields\n\n mailinfo.c                   | 46 +++++++++++++++++++++++++++++\n t/t5100-mailinfo.sh          | 70 +++++++++++++++++++++++++++-----------------\n t/t5100/comment.expect       |  5 ++++\n t/t5100/comment.in           |  9 ++++++\n t/t5100/quoted-string.expect |  5 ++++\n t/t5100/quoted-string.in     |  9 ++++++\n 6 files changed, 117 insertions(+), 27 deletions(-)\n create mode 100644 t/t5100/comment.expect\n create mode 100644 t/t5100/comment.in\n create mode 100644 t/t5100/quoted-string.expect\n create mode 100644 t/t5100/quoted-string.in\n\n-- \n2.10.0.86.g6ffa4f1.dirty\n\n"},{"id":"302169","messageId":"20160919185440.18234-3-me@ikke.info","threadId":"44099","inReplyTo":"20160919185440.18234-1-me@ikke.info","subject":"[PATCH v2 2/2] mailinfo: unescape quoted-pair in header fields","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-19T18:54:40Z","receivedAt":"2016-09-19T18:55:12Z","isPatch":true,"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\nThe only thing git currently does is removing exterior quotes, but\nquotes within are left alone.\n\nRemove exterior quotes and remove escape characters so that they don't\nshow up in the author field.\n\nSigned-off-by: Kevin Daudt <me@ikke.info>\n---\n mailinfo.c                   | 46 ++++++++++++++++++++++++++++++++++++++++++++\n t/t5100-mailinfo.sh          | 14 ++++++++++++++\n t/t5100/comment.expect       |  5 +++++\n t/t5100/comment.in           |  9 +++++++++\n t/t5100/quoted-string.expect |  5 +++++\n t/t5100/quoted-string.in     |  9 +++++++++\n 6 files changed, 88 insertions(+)\n create mode 100644 t/t5100/comment.expect\n create mode 100644 t/t5100/comment.in\n create mode 100644 t/t5100/quoted-string.expect\n create mode 100644 t/t5100/quoted-string.in\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex e19abe3..6a7c2f2 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -54,6 +54,50 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line)\n \tget_sane_name(&mi->name, &mi->name, &mi->email);\n }\n \n+static void unquote_quoted_string(struct strbuf *line)\n+{\n+\tconst char *in = strbuf_detach(line, NULL);\n+\tint c, take_next_literally = 0;\n+\tint found_error = 0;\n+\n+\t/*\n+\t * Stores the character that started the escape mode so that we know what\n+\t * character will stop it\n+\t */\n+\tchar escape_context = 0;\n+\n+\twhile ((c = *in++) != 0) {\n+\t\tif (take_next_literally) {\n+\t\t\ttake_next_literally = 0;\n+\t\t} else {\n+\t\t\tswitch (c) {\n+\t\t\tcase '\"':\n+\t\t\t\tif (!escape_context)\n+\t\t\t\t\tescape_context = '\"';\n+\t\t\t\telse if (escape_context == '\"')\n+\t\t\t\t\tescape_context = 0;\n+\t\t\t\tcontinue;\n+\t\t\tcase '\\\\':\n+\t\t\t\tif (escape_context) {\n+\t\t\t\t\ttake_next_literally = 1;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\t\t\t\tbreak;\n+\t\t\tcase '(':\n+\t\t\t\tif (!escape_context)\n+\t\t\t\t\tescape_context = '(';\n+\t\t\t\tbreak;\n+\t\t\tcase ')':\n+\t\t\t\tif (escape_context == '(')\n+\t\t\t\t\tescape_context = 0;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tstrbuf_addch(line, c);\n+\t}\n+}\n+\n static void handle_from(struct mailinfo *mi, const struct strbuf *from)\n {\n \tchar *at;\n@@ -63,6 +107,8 @@ static void handle_from(struct mailinfo *mi, const struct strbuf *from)\n \tstrbuf_init(&f, from->len);\n \tstrbuf_addbuf(&f, from);\n \n+\tunquote_quoted_string(&f);\n+\n \tat = strchr(f.buf, '@');\n \tif (!at) {\n \t\tparse_bogus_from(mi, from);\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex 27bf3b8..8c21434 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -144,4 +144,18 @@ test_expect_success 'mailinfo unescapes with --mboxrd' '\n \ttest_cmp expect mboxrd/msg\n '\n \n+test_expect_success 'mailinfo handles rfc2822 quoted-string' '\n+\tmkdir quoted-string &&\n+\tgit mailinfo /dev/null /dev/null <\"$DATA\"/quoted-string.in \\\n+\t\t>quoted-string/info &&\n+\ttest_cmp \"$DATA\"/quoted-string.expect quoted-string/info\n+'\n+\n+test_expect_success 'mailinfo handles rfc2822 comment' '\n+\tmkdir comment &&\n+\tgit mailinfo /dev/null /dev/null <\"$DATA\"/comment.in \\\n+\t\t>comment/info &&\n+\ttest_cmp \"$DATA\"/comment.expect comment/info\n+'\n+\n test_done\ndiff --git a/t/t5100/comment.expect b/t/t5100/comment.expect\nnew file mode 100644\nindex 0000000..1197e76\n--- /dev/null\n+++ b/t/t5100/comment.expect\n@@ -0,0 +1,5 @@\n+Author: A U Thor (this is a comment (really))\n+Email: somebody@example.com\n+Subject: testing comments\n+Date: Sun, 25 May 2008 00:38:18 -0700\n+\ndiff --git a/t/t5100/comment.in b/t/t5100/comment.in\nnew file mode 100644\nindex 0000000..430ba97\n--- /dev/null\n+++ b/t/t5100/comment.in\n@@ -0,0 +1,9 @@\n+From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001\n+From: \"A U Thor\" <somebody@example.com> (this is a comment \\(really\\))\n+Date: Sun, 25 May 2008 00:38:18 -0700\n+Subject: [PATCH] testing comments\n+\n+\n+\n+---\n+patch\ndiff --git a/t/t5100/quoted-string.expect b/t/t5100/quoted-string.expect\nnew file mode 100644\nindex 0000000..cab1bce\n--- /dev/null\n+++ b/t/t5100/quoted-string.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-string.in b/t/t5100/quoted-string.in\nnew file mode 100644\nindex 0000000..e2e627a\n--- /dev/null\n+++ b/t/t5100/quoted-string.in\n@@ -0,0 +1,9 @@\n+From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001\n+From: \"Author \\\"The Author\\\" Name\" <somebody@example.com>\n+Date: Sun, 25 May 2008 00:38:18 -0700\n+Subject: [PATCH] testing quoted-pair\n+\n+\n+\n+---\n+patch\n-- \n2.10.0.86.g6ffa4f1.dirty\n\n"},{"id":"302170","messageId":"20160919185440.18234-2-me@ikke.info","threadId":"44099","inReplyTo":"20160919185440.18234-1-me@ikke.info","subject":"[PATCH v2 1/2] t5100-mailinfo: replace common path prefix with variable","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-19T18:54:39Z","receivedAt":"2016-09-19T18:55:15Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"Many tests need to store data in a file, and repeat the same pattern to\nrefer to that path:\n\n    \"$TEST_DATA\"/t5100/\n\nCreate a variable that contains this path, and use that instead.\n\nSigned-off-by: Kevin Daudt <me@ikke.info>\n---\n t/t5100-mailinfo.sh | 56 +++++++++++++++++++++++++++--------------------------\n 1 file changed, 29 insertions(+), 27 deletions(-)\n\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex 1a5a546..27bf3b8 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -7,8 +7,10 @@ test_description='git mailinfo and git mailsplit test'\n \n . ./test-lib.sh\n \n+DATA=\"$TEST_DIRECTORY/t5100\"\n+\n test_expect_success 'split sample box' \\\n-\t'git mailsplit -o. \"$TEST_DIRECTORY\"/t5100/sample.mbox >last &&\n+\t'git mailsplit -o. \"$DATA\"/sample.mbox >last &&\n \tlast=$(cat last) &&\n \techo total is $last &&\n \ttest $(cat last) = 17'\n@@ -17,9 +19,9 @@ check_mailinfo () {\n \tmail=$1 opt=$2\n \tmo=\"$mail$opt\"\n \tgit mailinfo -u $opt msg$mo patch$mo <$mail >info$mo &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/msg$mo msg$mo &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/patch$mo patch$mo &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/info$mo info$mo\n+\ttest_cmp \"$DATA\"/msg$mo msg$mo &&\n+\ttest_cmp \"$DATA\"/patch$mo patch$mo &&\n+\ttest_cmp \"$DATA\"/info$mo info$mo\n }\n \n \n@@ -27,15 +29,15 @@ for mail in 00*\n do\n \ttest_expect_success \"mailinfo $mail\" '\n \t\tcheck_mailinfo $mail \"\" &&\n-\t\tif test -f \"$TEST_DIRECTORY\"/t5100/msg$mail--scissors\n+\t\tif test -f \"$DATA\"/msg$mail--scissors\n \t\tthen\n \t\t\tcheck_mailinfo $mail --scissors\n \t\tfi &&\n-\t\tif test -f \"$TEST_DIRECTORY\"/t5100/msg$mail--no-inbody-headers\n+\t\tif test -f \"$DATA\"/msg$mail--no-inbody-headers\n \t\tthen\n \t\t\tcheck_mailinfo $mail --no-inbody-headers\n \t\tfi &&\n-\t\tif test -f \"$TEST_DIRECTORY\"/t5100/msg$mail--message-id\n+\t\tif test -f \"$DATA\"/msg$mail--message-id\n \t\tthen\n \t\t\tcheck_mailinfo $mail --message-id\n \t\tfi\n@@ -45,7 +47,7 @@ done\n \n test_expect_success 'split box with rfc2047 samples' \\\n \t'mkdir rfc2047 &&\n-\tgit mailsplit -orfc2047 \"$TEST_DIRECTORY\"/t5100/rfc2047-samples.mbox \\\n+\tgit mailsplit -orfc2047 \"$DATA\"/rfc2047-samples.mbox \\\n \t  >rfc2047/last &&\n \tlast=$(cat rfc2047/last) &&\n \techo total is $last &&\n@@ -56,18 +58,18 @@ do\n \ttest_expect_success \"mailinfo $mail\" '\n \t\tgit mailinfo -u $mail-msg $mail-patch <$mail >$mail-info &&\n \t\techo msg &&\n-\t\ttest_cmp \"$TEST_DIRECTORY\"/t5100/empty $mail-msg &&\n+\t\ttest_cmp \"$DATA\"/empty $mail-msg &&\n \t\techo patch &&\n-\t\ttest_cmp \"$TEST_DIRECTORY\"/t5100/empty $mail-patch &&\n+\t\ttest_cmp \"$DATA\"/empty $mail-patch &&\n \t\techo info &&\n-\t\ttest_cmp \"$TEST_DIRECTORY\"/t5100/rfc2047-info-$(basename $mail) $mail-info\n+\t\ttest_cmp \"$DATA\"/rfc2047-info-$(basename $mail) $mail-info\n \t'\n done\n \n test_expect_success 'respect NULs' '\n \n-\tgit mailsplit -d3 -o. \"$TEST_DIRECTORY\"/t5100/nul-plain &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/nul-plain 001 &&\n+\tgit mailsplit -d3 -o. \"$DATA\"/nul-plain &&\n+\ttest_cmp \"$DATA\"/nul-plain 001 &&\n \t(cat 001 | git mailinfo msg patch) &&\n \ttest_line_count = 4 patch\n \n@@ -75,52 +77,52 @@ test_expect_success 'respect NULs' '\n \n test_expect_success 'Preserve NULs out of MIME encoded message' '\n \n-\tgit mailsplit -d5 -o. \"$TEST_DIRECTORY\"/t5100/nul-b64.in &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/nul-b64.in 00001 &&\n+\tgit mailsplit -d5 -o. \"$DATA\"/nul-b64.in &&\n+\ttest_cmp \"$DATA\"/nul-b64.in 00001 &&\n \tgit mailinfo msg patch <00001 &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/nul-b64.expect patch\n+\ttest_cmp \"$DATA\"/nul-b64.expect patch\n \n '\n \n test_expect_success 'mailinfo on from header without name works' '\n \n \tmkdir info-from &&\n-\tgit mailsplit -oinfo-from \"$TEST_DIRECTORY\"/t5100/info-from.in &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/info-from.in info-from/0001 &&\n+\tgit mailsplit -oinfo-from \"$DATA\"/info-from.in &&\n+\ttest_cmp \"$DATA\"/info-from.in info-from/0001 &&\n \tgit mailinfo info-from/msg info-from/patch \\\n \t  <info-from/0001 >info-from/out &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/info-from.expect info-from/out\n+\ttest_cmp \"$DATA\"/info-from.expect info-from/out\n \n '\n \n test_expect_success 'mailinfo finds headers after embedded From line' '\n \tmkdir embed-from &&\n-\tgit mailsplit -oembed-from \"$TEST_DIRECTORY\"/t5100/embed-from.in &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/embed-from.in embed-from/0001 &&\n+\tgit mailsplit -oembed-from \"$DATA\"/embed-from.in &&\n+\ttest_cmp \"$DATA\"/embed-from.in embed-from/0001 &&\n \tgit mailinfo embed-from/msg embed-from/patch \\\n \t  <embed-from/0001 >embed-from/out &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/embed-from.expect embed-from/out\n+\ttest_cmp \"$DATA\"/embed-from.expect embed-from/out\n '\n \n test_expect_success 'mailinfo on message with quoted >From' '\n \tmkdir quoted-from &&\n-\tgit mailsplit -oquoted-from \"$TEST_DIRECTORY\"/t5100/quoted-from.in &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/quoted-from.in quoted-from/0001 &&\n+\tgit mailsplit -oquoted-from \"$DATA\"/quoted-from.in &&\n+\ttest_cmp \"$DATA\"/quoted-from.in quoted-from/0001 &&\n \tgit mailinfo quoted-from/msg quoted-from/patch \\\n \t  <quoted-from/0001 >quoted-from/out &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/quoted-from.expect quoted-from/msg\n+\ttest_cmp \"$DATA\"/quoted-from.expect quoted-from/msg\n '\n \n test_expect_success 'mailinfo unescapes with --mboxrd' '\n \tmkdir mboxrd &&\n \tgit mailsplit -omboxrd --mboxrd \\\n-\t\t\"$TEST_DIRECTORY\"/t5100/sample.mboxrd >last &&\n+\t\t\"$DATA\"/sample.mboxrd >last &&\n \ttest x\"$(cat last)\" = x2 &&\n \tfor i in 0001 0002\n \tdo\n \t\tgit mailinfo mboxrd/msg mboxrd/patch \\\n \t\t  <mboxrd/$i >mboxrd/out &&\n-\t\ttest_cmp \"$TEST_DIRECTORY\"/t5100/${i}mboxrd mboxrd/msg\n+\t\ttest_cmp \"$DATA\"/${i}mboxrd mboxrd/msg\n \tdone &&\n \tsp=\" \" &&\n \techo \"From \" >expect &&\n-- \n2.10.0.86.g6ffa4f1.dirty\n\n"},{"id":"302193","messageId":"xmqqzin3d1zs.fsf@gitster.mtv.corp.google.com","threadId":"44099","inReplyTo":"20160919185440.18234-2-me@ikke.info","subject":"Re: [PATCH v2 1/2] t5100-mailinfo: replace common path prefix with variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-19T21:16:23Z","receivedAt":"2016-09-19T21:16:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Daudt <me@ikke.info> writes:\n\n> Many tests need to store data in a file, and repeat the same pattern to\n> refer to that path:\n>\n>     \"$TEST_DATA\"/t5100/\n\nThat obviously is a typo of\n\n\t\"$TEST_DIRECTORY/t5100\"\n\nIt is a good change, even though I would have chosen a name\nthat is a bit more descriptive than \"$DATA\".\n\n>  test_expect_success 'split sample box' \\\n> -\t'git mailsplit -o. \"$TEST_DIRECTORY\"/t5100/sample.mbox >last &&\n> +\t'git mailsplit -o. \"$DATA\"/sample.mbox >last &&\n\nYou are just following the pattern, and this instance is not too\nbad, but lines like these\n\n> -\ttest_cmp \"$TEST_DIRECTORY\"/t5100/msg$mo msg$mo &&\n> -\ttest_cmp \"$TEST_DIRECTORY\"/t5100/patch$mo patch$mo &&\n> -\ttest_cmp \"$TEST_DIRECTORY\"/t5100/info$mo info$mo\n> +\ttest_cmp \"$DATA\"/msg$mo msg$mo &&\n> +\ttest_cmp \"$DATA\"/patch$mo patch$mo &&\n> +\ttest_cmp \"$DATA\"/info$mo info$mo\n\nmake me wonder why we don't quote the whole thing, i.e.\n\n\ttest_cmp \"$TEST_DATA/info$mo\" \"info$mo\"\n\nas leaving $mo part unquoted forces reader to wonder if it is our\ndeliberate attempt to allow shell $IFS in $mo and have the argument\nsplit when that happens, which can be avoided if we quoted more\nexplicitly.\n\nPerhaps we'd leave that as a low-hanging fruit for future people.\n\nThanks.\n"},{"id":"302194","messageId":"xmqqvaxrd1ml.fsf@gitster.mtv.corp.google.com","threadId":"44099","inReplyTo":"20160919185440.18234-3-me@ikke.info","subject":"Re: [PATCH v2 2/2] mailinfo: unescape quoted-pair in header fields","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-19T21:24:18Z","receivedAt":"2016-09-19T21:24:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Daudt <me@ikke.info> writes:\n\n> +static void unquote_quoted_string(struct strbuf *line)\n> +{\n> +\tconst char *in = strbuf_detach(line, NULL);\n> +\tint c, take_next_literally = 0;\n> +\tint found_error = 0;\n> +\n> +\t/*\n> +\t * Stores the character that started the escape mode so that we know what\n> +\t * character will stop it\n> +\t */\n> +\tchar escape_context = 0;\n> +\n> +\twhile ((c = *in++) != 0) {\n> +\t\tif (take_next_literally) {\n> +\t\t\ttake_next_literally = 0;\n> +\t\t} else {\n> +\t\t\tswitch (c) {\n> +\t\t\tcase '\"':\n> +\t\t\t\tif (!escape_context)\n> +\t\t\t\t\tescape_context = '\"';\n> +\t\t\t\telse if (escape_context == '\"')\n> +\t\t\t\t\tescape_context = 0;\n> +\t\t\t\tcontinue;\n> +\t\t\tcase '\\\\':\n> +\t\t\t\tif (escape_context) {\n> +\t\t\t\t\ttake_next_literally = 1;\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\t}\n> +\t\t\t\tbreak;\n> +\t\t\tcase '(':\n> +\t\t\t\tif (!escape_context)\n> +\t\t\t\t\tescape_context = '(';\n> +\t\t\t\tbreak;\n> +\t\t\tcase ')':\n> +\t\t\t\tif (escape_context == '(')\n> +\t\t\t\t\tescape_context = 0;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tstrbuf_addch(line, c);\n> +\t}\n> +}\n\nThe additional comment makes it very clear what is going on.\n\nIs it an event unusual enough that is worth warning() about if we\nhave either take_next_literally or escape_context set to non-NUL\nupon leaving the loop, I wonder?\n\nWill queue.  Thanks.\n\n\n"},{"id":"302196","messageId":"xmqqmvj3czsf.fsf@gitster.mtv.corp.google.com","threadId":"44099","inReplyTo":"xmqqvaxrd1ml.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] mailinfo: unescape quoted-pair in header fields","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-19T22:04:00Z","receivedAt":"2016-09-19T22:04:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Kevin Daudt <me@ikke.info> writes:\n>\n>> +static void unquote_quoted_string(struct strbuf *line)\n>> +{\n>> +\tconst char *in = strbuf_detach(line, NULL);\n>> +\tint c, take_next_literally = 0;\n>> +\tint found_error = 0;\n> ...\n>> +\t}\n>> +}\n>\n> The additional comment makes it very clear what is going on.\n>\n> Is it an event unusual enough that is worth warning() about if we\n> have either take_next_literally or escape_context set to non-NUL\n> upon leaving the loop, I wonder?\n>\n> Will queue.  Thanks.\n\nIt turns out that found_error is not used anywhere and tripped the\n-Werror=unused-variable check.  I've removed that line while\nqueuing.\n\nThanks.\n"},{"id":"302209","messageId":"20160920035710.qw2byl3qeqwih7t5@sigill.intra.peff.net","threadId":"44099","inReplyTo":"20160919105133.GA10901@ikke.info","subject":"Re: [PATCH] mailinfo: unescape quoted-pair in header fields","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-20T03:57:11Z","receivedAt":"2016-09-20T03:57:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 19, 2016 at 12:51:33PM +0200, Kevin Daudt wrote:\n\n> > I didn't look in the RFC. Is:\n> > \n> >   From: my \\\"name\\\" <foo@example.com>\n> > \n> > really the same as:\n> > \n> >   From: \"my \\\\\\\"name\\\\\\\"\" <foo@example.com>\n> > \n> > ? That seems weird, but I think it may be that the former is simply\n> > bogus (you are not supposed to use backslashes outside of the quoted\n> > section at all).\n> \n> Correct, the quoted-pair (escape sequence) can only occur in a quoted\n> string or a comment. Even more so, the display name *needs* to be quoted\n> when consisting of more then one word according to the RFC.\n\nHmm. So, I guess a follow-up question is: what would it be OK to do if\nwe see a quoted-pair outside of quotes? If the top one above violates\nthe RFC, it seems like stripping the backslashes would be a reasonable\noutcome.\n\nSo if that's the case, do we actually need to care if we see any\nparenthesized comments? I think we should just leave comments in place\neither way, so syntactically they are only interesting insofar as we\nreplace quoted pairs or not.\n\nIOW, I wonder if:\n\n  while ((c = *in++)) {\n\tswitch (c) {\n\tcase '\\\\':\n\t\tif (!*in)\n\t\t\treturn 0; /* ignore trailing backslash */\n\t\t/* quoted pair */\n\t\tstrbuf_addch(out, *in++);\n\t\tbreak;\n\tcase '\"':\n\t\t/*\n\t\t * This may be starting or ending a quoted section,\n\t\t * but we do not care whether we are in such a section.\n\t\t * We _do_ need to remove the quotes, though, as they\n\t\t * are syntactic.\n\t\t */\n\t\tbreak;\n\tdefault:\n\t\t/*\n\t\t * Anything else is a normal character we keep. These\n\t\t * _might_ be violating the RFC if they are magic\n\t\t * characters outside of a quoted section, but we'd\n\t\t * rather be liberal and pass them through.\n\t\t */\n\t\tstrbuf_addch(out, c);\n\t\tbreak;\n\t}\n  }\n\nwould work. I certainly do not mind following the RFC more closely, but\nAFAICT the very simple code above gives a pretty forgiving outcome.\n\n> > This is obviously getting pretty silly, but if we are going to follow\n> > the RFC, I think you actually have to do a recursive parse, and keep\n> > track of an arbitrary depth of context.\n> > \n> > I dunno. This method probably covers most cases in practice, and it's\n> > easy to reason about.\n> \n> The problem is, how do you differentiate between nested comments, and\n> escaped braces within a comment after one run?\n\nI'm not sure what you mean. Escaped characters are always handled first\nin your loop. Can you give an example (although if you agree with what I\nwrote above, it may not be worth discussing further)?\n\n-Peff\n"},{"id":"302210","messageId":"20160920035947.nicx55ql4xlue66m@sigill.intra.peff.net","threadId":"44099","inReplyTo":"xmqqzin3d1zs.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/2] t5100-mailinfo: replace common path prefix with variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-20T03:59:47Z","receivedAt":"2016-09-20T03:59:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 19, 2016 at 02:16:23PM -0700, Junio C Hamano wrote:\n\n> Kevin Daudt <me@ikke.info> writes:\n> \n> > Many tests need to store data in a file, and repeat the same pattern to\n> > refer to that path:\n> >\n> >     \"$TEST_DATA\"/t5100/\n> \n> That obviously is a typo of\n> \n> \t\"$TEST_DIRECTORY/t5100\"\n> \n> It is a good change, even though I would have chosen a name\n> that is a bit more descriptive than \"$DATA\".\n\nThe name \"$DATA\" was my suggestion. I was shooting for something short\nsince this is used a lot and is really a script-local variable (I'd have\nkept it lowercase to indicate that, but maybe that is just me).\nSomething like \"$root\" would also work. I dunno.\n\n> > -\ttest_cmp \"$TEST_DIRECTORY\"/t5100/msg$mo msg$mo &&\n> > -\ttest_cmp \"$TEST_DIRECTORY\"/t5100/patch$mo patch$mo &&\n> > -\ttest_cmp \"$TEST_DIRECTORY\"/t5100/info$mo info$mo\n> > +\ttest_cmp \"$DATA\"/msg$mo msg$mo &&\n> > +\ttest_cmp \"$DATA\"/patch$mo patch$mo &&\n> > +\ttest_cmp \"$DATA\"/info$mo info$mo\n> \n> make me wonder why we don't quote the whole thing, i.e.\n> \n> \ttest_cmp \"$TEST_DATA/info$mo\" \"info$mo\"\n> \n> as leaving $mo part unquoted forces reader to wonder if it is our\n> deliberate attempt to allow shell $IFS in $mo and have the argument\n> split when that happens, which can be avoided if we quoted more\n> explicitly.\n> \n> Perhaps we'd leave that as a low-hanging fruit for future people.\n\nYeah, I agree that quoting the whole thing makes it more obvious (though\nI guess quoting the second info$mo does add two characters).\n\n-Peff\n"},{"id":"302212","messageId":"20160920042832.7xzazxsfiug3llyl@sigill.intra.peff.net","threadId":"44099","inReplyTo":"20160919185440.18234-3-me@ikke.info","subject":"Re: [PATCH v2 2/2] mailinfo: unescape quoted-pair in header fields","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-20T04:28:33Z","receivedAt":"2016-09-20T04:28:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 19, 2016 at 08:54:40PM +0200, Kevin Daudt wrote:\n\n> diff --git a/t/t5100/comment.expect b/t/t5100/comment.expect\n> new file mode 100644\n> index 0000000..1197e76\n> --- /dev/null\n> +++ b/t/t5100/comment.expect\n> @@ -0,0 +1,5 @@\n> +Author: A U Thor (this is a comment (really))\n\nHmm. I don't see any recursion in your parsing, so after the first \")\"\nour escape_context would be 0 again, right? So a more tricky test is:\n\n  Author: A U Thor (this is a comment (really) with \\(quoted\\) pairs)\n\nWe are still inside \"ctext\" when we hit those quoted pairs, and they\nshould be unquoted, but your code would not do so (unless we go the\nroute of simply unquoting pairs everywhere).\n\nI think your parser would have to follow the BNF more closely with a\nrecursive descent parser, like:\n\n  const char *parse_comment(const char *in, struct strbuf *out)\n  {\n        size_t orig_out = out->len;\n\n        if ((in = parse_char('(', in, out))) &&\n            (in = parse_ccontent(in, out)) &&\n            (in = parse_char(')', in, out))))\n                return in;\n\n        strbuf_setlen(out, orig_out);\n        return NULL;\n  }\n\n  const char *parse_ccontent(const char *in, struct strbuf *out)\n  {\n        while (*in && *in != ')') {\n                const char *next;\n\n                if ((next = parse_quoted_pair(in, out)) ||\n                    (next = parse_comment(in, out)) ||\n                    (next = parse_ctext(in, out))) {\n                        in = next;\n                        continue;\n                }\n        }\n\n\t/*\n\t * if \"in\" is NUL here we have an unclosed comment; but we'll\n\t * just silently ignore and accept it\n\t */\n\treturn in;\n  }\n\n  const char *parse_char(char c, const char *in, struct strbuf *out)\n  {\n        if (*in != c)\n                return NULL;\n        strbuf_addch(out, c);\n        return in + 1;\n  }\n\nYou can probably guess at the implementation of parse_quoted_pair(),\nparse_ctext(), etc (and naturally, the above is completely untested and\nprobably has some bugs in it).\n\nIn a former life (back when it was still rfc822!) I remember\nimplementing a similar parser, which I think was in turn based on the\ncclient code in pine. It's not _too_ hard to get it all right based on\nthe BNF in the RFC, but as you can see it's a bit tedious. And I'm not\nconvinced we actually need it to be completely right for our purposes.\nWe really are looking for a single address, with the email in \"<>\" and\nthe name as everything before that, but de-quoted.\n\n-Peff\n"},{"id":"302258","messageId":"20160921110934.f6eu2dz6i2mlpa45@sigill.intra.peff.net","threadId":"44099","inReplyTo":"20160919185440.18234-3-me@ikke.info","subject":"Re: [PATCH v2 2/2] mailinfo: unescape quoted-pair in header fields","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-21T11:09:35Z","receivedAt":"2016-09-21T11:09:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 19, 2016 at 08:54:40PM +0200, Kevin Daudt wrote:\n\n> diff --git a/mailinfo.c b/mailinfo.c\n> index e19abe3..6a7c2f2 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -54,6 +54,50 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line)\n>  \tget_sane_name(&mi->name, &mi->name, &mi->email);\n>  }\n>  \n> +static void unquote_quoted_string(struct strbuf *line)\n> +{\n> +\tconst char *in = strbuf_detach(line, NULL);\n\nI see that this version uses the \"detach, and then write into the\nreplacement\" approach, which is good. But...\n\n> +\tint c, take_next_literally = 0;\n> +\tint found_error = 0;\n> +\n> +\t/*\n> +\t * Stores the character that started the escape mode so that we know what\n> +\t * character will stop it\n> +\t */\n> +\tchar escape_context = 0;\n> +\n> +\twhile ((c = *in++) != 0) {\n> +\t\tif (take_next_literally) {\n> +\t\t\ttake_next_literally = 0;\n> +\t\t} else {\n> [...]\n> +\t\t}\n> +\n> +\t\tstrbuf_addch(line, c);\n> +\t}\n> +}\n\nIt needs to `free(in)` at the end of the function.\n\nYour original also fed \"line->len\" as a hint, but I doubt it really\nmatters in practice, so I don't mind losing that.\n\n-Peff\n"},{"id":"302281","messageId":"xmqqtwd9b5ie.fsf@gitster.mtv.corp.google.com","threadId":"44099","inReplyTo":"20160920035710.qw2byl3qeqwih7t5@sigill.intra.peff.net","subject":"Re: [PATCH] mailinfo: unescape quoted-pair in header fields","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-21T16:07:53Z","receivedAt":"2016-09-21T16:08:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So if that's the case, do we actually need to care if we see any\n> parenthesized comments? I think we should just leave comments in place\n> either way, so syntactically they are only interesting insofar as we\n> replace quoted pairs or not.\n>\n> IOW, I wonder if:\n>\n>   while ((c = *in++)) {\n> \tswitch (c) {\n> \tcase '\\\\':\n> \t\tif (!*in)\n> \t\t\treturn 0; /* ignore trailing backslash */\n> \t\t/* quoted pair */\n> \t\tstrbuf_addch(out, *in++);\n> \t\tbreak;\n> \tcase '\"':\n> \t\t/*\n> \t\t * This may be starting or ending a quoted section,\n> \t\t * but we do not care whether we are in such a section.\n> \t\t * We _do_ need to remove the quotes, though, as they\n> \t\t * are syntactic.\n> \t\t */\n> \t\tbreak;\n> \tdefault:\n> \t\t/*\n> \t\t * Anything else is a normal character we keep. These\n> \t\t * _might_ be violating the RFC if they are magic\n> \t\t * characters outside of a quoted section, but we'd\n> \t\t * rather be liberal and pass them through.\n> \t\t */\n> \t\tstrbuf_addch(out, c);\n> \t\tbreak;\n> \t}\n>   }\n>\n> would work. I certainly do not mind following the RFC more closely, but\n> AFAICT the very simple code above gives a pretty forgiving outcome.\n\nThe simplicity of the code does look attractive to me.  I do not\noffhand see an obvious case/flaw that this simplified rule would\nmangle a valid human-readable part.\n\n"},{"id":"302417","messageId":"xmqq60pn37gs.fsf@gitster.mtv.corp.google.com","threadId":"44099","inReplyTo":"20160921110934.f6eu2dz6i2mlpa45@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] mailinfo: unescape quoted-pair in header fields","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-22T22:17:23Z","receivedAt":"2016-09-22T22:17:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Sep 19, 2016 at 08:54:40PM +0200, Kevin Daudt wrote:\n>\n>> + ...\n>> +\twhile ((c = *in++) != 0) {\n>> +\t\tif (take_next_literally) {\n>> +\t\t\ttake_next_literally = 0;\n>> +\t\t} else {\n>> [...]\n>> +\t\t}\n>> +\n>> +\t\tstrbuf_addch(line, c);\n>> +\t}\n>> +}\n>\n> It needs to `free(in)` at the end of the function.\n\nEhh, in has been incremented and is pointing at the terminating NUL\nthere, so it would be more like\n\n\tchar *to_free, *in;\n\n        to_free = strbuf_detach(line, NULL);\n        in = to_free;\n\t...\n        while ((c = *in++)) {\n        \t...\n\t}\n        free(to_free);\n\nI would think ;-).\n\n        \n"},{"id":"302426","messageId":"20160923041540.5fvl6ytp2tvcflsk@sigill.intra.peff.net","threadId":"44099","inReplyTo":"xmqq60pn37gs.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] mailinfo: unescape quoted-pair in header fields","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-23T04:15:41Z","receivedAt":"2016-09-23T04:15:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 22, 2016 at 03:17:23PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Mon, Sep 19, 2016 at 08:54:40PM +0200, Kevin Daudt wrote:\n> >\n> >> + ...\n> >> +\twhile ((c = *in++) != 0) {\n> >> +\t\tif (take_next_literally) {\n> >> +\t\t\ttake_next_literally = 0;\n> >> +\t\t} else {\n> >> [...]\n> >> +\t\t}\n> >> +\n> >> +\t\tstrbuf_addch(line, c);\n> >> +\t}\n> >> +}\n> >\n> > It needs to `free(in)` at the end of the function.\n> \n> Ehh, in has been incremented and is pointing at the terminating NUL\n> there, so it would be more like\n> \n> \tchar *to_free, *in;\n> \n>         to_free = strbuf_detach(line, NULL);\n>         in = to_free;\n> \t...\n>         while ((c = *in++)) {\n>         \t...\n> \t}\n>         free(to_free);\n> \n> I would think ;-).\n\nOops, yes. It is beginning to make the \"strbuf_swap()\" look less\nconvoluted. :)\n\n-Peff\n"},{"id":"302533","messageId":"20160925201713.GA6937@ikke.info","threadId":"44099","inReplyTo":"20160923041540.5fvl6ytp2tvcflsk@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] mailinfo: unescape quoted-pair in header fields","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-25T20:17:13Z","receivedAt":"2016-09-25T20:17:20Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Fri, Sep 23, 2016 at 12:15:41AM -0400, Jeff King wrote:\n> On Thu, Sep 22, 2016 at 03:17:23PM -0700, Junio C Hamano wrote:\n> \n> > Jeff King <peff@peff.net> writes:\n> > \n> > > On Mon, Sep 19, 2016 at 08:54:40PM +0200, Kevin Daudt wrote:\n> > >\n> > >> + ...\n> > >> +\twhile ((c = *in++) != 0) {\n> > >> +\t\tif (take_next_literally) {\n> > >> +\t\t\ttake_next_literally = 0;\n> > >> +\t\t} else {\n> > >> [...]\n> > >> +\t\t}\n> > >> +\n> > >> +\t\tstrbuf_addch(line, c);\n> > >> +\t}\n> > >> +}\n> > >\n> > > It needs to `free(in)` at the end of the function.\n> > \n> > Ehh, in has been incremented and is pointing at the terminating NUL\n> > there, so it would be more like\n> > \n> > \tchar *to_free, *in;\n> > \n> >         to_free = strbuf_detach(line, NULL);\n> >         in = to_free;\n> > \t...\n> >         while ((c = *in++)) {\n> >         \t...\n> > \t}\n> >         free(to_free);\n> > \n> > I would think ;-).\n> \n> Oops, yes. It is beginning to make the \"strbuf_swap()\" look less\n> convoluted. :)\n> \n\nI've switched to strbuf_swap now, much better. I've implemented\nrecursive parsing without looking at what you provided, just to see what\nI'd came up with. Though I've not implemented a recursive descent\nparser, but it might suffice.\n\nI'm sending the patches now.\n\n"},{"id":"302534","messageId":"20160925210808.26424-1-me@ikke.info","threadId":"44099","inReplyTo":"20160919185440.18234-1-me@ikke.info","subject":"[PATCH v3 1/2] t5100-mailinfo: replace common path prefix with variable","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-25T21:08:07Z","receivedAt":"2016-09-25T21:08:35Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"Many tests need to store data in a file, and repeat the same pattern to\nrefer to that path:\n\n    \"$TEST_DIRECTORY\"/t5100/\n\nCreate a variable that contains this path, and use that instead.\n\nSigned-off-by: Kevin Daudt <me@ikke.info>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Changes since v2:\n - changed $DATA to $data to indicate it's a script-local variable\n\n t/t5100-mailinfo.sh | 56 +++++++++++++++++++++++++++--------------------------\n 1 file changed, 29 insertions(+), 27 deletions(-)\n\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex 1a5a546..c4ed0f4 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -7,8 +7,10 @@ test_description='git mailinfo and git mailsplit test'\n \n . ./test-lib.sh\n \n+data=\"$TEST_DIRECTORY/t5100\"\n+\n test_expect_success 'split sample box' \\\n-\t'git mailsplit -o. \"$TEST_DIRECTORY\"/t5100/sample.mbox >last &&\n+\t'git mailsplit -o. \"$data\"/sample.mbox >last &&\n \tlast=$(cat last) &&\n \techo total is $last &&\n \ttest $(cat last) = 17'\n@@ -17,9 +19,9 @@ check_mailinfo () {\n \tmail=$1 opt=$2\n \tmo=\"$mail$opt\"\n \tgit mailinfo -u $opt msg$mo patch$mo <$mail >info$mo &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/msg$mo msg$mo &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/patch$mo patch$mo &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/info$mo info$mo\n+\ttest_cmp \"$data\"/msg$mo msg$mo &&\n+\ttest_cmp \"$data\"/patch$mo patch$mo &&\n+\ttest_cmp \"$data\"/info$mo info$mo\n }\n \n \n@@ -27,15 +29,15 @@ for mail in 00*\n do\n \ttest_expect_success \"mailinfo $mail\" '\n \t\tcheck_mailinfo $mail \"\" &&\n-\t\tif test -f \"$TEST_DIRECTORY\"/t5100/msg$mail--scissors\n+\t\tif test -f \"$data\"/msg$mail--scissors\n \t\tthen\n \t\t\tcheck_mailinfo $mail --scissors\n \t\tfi &&\n-\t\tif test -f \"$TEST_DIRECTORY\"/t5100/msg$mail--no-inbody-headers\n+\t\tif test -f \"$data\"/msg$mail--no-inbody-headers\n \t\tthen\n \t\t\tcheck_mailinfo $mail --no-inbody-headers\n \t\tfi &&\n-\t\tif test -f \"$TEST_DIRECTORY\"/t5100/msg$mail--message-id\n+\t\tif test -f \"$data\"/msg$mail--message-id\n \t\tthen\n \t\t\tcheck_mailinfo $mail --message-id\n \t\tfi\n@@ -45,7 +47,7 @@ done\n \n test_expect_success 'split box with rfc2047 samples' \\\n \t'mkdir rfc2047 &&\n-\tgit mailsplit -orfc2047 \"$TEST_DIRECTORY\"/t5100/rfc2047-samples.mbox \\\n+\tgit mailsplit -orfc2047 \"$data\"/rfc2047-samples.mbox \\\n \t  >rfc2047/last &&\n \tlast=$(cat rfc2047/last) &&\n \techo total is $last &&\n@@ -56,18 +58,18 @@ do\n \ttest_expect_success \"mailinfo $mail\" '\n \t\tgit mailinfo -u $mail-msg $mail-patch <$mail >$mail-info &&\n \t\techo msg &&\n-\t\ttest_cmp \"$TEST_DIRECTORY\"/t5100/empty $mail-msg &&\n+\t\ttest_cmp \"$data\"/empty $mail-msg &&\n \t\techo patch &&\n-\t\ttest_cmp \"$TEST_DIRECTORY\"/t5100/empty $mail-patch &&\n+\t\ttest_cmp \"$data\"/empty $mail-patch &&\n \t\techo info &&\n-\t\ttest_cmp \"$TEST_DIRECTORY\"/t5100/rfc2047-info-$(basename $mail) $mail-info\n+\t\ttest_cmp \"$data\"/rfc2047-info-$(basename $mail) $mail-info\n \t'\n done\n \n test_expect_success 'respect NULs' '\n \n-\tgit mailsplit -d3 -o. \"$TEST_DIRECTORY\"/t5100/nul-plain &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/nul-plain 001 &&\n+\tgit mailsplit -d3 -o. \"$data\"/nul-plain &&\n+\ttest_cmp \"$data\"/nul-plain 001 &&\n \t(cat 001 | git mailinfo msg patch) &&\n \ttest_line_count = 4 patch\n \n@@ -75,52 +77,52 @@ test_expect_success 'respect NULs' '\n \n test_expect_success 'Preserve NULs out of MIME encoded message' '\n \n-\tgit mailsplit -d5 -o. \"$TEST_DIRECTORY\"/t5100/nul-b64.in &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/nul-b64.in 00001 &&\n+\tgit mailsplit -d5 -o. \"$data\"/nul-b64.in &&\n+\ttest_cmp \"$data\"/nul-b64.in 00001 &&\n \tgit mailinfo msg patch <00001 &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/nul-b64.expect patch\n+\ttest_cmp \"$data\"/nul-b64.expect patch\n \n '\n \n test_expect_success 'mailinfo on from header without name works' '\n \n \tmkdir info-from &&\n-\tgit mailsplit -oinfo-from \"$TEST_DIRECTORY\"/t5100/info-from.in &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/info-from.in info-from/0001 &&\n+\tgit mailsplit -oinfo-from \"$data\"/info-from.in &&\n+\ttest_cmp \"$data\"/info-from.in info-from/0001 &&\n \tgit mailinfo info-from/msg info-from/patch \\\n \t  <info-from/0001 >info-from/out &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/info-from.expect info-from/out\n+\ttest_cmp \"$data\"/info-from.expect info-from/out\n \n '\n \n test_expect_success 'mailinfo finds headers after embedded From line' '\n \tmkdir embed-from &&\n-\tgit mailsplit -oembed-from \"$TEST_DIRECTORY\"/t5100/embed-from.in &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/embed-from.in embed-from/0001 &&\n+\tgit mailsplit -oembed-from \"$data\"/embed-from.in &&\n+\ttest_cmp \"$data\"/embed-from.in embed-from/0001 &&\n \tgit mailinfo embed-from/msg embed-from/patch \\\n \t  <embed-from/0001 >embed-from/out &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/embed-from.expect embed-from/out\n+\ttest_cmp \"$data\"/embed-from.expect embed-from/out\n '\n \n test_expect_success 'mailinfo on message with quoted >From' '\n \tmkdir quoted-from &&\n-\tgit mailsplit -oquoted-from \"$TEST_DIRECTORY\"/t5100/quoted-from.in &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/quoted-from.in quoted-from/0001 &&\n+\tgit mailsplit -oquoted-from \"$data\"/quoted-from.in &&\n+\ttest_cmp \"$data\"/quoted-from.in quoted-from/0001 &&\n \tgit mailinfo quoted-from/msg quoted-from/patch \\\n \t  <quoted-from/0001 >quoted-from/out &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/quoted-from.expect quoted-from/msg\n+\ttest_cmp \"$data\"/quoted-from.expect quoted-from/msg\n '\n \n test_expect_success 'mailinfo unescapes with --mboxrd' '\n \tmkdir mboxrd &&\n \tgit mailsplit -omboxrd --mboxrd \\\n-\t\t\"$TEST_DIRECTORY\"/t5100/sample.mboxrd >last &&\n+\t\t\"$data\"/sample.mboxrd >last &&\n \ttest x\"$(cat last)\" = x2 &&\n \tfor i in 0001 0002\n \tdo\n \t\tgit mailinfo mboxrd/msg mboxrd/patch \\\n \t\t  <mboxrd/$i >mboxrd/out &&\n-\t\ttest_cmp \"$TEST_DIRECTORY\"/t5100/${i}mboxrd mboxrd/msg\n+\t\ttest_cmp \"$data\"/${i}mboxrd mboxrd/msg\n \tdone &&\n \tsp=\" \" &&\n \techo \"From \" >expect &&\n-- \n2.10.0.89.ge802c3a.dirty\n\n"},{"id":"302535","messageId":"20160925210808.26424-2-me@ikke.info","threadId":"44099","inReplyTo":"20160925210808.26424-1-me@ikke.info","subject":"[PATCH v3 2/2] mailinfo: unescape quoted-pair in header fields","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-25T21:08:08Z","receivedAt":"2016-09-25T21:08:52Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"rfc2822 has provisions for quoted strings and comments in structured header\nfields, but also allows for escaping these with so-called quoted-pairs.\n\nThe only thing git currently does is removing exterior quotes, but\nquotes within are left alone.\n\nRemove exterior quotes and remove escape characters so that they don't\nshow up in the author field.\n\nSigned-off-by: Kevin Daudt <me@ikke.info>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Changes since v2:\n\n - handle comments inside comments recursively\n - renamed the main function to unquote_quoted_pairs because it also\n   handles quoted pairs in comments\n\n\n mailinfo.c                   | 82 ++++++++++++++++++++++++++++++++++++++++++++\n t/t5100-mailinfo.sh          | 14 ++++++++\n t/t5100/comment.expect       |  5 +++\n t/t5100/comment.in           |  9 +++++\n t/t5100/quoted-string.expect |  5 +++\n t/t5100/quoted-string.in     |  9 +++++\n 6 files changed, 124 insertions(+)\n create mode 100644 t/t5100/comment.expect\n create mode 100644 t/t5100/comment.in\n create mode 100644 t/t5100/quoted-string.expect\n create mode 100644 t/t5100/quoted-string.in\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex e19abe3..b4118a0 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -54,6 +54,86 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line)\n \tget_sane_name(&mi->name, &mi->name, &mi->email);\n }\n \n+static const char *unquote_comment(struct strbuf *outbuf, const char *in)\n+{\n+\tint c;\n+\tint take_next_litterally = 0;\n+\n+\tstrbuf_addch(outbuf, '(');\n+\n+\twhile ((c = *in++) != 0) {\n+\t\tif (take_next_litterally == 1) {\n+\t\t\ttake_next_litterally = 0;\n+\t\t} else {\n+\t\t\tswitch (c) {\n+\t\t\tcase '\\\\':\n+\t\t\t\ttake_next_litterally = 1;\n+\t\t\t\tcontinue;\n+\t\t\tcase '(':\n+\t\t\t\tin = unquote_comment(outbuf, in);\n+\t\t\t\tcontinue;\n+\t\t\tcase ')':\n+\t\t\t\tstrbuf_addch(outbuf, ')');\n+\t\t\t\treturn in;\n+\t\t\t}\n+\t\t}\n+\n+\t\tstrbuf_addch(outbuf, c);\n+\t}\n+\n+\treturn in;\n+}\n+\n+static const char *unquote_quoted_string(struct strbuf *outbuf, const char *in)\n+{\n+\tint c;\n+\tint take_next_litterally = 0;\n+\n+\twhile ((c = *in++) != 0) {\n+\t\tif (take_next_litterally == 1) {\n+\t\t\ttake_next_litterally = 0;\n+\t\t} else {\n+\t\t\tswitch (c) {\n+\t\t\tcase '\\\\':\n+\t\t\t\ttake_next_litterally = 1;\n+\t\t\t\tcontinue;\n+\t\t\tcase '\"':\n+\t\t\t\treturn in;\n+\t\t\t}\n+\t\t}\n+\n+\t\tstrbuf_addch(outbuf, c);\n+\t}\n+\n+\treturn in;\n+}\n+\n+static void unquote_quoted_pair(struct strbuf *line)\n+{\n+\tstruct strbuf outbuf;\n+\tconst char *in = line->buf;\n+\tint c;\n+\n+\tstrbuf_init(&outbuf, line->len);\n+\n+\twhile ((c = *in++) != 0) {\n+\t\tswitch (c) {\n+\t\tcase '\"':\n+\t\t\tin = unquote_quoted_string(&outbuf, in);\n+\t\t\tcontinue;\n+\t\tcase '(':\n+\t\t\tin = unquote_comment(&outbuf, in);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tstrbuf_addch(&outbuf, c);\n+\t}\n+\n+\tstrbuf_swap(&outbuf, line);\n+\tstrbuf_release(&outbuf);\n+\n+}\n+\n static void handle_from(struct mailinfo *mi, const struct strbuf *from)\n {\n \tchar *at;\n@@ -63,6 +143,8 @@ static void handle_from(struct mailinfo *mi, const struct strbuf *from)\n \tstrbuf_init(&f, from->len);\n \tstrbuf_addbuf(&f, from);\n \n+\tunquote_quoted_pair(&f);\n+\n \tat = strchr(f.buf, '@');\n \tif (!at) {\n \t\tparse_bogus_from(mi, from);\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex c4ed0f4..3e983c0 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -144,4 +144,18 @@ test_expect_success 'mailinfo unescapes with --mboxrd' '\n \ttest_cmp expect mboxrd/msg\n '\n \n+test_expect_success 'mailinfo handles rfc2822 quoted-string' '\n+\tmkdir quoted-string &&\n+\tgit mailinfo /dev/null /dev/null <\"$DATA\"/quoted-string.in \\\n+\t\t>quoted-string/info &&\n+\ttest_cmp \"$DATA\"/quoted-string.expect quoted-string/info\n+'\n+\n+test_expect_success 'mailinfo handles rfc2822 comment' '\n+\tmkdir comment &&\n+\tgit mailinfo /dev/null /dev/null <\"$DATA\"/comment.in \\\n+\t\t>comment/info &&\n+\ttest_cmp \"$DATA\"/comment.expect comment/info\n+'\n+\n test_done\ndiff --git a/t/t5100/comment.expect b/t/t5100/comment.expect\nnew file mode 100644\nindex 0000000..7228177\n--- /dev/null\n+++ b/t/t5100/comment.expect\n@@ -0,0 +1,5 @@\n+Author: A U Thor (this is (really) a comment (honestly))\n+Email: somebody@example.com\n+Subject: testing comments\n+Date: Sun, 25 May 2008 00:38:18 -0700\n+\ndiff --git a/t/t5100/comment.in b/t/t5100/comment.in\nnew file mode 100644\nindex 0000000..c53a192\n--- /dev/null\n+++ b/t/t5100/comment.in\n@@ -0,0 +1,9 @@\n+From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001\n+From: \"A U Thor\" <somebody@example.com> (this is \\(really\\) a comment (honestly))\n+Date: Sun, 25 May 2008 00:38:18 -0700\n+Subject: [PATCH] testing comments\n+\n+\n+\n+---\n+patch\ndiff --git a/t/t5100/quoted-string.expect b/t/t5100/quoted-string.expect\nnew file mode 100644\nindex 0000000..cab1bce\n--- /dev/null\n+++ b/t/t5100/quoted-string.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-string.in b/t/t5100/quoted-string.in\nnew file mode 100644\nindex 0000000..e2e627a\n--- /dev/null\n+++ b/t/t5100/quoted-string.in\n@@ -0,0 +1,9 @@\n+From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001\n+From: \"Author \\\"The Author\\\" Name\" <somebody@example.com>\n+Date: Sun, 25 May 2008 00:38:18 -0700\n+Subject: [PATCH] testing quoted-pair\n+\n+\n+\n+---\n+patch\n-- \n2.10.0.89.ge802c3a.dirty\n\n"},{"id":"302536","messageId":"0b6cbc53-e058-f064-59e8-b73203f3e400@gmail.com","threadId":"44099","inReplyTo":"20160925201713.GA6937@ikke.info","subject":"Re: [PATCH v2 2/2] mailinfo: unescape quoted-pair in header fields","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-09-25T22:38:42Z","receivedAt":"2016-09-25T22:38:55Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 25.09.2016 o 22:17, Kevin Daudt pisze:\n> On Fri, Sep 23, 2016 at 12:15:41AM -0400, Jeff King wrote:\n\n>> Oops, yes. It is beginning to make the \"strbuf_swap()\" look less\n>> convoluted. :)\n>>\n> \n> I've switched to strbuf_swap now, much better. I've implemented\n> recursive parsing without looking at what you provided, just to see what\n> I'd came up with. Though I've not implemented a recursive descent\n> parser, but it might suffice.\n\nI think you can implement a parser handling proper nesting of parens\nwithout recursion.\n\nThough... what is the definition in the RFC?\n-- \nJakub Narębski\n\n"},{"id":"302554","messageId":"20160926050230.GA19089@ikke.info","threadId":"44099","inReplyTo":"0b6cbc53-e058-f064-59e8-b73203f3e400@gmail.com","subject":"Re: [PATCH v2 2/2] mailinfo: unescape quoted-pair in header fields","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-26T05:02:30Z","receivedAt":"2016-09-26T05:02:36Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Mon, Sep 26, 2016 at 12:38:42AM +0200, Jakub Narębski wrote:\n> W dniu 25.09.2016 o 22:17, Kevin Daudt pisze:\n> > On Fri, Sep 23, 2016 at 12:15:41AM -0400, Jeff King wrote:\n> \n> >> Oops, yes. It is beginning to make the \"strbuf_swap()\" look less\n> >> convoluted. :)\n> >>\n> > \n> > I've switched to strbuf_swap now, much better. I've implemented\n> > recursive parsing without looking at what you provided, just to see what\n> > I'd came up with. Though I've not implemented a recursive descent\n> > parser, but it might suffice.\n> \n> I think you can implement a parser handling proper nesting of parens\n> without recursion.\n> \n> Though... what is the definition in the RFC?\n\nThis part describes comments.\n\n    ccontent        =       ctext / quoted-pair / comment\n\n    comment         =       \"(\" *([FWS] ccontent) [FWS] \")\"\n\n    CFWS            =       *([FWS] comment) (([FWS] comment) / FWS)\n\nSo each comment can itself also contain a comment.\n\nThis could be done without recursion by keeping a count of how many open\nparens we have encountered.\n\nKevin\n"},{"id":"302615","messageId":"xmqqd1jqscp7.fsf@gitster.mtv.corp.google.com","threadId":"44099","inReplyTo":"20160925210808.26424-1-me@ikke.info","subject":"Re: [PATCH v3 1/2] t5100-mailinfo: replace common path prefix with variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T19:06:28Z","receivedAt":"2016-09-26T19:06:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Daudt <me@ikke.info> writes:\n\n> Many tests need to store data in a file, and repeat the same pattern to\n> refer to that path:\n>\n>     \"$TEST_DIRECTORY\"/t5100/\n>\n> Create a variable that contains this path, and use that instead.\n>\n> Signed-off-by: Kevin Daudt <me@ikke.info>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  Changes since v2:\n>  - changed $DATA to $data to indicate it's a script-local variable\n\nIf you are rerolling anyway, I would have liked to see the \"why is\nonly the variable part quoted?\"  issue addressed which was raised\nduring the previous round of the review.  I may have said it is OK\nto leave it as a low-hanging fruit for others but that only meant\nthat it alone is not a strong enough reason to reroll this patch.\n\nOther than that, looks good to me, though ;-)\n\n"},{"id":"302617","messageId":"xmqq4m52scg7.fsf@gitster.mtv.corp.google.com","threadId":"44099","inReplyTo":"20160925210808.26424-2-me@ikke.info","subject":"Re: [PATCH v3 2/2] mailinfo: unescape quoted-pair in header fields","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T19:11:52Z","receivedAt":"2016-09-26T19:12:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Daudt <me@ikke.info> writes:\n\n> rfc2822 has provisions for quoted strings and comments in structured header\n> fields, but also allows for escaping these with so-called quoted-pairs.\n>\n> The only thing git currently does is removing exterior quotes, but\n> quotes within are left alone.\n>\n> Remove exterior quotes and remove escape characters so that they don't\n> show up in the author field.\n>\n> Signed-off-by: Kevin Daudt <me@ikke.info>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  Changes since v2:\n>\n>  - handle comments inside comments recursively\n>  - renamed the main function to unquote_quoted_pairs because it also\n>    handles quoted pairs in comments\n\nSounds good, and the implemention looked straight-forward from a\nquick scan.\n\n> diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\n> index c4ed0f4..3e983c0 100755\n> --- a/t/t5100-mailinfo.sh\n> +++ b/t/t5100-mailinfo.sh\n> @@ -144,4 +144,18 @@ test_expect_success 'mailinfo unescapes with --mboxrd' '\n>  \ttest_cmp expect mboxrd/msg\n>  '\n>  \n> +test_expect_success 'mailinfo handles rfc2822 quoted-string' '\n> +\tmkdir quoted-string &&\n> +\tgit mailinfo /dev/null /dev/null <\"$DATA\"/quoted-string.in \\\n> +\t\t>quoted-string/info &&\n> +\ttest_cmp \"$DATA\"/quoted-string.expect quoted-string/info\n> +'\n> +\n> +test_expect_success 'mailinfo handles rfc2822 comment' '\n> +\tmkdir comment &&\n> +\tgit mailinfo /dev/null /dev/null <\"$DATA\"/comment.in \\\n> +\t\t>comment/info &&\n> +\ttest_cmp \"$DATA\"/comment.expect comment/info\n> +'\n> +\n>  test_done\n\nDon't these also need to be downcased if you prefer $data over\n$DATA, though?\n\nThanks.\n"},{"id":"302619","messageId":"xmqqzimuqx7u.fsf@gitster.mtv.corp.google.com","threadId":"44099","inReplyTo":"xmqq4m52scg7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/2] mailinfo: unescape quoted-pair in header fields","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T19:26:13Z","receivedAt":"2016-09-26T19:27:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Don't these also need to be downcased if you prefer $data over\n> $DATA, though?\n\nFor now, I'll queue a SQUASH??? that reverts s/DATA/data/ you did to\n1/2 between your 1/2 and 2/2.\n\nThanks.\n"},{"id":"302626","messageId":"20160926194455.GB19089@ikke.info","threadId":"44099","inReplyTo":"xmqqzimuqx7u.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/2] mailinfo: unescape quoted-pair in header fields","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-26T19:44:55Z","receivedAt":"2016-09-26T19:45:02Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Mon, Sep 26, 2016 at 12:26:13PM -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Don't these also need to be downcased if you prefer $data over\n> > $DATA, though?\n> \n> For now, I'll queue a SQUASH??? that reverts s/DATA/data/ you did to\n> 1/2 between your 1/2 and 2/2.\n> \n\nUgh, thanks. I'd replaced it in the first patch, but forgot it in the\nsecond.\n"},{"id":"302647","messageId":"xmqq7f9ypag4.fsf@gitster.mtv.corp.google.com","threadId":"44099","inReplyTo":"20160926194455.GB19089@ikke.info","subject":"Re: [PATCH v3 2/2] mailinfo: unescape quoted-pair in header fields","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T22:23:23Z","receivedAt":"2016-09-26T22:23:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Daudt <me@ikke.info> writes:\n\n> On Mon, Sep 26, 2016 at 12:26:13PM -0700, Junio C Hamano wrote:\n>> Junio C Hamano <gitster@pobox.com> writes:\n>> \n>> > Don't these also need to be downcased if you prefer $data over\n>> > $DATA, though?\n>> \n>> For now, I'll queue a SQUASH??? that reverts s/DATA/data/ you did to\n>> 1/2 between your 1/2 and 2/2.\n>\n> Ugh, thanks. I'd replaced it in the first patch, but forgot it in the\n> second.\n\nHeh, I already guessed that much that these were sent without even\nbe in the tree, after editing only the patch files.  Don't do that\n;-)\n\nI am not sure I agree that $data is better over $DATA, though.\nUnlike the lowercase $mail and others used in the script that are\nclearly \"variables\", this thing is used as a constant during the\nlifetime of the test script.\n\n\n"},{"id":"302692","messageId":"20160927102655.GA22520@ikke.info","threadId":"44099","inReplyTo":"xmqq7f9ypag4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/2] mailinfo: unescape quoted-pair in header fields","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-27T10:26:55Z","receivedAt":"2016-09-27T10:27:04Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Mon, Sep 26, 2016 at 03:23:23PM -0700, Junio C Hamano wrote:\n> Kevin Daudt <me@ikke.info> writes:\n> \n> > On Mon, Sep 26, 2016 at 12:26:13PM -0700, Junio C Hamano wrote:\n> >> Junio C Hamano <gitster@pobox.com> writes:\n> >> \n> >> > Don't these also need to be downcased if you prefer $data over\n> >> > $DATA, though?\n> >> \n> >> For now, I'll queue a SQUASH??? that reverts s/DATA/data/ you did to\n> >> 1/2 between your 1/2 and 2/2.\n> >\n> > Ugh, thanks. I'd replaced it in the first patch, but forgot it in the\n> > second.\n> \n> Heh, I already guessed that much that these were sent without even\n> be in the tree, after editing only the patch files.  Don't do that\n> ;-)\n\nThat was just my poor wording. I did a proper rebase, but only changed\nit for the first commit.\n\n\n\n"},{"id":"302830","messageId":"20160928194939.7706-1-me@ikke.info","threadId":"44099","inReplyTo":"20160925210808.26424-1-me@ikke.info","subject":"[PATCH v4 0/2] Handle RFC2822 quoted-pairs in From header","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-28T19:49:37Z","receivedAt":"2016-09-28T19:49:52Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"Changes since v3:\n- t5100-mailinfo: Reverted back to capital $DATA\n- t5100-mailinfo: Moved quotes to around the entire string, instead of the\n  variable, as per Junio's suggestion\n\n\nKevin Daudt (2):\n  t5100-mailinfo: replace common path prefix with variable\n  mailinfo: unescape quoted-pair in header fields\n\n mailinfo.c                   | 82 ++++++++++++++++++++++++++++++++++++++++++++\n t/t5100-mailinfo.sh          | 82 ++++++++++++++++++++++++++------------------\n t/t5100/comment.expect       |  5 +++\n t/t5100/comment.in           |  9 +++++\n t/t5100/quoted-string.expect |  5 +++\n t/t5100/quoted-string.in     |  9 +++++\n 6 files changed, 159 insertions(+), 33 deletions(-)\n create mode 100644 t/t5100/comment.expect\n create mode 100644 t/t5100/comment.in\n create mode 100644 t/t5100/quoted-string.expect\n create mode 100644 t/t5100/quoted-string.in\n\n-- \n2.10.0.372.g6fe1b14\n\n"},{"id":"302831","messageId":"20160928195232.7843-1-me@ikke.info","threadId":"44099","inReplyTo":"20160928194939.7706-1-me@ikke.info","subject":"[PATCH v4 1/2] t5100-mailinfo: replace common path prefix with variable","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-28T19:52:31Z","receivedAt":"2016-09-28T19:52:45Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"Many tests need to store data in a file, and repeat the same pattern to\nrefer to that path:\n\n    \"$TEST_DIRECTORY\"/t5100/\n\nCreate a variable that contains this path, and use that instead.\n\nWhile we're making this change, make sure the quotes are not just around\nthe variable, but around the entire string to not give the impression\nwe want shell splitting to affect the other variables.\n\nSigned-off-by: Kevin Daudt <me@ikke.info>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t5100-mailinfo.sh | 68 +++++++++++++++++++++++++++--------------------------\n 1 file changed, 35 insertions(+), 33 deletions(-)\n\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex 1a5a546..56988b7 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -7,8 +7,10 @@ test_description='git mailinfo and git mailsplit test'\n \n . ./test-lib.sh\n \n+DATA=\"$TEST_DIRECTORY/t5100\"\n+\n test_expect_success 'split sample box' \\\n-\t'git mailsplit -o. \"$TEST_DIRECTORY\"/t5100/sample.mbox >last &&\n+\t'git mailsplit -o. \"$DATA/sample.mbox\" >last &&\n \tlast=$(cat last) &&\n \techo total is $last &&\n \ttest $(cat last) = 17'\n@@ -16,28 +18,28 @@ test_expect_success 'split sample box' \\\n check_mailinfo () {\n \tmail=$1 opt=$2\n \tmo=\"$mail$opt\"\n-\tgit mailinfo -u $opt msg$mo patch$mo <$mail >info$mo &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/msg$mo msg$mo &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/patch$mo patch$mo &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/info$mo info$mo\n+\tgit mailinfo -u $opt \"msg$mo\" \"patch$mo\" <\"$mail\" >\"info$mo\" &&\n+\ttest_cmp \"$DATA/msg$mo\" \"msg$mo\" &&\n+\ttest_cmp \"$DATA/patch$mo\" \"patch$mo\" &&\n+\ttest_cmp \"$DATA/info$mo\" \"info$mo\"\n }\n \n \n for mail in 00*\n do\n \ttest_expect_success \"mailinfo $mail\" '\n-\t\tcheck_mailinfo $mail \"\" &&\n-\t\tif test -f \"$TEST_DIRECTORY\"/t5100/msg$mail--scissors\n+\t\tcheck_mailinfo \"$mail\" \"\" &&\n+\t\tif test -f \"$DATA/msg$mail--scissors\"\n \t\tthen\n-\t\t\tcheck_mailinfo $mail --scissors\n+\t\t\tcheck_mailinfo \"$mail\" --scissors\n \t\tfi &&\n-\t\tif test -f \"$TEST_DIRECTORY\"/t5100/msg$mail--no-inbody-headers\n+\t\tif test -f \"$DATA/msg$mail--no-inbody-headers\"\n \t\tthen\n-\t\t\tcheck_mailinfo $mail --no-inbody-headers\n+\t\t\tcheck_mailinfo \"$mail\" --no-inbody-headers\n \t\tfi &&\n-\t\tif test -f \"$TEST_DIRECTORY\"/t5100/msg$mail--message-id\n+\t\tif test -f \"$DATA/msg$mail--message-id\"\n \t\tthen\n-\t\t\tcheck_mailinfo $mail --message-id\n+\t\t\tcheck_mailinfo \"$mail\" --message-id\n \t\tfi\n \t'\n done\n@@ -45,7 +47,7 @@ done\n \n test_expect_success 'split box with rfc2047 samples' \\\n \t'mkdir rfc2047 &&\n-\tgit mailsplit -orfc2047 \"$TEST_DIRECTORY\"/t5100/rfc2047-samples.mbox \\\n+\tgit mailsplit -orfc2047 \"$DATA/rfc2047-samples.mbox\" \\\n \t  >rfc2047/last &&\n \tlast=$(cat rfc2047/last) &&\n \techo total is $last &&\n@@ -54,20 +56,20 @@ test_expect_success 'split box with rfc2047 samples' \\\n for mail in rfc2047/00*\n do\n \ttest_expect_success \"mailinfo $mail\" '\n-\t\tgit mailinfo -u $mail-msg $mail-patch <$mail >$mail-info &&\n+\t\tgit mailinfo -u \"$mail-msg\" \"$mail-patch\" <\"$mail\" >\"$mail-info\" &&\n \t\techo msg &&\n-\t\ttest_cmp \"$TEST_DIRECTORY\"/t5100/empty $mail-msg &&\n+\t\ttest_cmp \"$DATA/empty\" \"$mail-msg\" &&\n \t\techo patch &&\n-\t\ttest_cmp \"$TEST_DIRECTORY\"/t5100/empty $mail-patch &&\n+\t\ttest_cmp \"$DATA/empty\" \"$mail-patch\" &&\n \t\techo info &&\n-\t\ttest_cmp \"$TEST_DIRECTORY\"/t5100/rfc2047-info-$(basename $mail) $mail-info\n+\t\ttest_cmp \"$DATA/rfc2047-info-$(basename $mail)\" \"$mail-info\"\n \t'\n done\n \n test_expect_success 'respect NULs' '\n \n-\tgit mailsplit -d3 -o. \"$TEST_DIRECTORY\"/t5100/nul-plain &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/nul-plain 001 &&\n+\tgit mailsplit -d3 -o. \"$DATA/nul-plain\" &&\n+\ttest_cmp \"$DATA/nul-plain\" 001 &&\n \t(cat 001 | git mailinfo msg patch) &&\n \ttest_line_count = 4 patch\n \n@@ -75,52 +77,52 @@ test_expect_success 'respect NULs' '\n \n test_expect_success 'Preserve NULs out of MIME encoded message' '\n \n-\tgit mailsplit -d5 -o. \"$TEST_DIRECTORY\"/t5100/nul-b64.in &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/nul-b64.in 00001 &&\n+\tgit mailsplit -d5 -o. \"$DATA/nul-b64.in\" &&\n+\ttest_cmp \"$DATA/nul-b64.in\" 00001 &&\n \tgit mailinfo msg patch <00001 &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/nul-b64.expect patch\n+\ttest_cmp \"$DATA/nul-b64.expect\" patch\n \n '\n \n test_expect_success 'mailinfo on from header without name works' '\n \n \tmkdir info-from &&\n-\tgit mailsplit -oinfo-from \"$TEST_DIRECTORY\"/t5100/info-from.in &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/info-from.in info-from/0001 &&\n+\tgit mailsplit -oinfo-from \"$DATA/info-from.in\" &&\n+\ttest_cmp \"$DATA/info-from.in\" info-from/0001 &&\n \tgit mailinfo info-from/msg info-from/patch \\\n \t  <info-from/0001 >info-from/out &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/info-from.expect info-from/out\n+\ttest_cmp \"$DATA/info-from.expect\" info-from/out\n \n '\n \n test_expect_success 'mailinfo finds headers after embedded From line' '\n \tmkdir embed-from &&\n-\tgit mailsplit -oembed-from \"$TEST_DIRECTORY\"/t5100/embed-from.in &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/embed-from.in embed-from/0001 &&\n+\tgit mailsplit -oembed-from \"$DATA/embed-from.in\" &&\n+\ttest_cmp \"$DATA/embed-from.in\" embed-from/0001 &&\n \tgit mailinfo embed-from/msg embed-from/patch \\\n \t  <embed-from/0001 >embed-from/out &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/embed-from.expect embed-from/out\n+\ttest_cmp \"$DATA/embed-from.expect\" embed-from/out\n '\n \n test_expect_success 'mailinfo on message with quoted >From' '\n \tmkdir quoted-from &&\n-\tgit mailsplit -oquoted-from \"$TEST_DIRECTORY\"/t5100/quoted-from.in &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/quoted-from.in quoted-from/0001 &&\n+\tgit mailsplit -oquoted-from \"$DATA/quoted-from.in\" &&\n+\ttest_cmp \"$DATA/quoted-from.in\" quoted-from/0001 &&\n \tgit mailinfo quoted-from/msg quoted-from/patch \\\n \t  <quoted-from/0001 >quoted-from/out &&\n-\ttest_cmp \"$TEST_DIRECTORY\"/t5100/quoted-from.expect quoted-from/msg\n+\ttest_cmp \"$DATA/quoted-from.expect\" quoted-from/msg\n '\n \n test_expect_success 'mailinfo unescapes with --mboxrd' '\n \tmkdir mboxrd &&\n \tgit mailsplit -omboxrd --mboxrd \\\n-\t\t\"$TEST_DIRECTORY\"/t5100/sample.mboxrd >last &&\n+\t\t\"$DATA/sample.mboxrd\" >last &&\n \ttest x\"$(cat last)\" = x2 &&\n \tfor i in 0001 0002\n \tdo\n \t\tgit mailinfo mboxrd/msg mboxrd/patch \\\n \t\t  <mboxrd/$i >mboxrd/out &&\n-\t\ttest_cmp \"$TEST_DIRECTORY\"/t5100/${i}mboxrd mboxrd/msg\n+\t\ttest_cmp \"$DATA/${i}mboxrd\" mboxrd/msg\n \tdone &&\n \tsp=\" \" &&\n \techo \"From \" >expect &&\n-- \n2.10.0.372.g6fe1b14\n\n"},{"id":"302832","messageId":"20160928195232.7843-2-me@ikke.info","threadId":"44099","inReplyTo":"20160928194939.7706-1-me@ikke.info","subject":"[PATCH v4 2/2] mailinfo: unescape quoted-pair in header fields","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-28T19:52:32Z","receivedAt":"2016-09-28T19:52:49Z","isPatch":true,"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\nThe only thing git currently does is removing exterior quotes, but\nquotes within are left alone.\n\nRemove exterior quotes and remove escape characters so that they don't\nshow up in the author field.\n\nSigned-off-by: Kevin Daudt <me@ikke.info>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n mailinfo.c                   | 82 ++++++++++++++++++++++++++++++++++++++++++++\n t/t5100-mailinfo.sh          | 14 ++++++++\n t/t5100/comment.expect       |  5 +++\n t/t5100/comment.in           |  9 +++++\n t/t5100/quoted-string.expect |  5 +++\n t/t5100/quoted-string.in     |  9 +++++\n 6 files changed, 124 insertions(+)\n create mode 100644 t/t5100/comment.expect\n create mode 100644 t/t5100/comment.in\n create mode 100644 t/t5100/quoted-string.expect\n create mode 100644 t/t5100/quoted-string.in\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex e19abe3..b4118a0 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -54,6 +54,86 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line)\n \tget_sane_name(&mi->name, &mi->name, &mi->email);\n }\n \n+static const char *unquote_comment(struct strbuf *outbuf, const char *in)\n+{\n+\tint c;\n+\tint take_next_litterally = 0;\n+\n+\tstrbuf_addch(outbuf, '(');\n+\n+\twhile ((c = *in++) != 0) {\n+\t\tif (take_next_litterally == 1) {\n+\t\t\ttake_next_litterally = 0;\n+\t\t} else {\n+\t\t\tswitch (c) {\n+\t\t\tcase '\\\\':\n+\t\t\t\ttake_next_litterally = 1;\n+\t\t\t\tcontinue;\n+\t\t\tcase '(':\n+\t\t\t\tin = unquote_comment(outbuf, in);\n+\t\t\t\tcontinue;\n+\t\t\tcase ')':\n+\t\t\t\tstrbuf_addch(outbuf, ')');\n+\t\t\t\treturn in;\n+\t\t\t}\n+\t\t}\n+\n+\t\tstrbuf_addch(outbuf, c);\n+\t}\n+\n+\treturn in;\n+}\n+\n+static const char *unquote_quoted_string(struct strbuf *outbuf, const char *in)\n+{\n+\tint c;\n+\tint take_next_litterally = 0;\n+\n+\twhile ((c = *in++) != 0) {\n+\t\tif (take_next_litterally == 1) {\n+\t\t\ttake_next_litterally = 0;\n+\t\t} else {\n+\t\t\tswitch (c) {\n+\t\t\tcase '\\\\':\n+\t\t\t\ttake_next_litterally = 1;\n+\t\t\t\tcontinue;\n+\t\t\tcase '\"':\n+\t\t\t\treturn in;\n+\t\t\t}\n+\t\t}\n+\n+\t\tstrbuf_addch(outbuf, c);\n+\t}\n+\n+\treturn in;\n+}\n+\n+static void unquote_quoted_pair(struct strbuf *line)\n+{\n+\tstruct strbuf outbuf;\n+\tconst char *in = line->buf;\n+\tint c;\n+\n+\tstrbuf_init(&outbuf, line->len);\n+\n+\twhile ((c = *in++) != 0) {\n+\t\tswitch (c) {\n+\t\tcase '\"':\n+\t\t\tin = unquote_quoted_string(&outbuf, in);\n+\t\t\tcontinue;\n+\t\tcase '(':\n+\t\t\tin = unquote_comment(&outbuf, in);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tstrbuf_addch(&outbuf, c);\n+\t}\n+\n+\tstrbuf_swap(&outbuf, line);\n+\tstrbuf_release(&outbuf);\n+\n+}\n+\n static void handle_from(struct mailinfo *mi, const struct strbuf *from)\n {\n \tchar *at;\n@@ -63,6 +143,8 @@ static void handle_from(struct mailinfo *mi, const struct strbuf *from)\n \tstrbuf_init(&f, from->len);\n \tstrbuf_addbuf(&f, from);\n \n+\tunquote_quoted_pair(&f);\n+\n \tat = strchr(f.buf, '@');\n \tif (!at) {\n \t\tparse_bogus_from(mi, from);\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex 56988b7..45d228e 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -144,4 +144,18 @@ test_expect_success 'mailinfo unescapes with --mboxrd' '\n \ttest_cmp expect mboxrd/msg\n '\n \n+test_expect_success 'mailinfo handles rfc2822 quoted-string' '\n+\tmkdir quoted-string &&\n+\tgit mailinfo /dev/null /dev/null <\"$DATA/quoted-string.in\" \\\n+\t\t>quoted-string/info &&\n+\ttest_cmp \"$DATA/quoted-string.expect\" quoted-string/info\n+'\n+\n+test_expect_success 'mailinfo handles rfc2822 comment' '\n+\tmkdir comment &&\n+\tgit mailinfo /dev/null /dev/null <\"$DATA/comment.in\" \\\n+\t\t>comment/info &&\n+\ttest_cmp \"$DATA/comment.expect\" comment/info\n+'\n+\n test_done\ndiff --git a/t/t5100/comment.expect b/t/t5100/comment.expect\nnew file mode 100644\nindex 0000000..7228177\n--- /dev/null\n+++ b/t/t5100/comment.expect\n@@ -0,0 +1,5 @@\n+Author: A U Thor (this is (really) a comment (honestly))\n+Email: somebody@example.com\n+Subject: testing comments\n+Date: Sun, 25 May 2008 00:38:18 -0700\n+\ndiff --git a/t/t5100/comment.in b/t/t5100/comment.in\nnew file mode 100644\nindex 0000000..c53a192\n--- /dev/null\n+++ b/t/t5100/comment.in\n@@ -0,0 +1,9 @@\n+From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001\n+From: \"A U Thor\" <somebody@example.com> (this is \\(really\\) a comment (honestly))\n+Date: Sun, 25 May 2008 00:38:18 -0700\n+Subject: [PATCH] testing comments\n+\n+\n+\n+---\n+patch\ndiff --git a/t/t5100/quoted-string.expect b/t/t5100/quoted-string.expect\nnew file mode 100644\nindex 0000000..cab1bce\n--- /dev/null\n+++ b/t/t5100/quoted-string.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-string.in b/t/t5100/quoted-string.in\nnew file mode 100644\nindex 0000000..e2e627a\n--- /dev/null\n+++ b/t/t5100/quoted-string.in\n@@ -0,0 +1,9 @@\n+From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001\n+From: \"Author \\\"The Author\\\" Name\" <somebody@example.com>\n+Date: Sun, 25 May 2008 00:38:18 -0700\n+Subject: [PATCH] testing quoted-pair\n+\n+\n+\n+---\n+patch\n-- \n2.10.0.372.g6fe1b14\n\n"},{"id":"302834","messageId":"xmqqoa37ixmu.fsf@gitster.mtv.corp.google.com","threadId":"44099","inReplyTo":"20160928195232.7843-1-me@ikke.info","subject":"Re: [PATCH v4 1/2] t5100-mailinfo: replace common path prefix with variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-28T20:21:13Z","receivedAt":"2016-09-28T20:21:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Daudt <me@ikke.info> writes:\n\n> Many tests need to store data in a file, and repeat the same pattern to\n> refer to that path:\n>\n>     \"$TEST_DIRECTORY\"/t5100/\n>\n> Create a variable that contains this path, and use that instead.\n>\n> While we're making this change, make sure the quotes are not just around\n> the variable, but around the entire string to not give the impression\n> we want shell splitting to affect the other variables.\n\nWow.  I was half expecting that you'd say something like \"1/2 plus\nthe SQUASH is OK by me\", but you went extra mile to do it right.\n\nImpressed, and very much appreciated.\n\n"},{"id":"302835","messageId":"20160928202739.GB22520@ikke.info","threadId":"44099","inReplyTo":"xmqqoa37ixmu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 1/2] t5100-mailinfo: replace common path prefix with variable","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2016-09-28T20:27:39Z","receivedAt":"2016-09-28T20:27:45Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Wed, Sep 28, 2016 at 01:21:13PM -0700, Junio C Hamano wrote:\n> Kevin Daudt <me@ikke.info> writes:\n> \n> > Many tests need to store data in a file, and repeat the same pattern to\n> > refer to that path:\n> >\n> >     \"$TEST_DIRECTORY\"/t5100/\n> >\n> > Create a variable that contains this path, and use that instead.\n> >\n> > While we're making this change, make sure the quotes are not just around\n> > the variable, but around the entire string to not give the impression\n> > we want shell splitting to affect the other variables.\n> \n> Wow.  I was half expecting that you'd say something like \"1/2 plus\n> the SQUASH is OK by me\", but you went extra mile to do it right.\n> \n> Impressed, and very much appreciated.\n> \n\nYou're What's Cooking mail mentioned you expected a reroll, so I guessed\nthat I could just fix this part as well.\n"}]}