code review 126560043: net/url: make Userinfo.String() escape ? and add test f...

net/url: make Userinfo.String() escape ? and add test for shouldEscape

See RFC 3986 §3.2.1.
Fixes issue 6573.

[esk, 2014-08-24 02:31:54]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2014-08-25 17:31:17]

https://codereview.appspot.com/126560043/diff/2/src/pkg/net/url/url.go
File src/pkg/net/url/url.go (right):

https://codereview.appspot.com/126560043/diff/2/src/pkg/net/url/url.go#newcode89
src/pkg/net/url/url.go:89: // The RFC allows ; : & = + $ , in userinfo, so we
must escape only @ / ?.
the slash here looks like the word "and".  I'd write '@', '/', and '?'.

https://codereview.appspot.com/126560043/diff/2/src/pkg/net/url/url.go#newcode89
src/pkg/net/url/url.go:89: // The RFC allows ; : & = + $ , in userinfo, so we
must escape only @ / ?.
unclear whether ',' is allowed or is a comma as part of this sentence. Put all
in quotes.

https://codereview.appspot.com/126560043/diff/2/src/pkg/net/url/url.go#newcode90
src/pkg/net/url/url.go:90: // The parsing of userinfo treats : as special so we
must escape that too.
likewise ':'

https://codereview.appspot.com/126560043/diff/2/src/pkg/net/url/url.go#newcode91
src/pkg/net/url/url.go:91: return c == '@' || c == '/' || c == ':' || c == '?'
order these the same as they are in the comment

https://codereview.appspot.com/126560043/diff/2/src/pkg/net/url/url_test.go
File src/pkg/net/url/url_test.go (right):

https://codereview.appspot.com/126560043/diff/2/src/pkg/net/url/url_test.go#n...
src/pkg/net/url/url_test.go:906: 
also add a higher-level test encoding a whole URL, like one in the issue

[esk, 2014-08-30 03:57:49]

PTAL

https://codereview.appspot.com/126560043/diff/2/src/pkg/net/url/url.go
File src/pkg/net/url/url.go (right):

https://codereview.appspot.com/126560043/diff/2/src/pkg/net/url/url.go#newcode89
src/pkg/net/url/url.go:89: // The RFC allows ; : & = + $ , in userinfo, so we
must escape only @ / ?.
On 2014/08/25 17:31:17, bradfitz wrote:
> the slash here looks like the word "and".  I'd write '@', '/', and '?'.

Done.

https://codereview.appspot.com/126560043/diff/2/src/pkg/net/url/url.go#newcode90
src/pkg/net/url/url.go:90: // The parsing of userinfo treats : as special so we
must escape that too.
On 2014/08/25 17:31:17, bradfitz wrote:
> likewise ':'

Done.

https://codereview.appspot.com/126560043/diff/2/src/pkg/net/url/url.go#newcode91
src/pkg/net/url/url.go:91: return c == '@' || c == '/' || c == ':' || c == '?'
On 2014/08/25 17:31:17, bradfitz wrote:
> order these the same as they are in the comment

Done.

https://codereview.appspot.com/126560043/diff/2/src/pkg/net/url/url_test.go
File src/pkg/net/url/url_test.go (right):

https://codereview.appspot.com/126560043/diff/2/src/pkg/net/url/url_test.go#n...
src/pkg/net/url/url_test.go:906: 
On 2014/08/25 17:31:17, bradfitz wrote:
> also add a higher-level test encoding a whole URL, like one in the issue

Done.

[bradfitz, 2014-08-30 17:34:06]

LGTM

[bradfitz, 2014-08-30 17:34:59]

*** Submitted as https://code.google.com/p/go/source/detail?r=7786bd1553ab ***

net/url: make Userinfo.String() escape ? and add test for shouldEscape

See RFC 3986 §3.2.1.
Fixes issue 6573.

LGTM=bradfitz
R=golang-codereviews, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/126560043

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[esk, 2014-08-31 03:37:24]

*** Abandoned ***