code review 4235060: path/filepath: add EvalSymlinks function

path/filepath: add EvalSymlinks function

[adg, 2011-03-08 06:57:00]

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

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

[r2, 2011-03-08 08:05:51]

'expand' isn't a word that comes to mind when i think of symbolic links.  (the
first word that comes to mind is 'fiasco', but that's a different point.)

-rob

[adg, 2011-03-08 08:23:35]

On 8 March 2011 19:05, Rob 'Commander' Pike <r@google.com> wrote:
> 'expand' isn't a word that comes to mind when i think of symbolic links.  (the
first word that comes to mind is 'fiasco', but that's a different point.)

I grappled a little with the naming. What I would use this for is to
return the canonical path, after all symlinks have been followed.
"Canonical" makes sense when given an absolute path, but doesn't quite
describe this function's behaviour when given a relative path, but
maybe it's a better name.

I'd appreciate any other suggestions.

Andrew

[rog, 2011-03-08 08:23:56]

Real ? (after realpath(3))

also, i'm not sure that it's correct to call Clean before
processing the path, particularly when we're dealing
specifically with symbolic links, because x/y/../z might
not be equivalent to x/z if y is a symbolic link.

On 8 March 2011 08:05, Rob 'Commander' Pike <r@google.com> wrote:
> 'expand' isn't a word that comes to mind when i think of symbolic links.  (the
first word that comes to mind is 'fiasco', but that's a different point.)
>
> -rob
>
>

[adg, 2011-03-08 08:51:49]

On 8 March 2011 19:23, roger peppe <rogpeppe@gmail.com> wrote:
> Real ? (after realpath(3))

Real sounds good.

> also, i'm not sure that it's correct to call Clean before
> processing the path, particularly when we're dealing
> specifically with symbolic links, because x/y/../z might
> not be equivalent to x/z if y is a symbolic link.

You're right, I think. I need to tweak the algorithm, too, in that
case. Will update.

Andrew

[adg, 2011-03-08 08:58:06]

On 8 March 2011 19:51, Andrew Gerrand <adg@golang.org> wrote:
> You're right, I think. I need to tweak the algorithm, too, in that
> case. Will update.

Done. PTAL.

[rog, 2011-03-08 09:24:52]

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

http://codereview.appspot.com/4235060/diff/4/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:195: if err != nil {
i'm wondering whether it might be good to allow Real to work on paths that don't
exist. for instance, if /usr/rog is a symlink to /Volumes/ext/rog, then it could
be nice if Real("/usr/rog/tmp/newfile") would return
"/Volumes/ext/rog/tmp/newfile" even if tmp or newfile did not exist.

i think this fits with the existing documentation, which does not say that the
path must exist.

