{"thread":{"id":"31128","subject":"bug (?) in send email","startedAt":"2012-07-28T21:33:04Z","lastAt":"2012-07-30T16:34:40Z","messageCount":6,"participants":["Christoph Miebach","Thomas Rast","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"196050","messageId":"50145A90.1090904@web.de","threadId":"31128","inReplyTo":null,"subject":"bug (?) in send email","fromName":"Christoph Miebach","fromEmail":"christoph.miebach@web.de","sentAt":"2012-07-28T21:33:04Z","receivedAt":"2012-07-28T21:33:04Z","isPatch":false,"sender":{"key":"christoph.miebach@web.de","avatar":null},"body":"Hello!\n\nsend-email (tested versions 1.7.9.2 and 1.7.10.4) breaks email addresses.\n\nSteps to reproduce:\n\n\nModify file.\n\ngit commit --author=\"Michał Tz \" modified.file -m \"Test\"\n\ngit format-patch -o patches origin\n\nNow, the patch seems to have the address right, see [1]\n\ngit send-email --to MYOWN.ADDRESS@mail.com --suppress-cc=author \npatches/0001-Test.patch\n\nBut checking my inbox now shows an email starting with:\nFrom: Michał Tz \n\nSo the address is splitted at the underscore.\n\nFurthermore, if I don't use --suppress-cc=author, the CC field shows the \nright address.\n\nRegards\n\nChristoph\n\n[1]\nless patches/0001-Test.patch\nFrom: =?UTF-8?q?Micha=C5=82=20Tz?= \n\ngit show\nAuthor: Michał Tz \n"},{"id":"196115","messageId":"50166408.8000008@web.de","threadId":"31128","inReplyTo":"50145A90.1090904@web.de","subject":"Re: bug (?) in send email","fromName":"Christoph Miebach","fromEmail":"christoph.miebach@web.de","sentAt":"2012-07-30T10:38:00Z","receivedAt":"2012-07-30T10:38:00Z","isPatch":false,"sender":{"key":"christoph.miebach@web.de","avatar":null},"body":"Hello!\n\nRemoving this line\ns/_/ /g;\nhere\nhttps://github.com/git/git/blob/master/git-send-email.perl#L867\n\nSolves this problem for me. But I really don't have any clue, what kind \nof side effects this modification on \"sub unquote_rfc2047\" might have.\n\nRegards\n\nChristoph\n\nOn 28.07.2012 23:33, Christoph Miebach wrote:\n> Hello!\n>\n> send-email (tested versions 1.7.9.2 and 1.7.10.4) breaks email addresses.\n>\n> Steps to reproduce:\n>\n>\n> Modify file.\n>\n> git commit --author=\"Michał Tz \" modified.file -m\n> \"Test\"\n>\n> git format-patch -o patches origin\n>\n> Now, the patch seems to have the address right, see [1]\n>\n> git send-email --to MYOWN.ADDRESS@mail.com --suppress-cc=author\n> patches/0001-Test.patch\n>\n> But checking my inbox now shows an email starting with:\n> From: Michał Tz \n>\n> So the address is splitted at the underscore.\n>\n> Furthermore, if I don't use --suppress-cc=author, the CC field shows the\n> right address.\n>\n> Regards\n>\n> Christoph\n>\n> [1]\n> less patches/0001-Test.patch\n> From: =?UTF-8?q?Micha=C5=82=20Tz?= \n>\n> git show\n> Author: Michał Tz \n"},{"id":"196120","messageId":"873949mnx0.fsf@thomas.inf.ethz.ch","threadId":"31128","inReplyTo":"50166408.8000008@web.de","subject":"Re: bug (?) in send email","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-07-30T12:30:35Z","receivedAt":"2012-07-30T12:30:35Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"[+Cc people involved with this function]\n\nChristoph Miebach writes:\n\n> > git commit --author=\"Michał Tz \" modified.file -m\n> > \"Test\"\n> >\n> > git format-patch -o patches origin\n> >\n> > Now, the patch seems to have the address right, see [1]\n> >\n> > git send-email --to MYOWN.ADDRESS@mail.com --suppress-cc=author\n> > patches/0001-Test.patch\n> >\n> > But checking my inbox now shows an email starting with:\n> > From: Michał Tz \n> \n> Removing this line\n> s/_/ /g;\n> here\n> https://github.com/git/git/blob/master/git-send-email.perl#L867\n>\n> Solves this problem for me. But I really don't have any clue, what\n> kind of side effects this modification on \"sub unquote_rfc2047\" might\n> have.\n\nIt would prevent spaces from being decoded correctly if the encoding\nprogram chooses to make the '_'. git-format-patch does not actually do\nthis, see the big comment around pretty.c:304.\n\nI think this patch would be a better match for what RFC2047 specifies.\nOn the one hand it avoids substituting _ outside of encodings, but OTOH\nit also handles more than one encoded-word. It still does not handle\nthe case where there are several encoded-words of *different* encodings,\nbut who would do such a crazy thing?\n\n\ndiff --git i/git-send-email.perl w/git-send-email.perl\nindex ef30c55..88c4758 100755\n--- i/git-send-email.perl\n+++ w/git-send-email.perl\n@@ -862,11 +862,13 @@ sub make_message_id {\n sub unquote_rfc2047 {\n \tlocal ($_) = @_;\n \tmy $encoding;\n-\tif (s/=\\?([^?]+)\\?q\\?(.*)\\?=/$2/g) {\n+\ts{=\\?([^?]+)\\?q\\?(.*)\\?=}{\n \t\t$encoding = $1;\n-\t\ts/_/ /g;\n-\t\ts/=([0-9A-F]{2})/chr(hex($1))/eg;\n-\t}\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+\t}eg;\n \treturn wantarray ? ($_, $encoding) : $_;\n }\n \n\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"196145","messageId":"7vpq7ds1hu.fsf@alter.siamese.dyndns.org","threadId":"31128","inReplyTo":"873949mnx0.fsf@thomas.inf.ethz.ch","subject":"Re: bug (?) in send email","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-30T15:38:21Z","receivedAt":"2012-07-30T15:38:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> It would prevent spaces from being decoded correctly if the encoding\n> program chooses to make the '_'. git-format-patch does not actually do\n> this, see the big comment around pretty.c:304.\n>\n> I think this patch would be a better match for what RFC2047 specifies.\n> On the one hand it avoids substituting _ outside of encodings, but OTOH\n> it also handles more than one encoded-word.\n\nYeah, I think it is an improvement.\n\nI however wonder if the captured pattern for $2 should be minimized\nwith ? at the end, i.e. \"..\\?q\\?(.*?)\\?=\"?\n\n> It still does not handle\n> the case where there are several encoded-words of *different* encodings,\n> but who would do such a crazy thing?\n\nEven if somebody did so, it wouldn't have worked, and to make it\nwork, the sub and its caller (there is only one caller that actually\ncares what the original encoding was) needs to be rethought anyway,\nso I do not think it matters.\n\nIt may deserve an in-code NEEDSWORK comment, though.\n\nThanks.\n\n> diff --git i/git-send-email.perl w/git-send-email.perl\n> index ef30c55..88c4758 100755\n> --- i/git-send-email.perl\n> +++ w/git-send-email.perl\n> @@ -862,11 +862,13 @@ sub make_message_id {\n> sub unquote_rfc2047 {\n> \tlocal ($_) = @_;\n> \tmy $encoding;\n> -\tif (s/=\\?([^?]+)\\?q\\?(.*)\\?=/$2/g) {\n> +\ts{=\\?([^?]+)\\?q\\?(.*)\\?=}{\n> \t\t$encoding = $1;\n> -\t\ts/_/ /g;\n> -\t\ts/=([0-9A-F]{2})/chr(hex($1))/eg;\n> -\t}\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> +\t}eg;\n> \treturn wantarray ? ($_, $encoding) : $_;\n> }\n"},{"id":"196153","messageId":"20120730163240.GB16701@sigill.intra.peff.net","threadId":"31128","inReplyTo":"873949mnx0.fsf@thomas.inf.ethz.ch","subject":"Re: bug (?) in send email","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-30T16:32:40Z","receivedAt":"2012-07-30T16:32:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 30, 2012 at 02:30:35PM +0200, Thomas Rast wrote:\n\n> > Removing this line\n> > s/_/ /g;\n> > here\n> > https://github.com/git/git/blob/master/git-send-email.perl#L867\n> >\n> > Solves this problem for me. But I really don't have any clue, what\n> > kind of side effects this modification on \"sub unquote_rfc2047\" might\n> > have.\n> \n> It would prevent spaces from being decoded correctly if the encoding\n> program chooses to make the '_'. git-format-patch does not actually do\n> this, see the big comment around pretty.c:304.\n\nYeah, I think we need to be prepared to decode arbitrary rfc2047. Even\nthough most of our input will come from format-patch, people can munge\nthe input between format-patch and send-email.\n\n> I think this patch would be a better match for what RFC2047 specifies.\n> On the one hand it avoids substituting _ outside of encodings, but OTOH\n> it also handles more than one encoded-word. It still does not handle\n> the case where there are several encoded-words of *different* encodings,\n> but who would do such a crazy thing?\n> \n> diff --git i/git-send-email.perl w/git-send-email.perl\n> index ef30c55..88c4758 100755\n> --- i/git-send-email.perl\n> +++ w/git-send-email.perl\n> @@ -862,11 +862,13 @@ sub make_message_id {\n> sub unquote_rfc2047 {\n> \tlocal ($_) = @_;\n> \tmy $encoding;\n> -\tif (s/=\\?([^?]+)\\?q\\?(.*)\\?=/$2/g) {\n> +\ts{=\\?([^?]+)\\?q\\?(.*)\\?=}{\n> \t\t$encoding = $1;\n> -\t\ts/_/ /g;\n> -\t\ts/=([0-9A-F]{2})/chr(hex($1))/eg;\n> -\t}\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> +\t}eg;\n> \treturn wantarray ? ($_, $encoding) : $_;\n\nThat is definitely an improvement. The existing code was just plain\nwrong to assume that the whole string would be rfc2047 encoded, as that\nis never the case (e.g., I don't think it is even legal to encode the\naddr-spec part).\n\nYou are right that the result does not handle multiple encoded chunks\nwith different encodings. However, it also does not handle a single\nencoded chunk with non-encoded bits. E.g., consider:\n\n From: =?UTF-8?q?Some=20N=C3=A1me?= \n\nWe will return:\n\n (\"Some Náme \", \"UTF-8\")\n\nNote that the second half is unencoded ASCII, but we have simply\nconcatenated it with the encoded bit and claimed the whole thing at\nUTF-8. This works for utf-8, of course, because it is an ASCII superset.\nBut what about utf-16? You now have incompatible mixed encodings in a\nsingle string. Which of course is only the tip of the iceberg; we later\nmay stick the author field into the body, which does not necessarily\nhave the same encoding (and there is a big \"uh oh we should reencode\"\nargument that does not actually do so).\n\nFurthermore, even if we do use an ASCII superset, the callers do not use\nthe encoding field very well. They do things like:\n\n if (unquote_rfc2047($addr) eq $sender) {\n\nSo we are comparing the unquoted address, in some encoding (that we\nthrow away) to whatever we have in $sender, which may have come from the\nauthor ident, or the command-line, or the expansion of a MUA alias. So\nwe would fail to match utf-8 and latin1 versions of the same identity.\n\nI think the only sane thing for unquote_rfc2047 to do would be to\nactually return just a string in some canonical encoding (i.e., utf8),\nand iconv each of the bits separately from its appropriate\nencoding. And then we assume that everything internal is in the same\nencoding and we can just compare strings to get the right answer. And\noutgoing, we produce encoded utf8 for the headers, or match the body for\nan in-body \"From\" header.\n\nOf course, that means we would have to start using perl's encoding\nfunctions, which we do not use currently. I think Encode has been in the\nperl base system long enough for us to rely on it, but I am not very up\nto speed on encoding issues in perl. And nobody has been complaining\nabout those issues, which I assume means everybody is just using utf8\nfor outgoing messages. So it might not be worth the trouble to fix.\n\nI also notice that we do not handle the 'b' rfc2047 encoding at all.\nWe do handle this in mailinfo for incoming messages, but again, I guess\neverybody just uses the more common 'q' for outgoing (since it's mostly\ngenerated by git, anyway). So if nobody is complaining, I'd be inclined\nto just leave it.\n\nSo after all that rambling, I think my response is \"yes, your patch is a\nstep in the right direction, and we should do it\". There are still a ton\nof unsupported setups, but really, if we are going to handle everything,\nI'd rather see us make the jump to using one of the existing\nmail-handling modules rather than reinventing the wheel. And I know\nthat some people really like the no-dependencies aspect of what we have\ncurrently.\n\n-Peff\n"},{"id":"196154","messageId":"20120730163440.GC16701@sigill.intra.peff.net","threadId":"31128","inReplyTo":"7vpq7ds1hu.fsf@alter.siamese.dyndns.org","subject":"Re: bug (?) in send email","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-30T16:34:40Z","receivedAt":"2012-07-30T16:34:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 30, 2012 at 08:38:21AM -0700, Junio C Hamano wrote:\n\n> > I think this patch would be a better match for what RFC2047 specifies.\n> > On the one hand it avoids substituting _ outside of encodings, but OTOH\n> > it also handles more than one encoded-word.\n> \n> Yeah, I think it is an improvement.\n> \n> I however wonder if the captured pattern for $2 should be minimized\n> with ? at the end, i.e. \"..\\?q\\?(.*?)\\?=\"?\n\nYeah, definitely. \"?=\" cannot appear inside (it would need to be\nquoted).\n\n> > It still does not handle\n> > the case where there are several encoded-words of *different* encodings,\n> > but who would do such a crazy thing?\n> \n> Even if somebody did so, it wouldn't have worked, and to make it\n> work, the sub and its caller (there is only one caller that actually\n> cares what the original encoding was) needs to be rethought anyway,\n> so I do not think it matters.\n> \n> It may deserve an in-code NEEDSWORK comment, though.\n\nI rambled about this in much more detail in another reply, but the gist\nof it is that yes, that is the right step for now.\n\n-Peff\n"}]}