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

Issue 4252044: code review 4252044: path/filepath: New OS-specific path support (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 8 months ago by niemeyer
Modified:
13 years, 1 month ago
Reviewers:
rsc1
CC:
rsc, gri, mattn, brainman, golang-dev
Visibility:
Public.

Description

path/filepath: new OS-specific path support The path package now contains only functions which deal with slashed paths, sensible for any OS when dealing with network paths or URLs. OS-specific functionality has been moved into the new path/filepath package. This also includes fixes for godoc, goinstall and other packages which were mixing slashed and OS-specific paths.

Patch Set 1 #

Patch Set 2 : diff -r 4c0a44d9a0f6 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 4c0a44d9a0f6 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 4c0a44d9a0f6 https://go.googlecode.com/hg/ #

Total comments: 35

Patch Set 5 : diff -r 4c0a44d9a0f6 https://go.googlecode.com/hg/ #

Patch Set 6 : diff -r 4c0a44d9a0f6 https://go.googlecode.com/hg/ #

Patch Set 7 : diff -r 4c0a44d9a0f6 https://go.googlecode.com/hg/ #

Patch Set 8 : diff -r 4c0a44d9a0f6 https://go.googlecode.com/hg/ #

Patch Set 9 : diff -r 4c0a44d9a0f6 https://go.googlecode.com/hg/ #

Patch Set 10 : diff -r 4c0a44d9a0f6 https://go.googlecode.com/hg/ #

Total comments: 26

Patch Set 11 : diff -r 4c0a44d9a0f6 https://go.googlecode.com/hg/ #

Patch Set 12 : diff -r 03da6860bb39 https://go.googlecode.com/hg/ #

Total comments: 1

Patch Set 13 : diff -r 03da6860bb39 https://go.googlecode.com/hg/ #

Patch Set 14 : diff -r 03da6860bb39 https://go.googlecode.com/hg/ #

Patch Set 15 : diff -r 6e0c3b77928a https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+353 lines, -582 lines) Patch
M src/cmd/ebnflint/ebnflint.go View 1 2 chunks +2 lines, -2 lines 0 comments Download
M src/cmd/godoc/dirtrees.go View 1 4 chunks +4 lines, -4 lines 0 comments Download
M src/cmd/godoc/godoc.go View 1 2 3 4 5 6 7 8 9 10 11 15 chunks +33 lines, -30 lines 0 comments Download
M src/cmd/godoc/index.go View 1 4 chunks +4 lines, -4 lines 0 comments Download
M src/cmd/godoc/main.go View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +5 lines, -5 lines 0 comments Download
M src/cmd/godoc/mapping.go View 1 2 3 4 5 6 7 8 9 10 11 12 7 chunks +25 lines, -22 lines 0 comments Download
M src/cmd/godoc/utils.go View 1 4 chunks +6 lines, -6 lines 0 comments Download
M src/cmd/gofmt/gofmt.go View 1 2 chunks +2 lines, -2 lines 0 comments Download
M src/cmd/goinstall/download.go View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +10 lines, -9 lines 0 comments Download
M src/cmd/goinstall/main.go View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +6 lines, -5 lines 0 comments Download
M src/cmd/goinstall/parse.go View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +6 lines, -6 lines 0 comments Download
M src/cmd/govet/govet.go View 1 3 chunks +3 lines, -3 lines 0 comments Download
M src/cmd/hgpatch/main.go View 1 2 chunks +2 lines, -2 lines 0 comments Download
M src/pkg/Makefile View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
M src/pkg/go/parser/interface.go View 1 2 chunks +2 lines, -2 lines 0 comments Download
M src/pkg/go/printer/printer.go View 1 2 chunks +2 lines, -2 lines 0 comments Download
M src/pkg/go/printer/printer_test.go View 1 2 chunks +3 lines, -3 lines 0 comments Download
M src/pkg/go/scanner/scanner.go View 1 3 chunks +4 lines, -4 lines 0 comments Download
M src/pkg/http/fs.go View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +4 lines, -4 lines 0 comments Download
M src/pkg/io/ioutil/tempfile.go View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +3 lines, -4 lines 0 comments Download
M src/pkg/path/Makefile View 1 1 chunk +0 lines, -12 lines 0 comments Download
M src/pkg/path/filepath/Makefile View 1 2 3 4 5 6 7 8 2 chunks +4 lines, -4 lines 0 comments Download
M src/pkg/path/filepath/match.go View 1 2 3 4 5 6 7 8 9 10 10 chunks +17 lines, -13 lines 0 comments Download
M src/pkg/path/filepath/match_test.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +15 lines, -14 lines 0 comments Download
M src/pkg/path/filepath/path.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 9 chunks +99 lines, -41 lines 0 comments Download
M src/pkg/path/filepath/path_test.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 13 chunks +68 lines, -36 lines 0 comments Download
M src/pkg/path/filepath/path_unix.go View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -4 lines 0 comments Download
M src/pkg/path/match.go View 1 2 3 4 5 5 chunks +6 lines, -77 lines 0 comments Download
M src/pkg/path/match_test.go View 1 1 chunk +0 lines, -28 lines 0 comments Download
M src/pkg/path/path.go View 1 2 3 4 5 6 7 8 9 10 3 chunks +14 lines, -64 lines 0 comments Download
M src/pkg/path/path_test.go View 1 3 chunks +0 lines, -159 lines 0 comments Download
R src/pkg/path/path_windows.go View 1 2 5 6 7 8 1 chunk +0 lines, -11 lines 0 comments Download

