code review 3989064: path: work for windows. (issue3989064)

[matt...@gmail.com]

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golan...@googlegroups.com,

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

Description:
path: work for windows.
added IsDirSep, IsVolumeSep, IsSame.

Please review this at http://codereview.appspot.com/3989064/

Affected files:
M src/pkg/path/match.go
M src/pkg/path/match_test.go
M src/pkg/path/path.go
M src/pkg/path/path_test.go
M src/pkg/path/path_unix.go
M src/pkg/path/path_windows.go

[matt...@gmail.com]

Hello golan...@googlegroups.com (cc: golan...@googlegroups.com),

Please take another look.

http://codereview.appspot.com/3989064/

[bsie...@gmail.com]

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

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/match.go#newcode23
src/pkg/path/match.go:23: // '\\' c matches character c
I am concerned about using patterns with backslashes in Match. For
example, Match("C:\\*", "C:\autoexec.bat") will not match because "\\*"
matches a literal star, no?

I am not sure how to solve this.

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

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path.go#newcode85
src/pkg/path/path.go:85: buf[w] = DirSeps[0]
Is it really necessary to replace a literal slash with DirSeps[0]?

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path.go#newcode119
src/pkg/path/path.go:119: return Clean(strings.Join(elem[i:],
string(DirSeps[0])))
same as above

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path.go#newcode208
src/pkg/path/path.go:208: // IsVolumeSep return true if VolumeSeps
include the letter.
IsVolumeSep returns true if c is a volume separator character.

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path.go#newcode213
src/pkg/path/path.go:213: // IsDirSep return true if DirSeps include the
letter.
IsDirSep returns true if c is a directory separator character.

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path_test.go
File src/pkg/path/path_test.go (right):

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path_test.go#newcode326
src/pkg/path/path_test.go:326: var basetests []CleanTest
remove, basetests is defined above.

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path_unix.go
File src/pkg/path/path_unix.go (right):

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path_unix.go#newcode20
src/pkg/path/path_unix.go:20: return lhs == rhs
On Mac OS X, there are case-insensitive file systems. Should IsSame do
case-insensitive matching in this case?
(If yes, this would be for another CL.)

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path_windows.go
File src/pkg/path/path_windows.go (right):

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path_windows.go#newcode37
src/pkg/path/path_windows.go:37: // note that this function don't
normalize paths.
s/n/N/
s/don't/does not/

IMHO, the doc comment should mention that it uses different rules on
POSIX and Windows (case-insensitive, forward slash equal to backslash).

http://codereview.appspot.com/3989064/

[matt...@gmail.com]

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

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path.go#newcode85
src/pkg/path/path.go:85: buf[w] = DirSeps[0]

This become normalizer replacing slash to backslash for win32.

c:/go/src/package/runtime to c:\go\src\package\runtime

On 2011/02/04 09:38:47, bsiegert wrote:
> Is it really necessary to replace a literal slash with DirSeps[0]?

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path.go#newcode119
src/pkg/path/path.go:119: return Clean(strings.Join(elem[i:],
string(DirSeps[0])))

on win32, DirSeps[0] is backslash. but slash for unix.

On 2011/02/04 09:38:47, bsiegert wrote:
> same as above

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path.go#newcode208
src/pkg/path/path.go:208: // IsVolumeSep return true if VolumeSeps
include the letter.

On 2011/02/04 09:38:47, bsiegert wrote:
> IsVolumeSep returns true if c is a volume separator character.

Done.

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path.go#newcode213
src/pkg/path/path.go:213: // IsDirSep return true if DirSeps include the
letter.

On 2011/02/04 09:38:47, bsiegert wrote:
> IsDirSep returns true if c is a directory separator character.

Done.

http://codereview.appspot.com/3989064/

[matt...@gmail.com]