http://codereview.appspot.com/4235060/diff/4/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:198: if fi.IsDirectory() || fi.IsRegular() {
if !fi.IsSymLink() {

this code should work with irregular files too

http://codereview.appspot.com/4235060/diff/4/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:204: }
-3,.d

[rog, 2011-03-08 09:27:36]

http://codereview.appspot.com/4235060/diff/4/src/pkg/path/filepath/path_test.go
File src/pkg/path/filepath/path_test.go (right):

http://codereview.appspot.com/4235060/diff/4/src/pkg/path/filepath/path_test....
src/pkg/path/filepath/path_test.go:415: {"test/link2/link3/test", "test"},
it would be good to have some absolute path tests here too.
also paths with multiple consecutive separators.

[adg, 2011-03-08 10:11:50]

Thanks, rog.

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

http://codereview.appspot.com/4235060/diff/4/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:195: if err != nil {
On 2011/03/08 09:24:52, rog wrote:
> i'm wondering whether it might be good to allow Real to work on paths that
don't
> exist. for instance, if /usr/rog is a symlink to /Volumes/ext/rog, then it
could
> be nice if Real("/usr/rog/tmp/newfile") would return
> "/Volumes/ext/rog/tmp/newfile" even if tmp or newfile did not exist.

I would prefer for it to only work if the path exists. I would expect Real to
perform some validation. For your use case, why not just tack on the last part
of the path afterwards? It's more explicit and less surprising that way.

Also, if I can quote some prior art. This is from the realpath(3) manpage on
darwin:

"All components of file_name must exist when realpath() is called."

To quote FreeBSD:

"All but the last component of pathname must exist when realpath() is called."

I could be convinced that the latter case is worth implementing here.

http://codereview.appspot.com/4235060/diff/4/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:198: if fi.IsDirectory() || fi.IsRegular() {
On 2011/03/08 09:24:52, rog wrote:
> if !fi.IsSymLink() {
> 
> this code should work with irregular files too

I would argue that an should be allowed to be neither a directory nor a symlink
only when it is the final component of the path.

http://codereview.appspot.com/4235060/diff/4/src/pkg/path/filepath/path_test.go
File src/pkg/path/filepath/path_test.go (right):

http://codereview.appspot.com/4235060/diff/4/src/pkg/path/filepath/path_test....
src/pkg/path/filepath/path_test.go:415: {"test/link2/link3/test", "test"},
On 2011/03/08 09:27:37, rog wrote:
> it would be good to have some absolute path tests here too.
> also paths with multiple consecutive separators.

I agree, these tests are inadequate. Will fix once we've nailed the
functionality.

[rog, 2011-03-08 10:44:30]

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

http://codereview.appspot.com/4235060/diff/4/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:195: if err != nil {
On 2011/03/08 10:11:50, adg wrote:
> On 2011/03/08 09:24:52, rog wrote:
> > i'm wondering whether it might be good to allow Real to work on paths that
> don't
> > exist. for instance, if /usr/rog is a symlink to /Volumes/ext/rog, then it
> could
> > be nice if Real("/usr/rog/tmp/newfile") would return
> > "/Volumes/ext/rog/tmp/newfile" even if tmp or newfile did not exist.
> 
> I would prefer for it to only work if the path exists. I would expect Real to
> perform some validation. For your use case, why not just tack on the last part
> of the path afterwards? It's more explicit and less surprising that way.

as a caller of Real, i don't know which the last part of the path is.
Real is the only place with that info (maybe /Volumes/ext/rog/tmp is a symlink
too).

> Also, if I can quote some prior art. This is from the realpath(3) manpage on
> darwin:

i realise that realpath requires all the components but the last to exist, but i
don't really see why it's just the last component and not the last n. ok, i do -
it's because creat requires all the directories to exist, but i don't think
that's necessarily a good reason.

i'll get validation anyway when i try to use the path.

i'd be happy if the only reason for Real to return an error was if ReadLink
failed, and maybe not even then - then you wouldn't need to return an error.

realpath(3) always returns an absolute path, BTW, so this is already different
(actually i'd find that functionality useful too - a case for a bool abs
argument to Real?)

http://codereview.appspot.com/4235060/diff/4/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:198: if fi.IsDirectory() || fi.IsRegular() {
On 2011/03/08 10:11:50, adg wrote:
> On 2011/03/08 09:24:52, rog wrote:
> > if !fi.IsSymLink() {
> > 
> > this code should work with irregular files too
> 
> I would argue that an should be allowed to be neither a directory nor a
symlink
> only when it is the final component of the path.

perhaps (modulo the "don't return an error" possibility above), but that wasn't
what the original code implemented.

[rsc, 2011-03-08 16:07:14]

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

http://codereview.appspot.com/4235060/diff/4/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:203: return "", os.NewError("unexpected path
element: " + p)
doesn't look like an error to me.
i'd say change the test above (directory || regular)
to !symlink.

[rsc, 2011-03-08 16:09:40]

Should guard against infinite loops, does not.

Real is a vague name, despite the C library realpath.
This is pretty rare.  Something longer is probably better.
filepath.FollowSymlinks(name)  ?

What is this for?  I can't think of any useful and safe
applications of this function.

[r2, 2011-03-08 17:12:29]

On Mar 8, 2011, at 1:24 AM, rogpeppe@gmail.com wrote:

> 
> http://codereview.appspot.com/4235060/diff/4/src/pkg/path/filepath/path.go
> File src/pkg/path/filepath/path.go (right):
> 
>
http://codereview.appspot.com/4235060/diff/4/src/pkg/path/filepath/path.go#ne...
> src/pkg/path/filepath/path.go:195: if err != nil {
> i'm wondering whether it might be good to allow Real to work on paths
> that don't exist. for instance, if /usr/rog is a symlink to
> /Volumes/ext/rog, then it could be nice if Real("/usr/rog/tmp/newfile")
> would return "/Volumes/ext/rog/tmp/newfile" even if tmp or newfile did
> not exist.
> 
> i think this fits with the existing documentation, which does not say
> that the path must exist.

but not the existing name. the real path of a nonexistent file is.... ?

'Real' may remind the user of realpath(3) but maybe 'Hard' is better, as in hard
links vs. soft links?

-rob

[rsc, 2011-03-08 17:14:39]

ExpandSymlinks

[iant2, 2011-03-08 17:30:51]

FYI

Russ Cox <rsc@golang.org> writes:

> What is this for?  I can't think of any useful and safe
> applications of this function.

I can tell you that gcc needs realpath when running on Windows using
cygwin, because on Windows there is no reliable way to determine whether
two different filenames refer to the same file.

The gold linker uses realpath to determine whether a search path
provided on the command line is within the system root, also provided on
the command line.  Search paths often use .. and symlinks, but correct
implementation of the system root idea requires an accurate notion of
whether a path is within the system root or not.  The system root is
used when cross-linking; the system root is the name of the root
directory on the target system as seen from the host system.

[r2, 2011-03-08 17:56:17]

EvalSymlinks

I still don't see the connection between 'expand' and symlinks.  One expands
'globs', not symlinks.

-rob

[rog, 2011-03-08 20:56:47]

On 8 Mar 2011 16:07, "Russ Cox" <rsc@golang.org> wrote:
>
> Should guard against infinite loops, does not.
>
> Real is a vague name, despite the C library realpath.
> This is pretty rare.  Something longer is probably better.
> filepath.FollowSymlinks(name)  ?
>
> What is this for?  I can't think of any useful and safe
> applications of this function.

I imagine, although I haven't checked, that mercurial uses something similar
when it works out the repository root from the name of some file inside it.

That's a vote for always returning an absolute path, I suppose, as iant's
example would also want that.

[rsc, 2011-03-08 21:09:13]

Always returning an absolute path seems like an
unnecessary complication, different from evaluating
symbolic links.

[adg, 2011-03-08 23:04:18]

This is for the implementation of GOPATH in goinstall. Goinstall
should canonicalize the GOPATH elements so that it can determine if a
given path is inside a GOPATH. I could probably get by (for now)
without this new function, but I think it would help to clear up some
ambiguous corner cases.

As for naming, how about ResolveSymlinks? EvalSymlinks is okay, too.
filepath.Hard also has its charms, but is less obvious as to what it
does.

On 9 March 2011 08:09, Russ Cox <rsc@golang.org> wrote:
> Always returning an absolute path seems like an
> unnecessary complication, different from evaluating
> symbolic links.

It would be better to have a separate function, filepath.Abs, for that purpose.

On 9 March 2011 03:07, Russ Cox <rsc@golang.org> wrote:
> Should guard against infinite loops, does not.

Can you recommend a good strategy?

[rsc, 2011-03-08 23:10:27]

> This is for the implementation of GOPATH in goinstall. Goinstall
> should canonicalize the GOPATH elements so that it can determine if a
> given path is inside a GOPATH. I could probably get by (for now)
> without this new function, but I think it would help to clear up some
> ambiguous corner cases.

Such as?

[adg, 2011-03-08 23:20:16]

On 9 March 2011 10:10, Russ Cox <rsc@golang.org> wrote:
>> This is for the implementation of GOPATH in goinstall. Goinstall
>> should canonicalize the GOPATH elements so that it can determine if a
>> given path is inside a GOPATH. I could probably get by (for now)
>> without this new function, but I think it would help to clear up some
>> ambiguous corner cases.
>
> Such as?

/home is a symlink to /usr/home (not uncommon).
GOPATH=/home/adg/gocode
cwd is /usr/home/adg/gocode/adg/foo
$ goinstall .

Goinstall needs to figure out that adg/foo is a package inside a GOPATH element.

When I say I could get by, I meant forcing people to write "goinstall
adg/foo" instead of "goinstall .". This is fine at the moment, but I
would ultimately like to make goinstall as easy to use as "gomake
install".

Andrew

[rsc, 2011-03-11 14:30:52]

Ok.

EvalSymlinks is still a better name than Real.

[adg, 2011-03-15 06:08:04]

Hello rsc, niemeyer, r2, rog, iant2 (cc: golang-dev@googlegroups.com),

Please take another look.

[rog, 2011-03-15 08:08:01]

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

http://codereview.appspot.com/4235060/diff/18001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:182: i := strings.IndexRune(path, Separator)
now that you're not calling Clean on path, you'll need to cope with multiple
separators here.

http://codereview.appspot.com/4235060/diff/18001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:193: b.WriteString(string(Separator))
b.WriteRune(Separator)

http://codereview.appspot.com/4235060/diff/18001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:198: fi, err := os.Lstat(b.String() + p)
allow an error here when path=="" ?

http://codereview.appspot.com/4235060/diff/18001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:205: b.WriteString(string(Separator))
b.WriteRune(Separator)

[r, 2011-03-15 17:11:34]

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

http://codereview.appspot.com/4235060/diff/18001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:170: // links. If path is relative it will be
evaluated relative to the CWD.
s/CWD/current working directory/

http://codereview.appspot.com/4235060/diff/18001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:192: // must be absolute path
every string(Separator) is a malloc. do that once.

[rsc, 2011-03-15 17:21:13]

s/CWD/current directory/

[adg, 2011-03-16 06:47:50]

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

http://codereview.appspot.com/4235060/diff/18001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:170: // links. If path is relative it will be
evaluated relative to the CWD.
On 2011/03/15 17:11:34, r wrote:
> s/CWD/current working directory/

Done.

http://codereview.appspot.com/4235060/diff/18001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:182: i := strings.IndexRune(path, Separator)
On 2011/03/15 08:08:01, rog wrote:
> now that you're not calling Clean on path, you'll need to cope with multiple
> separators here.

Clean doesn't handle multiple separators yet (there's a TODO) so I won't handle
them here, either.

http://codereview.appspot.com/4235060/diff/18001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:192: // must be absolute path
On 2011/03/15 17:11:34, r wrote:
> every string(Separator) is a malloc. do that once.

Done.

http://codereview.appspot.com/4235060/diff/18001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:193: b.WriteString(string(Separator))
On 2011/03/15 08:08:01, rog wrote:
> b.WriteRune(Separator)

Done.

http://codereview.appspot.com/4235060/diff/18001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:198: fi, err := os.Lstat(b.String() + p)
On 2011/03/15 08:08:01, rog wrote:
> allow an error here when path=="" ?

Now that it's called EvalSymlinks, it would seems strange to me to make the last
element a special case. I'd prefer to leave it as-is.

http://codereview.appspot.com/4235060/diff/18001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:205: b.WriteString(string(Separator))
On 2011/03/15 08:08:01, rog wrote:
> b.WriteRune(Separator)

Done.

[r, 2011-03-17 01:22:23]

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

http://codereview.appspot.com/4235060/diff/21002/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:180: return "", os.NewError("links to deep to
follow")
s/to/too/
but it's not a great message
maybe
"EvalSymlinks: too many links in " + originalPath

http://codereview.appspot.com/4235060/diff/21002/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:319: return string(Separator)
sounds like string(Separator) should be a global var

[adg, 2011-03-17 01:31:24]

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

http://codereview.appspot.com/4235060/diff/21002/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:180: return "", os.NewError("links to deep to
follow")
On 2011/03/17 01:22:23, r wrote:
> s/to/too/
> but it's not a great message
> maybe
> "EvalSymlinks: too many links in " + originalPath

Done.

http://codereview.appspot.com/4235060/diff/21002/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:319: return string(Separator)
On 2011/03/17 01:22:23, r wrote:
> sounds like string(Separator) should be a global var

Probably, but let's do that in a separate CL.

[r2, 2011-03-17 04:35:58]

On Mar 16, 2011, at 6:31 PM, adg@golang.org wrote:

> 
> http://codereview.appspot.com/4235060/diff/21002/src/pkg/path/filepath/path.go
> File src/pkg/path/filepath/path.go (right):
> 
>
http://codereview.appspot.com/4235060/diff/21002/src/pkg/path/filepath/path.g...
> src/pkg/path/filepath/path.go:180: return "", os.NewError("links to deep
> to follow")
> On 2011/03/17 01:22:23, r wrote:
>> s/to/too/
>> but it's not a great message
>> maybe
>> "EvalSymlinks: too many links in " + originalPath
> 
> Done.
> 
>
http://codereview.appspot.com/4235060/diff/21002/src/pkg/path/filepath/path.g...
> src/pkg/path/filepath/path.go:319: return string(Separator)
> On 2011/03/17 01:22:23, r wrote:
>> sounds like string(Separator) should be a global var
> 
> Probably, but let's do that in a separate CL.

Why? It's easy and you're there.

-rob

[adg, 2011-03-17 04:43:10]

On 17 March 2011 15:35, Rob 'Commander' Pike <r@google.com> wrote:
> Why? It's easy and you're there.

Oh, I thought the Separator was a variable, not a const. No worries.

[adg, 2011-03-17 04:50:50]

Hello rsc, niemeyer, r2, rog, iant2, r (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2011-03-17 04:55:34]

LGTM

[adg, 2011-03-17 05:36:40]

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

path/filepath: add EvalSymlinks function

R=rsc, niemeyer, r2, rog, iant2, r
CC=golang-dev
http://codereview.appspot.com/4235060