Messages

Total messages: 34
niemeyer
Hello rsc (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
13 years, 8 months ago (2011-02-28 22:56:22 UTC) #1
niemeyer
Figuring some of the changes may be a bit less comfortable due to the copying ...
13 years, 8 months ago (2011-02-28 22:59:27 UTC) #2
gri
FYI. Superficial stuff from cursory review. - gri http://codereview.appspot.com/4252044/diff/1002/src/cmd/godoc/godoc.go File src/cmd/godoc/godoc.go (right): http://codereview.appspot.com/4252044/diff/1002/src/cmd/godoc/godoc.go#newcode250 src/cmd/godoc/godoc.go:250: // ...
13 years, 8 months ago (2011-02-28 23:54:43 UTC) #3
mattn
Have I better to merge in another CL for my windows patch? http://codereview.appspot.com/4252044/diff/1002/src/cmd/godoc/dirtrees.go File src/cmd/godoc/dirtrees.go ...
13 years, 8 months ago (2011-03-01 00:19:27 UTC) #4
rsc
On Mon, Feb 28, 2011 at 19:19, <mattn.jp@gmail.com> wrote: > Have I better to merge ...
13 years, 8 months ago (2011-03-01 04:58:50 UTC) #5
rsc
If you copy the filepath files somewhere else and then remove them and hg remove ...
13 years, 8 months ago (2011-03-01 05:01:03 UTC) #6
brainman
Thank you for doing this. http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/path.go File src/pkg/path/filepath/path.go (right): http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/path.go#newcode16 src/pkg/path/filepath/path.go:16: // BUG(niemeyer): Windows support ...
13 years, 8 months ago (2011-03-01 05:42:53 UTC) #7
niemeyer
All comments addressed and/or replied to. Russ, I've followed your suggestion (using hg cp --after), ...
13 years, 8 months ago (2011-03-01 22:14:06 UTC) #8
brainman
On 2011/03/01 22:14:06, niemeyer wrote: > > This won't work for filenames like "tmp/test.txt" on ...
13 years, 8 months ago (2011-03-01 23:57:06 UTC) #9
niemeyer
> It sure works in some situations: (...) > "... Note File I/O functions in ...
13 years, 8 months ago (2011-03-02 10:41:17 UTC) #10
brainman
On 2011/03/02 10:41:17, niemeyer wrote: > Can you please verify if the two separators can ...
13 years, 8 months ago (2011-03-02 11:18:49 UTC) #11
niemeyer
> It looks like it does: Thanks for checking that out. > C:\Documents and Settings\aa>echo ...
13 years, 8 months ago (2011-03-02 19:45:24 UTC) #12
rsc
please drop the windows support in this CL. just use path_unix.go for the windows build. ...
13 years, 8 months ago (2011-03-02 19:47:20 UTC) #13
niemeyer
> please drop the windows support in this CL. > just use path_unix.go for the ...
13 years, 8 months ago (2011-03-02 20:03:43 UTC) #14
niemeyer
> That's done. PTAL By the way, one alternative to having filepath.Separators being a set ...
13 years, 8 months ago (2011-03-02 21:46:41 UTC) #15
rsc
On Wed, Mar 2, 2011 at 16:46, <n13m3y3r@gmail.com> wrote: >> That's done. PTAL > > ...
13 years, 8 months ago (2011-03-02 22:10:58 UTC) #16
niemeyer
> i want separator to be a char not a string Feels much better. PTAL
13 years, 8 months ago (2011-03-02 23:40:09 UTC) #17
rsc
I'm happy with path and filepath modulo the below minor comments. http://codereview.appspot.com/4252044/diff/21003/src/pkg/path/filepath/match.go File src/pkg/path/filepath/match.go (right): ...
13 years, 8 months ago (2011-03-03 16:14:13 UTC) #18
rsc
@gri: please read over the godoc changes. Even though we are not doing Windows support ...
13 years, 8 months ago (2011-03-03 16:37:50 UTC) #19
gri
FYI. I think the godoc changes look good so far. Of course we will find ...
13 years, 8 months ago (2011-03-03 18:21:09 UTC) #20
rsc
On Thu, Mar 3, 2011 at 13:21, <gri@golang.org> wrote: > FYI. > > I think ...
13 years, 8 months ago (2011-03-03 19:07:22 UTC) #21
brainman
On 2011/03/03 18:21:09, gri wrote: > > What if / is a legal character in ...
13 years, 8 months ago (2011-03-03 22:21:42 UTC) #22
brainman
On 2011/03/03 19:07:22, rsc wrote: > > in practice / is treated as a separator ...
13 years, 8 months ago (2011-03-03 22:31:46 UTC) #23
rsc
> From what I've seen so far, all Windows API accept / in place of ...
13 years, 8 months ago (2011-03-04 02:12:45 UTC) #24
niemeyer
Uploaded fixes for the first review from Russ on path and filepath. Notes follow. http://codereview.appspot.com/4252044/diff/21003/src/pkg/path/filepath/match.go ...
13 years, 8 months ago (2011-03-04 11:22:20 UTC) #25
niemeyer
> Even though we are not doing Windows support for package filepath, > if we're ...
13 years, 8 months ago (2011-03-04 11:35:50 UTC) #26
niemeyer
> Sounds good. I'll handle these in my flight back home tomorrow. > Should be ...
13 years, 8 months ago (2011-03-06 19:00:36 UTC) #27
rsc
http://codereview.appspot.com/4252044/diff/20038/src/cmd/godoc/mapping.go File src/cmd/godoc/mapping.go (right): http://codereview.appspot.com/4252044/diff/20038/src/cmd/godoc/mapping.go#newcode75 src/cmd/godoc/mapping.go:75: // For instance, in a Linux OS, the argument: ...
13 years, 8 months ago (2011-03-06 19:09:31 UTC) #28
niemeyer
> For instance, under Unix, the argument: Done.
13 years, 8 months ago (2011-03-06 19:20:08 UTC) #29
rsc
LGTM I tried to apply this change but it has a circular dependency: path/filepath -> ...
13 years, 8 months ago (2011-03-06 20:23:14 UTC) #30
niemeyer
> I tried to apply this change but it has a circular > dependency: path/filepath ...
13 years, 8 months ago (2011-03-06 21:26:18 UTC) #31
rsc
*** Submitted as http://code.google.com/p/go/source/detail?r=9fa1ce8c4404 *** path/filepath: new OS-specific path support The path package now contains ...
13 years, 8 months ago (2011-03-06 22:33:29 UTC) #32
rsc1
No. path_windows.go can do whatever it needs to do for Windows but Separator is defined ...
13 years, 8 months ago (2011-03-09 04:28:04 UTC) #33
rsc
13 years, 8 months ago (2011-03-09 04:48:02 UTC) #34
> How do we deal with paths like that:
>
> G:\alex>gofmt -l c:\tmp\go\src\pkg\path\path.go
>
> G:\alex>gofmt -l c:/tmp/go/src/pkg/path/path.go
>
> G:\alex>
>
> ?

They should do the same thing, whatever that is.
filepath.Clean should convert / to \ on Windows.

> I think filepath.Separator is pretty pointless outside of filepath package.

That may be true.  Let's wait and see.

> Another issue with Windows paths, they are non case-sensitive. These
>
> c:\tmp\alex.txt and C:\tmp\alex.txt and C:\Tmp\Alex.TXT
>
> are all the same file. So code like this (from godoc):
>
> func (m *Mapping) ToRelative(fpath string) string {
>        for _, e := range m.list {
>                if strings.HasPrefix(fpath, e.path) {
>                        spath := filepath.ToSlash(fpath)
>                        // /absolute/prefix/foo -> prefix/foo
>                        return path.Join(e.prefix, spath[len(e.path):]) //
> Join will remove
> a trailing '/'
>                }
>        }
>        return "" // no match
> }
>
> with strings.HasPrefix is not going to work.

That code should be okay, because it is only ever translating
paths that it created itself using the prefixes it is looking for.
But in general you're right that that's something to watch out for.

Russ
Sign in to reply to this message.

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