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.