code review 4252044: path/filepath: New OS-specific path support

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.

[niemeyer, 2011-02-28 22:56:22]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[niemeyer, 2011-02-28 22:59:27]

Figuring some of the changes may be a bit less comfortable
due to the copying of the files.  If you want, there's a
detailed history with diffs of the individual changes at:

https://github.com/niemeyer/golang/commits/4252044-filepath-split

[gri, 2011-02-28 23:54:43]

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#newcod...
src/cmd/godoc/godoc.go:250: // BUG(niemeyer): Slashed and OS-specific paths are
being mixed in several places.
To make this clean in general, there will be a need for a set of conversion
functions that takes a path extracted from a URL and converts it into a file
path, and vice versa. Explain and add a TODO.

http://codereview.appspot.com/4252044/diff/1002/src/cmd/godoc/godoc.go#newcod...
src/cmd/godoc/godoc.go:726: 
The style in this file is two empty lines between functions.

http://codereview.appspot.com/4252044/diff/1002/src/cmd/godoc/mapping.go
File src/cmd/godoc/mapping.go (right):

http://codereview.appspot.com/4252044/diff/1002/src/cmd/godoc/mapping.go#newc...
src/cmd/godoc/mapping.go:157: 
two empty lines between functions please

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/Makefile
File src/pkg/path/filepath/Makefile (right):

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/Makefil...
src/pkg/path/filepath/Makefile:1: # Copyright 2009 The Go Authors. All rights
reserved.
2011

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/match.go
File src/pkg/path/filepath/match.go (right):

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/match.g...
src/pkg/path/filepath/match.go:1: package filepath
needs a copyright notice

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/match.g...
src/pkg/path/filepath/match.go:13: // The syntax used by pattern is:
The pattern syntax is:

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/match.g...
src/pkg/path/filepath/match.go:78: // scanChunk gets the next section of
pattern, which is a non-star string
the next chunk of pattern ?

(or use "segment" instead of "chunk" ?)

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/match.g...
src/pkg/path/filepath/match.go:95: continue
this continue is superfluous

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/match.g...
src/pkg/path/filepath/match.go:277: return strings.IndexAny(path, "*?[") != -1
s/!= -1/>= 0/

no need to know that the failure value is -1

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/match.g...
src/pkg/path/filepath/match.go:277: return strings.IndexAny(path, "*?[") != -1
there seem to be more magic characters in the syntax

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/match_t...
File src/pkg/path/filepath/match_test.go (right):

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/match_t...
src/pkg/path/filepath/match_test.go:1: // Copyright 2009 The Go Authors. All
rights reserved.
2011

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...
src/pkg/path/filepath/path.go:1: // Copyright 2009 The Go Authors. All rights
reserved.
2011

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/path_te...
File src/pkg/path/filepath/path_test.go (right):

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/path_te...
src/pkg/path/filepath/path_test.go:1: // Copyright 2009 The Go Authors. All
rights reserved.
2011

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/path_un...
File src/pkg/path/filepath/path_unix.go (right):

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/path_un...
src/pkg/path/filepath/path_unix.go:1: // Copyright 2010 The Go Authors. All
rights reserved.
2011

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/path_wi...
File src/pkg/path/filepath/path_windows.go (right):

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/path_wi...
src/pkg/path/filepath/path_windows.go:1: // Copyright 2010 The Go Authors. All
rights reserved.
2011

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/match.go
File src/pkg/path/match.go (right):

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/match.go#newcode1
src/pkg/path/match.go:1: package path
needs a copyright notice

[mattn, 2011-03-01 00:19:27]

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 (left):

