code review 4271057: filepath: new Abs function

filepath: new Abs function

[niemeyer, 2011-03-21 02:56:59]

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

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

[rsc1, 2011-03-21 03:21:42]

Not really sure this should refer to the current directory.

[niemeyer, 2011-03-21 03:42:21]

> Not really sure this should refer to the current directory.

Unless there's an explicit root inferred from the context,
every function which accepts a relative path is talking about
something from the current directory.  That's the case for
Walk, Glob, Run, and so many others.

Being able to say filepath.Abs("src/") is both useful and
has an expected outcome, so what's the concern, more precisely?

[peterGo, 2011-03-21 06:24:20]

Russ,

Windows has a GetFullPathName function, which "merges the name of the current
drive and directory with a specified file name to determine the full path and
file name of a specified file."

"Note  The GetFullPathName function is not recommended for multithreaded
applications or shared library code."

http://msdn.microsoft.com/en-us/library/aa364963.aspx

Peter

On 2011/03/21 03:21:42, rsc1 wrote:
> Not really sure this should refer to the current directory.

[bsiegert, 2011-03-21 07:51:03]

On Mon, Mar 21, 2011 at 04:21, Russ Cox <rsc@google.com> wrote:
> Not really sure this should refer to the current directory.

Maybe I am biased but I would also like to prefer something like
rel2abs in Perl:

func Abs(path, base string) (string, os.Error)

If base is an empty string, use os.Getwd().

I already ran into situations where I needed to have absolute paths
relative to something different than the current working directory.

One more consideration to take into account on Windows, from the
perldoc page on File::Spec:

On systems with the concept of volume, if $path and $base appear to be
on two different volumes, we will not attempt to resolve the two
paths, and we will instead simply return $path . Note that previous
versions of this module ignored the volume of $base , which resulted
in garbage results part of the time.

--Benny.

-- 
The first essential in chemistry is that you should perform practical
work and conduct experiments, for he who performs not practical work
nor makes experiments will never attain the least degree of mastery.
        -- Abu Musa Jabir ibn Hayyan (721-815)

[niemeyer, 2011-03-21 13:18:59]

Hi Benny,

> I already ran into situations where I needed to have absolute paths
> relative to something different than the current working directory.

Please check the existing filepath.Join for that.

[niemeyer, 2011-04-04 14:10:22]

Any chance of better input on this?

On 2011/03/21 03:42:21, niemeyer wrote:
> > Not really sure this should refer to the current directory.
> 
> Unless there's an explicit root inferred from the context,
> every function which accepts a relative path is talking about
> something from the current directory.  That's the case for
> Walk, Glob, Run, and so many others.
> 
> Being able to say filepath.Abs("src/") is both useful and
> has an expected outcome, so what's the concern, more precisely?

[r, 2011-04-04 16:28:33]

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

http://codereview.appspot.com/4271057/diff/6001/src/pkg/path/filepath/path.go...
src/pkg/path/filepath/path.go:235: // If path is not absolute, it will be joined
with the current
-,.s/path/the path/

http://codereview.appspot.com/4271057/diff/6001/src/pkg/path/filepath/path.go...
src/pkg/path/filepath/path.go:236: // working directory to turn it into an
absolute path.
The absolute path name for a given file is not guaranteed to be unique.

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

http://codereview.appspot.com/4271057/diff/6001/src/pkg/path/filepath/path_te...
src/pkg/path/filepath/path_test.go:507: want :=
filepath.Clean(strings.Replace(test.result, "$CWD", cwd, -1))
i think this is a test that the implementation works the way you expect.
instead, you should check that the resulting name is an absolute path that
evaluates to the same file, verified with Stat.

[niemeyer, 2011-04-04 19:59:57]

All comments addressed. PTAL

[r, 2011-04-05 00:31:58]

LGTM but i'd like a windows reviewer to comment before i commit.

[mattn, 2011-04-05 00:48:44]

LGTM
I got PASS. and I guess there is enough tests.

On 2011/04/05 00:31:58, r wrote:
> LGTM but i'd like a windows reviewer to comment before i commit.

[r, 2011-04-05 01:29:30]

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

filepath: new Abs function

R=golang-dev, rsc1, peterGo, bsiegert, r, mattn
CC=golang-dev
http://codereview.appspot.com/4271057

Committer: Rob Pike <r@golang.org>