{"thread":{"id":"38032","subject":"[PATCH] send-email: handle adjacent RFC 2047-encoded words properly","startedAt":"2014-11-23T23:50:04Z","lastAt":"2014-11-24T23:03:47Z","messageCount":7,"participants":["Роман Донченко","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"252413","messageId":"1416786604-4988-1-git-send-email-dpb@corrigendum.ru","threadId":"38032","inReplyTo":null,"subject":"[PATCH] send-email: handle adjacent RFC 2047-encoded words properly","fromName":"Роман Донченко","fromEmail":"dpb@corrigendum.ru","sentAt":"2014-11-23T23:50:04Z","receivedAt":"2014-11-23T23:50:04Z","isPatch":true,"sender":{"key":"dpb@corrigendum.ru","avatar":"https://avatars.githubusercontent.com/u/2391761?v=4"},"body":"The RFC says that they are to be concatenated after decoding (i.e. the\nintervening whitespace is ignored).\n\nI change the sender's name to an all-Cyrillic string in the tests so that\nits encoded form goes over the 76 characters in a line limit, forcing\nformat-patch to split it into multiple encoded words.\n\nSince I have to modify the regular expression for an encoded word anyway,\nI take the opportunity to bring it closer to the spec, most notably\ndisallowing embedded spaces and making it case-insensitive (thus allowing\nthe encoding to be specified as both \"q\" and \"Q\").\n\nSigned-off-by: Роман Донченко <dpb@corrigendum.ru>\n---\n git-send-email.perl   | 21 +++++++++++++++------\n t/t9001-send-email.sh | 18 +++++++++---------\n 2 files changed, 24 insertions(+), 15 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 9949db0..4bb9f6f 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -913,13 +913,22 @@ $time = time - scalar $#files;\n \n sub unquote_rfc2047 {\n \tlocal ($_) = @_;\n+\n+\tmy $et = qr/[!->@-~]+/; # encoded-text from RFC 2047\n+\tmy $sep = qr/[ \\t]+/;\n+\tmy $encoded_word = qr/=\\?($et)\\?q\\?($et)\\?=/i;\n+\n \tmy $encoding;\n-\ts{=\\?([^?]+)\\?q\\?(.*?)\\?=}{\n-\t\t$encoding = $1;\n-\t\tmy $e = $2;\n-\t\t$e =~ s/_/ /g;\n-\t\t$e =~ s/=([0-9A-F]{2})/chr(hex($1))/eg;\n-\t\t$e;\n+\ts{$encoded_word(?:$sep$encoded_word)+}{\n+\t\tmy @words = split $sep, $&;\n+\t\tforeach (@words) {\n+\t\t\tm/$encoded_word/;\n+\t\t\t$encoding = $1;\n+\t\t\t$_ = $2;\n+\t\t\ts/_/ /g;\n+\t\t\ts/=([0-9A-F]{2})/chr(hex($1))/eg;\n+\t\t}\n+\t\tjoin '', @words;\n \t}eg;\n \treturn wantarray ? ($_, $encoding) : $_;\n }\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 19a3ced..318b870 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -236,7 +236,7 @@ test_expect_success $PREREQ 'self name with dot is suppressed' \"\n \"\n \n test_expect_success $PREREQ 'non-ascii self name is suppressed' \"\n-\ttest_suppress_self_quoted 'Füñný Nâmé' 'odd_?=mail@example.com' \\\n+\ttest_suppress_self_quoted 'Кириллическое Имя' 'odd_?=mail@example.com' \\\n \t\t'non_ascii_self_suppressed'\n \"\n \n@@ -946,25 +946,25 @@ test_expect_success $PREREQ 'utf8 author is correctly passed on' '\n \tclean_fake_sendmail &&\n \ttest_commit weird_author &&\n \ttest_when_finished \"git reset --hard HEAD^\" &&\n-\tgit commit --amend --author \"Füñný Nâmé <odd_?=mail@example.com>\" &&\n-\tgit format-patch --stdout -1 >funny_name.patch &&\n+\tgit commit --amend --author \"Кириллическое Имя <odd_?=mail@example.com>\" &&\n+\tgit format-patch --stdout -1 >nonascii_name.patch &&\n \tgit send-email --from=\"Example <nobody@example.com>\" \\\n \t  --to=nobody@example.com \\\n \t  --smtp-server=\"$(pwd)/fake.sendmail\" \\\n-\t  funny_name.patch &&\n-\tgrep \"^From: Füñný Nâmé <odd_?=mail@example.com>\" msgtxt1\n+\t  nonascii_name.patch &&\n+\tgrep \"^From: Кириллическое Имя <odd_?=mail@example.com>\" msgtxt1\n '\n \n test_expect_success $PREREQ 'utf8 sender is not duplicated' '\n \tclean_fake_sendmail &&\n \ttest_commit weird_sender &&\n \ttest_when_finished \"git reset --hard HEAD^\" &&\n-\tgit commit --amend --author \"Füñný Nâmé <odd_?=mail@example.com>\" &&\n-\tgit format-patch --stdout -1 >funny_name.patch &&\n-\tgit send-email --from=\"Füñný Nâmé <odd_?=mail@example.com>\" \\\n+\tgit commit --amend --author \"Кириллическое Имя <odd_?=mail@example.com>\" &&\n+\tgit format-patch --stdout -1 >nonascii_name.patch &&\n+\tgit send-email --from=\"Кириллическое Имя <odd_?=mail@example.com>\" \\\n \t  --to=nobody@example.com \\\n \t  --smtp-server=\"$(pwd)/fake.sendmail\" \\\n-\t  funny_name.patch &&\n+\t  nonascii_name.patch &&\n \tgrep \"^From: \" msgtxt1 >msgfrom &&\n \ttest_line_count = 1 msgfrom\n '\n-- \n2.1.1\n"},{"id":"252418","messageId":"CAPc5daVjNDg5CcWsMwfn=DZhwpCBdU2LYXOpFWZwhx2p8hLRww@mail.gmail.com","threadId":"38032","inReplyTo":"1416786604-4988-1-git-send-email-dpb@corrigendum.ru","subject":"Re: [PATCH] send-email: handle adjacent RFC 2047-encoded words properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-24T07:27:51Z","receivedAt":"2014-11-24T07:27:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Sun, Nov 23, 2014 at 3:50 PM, Роман Донченко <dpb@corrigendum.ru> wrote:\n> The RFC says that they are to be concatenated after decoding (i.e. the\n> intervening whitespace is ignored).\n>\n> I change the sender's name to an all-Cyrillic string in the tests so that\n> its encoded form goes over the 76 characters in a line limit, forcing\n> format-patch to split it into multiple encoded words.\n>\n> Since I have to modify the regular expression for an encoded word anyway,\n> I take the opportunity to bring it closer to the spec, most notably\n> disallowing embedded spaces and making it case-insensitive (thus allowing\n> the encoding to be specified as both \"q\" and \"Q\").\n>\n> Signed-off-by: Роман Донченко <dpb@corrigendum.ru>\n\nThis sounds like a worthy thing to do in general.\n\nI wonder if the C implementation we have for mailinfo needs similar\nupdate, though. I vaguely recall that we have case-insensitive start for\nq/b segments, but do not remember the details offhand.\n\nWas the change to the test to use Cyrillic really necessary, or did it\nsuffice if you simply extended the existsing \"Funny Name\" spelled with\nstrange accents, but you substituted the whole string anyway?\n\nUntil I found out what the new string says by running web-based\ntranslation on it, I felt somewhat uneasy. As I do not read\nCyrillic/Russian, we may have been adding some profanity without\nknowing. It turns out that the string just says \"Cyrillic Name\", so I am\nnot against using the new string, but it simply looked odd to replace the\nstring whole-sale when you merely need a longer string. It made it look\nas if a bug was specific to Cyrillic when it wasn't.\n\nAs you may notice by reading \"git log --no-merges\" from recent history,\nwe tend not to say \"I did X, I did Y\". If the tone of the above message\nwere more similar to them, it may have been easier to read.\n\nBut other than these minor nits, the change looks good from\na cursory read.\n\nThanks.\n\n> ---\n>  git-send-email.perl   | 21 +++++++++++++++------\n>  t/t9001-send-email.sh | 18 +++++++++---------\n>  2 files changed, 24 insertions(+), 15 deletions(-)\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 9949db0..4bb9f6f 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -913,13 +913,22 @@ $time = time - scalar $#files;\n>\n>  sub unquote_rfc2047 {\n>         local ($_) = @_;\n> +\n> +       my $et = qr/[!->@-~]+/; # encoded-text from RFC 2047\n> +       my $sep = qr/[ \\t]+/;\n> +       my $encoded_word = qr/=\\?($et)\\?q\\?($et)\\?=/i;\n> +\n>         my $encoding;\n> -       s{=\\?([^?]+)\\?q\\?(.*?)\\?=}{\n> -               $encoding = $1;\n> -               my $e = $2;\n> -               $e =~ s/_/ /g;\n> -               $e =~ s/=([0-9A-F]{2})/chr(hex($1))/eg;\n> -               $e;\n> +       s{$encoded_word(?:$sep$encoded_word)+}{\n> +               my @words = split $sep, $&;\n> +               foreach (@words) {\n> +                       m/$encoded_word/;\n> +                       $encoding = $1;\n> +                       $_ = $2;\n> +                       s/_/ /g;\n> +                       s/=([0-9A-F]{2})/chr(hex($1))/eg;\n> +               }\n> +               join '', @words;\n>         }eg;\n>         return wantarray ? ($_, $encoding) : $_;\n>  }\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> index 19a3ced..318b870 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -236,7 +236,7 @@ test_expect_success $PREREQ 'self name with dot is suppressed' \"\n>  \"\n>\n>  test_expect_success $PREREQ 'non-ascii self name is suppressed' \"\n> -       test_suppress_self_quoted 'Füñný Nâmé' 'odd_?=mail@example.com' \\\n> +       test_suppress_self_quoted 'Кириллическое Имя' 'odd_?=mail@example.com' \\\n>                 'non_ascii_self_suppressed'\n>  \"\n>\n> @@ -946,25 +946,25 @@ test_expect_success $PREREQ 'utf8 author is correctly passed on' '\n>         clean_fake_sendmail &&\n>         test_commit weird_author &&\n>         test_when_finished \"git reset --hard HEAD^\" &&\n> -       git commit --amend --author \"Füñný Nâmé <odd_?=mail@example.com>\" &&\n> -       git format-patch --stdout -1 >funny_name.patch &&\n> +       git commit --amend --author \"Кириллическое Имя <odd_?=mail@example.com>\" &&\n> +       git format-patch --stdout -1 >nonascii_name.patch &&\n>         git send-email --from=\"Example <nobody@example.com>\" \\\n>           --to=nobody@example.com \\\n>           --smtp-server=\"$(pwd)/fake.sendmail\" \\\n> -         funny_name.patch &&\n> -       grep \"^From: Füñný Nâmé <odd_?=mail@example.com>\" msgtxt1\n> +         nonascii_name.patch &&\n> +       grep \"^From: Кириллическое Имя <odd_?=mail@example.com>\" msgtxt1\n>  '\n>\n>  test_expect_success $PREREQ 'utf8 sender is not duplicated' '\n>         clean_fake_sendmail &&\n>         test_commit weird_sender &&\n>         test_when_finished \"git reset --hard HEAD^\" &&\n> -       git commit --amend --author \"Füñný Nâmé <odd_?=mail@example.com>\" &&\n> -       git format-patch --stdout -1 >funny_name.patch &&\n> -       git send-email --from=\"Füñný Nâmé <odd_?=mail@example.com>\" \\\n> +       git commit --amend --author \"Кириллическое Имя <odd_?=mail@example.com>\" &&\n> +       git format-patch --stdout -1 >nonascii_name.patch &&\n> +       git send-email --from=\"Кириллическое Имя <odd_?=mail@example.com>\" \\\n>           --to=nobody@example.com \\\n>           --smtp-server=\"$(pwd)/fake.sendmail\" \\\n> -         funny_name.patch &&\n> +         nonascii_name.patch &&\n>         grep \"^From: \" msgtxt1 >msgfrom &&\n>         test_line_count = 1 msgfrom\n>  '\n> --\n> 2.1.1\n>\n"},{"id":"252430","messageId":"20141124153609.GA25912@peff.net","threadId":"38032","inReplyTo":"1416786604-4988-1-git-send-email-dpb@corrigendum.ru","subject":"Re: [PATCH] send-email: handle adjacent RFC 2047-encoded words properly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-24T15:36:09Z","receivedAt":"2014-11-24T15:36:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 24, 2014 at 02:50:04AM +0300, Роман Донченко wrote:\n\n> The RFC says that they are to be concatenated after decoding (i.e. the\n> intervening whitespace is ignored).\n> \n> I change the sender's name to an all-Cyrillic string in the tests so that\n> its encoded form goes over the 76 characters in a line limit, forcing\n> format-patch to split it into multiple encoded words.\n> \n> Since I have to modify the regular expression for an encoded word anyway,\n> I take the opportunity to bring it closer to the spec, most notably\n> disallowing embedded spaces and making it case-insensitive (thus allowing\n> the encoding to be specified as both \"q\" and \"Q\").\n\nThe overall goal makes sense to me. Thanks for working on this. I have a\nfew questions/comments, though.\n\n>  sub unquote_rfc2047 {\n>  \tlocal ($_) = @_;\n> +\n> +\tmy $et = qr/[!->@-~]+/; # encoded-text from RFC 2047\n> +\tmy $sep = qr/[ \\t]+/;\n> +\tmy $encoded_word = qr/=\\?($et)\\?q\\?($et)\\?=/i;\n\nThe first $et in $encoded_word is actually the charset, which is defined\nby RFC 2047 as:\n\n     charset = token    ; see section 3\n\n     token = 1*<Any CHAR except SPACE, CTLs, and especials>\n\n     especials = \"(\" / \")\" / \"<\" / \">\" / \"@\" / \",\" / \";\" / \":\" / \"\n\t               <\"> / \"/\" / \"[\" / \"]\" / \"?\" / \".\" / \"=\"\n\nYour regex is a little more liberal. I doubt that it is a big deal in\npractice (actually, in practice, I suspect [a-zA-Z0-9-] would be fine).\nBut if we are tightening things up in general, it may make sense to do\nso here (and I notice that is_rfc2047_quoted does a more thorough $token\ndefinition, and it probably makes sense for the two functions to be\nconsistent).\n\nFor your definition of encoded-text, RFC 2047 says:\n\n     encoded-text = 1*<Any printable ASCII character other than \"?\"\n                          or SPACE>\n\nIt looks like you pulled the definition of $et from is_rfc2047_quoted,\nbut I am not clear on where that original came from (it is from a3a8262,\nbut that commit message does not explain the regex).\n\nAlso, I note that we handle 'q'-style encodings here, but not 'b'. I\nwonder if it is worth adding that in while we are in the area (it is not\na big deal if you always send-email git-generated patches, as we never\ngenerate it).\n\n> +\ts{$encoded_word(?:$sep$encoded_word)+}{\n\nIf I am reading this right, it requires at least two $encoded_words.\nShould this \"+\" be a \"*\"?\n\n> +\t\tmy @words = split $sep, $&;\n> +\t\tforeach (@words) {\n> +\t\t\tm/$encoded_word/;\n> +\t\t\t$encoding = $1;\n> +\t\t\t$_ = $2;\n> +\t\t\ts/_/ /g;\n> +\t\t\ts/=([0-9A-F]{2})/chr(hex($1))/eg;\n\nIn the spirit of your earlier change, should this final regex be\ncase-insensitive? RFC 2047 says only \"Upper case should be used for\nhexadecimal digits \"A\" through \"F.\" but that does not seem like a \"MUST\"\nto me.\n\n-Peff\n"},{"id":"252431","messageId":"20141124154404.GB25912@peff.net","threadId":"38032","inReplyTo":"CAPc5daVjNDg5CcWsMwfn=DZhwpCBdU2LYXOpFWZwhx2p8hLRww@mail.gmail.com","subject":"Re: [PATCH] send-email: handle adjacent RFC 2047-encoded words properly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-24T15:44:04Z","receivedAt":"2014-11-24T15:44:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 23, 2014 at 11:27:51PM -0800, Junio C Hamano wrote:\n\n> Was the change to the test to use Cyrillic really necessary, or did it\n> suffice if you simply extended the existsing \"Funny Name\" spelled with\n> strange accents, but you substituted the whole string anyway?\n> \n> Until I found out what the new string says by running web-based\n> translation on it, I felt somewhat uneasy. As I do not read\n> Cyrillic/Russian, we may have been adding some profanity without\n> knowing. It turns out that the string just says \"Cyrillic Name\", so I am\n> not against using the new string, but it simply looked odd to replace the\n> string whole-sale when you merely need a longer string. It made it look\n> as if a bug was specific to Cyrillic when it wasn't.\n\nI do not mind hidden Cyrillic profanity[1], but I found the new text\nmuch harder to verify, because the shapes are very unfamiliar to my\neyes. I'd prefer if we can stick to accented Roman letters.  I realize\nthis is me being totally Anglo-centric. But for Cyrillic readers,\nconsider how much more difficult it would be to manually verify the test\nif it were written in an unfamiliar script (e.g., Hangul).  The\nsurrounding code is already written in Roman characters (and English),\nso it probably makes sense as a common denominator.\n\n-Peff\n\n[1] As long as it is only crude and not mean. :)\n"},{"id":"252436","messageId":"op.xpucqml8nngjn5@freezie","threadId":"38032","inReplyTo":"CAPc5daVjNDg5CcWsMwfn=DZhwpCBdU2LYXOpFWZwhx2p8hLRww@mail.gmail.com","subject":"Re: [PATCH] send-email: handle adjacent RFC 2047-encoded words properly","fromName":"Роман Донченко","fromEmail":"dpb@corrigendum.ru","sentAt":"2014-11-24T18:09:48Z","receivedAt":"2014-11-24T18:09:48Z","isPatch":true,"sender":{"key":"dpb@corrigendum.ru","avatar":"https://avatars.githubusercontent.com/u/2391761?v=4"},"body":"Junio C Hamano <gitster@pobox.com> писал в своём письме Mon, 24 Nov 2014  \n10:27:51 +0300:\n\n> On Sun, Nov 23, 2014 at 3:50 PM, Роман Донченко <dpb@corrigendum.ru>  \n> wrote:\n>> The RFC says that they are to be concatenated after decoding (i.e. the\n>> intervening whitespace is ignored).\n>>\n>> I change the sender's name to an all-Cyrillic string in the tests so  \n>> that\n>> its encoded form goes over the 76 characters in a line limit, forcing\n>> format-patch to split it into multiple encoded words.\n>>\n>> Since I have to modify the regular expression for an encoded word  \n>> anyway,\n>> I take the opportunity to bring it closer to the spec, most notably\n>> disallowing embedded spaces and making it case-insensitive (thus  \n>> allowing\n>> the encoding to be specified as both \"q\" and \"Q\").\n>>\n>> Signed-off-by: Роман Донченко <dpb@corrigendum.ru>\n>\n> This sounds like a worthy thing to do in general.\n>\n> I wonder if the C implementation we have for mailinfo needs similar\n> update, though. I vaguely recall that we have case-insensitive start for\n> q/b segments, but do not remember the details offhand.\n\nThat's what git am uses, right? I think that already works correctly (or  \nat least doesn't have the bug this patch fixes). I didn't do extensive  \ntesting or look at the code, though.\n\n>\n> Was the change to the test to use Cyrillic really necessary, or did it\n> suffice if you simply extended the existsing \"Funny Name\" spelled with\n> strange accents, but you substituted the whole string anyway?\n>\n> Until I found out what the new string says by running web-based\n> translation on it, I felt somewhat uneasy. As I do not read\n> Cyrillic/Russian, we may have been adding some profanity without\n> knowing. It turns out that the string just says \"Cyrillic Name\", so I am\n> not against using the new string, but it simply looked odd to replace the\n> string whole-sale when you merely need a longer string. It made it look\n> as if a bug was specific to Cyrillic when it wasn't.\n\nAh, if only I had thought of including profanity beforehand. ;-)\n\nSeriously though, I just needed to hit the 76 character limit, and  \nswitching the keyboard layout is a lot easier than copypasting Latin  \nletters with diacritics (plus I had trouble coming up with a long enough  \nextension of \"Funny Name\"...). I can see how that's problematic, though;  \nI'll change it.\n\n> As you may notice by reading \"git log --no-merges\" from recent history,\n> we tend not to say \"I did X, I did Y\". If the tone of the above message\n> were more similar to them, it may have been easier to read.\n\nTechnically, I said \"I do\", not \"I did\"... but sure, point taken.\n\nRoman.\n"},{"id":"252438","messageId":"op.xpudh8c3nngjn5@freezie","threadId":"38032","inReplyTo":"20141124153609.GA25912@peff.net","subject":"Re: [PATCH] send-email: handle adjacent RFC 2047-encoded words properly","fromName":"Роман Донченко","fromEmail":"dpb@corrigendum.ru","sentAt":"2014-11-24T18:26:22Z","receivedAt":"2014-11-24T18:26:22Z","isPatch":true,"sender":{"key":"dpb@corrigendum.ru","avatar":"https://avatars.githubusercontent.com/u/2391761?v=4"},"body":"Jeff King <peff@peff.net> писал в своём письме Mon, 24 Nov 2014 18:36:09  \n+0300:\n\n> On Mon, Nov 24, 2014 at 02:50:04AM +0300, Роман Донченко wrote:\n>\n>> The RFC says that they are to be concatenated after decoding (i.e. the\n>> intervening whitespace is ignored).\n>>\n>> I change the sender's name to an all-Cyrillic string in the tests so  \n>> that\n>> its encoded form goes over the 76 characters in a line limit, forcing\n>> format-patch to split it into multiple encoded words.\n>>\n>> Since I have to modify the regular expression for an encoded word  \n>> anyway,\n>> I take the opportunity to bring it closer to the spec, most notably\n>> disallowing embedded spaces and making it case-insensitive (thus  \n>> allowing\n>> the encoding to be specified as both \"q\" and \"Q\").\n>\n> The overall goal makes sense to me. Thanks for working on this. I have a\n> few questions/comments, though.\n>\n>>  sub unquote_rfc2047 {\n>>  \tlocal ($_) = @_;\n>> +\n>> +\tmy $et = qr/[!->@-~]+/; # encoded-text from RFC 2047\n>> +\tmy $sep = qr/[ \\t]+/;\n>> +\tmy $encoded_word = qr/=\\?($et)\\?q\\?($et)\\?=/i;\n>\n> The first $et in $encoded_word is actually the charset, which is defined\n> by RFC 2047 as:\n>\n>      charset = token    ; see section 3\n>\n>      token = 1*<Any CHAR except SPACE, CTLs, and especials>\n>\n>      especials = \"(\" / \")\" / \"<\" / \">\" / \"@\" / \",\" / \";\" / \":\" / \"\n> \t               <\"> / \"/\" / \"[\" / \"]\" / \"?\" / \".\" / \"=\"\n>\n> Your regex is a little more liberal. I doubt that it is a big deal in\n> practice (actually, in practice, I suspect [a-zA-Z0-9-] would be fine).\n> But if we are tightening things up in general, it may make sense to do\n> so here (and I notice that is_rfc2047_quoted does a more thorough $token\n> definition, and it probably makes sense for the two functions to be\n> consistent).\n\nYeah, I did realize that token is more restrictive than encoded-text, but  \nI didn't want to stray too far from the subject line of the patch. What  \nI'll probably do is split the patch into two, one for regex tweaking and  \none for multiple-word handling. And yeah, I'll try to make the two  \nfunctions use the same regexes.\n\n>\n> For your definition of encoded-text, RFC 2047 says:\n>\n>      encoded-text = 1*<Any printable ASCII character other than \"?\"\n>                           or SPACE>\n>\n> It looks like you pulled the definition of $et from is_rfc2047_quoted,\n> but I am not clear on where that original came from (it is from a3a8262,\n> but that commit message does not explain the regex).\n\nNo, it's actually an independent discovery. :-) I don't think it needs  \nexplanation, though - it's just a character class with two ranges covering  \nevery printable character but the question mark.\n\n> Also, I note that we handle 'q'-style encodings here, but not 'b'. I\n> wonder if it is worth adding that in while we are in the area (it is not\n> a big deal if you always send-email git-generated patches, as we never\n> generate it).\n\nI could add \"b\" decoding, but since format-patch never generates \"b\"  \nencodings, testing would be a problem. And I'd rather not do it without  \nany tests.\n\n>\n>> +\ts{$encoded_word(?:$sep$encoded_word)+}{\n>\n> If I am reading this right, it requires at least two $encoded_words.\n> Should this \"+\" be a \"*\"?\n\nI hang my head in shame. Looks like I'll have to add more tests...\n\n>\n>> +\t\tmy @words = split $sep, $&;\n>> +\t\tforeach (@words) {\n>> +\t\t\tm/$encoded_word/;\n>> +\t\t\t$encoding = $1;\n>> +\t\t\t$_ = $2;\n>> +\t\t\ts/_/ /g;\n>> +\t\t\ts/=([0-9A-F]{2})/chr(hex($1))/eg;\n>\n> In the spirit of your earlier change, should this final regex be\n> case-insensitive? RFC 2047 says only \"Upper case should be used for\n> hexadecimal digits \"A\" through \"F.\" but that does not seem like a \"MUST\"\n> to me.\n\nSounds reasonable.\n\nRoman.\n"},{"id":"252468","messageId":"20141124230346.GA10064@peff.net","threadId":"38032","inReplyTo":"op.xpudh8c3nngjn5@freezie","subject":"Re: [PATCH] send-email: handle adjacent RFC 2047-encoded words properly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-24T23:03:47Z","receivedAt":"2014-11-24T23:03:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 24, 2014 at 09:26:22PM +0300, Роман Донченко wrote:\n\n> Yeah, I did realize that token is more restrictive than encoded-text, but I\n> didn't want to stray too far from the subject line of the patch. What I'll\n> probably do is split the patch into two, one for regex tweaking and one for\n> multiple-word handling. And yeah, I'll try to make the two functions use the\n> same regexes.\n\nThanks, I think that sounds like a good plan.\n\n> >For your definition of encoded-text, RFC 2047 says:\n> >\n> >     encoded-text = 1*<Any printable ASCII character other than \"?\"\n> >                          or SPACE>\n> >\n> >It looks like you pulled the definition of $et from is_rfc2047_quoted,\n> >but I am not clear on where that original came from (it is from a3a8262,\n> >but that commit message does not explain the regex).\n> \n> No, it's actually an independent discovery. :-) I don't think it needs\n> explanation, though - it's just a character class with two ranges covering\n> every printable character but the question mark.\n\nAnd now it is my turn to hang my head in shame. I viewed that as a set\nof characters, rather than ranges. The \"-\" just blended into the mass of\npunctuation, and I mistook the \"!\" for negation.\n\nI wonder if it would be more readable as:\n\n  [\\x21-\\x3e\\x40-\\x7e]\n\nor something. I guess perl even has classes pre-made for \"printable\nascii\". I dunno. It may be OK as-is, too, and I just need to read more\ncarefully. :)\n\n> >Also, I note that we handle 'q'-style encodings here, but not 'b'. I\n> >wonder if it is worth adding that in while we are in the area (it is not\n> >a big deal if you always send-email git-generated patches, as we never\n> >generate it).\n> \n> I could add \"b\" decoding, but since format-patch never generates \"b\"\n> encodings, testing would be a problem. And I'd rather not do it without any\n> tests.\n\nI think you could include some literal fixtures in the test suite (t5100\nalready does this for mailinfo). But I don't think handling 'b' is a\nrequirement here. It's really orthogonal to your patches, and nobody has\nactually asked for it, so I don't mind leaving it for another day.\n\n-Peff\n"}]}