http://codereview.appspot.com/4252044/diff/1002/src/cmd/godoc/dirtrees.go#old...
src/cmd/godoc/dirtrees.go:17: pathutil "path"
(For my note, I should change the part that checking '/' as path separator using
fileutil.IsDirSep(). perhaps, include my CL:
http://codereview.appspot.com/3989064/)

[rsc, 2011-03-01 04:58:50]

On Mon, Feb 28, 2011 at 19:19,  <mattn.jp@gmail.com> wrote:
> Have I better to merge in another CL for my windows patch?

Windows support is still a work in progress for all of Go.
This CL is about creating a filepath that works on Unix and
updating the various files that should use it.  A second CL
can fix the Windows support in filepath separately.

Russ

[rsc, 2011-03-01 05:01:03]

If you copy the filepath files somewhere else and then
remove them and hg remove them, you should be able
to do

hg cp path/match.go filepath/match.go
etc

and then copy the real ones back, and we'll see more
useful diffs.

Please give that a shot.  It may not work (yay Mercurial)
but I think it is likely to work.

Russ

[brainman, 2011-03-01 05:42:53]

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...
src/pkg/path/filepath/path.go:16: // BUG(niemeyer): Windows support is missing
in Clean, Join, Ext, Walk, Base, IsAbs and Match.
s/BUG/TODO/

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/path.go...
src/pkg/path/filepath/path.go:36: func Clean(path string) string {
Shouldn't you just call path.Clean instead?
For now these two are identical anyway.
Same for many others.

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/path.go...
src/pkg/path/filepath/path.go:109: const pathSeps = Separator + VolumeSeparator
This won't work for filenames like "tmp/test.txt" on windows. I'm sure we want
to keep these working.

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/path_te...
File src/pkg/path/filepath/path_test.go (right):

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/path_te...
src/pkg/path/filepath/path_test.go:6: 
The test fails on windows:

--- FAIL: filepath.TestGlob (0.0 seconds)
	Glob(`../*/match.go`) = []string{} want ../filepath/match.go
--- FAIL: filepath.TestWalk (0.4 seconds)
	1. error expected, none found
	node testdata/d/x mark = 2; expected 1
	node testdata/d/y mark = 2; expected 1
	node testdata/d/z mark = 2; expected 1
	node testdata/d/z/u mark = 2; expected 1
	node testdata/d/z/v mark = 2; expected 1
FAIL

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/path.go
File src/pkg/path/path.go (right):

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/path.go#newcode160
src/pkg/path/path.go:160: // TODO: Add Windows support
Please, remove TODO.

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/path.go#newcode161
src/pkg/path/path.go:161: return strings.HasPrefix(path, "/")
Perhaps, you could inline it:
return len(path) > 0 && path[0] == '/'

[niemeyer, 2011-03-01 22:14:06]

All comments addressed and/or replied to.

Russ, I've followed your suggestion (using hg cp --after), and that has indeed
produced reasonable diffs.  Thanks.

Should be ready for another look.

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#newcod...
src/cmd/godoc/godoc.go:250: // BUG(niemeyer): Slashed and OS-specific paths are
being mixed in several places.
Changed to a TODO and added those notes.

http://codereview.appspot.com/4252044/diff/1002/src/cmd/godoc/godoc.go#newcod...
src/cmd/godoc/godoc.go:726: 
Sorry. Removed the space by mistake when taking a comment out.

http://codereview.appspot.com/4252044/diff/1002/src/cmd/godoc/mapping.go
File src/cmd/godoc/mapping.go (right):

http://codereview.appspot.com/4252044/diff/1002/src/cmd/godoc/mapping.go#newc...
src/cmd/godoc/mapping.go:157: 
Fixed.

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/Makefile
File src/pkg/path/filepath/Makefile (right):

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/Makefil...
src/pkg/path/filepath/Makefile:1: # Copyright 2009 The Go Authors. All rights
reserved.
I believe 2009 is right.  The content was just copied over.

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/match.go
File src/pkg/path/filepath/match.go (right):

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/match.g...
src/pkg/path/filepath/match.go:1: package filepath
> needs a copyright notice

Added a 2010 (original creation) notice on both path/match.go and
filepath/match.go.

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/match.g...
src/pkg/path/filepath/match.go:13: // The syntax used by pattern is:
> The pattern syntax is:

Fixed all of these style issues in both filepath and path
itself.  Please note that the content was just copied over
in this and in the cases below.

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/match.g...
src/pkg/path/filepath/match.go:277: return strings.IndexAny(path, "*?[") != -1
> there seem to be more magic characters in the syntax

That was also copied over, and it's a semantic change
which requires a proper test in its own CL, so I've
just added a comment for now.

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...
src/pkg/path/filepath/path.go:16: // BUG(niemeyer): Windows support is missing
in Clean, Join, Ext, Walk, Base, IsAbs and Match.
> s/BUG/TODO/

This is really a bug IMO.  The filepath package now promises to be OS-specific,
but cannot handle that specific OS.  Just yesterday we had a report about it in
the mailing list.

It should be fixed very shortly, in a separate CL, but right now it's indeed a
bug.

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/path.go...
src/pkg/path/filepath/path.go:36: func Clean(path string) string {
> Shouldn't you just call path.Clean instead?

I pondered about it too, but the next CL which implements OS-specific logic will
have to diverge.  I suggest waiting
until this is made generic before dropping logic.

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/path.go...
src/pkg/path/filepath/path.go:109: const pathSeps = Separator + VolumeSeparator
> This won't work for filenames like "tmp/test.txt" on
> windows. I'm sure we want

Is that a valid path on Windows?

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/path_te...
File src/pkg/path/filepath/path_test.go (right):

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/filepath/path_te...
src/pkg/path/filepath/path_test.go:6: 
> The test fails on windows:

Both Glob and Match were OS-specific, so they were (and continue to be) broken
on Windows.  A following CL should address this.

Should the test be disabled for the moment?

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/path.go
File src/pkg/path/path.go (right):

http://codereview.appspot.com/4252044/diff/1002/src/pkg/path/path.go#newcode161
src/pkg/path/path.go:161: return strings.HasPrefix(path, "/")
Sounds good, thanks.

[brainman, 2011-03-01 23:57:06]

On 2011/03/01 22:14:06, niemeyer wrote:

> > This won't work for filenames like "tmp/test.txt" on
> > windows. I'm sure we want
> 
> Is that a valid path on Windows?
> 

It sure works in some situations:

C:\TMP>mkdir tmp

C:\TMP>echo alex > tmp/test.txt

C:\TMP>dir tmp
 Volume in drive C is Local Disk
 Volume Serial Number is C458-E628

 Directory of C:\TMP\tmp

02/03/2011  10:33a      <DIR>          .
02/03/2011  10:33a      <DIR>          ..
02/03/2011  10:33a                   7 test.txt
               1 File(s)              7 bytes
               2 Dir(s)   5,476,302,848 bytes free

C:\TMP>

Also, from http://msdn.microsoft.com/en-us/library/aa365247%28v=vs.85%29.aspx:

"... Note  File I/O functions in the Windows API convert "/" to "\" as part of
converting the name to an NT-style name ..."

> > The test fails on windows:
> 
> Both Glob and Match were OS-specific, so they were (and continue to be) broken
> on Windows.  A following CL should address this.
> 

Sure.

> Should the test be disabled for the moment?

I don't think test should be disabled for the moment.
These test work now. They should work after this CL is applied.
As Russ said, let's change nothing, let's just rearrange things (split path into
path + filepath).

Alex

[niemeyer, 2011-03-02 10:41:17]

> It sure works in some situations:
(...)
> "... Note  File I/O functions in the Windows API convert "/" to "\" as part of
> converting the name to an NT-style name ..."

Interesting, thanks for pointing that out.

Can you please verify if the two separators can be mixed as well?

> These test work now. They should work after this CL is applied.
> As Russ said, let's change nothing

The test broke also because of the handling of slashes,
so if slashed paths are supposed to be supported in
Windows, I'll certainly fix that.
 
Either way, don't worry too much about it.  These functions
were really broken on Windows before, and will continue to be.
The test passes just because of the paths selected for testing.
Let's handle the splitting properly so that the real fixes for
Windows can move on.

[brainman, 2011-03-02 11:18:49]

On 2011/03/02 10:41:17, niemeyer wrote:
> Can you please verify if the two separators can be mixed as well?
> 

It looks like it does:

C:\Documents and Settings\aa>mkdir tmp

C:\Documents and Settings\aa>mkdir tmp\tmp2

C:\Documents and Settings\aa>echo alex > tmp/tmp2\a

C:\Documents and Settings\aa>echo alex > tmp\tmp2/b

C:\Documents and Settings\aa>echo alex > tmp/tmp2/c

C:\Documents and Settings\aa>echo alex > tmp\tmp2\d

C:\Documents and Settings\aa>echo alex > tmp\tmp2\\e

C:\Documents and Settings\aa>echo alex > tmp\tmp2\/f

C:\Documents and Settings\aa>

Alex

[niemeyer, 2011-03-02 19:45:24]

> It looks like it does:

Thanks for checking that out.

> C:\Documents and Settings\aa>echo alex > tmp\tmp2\/f

Ugh.. that's unfortunate.

Separators are now multiple entries again.  We'll have to
establish some kind of special role for the very first entry
there, for usage in Join and similar, but that should be
alright.

Please take another look.

[rsc, 2011-03-02 19:47:20]

please drop the windows support in this CL.
just use path_unix.go for the windows build.
i want to keep windows separate.

[niemeyer, 2011-03-02 20:03:43]

> please drop the windows support in this CL.
> just use path_unix.go for the windows build.
> i want to keep windows separate.

That's done. PTAL

[niemeyer, 2011-03-02 21:46:41]

> That's done. PTAL

By the way, one alternative to having filepath.Separators
being a set of separators would be having it as the default
separator only (Separator = "\\", for Windows) and then
having a separate public or private variable with the full
set to be used in Split, so that it can deal with slashed
paths as well.

How do you feel about this?

I still feel slightly awkward about the multiple conventions
in Windows and having to have a list of chars in Separators,
but maybe it's still the surprise of mixed paths being
accepted.

[rsc, 2011-03-02 22:10:58]

On Wed, Mar 2, 2011 at 16:46,  <n13m3y3r@gmail.com> wrote:
>> That's done. PTAL
>
> By the way, one alternative to having filepath.Separators
> being a set of separators would be having it as the default
> separator only (Separator = "\\", for Windows) and then
> having a separate public or private variable with the full
> set to be used in Split, so that it can deal with slashed
> paths as well.
>
> How do you feel about this?

i want separator to be a char not a string

[niemeyer, 2011-03-02 23:40:09]

> i want separator to be a char not a string

Feels much better.  PTAL

[rsc, 2011-03-03 16:14:13]

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):

http://codereview.appspot.com/4252044/diff/21003/src/pkg/path/filepath/match....
src/pkg/path/filepath/match.go:213: // /usr/*/bin/ed, assuming Separator is '/'.
/usr/*/bin/ed (assuming the Separator is '/').

http://codereview.appspot.com/4252044/diff/21003/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

http://codereview.appspot.com/4252044/diff/21003/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:6: // filename paths in a way compatible with the
target operating
// operating system-defined file paths.

http://codereview.appspot.com/4252044/diff/21003/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:16: // BUG(niemeyer): Windows support is missing
in Clean, Join, Ext, Walk, Base, IsAbs and Match.
// BUG(niemeyer): Package filepath does not yet work on Windows.

http://codereview.appspot.com/4252044/diff/21003/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:111: // In operating systems where VolumeSeparator
is not empty and
delete this sentence

http://codereview.appspot.com/4252044/diff/21003/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:116: func Split(path string) (base, file string) {
why did this get renamed to base?
it's very different than, say, filepath.Base.

http://codereview.appspot.com/4252044/diff/21003/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:134: // in the final Separator-partitioned element
of path;
s/Separator-partitioned //

http://codereview.appspot.com/4252044/diff/21003/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:193: // Base returns the last path element of the
Separator-partitioned name.
rename name to path

// Base returns the last element of path.
// Trailing path separators are removed before extracting the last element.
// If the path is empty, Base returns ".".
// If the path consists entirely of separators, Base returns a single separator.

[rsc, 2011-03-03 16:37:50]

@gri: please read over the godoc changes.

Even though we are not doing Windows support for package filepath,
if we're going to go through path vs filepath we might as well make
the changes so that it will be correct once filepath does Windows.

I think we should add the below functions, for godoc and probably
others to use:

package filepath

// ToSlash returns the result of replacing each separator character
// in path with a slash ('/') character.
func ToSlash(path string) string

// FromSlash returns the result of replacing each slash ('/') character
// in path with a separator character.
func FromSlash(path string) string

I bet http's file server could make use of these too.

http://codereview.appspot.com/4252044/diff/21003/src/cmd/godoc/godoc.go
File src/cmd/godoc/godoc.go (right):

http://codereview.appspot.com/4252044/diff/21003/src/cmd/godoc/godoc.go#newco...
src/cmd/godoc/godoc.go:250: // TODO(niemeyer): Slashed and OS-specific paths are
being mixed in several places.
// Absolute paths are file system paths (backslash-separated on Windows),
// but relative paths are always slash-separated.

http://codereview.appspot.com/4252044/diff/21003/src/cmd/godoc/godoc.go#newco...
src/cmd/godoc/godoc.go:258: abspath = filepath.Join(defaultRoot, path)
needs a conversion of path / to \.

http://codereview.appspot.com/4252044/diff/21003/src/cmd/godoc/godoc.go#newco...
src/cmd/godoc/godoc.go:264: func relativePath(path string) string {
relativeURL

http://codereview.appspot.com/4252044/diff/21003/src/cmd/godoc/godoc.go#newco...
src/cmd/godoc/godoc.go:267: // prefix must end in '/'
needs a conversion of \ to /

http://codereview.appspot.com/4252044/diff/21003/src/cmd/godoc/mapping.go
File src/cmd/godoc/mapping.go (right):

http://codereview.appspot.com/4252044/diff/21003/src/cmd/godoc/mapping.go#new...
src/cmd/godoc/mapping.go:84: pathlist := canonicalizePaths(strings.Split(paths,
":", -1), nil)
I think we will need to introduce filepath.ListSeparator which would be ':' on
Unix and ';' on Windows.

http://codereview.appspot.com/4252044/diff/21003/src/cmd/godoc/mapping.go#new...
src/cmd/godoc/mapping.go:159: // ToAbsolute maps a relative filesystem path to
an absolute filesystem
s/relative filesystem path/slash-separated relative path/

http://codereview.appspot.com/4252044/diff/21003/src/cmd/godoc/mapping.go#new...
src/cmd/godoc/mapping.go:164: prefix, tail := splitFirst(path)
if filepath.Separator != '/' {
    tail = strings.Replace(tail, "/", string(filepath.Separator), -1)
}

http://codereview.appspot.com/4252044/diff/21003/src/cmd/godoc/mapping.go#new...
src/cmd/godoc/mapping.go:184: // ToRelative maps an absolute filesystem path to
a relative filesystem
s2/filesystem/slash-separated/

[s2 means replace second match]

http://codereview.appspot.com/4252044/diff/21003/src/cmd/godoc/mapping.go#new...
src/cmd/godoc/mapping.go:191: // /absolute/prefix/foo -> prefix/foo
convert slashes here

http://codereview.appspot.com/4252044/diff/21003/src/cmd/goinstall/download.go
File src/cmd/goinstall/download.go (right):

http://codereview.appspot.com/4252044/diff/21003/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:39: // BUG(niemeyer): Call sites of vcsCheckout
mix slashed and OS-specific paths.
s/BUG/TODO/

BUG appears in godoc output, which has no business mentioning internal function
names like vcsCheckout.

http://codereview.appspot.com/4252044/diff/21003/src/cmd/goinstall/main.go
File src/cmd/goinstall/main.go (right):

http://codereview.appspot.com/4252044/diff/21003/src/cmd/goinstall/main.go#ne...
src/cmd/goinstall/main.go:163: // BUG(niemeyer): Slashed path from package must
be converted to OS-specific path.
s/BUG/TODO/

http://codereview.appspot.com/4252044/diff/21003/src/cmd/goinstall/parse.go
File src/cmd/goinstall/parse.go (right):

http://codereview.appspot.com/4252044/diff/21003/src/cmd/goinstall/parse.go#n...
src/cmd/goinstall/parse.go:10: "path/filepath"
while you are here, sort this list

http://codereview.appspot.com/4252044/diff/21003/src/pkg/patch/patch.go
File src/pkg/patch/patch.go (right):

http://codereview.appspot.com/4252044/diff/21003/src/pkg/patch/patch.go#newco...
src/pkg/patch/patch.go:115: p.Dst = filepath.Clean(p.Dst)
I don't know about this one.  
It depends on whether Mercurial and Git print \ on Windows
in their patches.  I am guessing probably not, and you didn't
change the lines that follow anyway, so let's put this one back
to path.

[gri, 2011-03-03 18:21:09]

FYI.

I think the godoc changes look good so far. Of course we will find out for sure
once we run it...

http://codereview.appspot.com/4252044/diff/21003/src/cmd/godoc/mapping.go
File src/cmd/godoc/mapping.go (right):

http://codereview.appspot.com/4252044/diff/21003/src/cmd/godoc/mapping.go#new...
src/cmd/godoc/mapping.go:164: prefix, tail := splitFirst(path)
On 2011/03/03 16:37:50, rsc wrote:
> if filepath.Separator != '/' {
>     tail = strings.Replace(tail, "/", string(filepath.Separator), -1)
> }

What if / is a legal character in a name? Perhaps not an issue in practice?

[rsc, 2011-03-03 19:07:22]

On Thu, Mar 3, 2011 at 13:21,  <gri@golang.org> wrote:
> FYI.
>
> I think the godoc changes look good so far. Of course we will find out
> for sure once we run it...
>
>
> http://codereview.appspot.com/4252044/diff/21003/src/cmd/godoc/mapping.go
> File src/cmd/godoc/mapping.go (right):
>
>
http://codereview.appspot.com/4252044/diff/21003/src/cmd/godoc/mapping.go#new...
> src/cmd/godoc/mapping.go:164: prefix, tail := splitFirst(path)
> On 2011/03/03 16:37:50, rsc wrote:
>>
>> if filepath.Separator != '/' {
>>     tail = strings.Replace(tail, "/", string(filepath.Separator), -1)
>> }
>
> What if / is a legal character in a name? Perhaps not an issue in
> practice?

in practice / is treated as a separator on every system we use.
even on windows while \ is preferred / is recognized.

[brainman, 2011-03-03 22:21:42]

On 2011/03/03 18:21:09, gri wrote:
> 
> What if / is a legal character in a name? ...

/ is not legal character in a name on Windows.

From http://msdn.microsoft.com/en-us/library/aa365247%28v=VS.85%29.aspx:

"... 

Use any character in the current code page for a name, including Unicode
characters and characters in the extended character set (128–255), except for
the following:

    *

      The following reserved characters:
          o < (less than)
          o > (greater than)
          o : (colon)
          o " (double quote)
          o / (forward slash)
          o \ (backslash)
          o | (vertical bar or pipe)
          o ? (question mark)
          o * (asterisk)

..."

[brainman, 2011-03-03 22:31:46]

On 2011/03/03 19:07:22, rsc wrote:
> 
> in practice / is treated as a separator on every system we use.
> even on windows while \ is preferred / is recognized.

From what I've seen so far, all Windows API accept / in place of \.

But this rule doesn't work when applied to individual programs. Passing file
name with / instead of \ will trick most of Windows programs. In fact / is often
used as a start of optional parameters (unix equivalent of -). Here to
demonstrate:

C:\>mkdir tt

C:\>echo > tt\a.txt

C:\>dir tt
 Volume in drive C is Local Disk
 Volume Serial Number is C458-E628

 Directory of C:\tt

04/03/2011  09:28a      <DIR>          .
04/03/2011  09:28a      <DIR>          ..
04/03/2011  09:28a                  13 a.txt
               1 File(s)             13 bytes
               2 Dir(s)   5,175,754,752 bytes free

C:\>dir \tt
 Volume in drive C is Local Disk
 Volume Serial Number is C458-E628

 Directory of C:\tt

04/03/2011  09:28a      <DIR>          .
04/03/2011  09:28a      <DIR>          ..
04/03/2011  09:28a                  13 a.txt
               1 File(s)             13 bytes
               2 Dir(s)   5,175,754,752 bytes free

C:\>dir /tt
Parameter format not correct - "t".

C:\>dir /?
Displays a list of files and subdirectories in a directory.

DIR [drive:][path][filename] [/A[[:]attributes]] [/B] [/C] [/D] [/L] [/N]
  [/O[[:]sortorder]] [/P] [/Q] [/S] [/T[[:]timefield]] [/W] [/X] [/4]

  [drive:][path][filename]
              Specifies drive, directory, and/or files to list.

  /A          Displays files with specified attributes.
  attributes   D  Directories                R  Read-only files
               H  Hidden files               A  Files ready for archiving
               S  System files               -  Prefix meaning not
  /B          Uses bare format (no heading information or summary).
  /C          Display the thousand separator in file sizes.  This is the
              default.  Use /-C to disable display of separator.
  /D          Same as wide but files are list sorted by column.
  /L          Uses lowercase.
  /N          New long list format where filenames are on the far right.
  /O          List by files in sorted order.
  sortorder    N  By name (alphabetic)       S  By size (smallest first)
               E  By extension (alphabetic)  D  By date/time (oldest first)
               G  Group directories first    -  Prefix to reverse order
  /P          Pauses after each screenful of information.
  /Q          Display the owner of the file.
  /S          Displays files in specified directory and all subdirectories.
  /T          Controls which time field displayed or used for sorting
  timefield   C  Creation
              A  Last Access
              W  Last Written
  /W          Uses wide list format.
  /X          This displays the short names generated for non-8dot3 file
              names.  The format is that of /N with the short name inserted
              before the long name. If no short name is present, blanks are
              displayed in its place.
  /4          Displays four-digit years

Switches may be preset in the DIRCMD environment variable.  Override
preset switches by prefixing any switch with - (hyphen)--for example, /-W.

C:\>

Alex

[rsc, 2011-03-04 02:12:45]

> From what I've seen so far, all Windows API accept / in place of \.
>
> But this rule doesn't work when applied to individual programs. Passing
> file name with / instead of \ will trick most of Windows programs.

fair enough, but command line parsing is always different.
unix allows a file named "-foo" but you can't cat -foo.

russ

[niemeyer, 2011-03-04 11:22:20]

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
File src/pkg/path/filepath/match.go (right):

http://codereview.appspot.com/4252044/diff/21003/src/pkg/path/filepath/match....
src/pkg/path/filepath/match.go:213: // /usr/*/bin/ed, assuming Separator is '/'.
Done.

http://codereview.appspot.com/4252044/diff/21003/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

http://codereview.appspot.com/4252044/diff/21003/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:111: // In operating systems where VolumeSeparator
is not empty and
> delete this sentence

Done.

http://codereview.appspot.com/4252044/diff/21003/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:116: func Split(path string) (base, file string) {
> why did this get renamed to base?
> it's very different than, say, filepath.Base.

Because "C:" is not a directory, and this used to split
the volume as well. Agreed it conflicts with
filepath.Base.  I reverted it for now.

http://codereview.appspot.com/4252044/diff/21003/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:134: // in the final Separator-partitioned element
of path;
> s/Separator-partitioned //

Done.

http://codereview.appspot.com/4252044/diff/21003/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:193: // Base returns the last path element of the
Separator-partitioned name.
> rename name to path
> 
> // Base returns the last element of path.

Done.  I've also made equivalent changes in the generic
'path' package.

[niemeyer, 2011-03-04 11:35:50]

> Even though we are not doing Windows support for package filepath,
> if we're going to go through path vs filepath we might as well make
> the changes so that it will be correct once filepath does Windows.

Sounds good. I'll handle these in my flight back home tomorrow.
Should be up for review on Monday.

[niemeyer, 2011-03-06 19:00:36]

> Sounds good. I'll handle these in my flight back home tomorrow.
> Should be up for review on Monday.

Alright, filepath now contains FromSlash, ToSlash, ListSeparator,
and SplitList.  Also, godoc, goinstall, and ioutil/tempfile were
changed to make use of filepath.

This is getting rather large, so I suggest we try to focus on
polishing the existing changes, and handle any other previously
existing issues in separate CLs.

Again, if you want a more fine grained sequential list of changes,
the following is up-to-date:

https://github.com/niemeyer/golang/commits/4252044-filepath-split

PTAL

[rsc, 2011-03-06 19:09:31]

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#new...
src/cmd/godoc/mapping.go:75: // For instance, in a Linux OS, the argument:
let's not mention linux unless absolutely necessary.

For instance, under Unix, the argument:

[niemeyer, 2011-03-06 19:20:08]

> For instance, under Unix, the argument:

Done.

[rsc, 2011-03-06 20:23:14]

LGTM

I tried to apply this change but it has a circular
dependency: path/filepath -> io/ioutil -> path/filepath.

For now please just copy the implementation of
ioutil.ReadDir into filepath (as readDir) so that
Walk can use it.

Russ

[niemeyer, 2011-03-06 21:26:18]

> I tried to apply this change but it has a circular
> dependency: path/filepath -> io/ioutil -> path/filepath.

Oops, sorry.  Also had to change the test module name,
since there was another loop with testing.

Should be good now.

[rsc, 2011-03-06 22:33:29]

*** Submitted as http://code.google.com/p/go/source/detail?r=9fa1ce8c4404 ***

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.

R=rsc, gri, mattn, brainman
CC=golang-dev
http://codereview.appspot.com/4252044

Committer: Russ Cox <rsc@golang.org>

[rsc1, 2011-03-09 04:28:04]

No.

path_windows.go can do whatever it needs to do for Windows
but Separator is defined as the canonical separator character
for that operating system.  For Windows, that is \.

Normal code should call the path routines instead of
trying to recreate the functionality by looking at the
Separator.

Russ

[rsc, 2011-03-09 04:48:02]

> 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