code review 14654045: net/url: Fix regression from issue 11698045:

net/url: Fix regression from issue 11698045:

I belive the current fix is not 100% correct (or at least it breaks a use case that used to work before the fix).

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

The fix only prepend a "/" if the buffer is not empty (i.e. it should have scheme:host).
Also reworked the test case (added a new TestURLToString test and added a list of urltostringtests, for now containing only the two cases involved with this issue)

[raff, 2013-10-17 04:56:24]

Hello golang-dev@googlegroups.com,

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

[dave_cheney.net, 2013-10-17 05:01:06]

Did you raise an issue for this regression ?

On Thu, Oct 17, 2013 at 3:56 PM,  <raff367@gmail.com> wrote:
> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net/url: Fix regression from issue 11698045:
>
> I belive the current fix is not 100% correct (or at least it breaks a
> use case that used to work before the fix).
>
> 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
>
> The fix only prepend a "/" if the buffer is not empty (i.e. it should
> have scheme:host).
> Also reworked the test case (added a new TestURLToString test and added
> a list of urltostringtests, for now containing only the two cases
> involved with this issue)
>
> Please review this at https://codereview.appspot.com/14654045/
>
> Affected files (+46, -19 lines):
>   M src/pkg/net/url/url.go
>   M src/pkg/net/url/url_test.go
>
>
> Index: src/pkg/net/url/url.go
> ===================================================================
> --- a/src/pkg/net/url/url.go
> +++ b/src/pkg/net/url/url.go
> @@ -459,10 +459,12 @@
>                                 buf.WriteString(h)
>                         }
>                 }
> -               if u.Path != "" && u.Path[0] != '/' {
> -                       buf.WriteByte('/')
> +               if u.Path != "" {
> +                       if u.Path[0] != '/' && buf.Len() > 0 {
> +                               buf.WriteByte('/')
> +                       }
> +                       buf.WriteString(escape(u.Path, encodePath))
>                 }
> -               buf.WriteString(escape(u.Path, encodePath))
>         }
>         if u.RawQuery != "" {
>                 buf.WriteByte('?')
> Index: src/pkg/net/url/url_test.go
> ===================================================================
> --- a/src/pkg/net/url/url_test.go
> +++ b/src/pkg/net/url/url_test.go
> @@ -168,6 +168,14 @@
>                 },
>                 "",
>         },
> +       // relative path should remain relative
> +       {
> +               "path/to/foo",
> +               &URL{
> +                       Path: "path/to/foo",
> +               },
> +               "",
> +       },
>         // Three leading slashes isn't an authority, but doesn't return an
> error.
>         // (We can't return an error, as this code is also used via
>         // ServeHTTP -> ReadRequest -> Parse, which is arguably a
> @@ -372,22 +380,6 @@
>
>  func TestURLString(t *testing.T) {
>         DoTestString(t, Parse, "Parse", urltests)
> -
> -       // no leading slash on path should prepend
> -       // slash on String() call
> -       noslash := URLTest{
> -               "http://www.google.com/search",
> -               &URL{
> -                       Scheme: "http",
> -                       Host:   "www.google.com",
> -                       Path:   "search",
> -               },
> -               "",
> -       }
> -       s := noslash.out.String()
> -       if s != noslash.in {
> -               t.Errorf("Expected %s; go %s", noslash.in, s)
> -       }
>  }
>
>  type EscapeTest struct {
> @@ -875,6 +867,39 @@
>         }
>  }
>
> +type URLToStringTest struct {
> +       url *URL
> +       out string
> +}
> +
> +var urltostringtests = []URLToStringTest{
> +       // no leading slash on path should prepend slash on String() call
> +       {
> +               &URL{
> +                       Scheme: "http",
> +                       Host:   "www.google.com",
> +                       Path:   "search",
> +               },
> +               "http://www.google.com/search",
> +       },
> +       // a relative path should remain relative
> +       {
> +               &URL{
> +                       Path: "search",
> +               },
> +               "search",
> +       },
> +}
> +
> +func TestURLToString(t *testing.T) {
> +       for _, tt := range urltostringtests {
> +               s := tt.url.String()
> +               if s != tt.out {
> +                       t.Errorf("%q.String() == %q (expected %q)", tt.url,
> s, tt.out)
> +               }
> +       }
> +}
> +
>  func TestParseFailure(t *testing.T) {
>         // Test that the first parse error is returned.
>         const url = "%gh&%ij"
>
>
> --
>
> ---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.

[dave_cheney.net, 2013-11-02 07:04:21]

ping! go 1.2 is very close to shipping.

[minux1, 2013-11-02 07:23:06]

This issue has already been fixed by https://codereview.appspot.com/14815043.

[gobot, 2013-12-20 16:26:03]

Replacing golang-dev with golang-codereviews.

[dave_cheney.net, 2013-12-20 21:26:22]

On 2013/12/20 16:26:03, gobot wrote:
> Replacing golang-dev with golang-codereviews.

R=close