code review 11219045: go.talks/present: URL use `/` instead of `\` on windows

go.talks/present: URL use `/` instead of `\` on windows

Fixes issue 5876.

[chai2010, 2013-07-13 00:36:12]

Hello golang-dev@googlegroups.com,

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

[dave_cheney.net, 2013-07-13 01:33:27]

Please add a test if possible.

https://codereview.appspot.com/11219045/diff/6001/present/dir.go
File present/dir.go (right):

https://codereview.appspot.com/11219045/diff/6001/present/dir.go#newcode132
present/dir.go:132: Path: strings.Replace(filepath.Join(name, fi.Name()), `\`,
`/`, -1),
why not use path.Join ?

[chai2010, 2013-07-13 01:50:58]

sorry, i don't know how to add this test.
can you give me some suggest?

https://codereview.appspot.com/11219045/diff/6001/present/dir.go
File present/dir.go (right):

https://codereview.appspot.com/11219045/diff/6001/present/dir.go#newcode132
present/dir.go:132: Path: strings.Replace(filepath.Join(name, fi.Name()), `\`,
`/`, -1),
On 2013/07/13 01:33:27, dfc wrote:
> why not use path.Join ?

I don't know what the separator uesd in path.Join (godoc not said).
if it use os.PathSeparator on windows(`\`), it will cause this issue.

[dave_cheney.net, 2013-07-13 01:54:41]

On 2013/07/13 01:50:58, chai2010 wrote:
> sorry, i don't know how to add this test.
> can you give me some suggest?

I didn't realise this was a main package, that makes testing a bit harder. Don't
worry about it.

> I don't know what the separator uesd in path.Join (godoc not said).
> if it use os.PathSeparator on windows(`\`), it will cause this issue.

package path
    import "path"

    Package path implements utility routines for manipulating
    slash-separated paths.

the path pkg deals with ideal (slash separated) paths, which map to urls well.
pkg path/filepath deals with operating specific path separators.

[chai2010, 2013-07-13 02:07:30]

On 2013/07/13 01:54:41, dfc wrote:
> On 2013/07/13 01:50:58, chai2010 wrote:
> > sorry, i don't know how to add this test.
> > can you give me some suggest?
> 
> I didn't realise this was a main package, that makes testing a bit harder.
Don't
> worry about it.
> 
> > I don't know what the separator uesd in path.Join (godoc not said).
> > if it use os.PathSeparator on windows(`\`), it will cause this issue.
> 
> package path
>     import "path"
> 
>     Package path implements utility routines for manipulating
>     slash-separated paths.
> 
> the path pkg deals with ideal (slash separated) paths, which map to urls well.
> pkg path/filepath deals with operating specific path separators.

sorry for my poor english, and thanks for you answer.

i change `filepath.Join` to `path.Join`.

please take another look.

[dave_cheney.net, 2013-07-13 02:14:24]

Thanks for taking a look at this and making it work on windows.

Was there a problem in the first place ? It looked like, with the exception of
one place where the os specific separator was used, '/' was used for both urls
and paths which will work on both windows and linux.

https://codereview.appspot.com/11219045/diff/11001/present/dir.go
File present/dir.go (right):

https://codereview.appspot.com/11219045/diff/11001/present/dir.go#newcode32
present/dir.go:32: name := path.Join(base, r.URL.Path)
not sure about this, name is a path on disk.

https://codereview.appspot.com/11219045/diff/11001/present/dir.go#newcode80
present/dir.go:80: actionTmpl := path.Join(base, "templates/action.tmpl")
Sorry, these two need to be filepath.Join as they are dealing with files on
disk.

https://codereview.appspot.com/11219045/diff/11001/present/dir.go#newcode81
present/dir.go:81: contentTmpl = path.Join(base, "templates", contentTmpl)
also, please fix these two lines so they use the same form.

[chai2010, 2013-07-13 02:40:06]

please take another look.

https://codereview.appspot.com/11219045/diff/11001/present/dir.go
File present/dir.go (right):

https://codereview.appspot.com/11219045/diff/11001/present/dir.go#newcode32
present/dir.go:32: name := path.Join(base, r.URL.Path)
On 2013/07/13 02:14:25, dfc wrote:
> not sure about this, name is a path on disk.

if use filepath.Join, can't visit x.go file (issue5879).

https://codereview.appspot.com/11219045/diff/11001/present/dir.go#newcode80
present/dir.go:80: actionTmpl := path.Join(base, "templates/action.tmpl")
On 2013/07/13 02:14:25, dfc wrote:
> Sorry, these two need to be filepath.Join as they are dealing with files on
> disk.

Done.

https://codereview.appspot.com/11219045/diff/11001/present/dir.go#newcode81
present/dir.go:81: contentTmpl = path.Join(base, "templates", contentTmpl)
On 2013/07/13 02:14:25, dfc wrote:
> also, please fix these two lines so they use the same form.

Done.

[dave_cheney.net, 2013-07-14 09:34:28]

Very close, please revert that one change.

https://codereview.appspot.com/11219045/diff/17001/present/dir.go
File present/dir.go (right):

https://codereview.appspot.com/11219045/diff/17001/present/dir.go#newcode32
present/dir.go:32: name := path.Join(base, r.URL.Path)
please revert this change.

[chai2010, 2013-07-14 10:31:46]

i don't know does path.Join(name, fi.Name()) return path can contain `\`
character.
if can't contain `\` character, maybe we need fix path.Join issue.

https://codereview.appspot.com/11219045/diff/17001/present/dir.go
File present/dir.go (right):

https://codereview.appspot.com/11219045/diff/17001/present/dir.go#newcode32
present/dir.go:32: name := path.Join(base, r.URL.Path)
On 2013/07/14 09:34:28, dfc wrote:
> please revert this change.

Done.

https://codereview.appspot.com/11219045/diff/23001/present/dir.go
File present/dir.go (right):

https://codereview.appspot.com/11219045/diff/23001/present/dir.go#newcode132
present/dir.go:132: Path: strings.Replace(filepath.Join(name, fi.Name()), `\`,
`/`, -1),
if we use path.Join, still can't view x.go file page on windows(issue5879)

pingpong1.go file url:
http://127.0.0.1:3999/advconc%5cpingpong1.go

path.Join(name, fi.Name()) return:
2013\advconc/pingpong1.go

[dave_cheney.net, 2013-07-14 11:00:55]

Please try this suggestion.

https://codereview.appspot.com/11219045/diff/23001/present/dir.go
File present/dir.go (right):

https://codereview.appspot.com/11219045/diff/23001/present/dir.go#newcode132
present/dir.go:132: Path: strings.Replace(filepath.Join(name, fi.Name()), `\`,
`/`, -1),
On 2013/07/14 10:31:47, chai2010 wrote:
> if we use path.Join, still can't view x.go file page on windows(issue5879)
> 
> pingpong1.go file url:
> http://127.0.0.1:3999/advconc%255cpingpong1.go
> 
> path.Join(name, fi.Name()) return:
> 2013\advconc/pingpong1.go

Try filepath.Split then path.Join.

[chai2010, 2013-07-14 11:15:45]

https://codereview.appspot.com/11219045/diff/23001/present/dir.go
File present/dir.go (right):

https://codereview.appspot.com/11219045/diff/23001/present/dir.go#newcode132
present/dir.go:132: Path: strings.Replace(filepath.Join(name, fi.Name()), `\`,
`/`, -1),
On 2013/07/14 11:00:56, dfc wrote:
> On 2013/07/14 10:31:47, chai2010 wrote:
> > if we use path.Join, still can't view x.go file page on windows(issue5879)
> > 
> > pingpong1.go file url:
> > http://127.0.0.1:3999/advconc%25255cpingpong1.go
> > 
> > path.Join(name, fi.Name()) return:
> > 2013\advconc/pingpong1.go
> 
> Try filepath.Split then path.Join.

use filepath.Split and path.Join will also return:
2013\advconc/pingpong1.go

		dir, file := filepath.Split(fi.Name())
		e := dirEntry{
			Name: fi.Name(),
			Path: path.Join(name, dir, file),
		}

[dave_cheney.net, 2013-07-14 11:22:23]

Please try one more thing, I'm sorry this has been such a beast to get right.

https://codereview.appspot.com/11219045/diff/23001/present/dir.go
File present/dir.go (right):

https://codereview.appspot.com/11219045/diff/23001/present/dir.go#newcode132
present/dir.go:132: Path: strings.Replace(filepath.Join(name, fi.Name()), `\`,
`/`, -1),
What about 

Path: fmt.Sprintf("%s/%s/%s", name, dir, file)

[chai2010, 2013-07-14 11:35:53]

https://codereview.appspot.com/11219045/diff/23001/present/dir.go
File present/dir.go (right):

https://codereview.appspot.com/11219045/diff/23001/present/dir.go#newcode132
present/dir.go:132: Path: strings.Replace(filepath.Join(name, fi.Name()), `\`,
`/`, -1),
On 2013/07/14 11:22:23, dfc wrote:
> What about 
> 
> Path: fmt.Sprintf("%s/%s/%s", name, dir, file)

2013\advconc//pingpong1.go

[minux1, 2013-07-14 17:55:25]

how about using filepath.ToSlash?
i think it makes the intent clearer.

[chai2010, 2013-07-14 23:26:26]

On 2013/07/14 17:55:25, minux wrote:
> how about using filepath.ToSlash?
> i think it makes the intent clearer.

filepath.ToSlash is good.

Thanks dave and minux!

[chai2010, 2013-07-14 23:29:05]

PTAL

[adg, 2013-07-15 00:07:52]

Does this really fix both issues?

I can see how it fixes 5879, but not 5876.

Also please edit the change description, with one "Fixes issue n." line for each
issue being fixed (the issue tracker won't close the issues automatically
otherwise.)

[chai2010, 2013-07-15 00:43:10]

On 2013/07/15 00:07:52, adg wrote:
> Does this really fix both issues?
> 
> I can see how it fixes 5879, but not 5876.

URL contain `\` also cause the 5876.
5879 and 5876 are same: 5876 is x.jpg, 5879 is x.go.

> 
> Also please edit the change description, with one "Fixes issue n." line for
each
> issue being fixed (the issue tracker won't close the issues automatically
> otherwise.)

Done.

[adg, 2013-07-15 00:49:59]

LGTM

Okay, then, I've merged the two issues. Make the change description say
"Fixes issue 5876." and I'll submit it.

[chai2010, 2013-07-15 01:07:29]

On 2013/07/15 00:49:59, adg wrote:
> LGTM
> 
> Okay, then, I've merged the two issues. Make the change description say
> "Fixes issue 5876." and I'll submit it.

Done.

[adg, 2013-07-15 01:15:20]

*** Submitted as
https://code.google.com/p/go/source/detail?r=2fa985dc2a92&repo=talks ***

go.talks/present: URL use `/` instead of `\` on windows

Fixes issue 5876.

R=golang-dev, dave, minux.ma, adg
CC=golang-dev
https://codereview.appspot.com/11219045

Committer: Andrew Gerrand <adg@golang.org>