code review 55770043: net/mail: correctly handle whitespaces when formatting ...

net/mail: correctly handle whitespaces when formatting an email address Whitespace characters are allowed in quoted-string according to RFC 5322 without being "Q"-encoding. Address.String() already always formats the name portion in quoted string, so whitespace characters should be allowed in there. Fixes issue 6641.

[j.czarnowicz, 2014-01-22 19:58:22]

Hello golang-codereviews@googlegroups.com, I'd like you to review this change to http://code.google.com/p/go/

[gobot, 2014-01-22 21:50:34]

R=dsymonds@golang.org (assigned by bradfitz@golang.org)

[dave_cheney.net, 2014-02-05 02:59:09]

LGTM. Can you please respond to the review comments then I will submit this. https://codereview.appspot.com/55770043/diff/30003/src/pkg/net/mail/message.go File src/pkg/net/mail/message.go (right): https://codereview.appspot.com/55770043/diff/30003/src/pkg/net/mail/message.g... src/pkg/net/mail/message.go:539: // isWSP returns true if c is a WSP (white space) please rename this function // isWhitespace returns true if c is a whitespace // character as defined by RFC 5234 Appendix B.

[dsymonds1, 2014-02-05 03:21:37]

NOT LGTM just yet https://codereview.appspot.com/55770043/diff/30003/src/pkg/net/mail/message.go File src/pkg/net/mail/message.go (right): https://codereview.appspot.com/55770043/diff/30003/src/pkg/net/mail/message.g... src/pkg/net/mail/message.go:162: if !isVchar(a.Name[i]) && !isWSP(a.Name[i]) { add a comment above here: // isWSP here should actually be isFWS, // but we don't support folding yet. https://codereview.appspot.com/55770043/diff/30003/src/pkg/net/mail/message.g... src/pkg/net/mail/message.go:539: // isWSP returns true if c is a WSP (white space) On 2014/02/05 02:59:10, dfc wrote: > please rename this function > > // isWhitespace returns true if c is a whitespace > // character as defined by RFC 5234 Appendix B. Actually the relevant thing being captured here is FWS (folding whitespace), per RFC 5322 section 3.2.2. I interpret that via the path of productions name-addr -> display-name -> phrase -> word -> quoted-string -> FWS (since whitespace is *not* qtext). However, we don't handle folding in general here (but may in the future). The helper functions down here all follow the names of the RFC productions (it makes it a lot easier to follow the spec), so I'd prefer this stays as isWSP. Please add a period to the end of this line, and fix the misspelling of "Appendix" on the next line. https://codereview.appspot.com/55770043/diff/30003/src/pkg/net/mail/message_t... File src/pkg/net/mail/message_test.go (right): https://codereview.appspot.com/55770043/diff/30003/src/pkg/net/mail/message_t... src/pkg/net/mail/message_test.go:281: &Address{Name: "Bob Bob", Address: "bob@example.com"}, make the name "Bob Jane"

[j.czarnowicz, 2014-02-06 12:21:24]

Hello golang-codereviews@googlegroups.com, gobot@golang.org, dsymonds@golang.org, dave@cheney.net, dsymonds@google.com (cc: golang-codereviews@googlegroups.com), Please take another look.

[dsymonds, 2014-02-06 23:45:17]

LGTM

[dsymonds, 2014-02-06 23:49:28]

*** Submitted as https://code.google.com/p/go/source/detail?r=9623db1cea87 *** net/mail: correctly handle whitespaces when formatting an email address Whitespace characters are allowed in quoted-string according to RFC 5322 without being "Q"-encoding. Address.String() already always formats the name portion in quoted string, so whitespace characters should be allowed in there. Fixes issue 6641. LGTM=dave, dsymonds R=golang-codereviews, gobot, dsymonds, dave CC=golang-codereviews https://codereview.appspot.com/55770043 Committer: David Symonds <dsymonds@golang.org>

[gobot, 2014-02-06 23:54:10]

This CL appears to have broken the freebsd-amd64 builder.