|
|
Descriptiongo.talks/present: URL use `/` instead of `\` on windows
Fixes issue 5876.
Patch Set 1 #Patch Set 2 : diff -r 066b009fee9d https://code.google.com/p/go.talks #Patch Set 3 : diff -r 066b009fee9d https://code.google.com/p/go.talks #
Total comments: 2
Patch Set 4 : diff -r 066b009fee9d https://code.google.com/p/go.talks #
Total comments: 6
Patch Set 5 : diff -r 066b009fee9d https://code.google.com/p/go.talks #
Total comments: 2
Patch Set 6 : diff -r 066b009fee9d https://code.google.com/p/go.talks #
Total comments: 5
Patch Set 7 : diff -r 066b009fee9d https://code.google.com/p/go.talks #Patch Set 8 : diff -r 066b009fee9d https://code.google.com/p/go.talks #Patch Set 9 : diff -r 066b009fee9d https://code.google.com/p/go.talks #MessagesTotal messages: 21
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go.talks
Sign in to reply to this message.
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 ?
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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), }
Sign in to reply to this message.
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)
Sign in to reply to this message.
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
Sign in to reply to this message.
how about using filepath.ToSlash? i think it makes the intent clearer.
Sign in to reply to this message.
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!
Sign in to reply to this message.
PTAL
Sign in to reply to this message.
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.)
Sign in to reply to this message.
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.
Sign in to reply to this message.
LGTM Okay, then, I've merged the two issues. Make the change description say "Fixes issue 5876." and I'll submit it.
Sign in to reply to this message.
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.
Sign in to reply to this message.
*** 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>
Sign in to reply to this message.
|