Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(677)

Issue 11698045: code review 11698045: net/url: prepend slash to path in String() (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 8 months ago by scottferg
Modified:
10 years, 5 months ago
Reviewers:
dfc, rsc, raff
CC:
golang-dev, bradfitz, rsc, 0xjnml
Visibility:
Public.

Description

net/url: prepend slash to path in String() Previously if a path was set manually without a leading /, String() would not insert the slash when writing its output. This would lead to situations where a URL that should be http://www.google.com/search is output as http://www.google.comsearch Fixes issue 5927.

Patch Set 1 #

Patch Set 2 : diff -r 86a2e482982f https://code.google.com/p/go #

Patch Set 3 : diff -r 86a2e482982f https://code.google.com/p/go #

Patch Set 4 : diff -r 86a2e482982f https://code.google.com/p/go #

Patch Set 5 : diff -r 86a2e482982f https://code.google.com/p/go #

Total comments: 1

Patch Set 6 : diff -r 86a2e482982f https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+19 lines, -0 lines) Patch
M src/pkg/net/url/url.go View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M src/pkg/net/url/url_test.go View 1 2 3 4 1 chunk +16 lines, -0 lines 0 comments Download

Messages

Total messages: 13
scottferg
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
10 years, 8 months ago (2013-07-23 20:24:21 UTC) #1
bradfitz
"its output". Also: if this is an acceptable change, it would need a test. On ...
10 years, 8 months ago (2013-07-23 20:39:20 UTC) #2
rsc
I can't decide if this is correct.
10 years, 8 months ago (2013-07-23 21:15:17 UTC) #3
scottferg
On 2013/07/23 20:39:20, bradfitz wrote: > "its output". > > Also: if this is an ...
10 years, 8 months ago (2013-07-23 22:46:33 UTC) #4
scottferg
Nevermind that, realized there was a bug in that test. I've added the specific test ...
10 years, 8 months ago (2013-07-23 23:00:59 UTC) #5
0xjnml
https://codereview.appspot.com/11698045/diff/14001/src/pkg/net/url/url.go File src/pkg/net/url/url.go (right): https://codereview.appspot.com/11698045/diff/14001/src/pkg/net/url/url.go#newcode463 src/pkg/net/url/url.go:463: if u.Path[0] != '/' { You can save two ...
10 years, 8 months ago (2013-07-24 07:49:04 UTC) #6
scottferg
On 2013/07/24 07:49:04, 0xjnml wrote: > https://codereview.appspot.com/11698045/diff/14001/src/pkg/net/url/url.go > File src/pkg/net/url/url.go (right): > > https://codereview.appspot.com/11698045/diff/14001/src/pkg/net/url/url.go#newcode463 > ...
10 years, 8 months ago (2013-07-24 13:25:51 UTC) #7
rsc
LGTM There's no good answer here: no URL we generate will reproduce the original struct. ...
10 years, 8 months ago (2013-08-01 21:17:02 UTC) #8
rsc
LGTM Please complete a CLA as described at golang.org/doc/contribute.html#copyright
10 years, 8 months ago (2013-08-01 22:05:21 UTC) #9
scottferg
On 2013/08/01 22:05:21, rsc wrote: > LGTM > > Please complete a CLA as described ...
10 years, 8 months ago (2013-08-01 22:45:39 UTC) #10
bradfitz
*** Submitted as https://code.google.com/p/go/source/detail?r=03265149ced8 *** net/url: prepend slash to path in String() Previously if a ...
10 years, 8 months ago (2013-08-01 22:52:58 UTC) #11
raff
I belive this fix is not 100% correct (or at least it breaks a use ...
10 years, 5 months ago (2013-10-17 01:21:09 UTC) #12
dfc
10 years, 5 months ago (2013-10-17 03:58:09 UTC) #13
Please open an issue for this regression. 

> On 17 Oct 2013, at 12:20, raff367@gmail.com wrote:
> 
> I belive this fix is not 100% correct (or at least it breaks a use case that
works in Go < 1.2).
> 
> the following test used to return "a/b/c" and now it returns "/a/b/c" (i.e. a
relative URL becomes absolute):
> 
> func main() {
>     path := "a/b/c"
>     u, _ := url.Parse(path)
> 
>     fmt.Println("path:", path)
>     fmt.Println("url:", u)
> }
> 
> I understand the root problem but I think the correct fix should be to also
check that the buffer is not empty (i.e. if I do create a URL with schema/host
and a relative path I DO WANT the "/", but if the URL only has the Path set, it
should be left alone).
> 
> Also, I don't see any test for this particular use case
> (if you want I can submit a patch for the fix and test case)
> 
> Thanks!
> 
> -- Raffaele
> 
> 
> 
>> On Tuesday, July 23, 2013 1:24:21 PM UTC-7, Scott Ferguson wrote:
>> Reviewers: golang-dev1, 
>> 
>> Message: 
>> Hello golan...@googlegroups.com, 
>> 
>> I'd like you to review this change to 
>> https://code.google.com/p/go 
>> 
>> 
>> Description: 
>> net/url: prepend slash to path in String() 
>> 
>> Previously if a path was set manually without a leading /, String() 
>> would not insert the slash when writing it's output. This would lead 
>> to situations where a URL that should be http://www.google.com/search 
>> is output as http://www.google.comsearch 
>> 
>> Fixes issue 5927. 
>> 
>> Please review this at https://codereview.appspot.com/11698045/ 
>> 
>> Affected files: 
>>    M src/pkg/net/url/url.go 
>> 
>> 
>> Index: src/pkg/net/url/url.go 
>> =================================================================== 
>> --- a/src/pkg/net/url/url.go 
>> +++ b/src/pkg/net/url/url.go 
>> @@ -459,6 +459,11 @@ 
>>                                   buf.WriteString(h) 
>>                           } 
>>                   } 
>> +                if u.Path != "" { 
>> +                        if u.Path[0] != '/' { 
>> +                                buf.WriteByte('/') 
>> +                        } 
>> +                } 
>>                   buf.WriteString(escape(u.Path, encodePath)) 
>>           } 
>>           if u.RawQuery != "" {
> 
> -- 
>  
> --- 
> You received this message because you are subscribed to the Google Groups
"golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b