Hello golan...@googlegroups.com, bsiegert, r, rsc (cc:

[matt...@gmail.com]

I'll add the comment about slash/backslash.
Thanks for your review.

On 2011/02/04 09:38:47, bsiegert wrote:

> http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/match.go
> File src/pkg/path/match.go (right):

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/match.go#newcode23
> src/pkg/path/match.go:23: // '\\' c matches character c
> I am concerned about using patterns with backslashes in Match. For
example,
> Match("C:\\*", "C:\autoexec.bat") will not match because "\\*" matches
a literal
> star, no?

> I am not sure how to solve this.

> http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path.go
> File src/pkg/path/path.go (right):

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path.go#newcode85
> src/pkg/path/path.go:85: buf[w] = DirSeps[0]

> Is it really necessary to replace a literal slash with DirSeps[0]?

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path.go#newcode119
> src/pkg/path/path.go:119: return Clean(strings.Join(elem[i:],
> string(DirSeps[0])))

> same as above

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path.go#newcode208
> src/pkg/path/path.go:208: // IsVolumeSep return true if VolumeSeps
include the
> letter.

> IsVolumeSep returns true if c is a volume separator character.

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path.go#newcode213
> src/pkg/path/path.go:213: // IsDirSep return true if DirSeps include
the letter.

> IsDirSep returns true if c is a directory separator character.

[matt...@gmail.com]

Ah sorry.

I didn't fix all. X-(

[Benny Siegert]

On Fri, Feb 4, 2011 at 10:54, <matt...@gmail.com> wrote:
> http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path.go#newcode85
> src/pkg/path/path.go:85: buf[w] = DirSeps[0]
> This become normalizer replacing slash to backslash for win32.
>
> c:/go/src/package/runtime to c:\go\src\package\runtime

Not sure if this is a good idea. Does c:\go\src\package\runtime still
match "c:/go/src/package/*"?

--Benny.

[matt...@gmail.com]

[matt...@gmail.com]

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

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/match.go#newcode23
src/pkg/path/match.go:23: // '\\' c matches character c

This is pattern, developer should know / will be match backslash and
backslash are escape. He shouldn't use backslash as path separator.

On 2011/02/04 09:38:47, bsiegert wrote:
> I am concerned about using patterns with backslashes in Match. For
example,
> Match("C:\\*", "C:\autoexec.bat") will not match because "\\*" matches
a literal
> star, no?

> I am not sure how to solve this.

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path_test.go
File src/pkg/path/path_test.go (right):

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path_test.go#newcode326
src/pkg/path/path_test.go:326: var basetests []CleanTest

On 2011/02/04 09:38:47, bsiegert wrote:
> remove, basetests is defined above.

Done.

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path_unix.go
File src/pkg/path/path_unix.go (right):

http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path_unix.go#newcode20
src/pkg/path/path_unix.go:20: return lhs == rhs

Yes, I'll path to another CL.
Thanks for notice.

[matt...@gmail.com]

> Not sure if this is a good idea. Does c:\go\src\package\runtime still
> match "c:/go/src/package/*"?

Argument of Match is not path. it's patter. We shouldn't treat pattern
as path.

http://codereview.appspot.com/3989064/

[matt...@gmail.com]

Or you we should use slash for path separator as default value on
windows, I'll change DirSeps to `/\` from `\/`.

On 2011/02/04 10:02:55, bsiegert wrote:

> On Fri, Feb 4, 2011 at 10:54, <mailto:matt...@gmail.com> wrote:
> >
http://codereview.appspot.com/3989064/diff/6001/src/pkg/path/path.go#newcode85
> > src/pkg/path/path.go:85: buf[w] = DirSeps[0]
> > This become normalizer replacing slash to backslash for win32.
> >

> > c:/go/src/package/runtime to c:\go\src\package\runtime

> Not sure if this is a good idea. Does c:\go\src\package\runtime still
> match "c:/go/src/package/*"?

> --Benny.

http://codereview.appspot.com/3989064/

[Benny Siegert]

On Fri, Feb 4, 2011 at 11:12, <matt...@gmail.com> wrote:
>> Not sure if this is a good idea. Does c:\go\src\package\runtime still
>> match "c:/go/src/package/*"?
>
> Argument of Match is not path. it's patter. We shouldn't treat pattern
> as path.

I always have the following case in mind: you write a program that
accepts file name arguments, and the user gives you "C:\*" as an arg.
On Unix, the shell expands this for you, on Windows you have to call
path.Glob (which uses path.Match internally). How do you treat the
user input? Do you replace backslashes by slashes?

[matt...@gmail.com]

Yes, I guess that we should escape user's input.
For example of perl, to search text with user input from a file,
We should escape the word and pass to regular expression.

$text =~ /\Q$text\E/

On 2011/02/04 10:21:27, bsiegert wrote:

http://codereview.appspot.com/3989064/

[mattn]

If you want to pass backslash as path separator to Match() or Glob(),

We have better to another function like Match(), MatchMeta(), Gob(), GlobMeta().

Or use flag for treating meta character.

  Match(pattern, name string, useMeta boolean)

  Glob(pattern string, useMeta boolean)

Thanks.

- Yasuhiro Matsumoto

[matt...@gmail.com]

[Russ Cox]

I appreciate all the effort here but until the design
question in issue 1423 gets resolved, it's all premature.
As I said yesterday, I'd like to think about what we want
package path to mean before we just start making changes.

Russ

[mattn]

Yes, I know.

Is this issue a problem discussing which is better to be slash and backslash?

However, If we accept slash for default directory separator for windows,

I can make it work only changing DirSeps to `/\` from `\/`. :)

Than holding the problem and staying there, I hope sooner fixing 'godoc' or another application which is not working.

And I can mention to issue 1423 about that some old application uses slash for leading command parameters like 'dir /s'.

- Yasuhiro Matsumoto

[Benny Siegert]

On Mon, Feb 7, 2011 at 01:16, mattn <matt...@gmail.com> wrote:
> Is this issue a problem discussing which is better to be slash and backslash?

How about adding a function to path in order to change the default separator?

--Benny.

[alex.b...@gmail.com]

On 2011/02/04 06:30:52, mattn wrote:
> Hello mailto:golan...@googlegroups.com,

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

Please, note http://code.google.com/p/go/issues/detail?id=1483.

Perhaps, you should add "Fixes issue 1483." at the end of your CL
description, if you're planning to resolve it.

Alex

http://codereview.appspot.com/3989064/

[mattn]

ok, I added "fix issue 1107, 1423, 1483".

[Rob 'Commander' Pike]

You need to write it the way the tools expect.

Fixes issue 1483.
Fixes issue 1107.
Fixes issue 1423.

-rob

[mattn]

Thanks. I fixed it.

On Tuesday, February 8, 2011 9:02:58 AM UTC+9, r wrote:

On Feb 7, 2011, at 3:57 PM, mattn wrote:

> ok, I added "fix issue 1107, 1423, 1483".
>
> On Tuesday, February 8, 2011 8:36:29 AM UTC+9, brainman wrote:
> On 2011/02/04 06:30:52, mattn wrote:

> > Hello mailto:gola...@googlegroups.com,

> > I'd like you to review this change to
> > http://go.googlecode.com/hg/
>
> Please, note http://code.google.com/p/go/issues/detail?id=1483.
>
> Perhaps, you should add "Fixes issue 1483." at the end of your CL
> description, if you're planning to resolve it.

[Russ Cox]

I am a bit unhappy about all this path hacking
going on without a clear understanding of what
the API should be. Issue 1423 is about deciding
an API, not implementing code.

It sounds like everyone thinks that package path
should be an OS-specific package. I might as well
go along, since path.Walk has set the precedent.

But before we just toss in every function you can
think of, I would like to see some discussion via email
to golang-dev, not via looking at code on the codereview
server, of what the final API is going to be.
Right now it feels like a midden heap of Windows artifacts.

Russ

[mattn]

What is your worry?

Concord between path package and URL? or Concord between behavior on windows and other OSs?

The forwarding slash does not working on windows completely. If you hope that all application working on windows, you'll have to make 'The common way to manipulate path separator on windows' to the world.

USING FORWARDING SLASH:

  GOOD:

    (A1) friendly to UN*X.

    (A2) friendly to URL.

  WRONG:

    (B1) not friendly to Windows.

USING BACKSLASH:

  GOOD:

    (C1) working completely to manipulating path on windows.

  WRONG:

    (D1) not friendly to UN*X.

    (D2) not friendly to URL.

    (D3) confusing for escape character. 

For about 'wrong' section,

B1 and D1, D2 should be fixed with positioning path package as os specific. 

D3 will fix with making constrain to mask pattern for windows.

However on windows, we should treat drive name like "C:". i.e. we can't treat path as part of URL completely.

And B1 is worst problem.

I guess that B1 is bigger problem than D1/D2/D3.

hehe

- Yasuhiro Matsumoto

[Russ Cox]

I am okay with making package path use \ on Windows.
But I want to see a list of the functions we're going to put
in package path first, in an email discussion, before we
start looking at coding details. There are too many and
more every day.

Russ

[Benny Siegert]

I proposed a long time ago to loosely follow the well-established API
of File::Spec in Perl. The nice thing about this API is its symmetry
-- the functions operating on paths have a corresponding reverse
function. Comparing File::Spec, to path, we have:

canonpath - similar to path.Clean
catdir, catfile, join - path.Join (I figure that the separation in the
Perl functions is needed on VMS)
splitdir - path.Split (but see below)
curdir, rootdir, updir - I don't think we need these
file_name_is_absolute - path.IsAbs
tmpdir, devnull - in other packages in Go

splitpath - path.Split. However, splitpath returns *three* values:
"Splits a path in to volume, directory, and filename portions. On
systems with no concept of volume, returns '' for volume."

For symmetry, there is also catpath: "Takes volume, directory and file
portions and returns an entire path. Under Unix, $volume is ignored,
and directory and file are concatenated. A '/' is inserted if need be.
On other OSes, $volume is significant."

Then, there are two functions that would be very useful IMHO:

abs2rel: Takes a destination path and an optional base path returns a
relative path from the base path to the destination path.

rel2abs: Converts a relative path to an absolute path.

The following two might also be interesting to have:

case_tolerant: Returns a true or false value indicating, respectively,
that alphabetic case is not or is significant when comparing file
specifications.

no_upwards: Given a list of file names, strip out those that refer to
a parent directory.

--Benny.

[mattn]

Sounds good.

And we may have better to provide interface 'Path' like following.

type Path interface { String() string }

type PathString string

func NewPathString(p string) PathString { return PathString(p) }

func (p *PathString) String() string { return string(*p) }

func (p *PathString) Join(rhs string) { *p = PathString(Join(string(*p), rhs)) }

func (p *PathString) Clean() { *p = PathString(Clean(string(*p))); }

Thanks.

[matt...@gmail.com]

Hello golan...@googlegroups.com, bsiegert, r, rsc, brainman, r2 (cc:

[matt...@gmail.com]

I fixed a problem of Clean() on windows.

Clean("C:/") returned "C:"

On 2011/02/10 00:42:46, mattn wrote:
> Hello mailto:golan...@googlegroups.com, bsiegert, r, rsc, brainman,
r2 (cc:
> mailto:golan...@googlegroups.com),

[matt...@gmail.com]

Ah, sorry.
I found problem.
Please wait.

[matt...@gmail.com]

Hello golan...@googlegroups.com, bsiegert, r, rsc, brainman, r2 (cc:

[matt...@gmail.com]

I changed:

* renamed IsSame to isSame
This is not needed for go user.
* fixed problem that Clean("C:/") return "C:"

On 2011/02/10 01:02:06, mattn wrote:
> Hello mailto:golan...@googlegroups.com, bsiegert, r, rsc, brainman,
r2 (cc:
> mailto:golan...@googlegroups.com),

[matt...@gmail.com]

Hello golan...@googlegroups.com, bsiegert, r, rsc, brainman, r2 (cc:

[alex.b...@gmail.com]

On 2011/02/10 01:24:51, mattn wrote:
> Hello mailto:golan...@googlegroups.com, bsiegert, r, rsc, brainman,
r2 (cc:
> mailto:golan...@googlegroups.com),

> Please take another look.

You're claiming this CL fixes these issues:

Fixes issue 1483.
Fixes issue 1107.
Fixes issue 1423.

But I don't think either 1483 or 1423 are fixed. After your patch this
program:

package main
import (
"path"
"fmt"
)
func main() {
fmt.Println(path.Join("directory", "file"))
println(path.IsAbs("C:\\hello.txt"))
}

prints this:

directory/file
false

Regardless, I think, you should always try and include reported issues
to the package tests, if possible.

Alex

http://codereview.appspot.com/3989064/

[matt...@gmail.com]

Rally? I get following.

directory\file
true

[mattn]

s/Rally/Really/

[alex.b...@gmail.com]

On 2011/02/15 01:53:27, mattn wrote:
> Rally? I get following.

> directory\file
> true

You're right, I'm wrong. It works.

LGTM.

http://codereview.appspot.com/3989064/diff/38001/src/pkg/path/path_test.go
File src/pkg/path/path_test.go (right):

http://codereview.appspot.com/3989064/diff/38001/src/pkg/path/path_test.go#newcode332
src/pkg/path/path_test.go:332:
Please, add test for above mentioned

path.Join("directory", "file")

and

path.IsAbs("C:\\hello.txt")

http://codereview.appspot.com/3989064/diff/38001/src/pkg/path/path_unix.go
File src/pkg/path/path_unix.go (right):

http://codereview.appspot.com/3989064/diff/38001/src/pkg/path/path_unix.go#newcode6
src/pkg/path/path_unix.go:6:
import "strings"

http://codereview.appspot.com/3989064/

[matt...@gmail.com]

Hello golan...@googlegroups.com, bsiegert, r, rsc, brainman, r2 (cc:
golan...@googlegroups.com),

Please take another look.

http://codereview.appspot.com/3989064/

[matt...@gmail.com]

http://codereview.appspot.com/3989064/diff/38001/src/pkg/path/path_test.go
File src/pkg/path/path_test.go (right):

http://codereview.appspot.com/3989064/diff/38001/src/pkg/path/path_test.go#newcode332
src/pkg/path/path_test.go:332:

On 2011/02/16 05:02:31, brainman wrote:
> Please, add test for above mentioned

> path.Join("directory", "file")

> and

> path.IsAbs("C:\\hello.txt")

Done.

http://codereview.appspot.com/3989064/diff/38001/src/pkg/path/path_unix.go
File src/pkg/path/path_unix.go (right):

http://codereview.appspot.com/3989064/diff/38001/src/pkg/path/path_unix.go#newcode6
src/pkg/path/path_unix.go:6:
On 2011/02/16 05:02:31, brainman wrote:
> import "strings"

Done.

http://codereview.appspot.com/3989064/

[Russ Cox]

Work on this CL is not a productive use of time.
I want time to design the path story first.

Russ