code review 4981049: path/filepath: added Rel as the complement of Abs

path/filepath: added Rel as the complement of Abs

[niemeyer, 2011-09-08 22:35:34]

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

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

[niemeyer, 2011-09-08 22:37:52]

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

Please take another look.

[rsc, 2011-09-09 00:06:48]

I am pretty sure adg wrote this code once
before and we argued against it.  I don't
remember the details.

[gustavo_niemeyer.net, 2011-09-09 00:29:16]

> I am pretty sure adg wrote this code once
> before and we argued against it.  I don't
> remember the details.

If you were against it, I'm glad you can't remember. ;-)

More seriously, I haven't noticed anything like this fly by, and the
list search doesn't help either.  The long discussion around this
topic that I recall with Andrew was around EvalSymlinks, and it turned
out to be more of a bikeshed around naming than anything else.

Regardless, this seems like a pretty obvious and useful function, so
I'd be glad to talk about it if possible.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[r, 2011-09-09 00:54:42]

obvious to whom and useful for what? i cannot remember ever needing
this functionality.

-rob

[gustavo_niemeyer.net, 2011-09-09 01:07:02]

> obvious to whom

Anything obvious is _always_ obvious to the subject only. You can
agree or not, but there are no doubts about that aspect. Then, when
the subject says "seems pretty obvious", it becomes even more clear
that the subject is in fact being open to people disagreeing and
providing arguments for a more fruitful debate.

> and useful for what? i cannot remember ever needing
> this functionality.

Maybe to implement a filepath.Visitor that actually works as
documented (!), or to create relative symlinks, or to pack contents
into a zip writer trivially without having to do the same operations
by hand, etc.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[r, 2011-09-09 01:14:18]

I still don't see the obviousness and utility, but I'll leave those aside.

Calling Abs on both paths is not an implementation I'm happy with: two
expensive system calls, one of which is likely to be repeated with the
same argument each time around the loop. If i call Rel("./foo", ".")
and need a system call, something's off.

-rob

[gustavo_niemeyer.net, 2011-09-09 01:20:01]

> Calling Abs on both paths is not an implementation I'm happy with: two
> expensive system calls, one of which is likely to be repeated with the
> same argument each time around the loop. If i call Rel("./foo", ".")
> and need a system call, something's off.

Solving the second problem could be done with a fancier algorithm, but
the first one (no system call per iteration) will require enforcing
absolute paths, since a call such as Rel("/foo", ".")  or Rel(".",
"/foo") can't be resolved otherwise.  Would you be happier if both of
these were enforced to be absolute?

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[r, 2011-09-09 03:20:54]

I don't see why the "root" should be absolute or not; the
relativization should be relative to the "root" regardless. I also am
uncertain why the operation can't be purely lexical (and therefore
trivial). Also, making it purely lexical is easy to define. Depending
on Abs working the same way for two paths is bug-prone for sure.

-rob

[gustavo_niemeyer.net, 2011-09-09 03:54:21]

> I don't see why the "root" should be absolute or not; the
> relativization should be relative to the "root" regardless. I also am
> uncertain why the operation can't be purely lexical (and therefore
> trivial). Also, making it purely lexical is easy to define. Depending
> on Abs working the same way for two paths is bug-prone for sure.

Ok, sounds good. I'll make it purely lexical and drop the behavior of
taking the current directory in consideration. Now I need some sleep,
though.

Thanks for explaining it,

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[rsc, 2011-09-09 04:42:05]

What is the context where you'd use this?

I'd love to see a purely lexical Rel in package path
(not path/filepath).  In this package I am not so
sure that Rel should be lexical (because operating
system behavior is not) nor that it should take a root
(because Abs does not).

Russ

[gustavo_niemeyer.net, 2011-09-09 14:31:15]

> What is the context where you'd use this?
>
> I'd love to see a purely lexical Rel in package path
> (not path/filepath).  In this package I am not so
> sure that Rel should be lexical (because operating
> system behavior is not) nor that it should take a root
> (because Abs does not).

In my mind there are two clear use cases for this that are interesting:


1) Breaking down a path based on an arbitrary root. E.g.:

a. Bundling a given directory in a zip. zip.Writer.Create takes a
    relative path used within the zip itself. I'm using Rel for this
    right now.

b. Changing the usage or implementation of filepath.Visitor so
    that the paths are relative to root. I'm also using it for this
    right now.


2) Converting an absolute path into one relative to cwd. E.g:

a. This is a common trick in general to make paths easier
    for humans to digest, avoiding line-long paths
    in the terminal for local contexts.

a. An easy example is "git status". It will show paths within
    the tree relative to the user's cwd.


Both of these cases are suited for filepath, and the current
implementation handles them both.

I understand what you mean by avoiding a root, because
you have the second case in mind.  Right now it's done
with Rel(path, "."), so it's easy to see that the root could
be avoided for it. But this would live the first case out,
and they're both the same underlying problem.

Rob's suggestion supports some of the first case, and
some of the second assuming paths are converted to
absolute externally. The problem with a purely lexical
analysis is that it rules out the cases where only one
of the two paths is absolute, like Rel("/foo", ".") or
Rel(".", "/foo").

As an attempt to make both of these cases well
supported, if that feels good I can try an intermediate
implementation later to see how you both feel, doing
purely lexical analysis whenever feasible, but still
resolving a relative path to absolute if strictly
necessary for the provided arguments.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[rsc, 2011-09-09 14:40:28]

None of these seem like terribly compelling reasons.
If filepath's Walk doc gets updated to say
that path begins with root, then you just strip
the root with a string slice.  Hg does the same
bogus context-sensitive path display that git does,
and it drives me nuts.

[rsc, 2011-09-09 14:42:06]

On Fri, Sep 9, 2011 at 10:40, Russ Cox <rsc@golang.org> wrote:
> None of these seem like terribly compelling reasons.
> If filepath's Walk doc gets updated to say
> that path begins with root, then you just strip
> the root with a string slice.  Hg does the same
> bogus context-sensitive path display that git does,
> and it drives me nuts.

Sorry, gmail sent earlier than I wanted to.  Continuing...

Assuming that Rel is a consistent operation
is a recipe for trouble on modern systems.
I'm willing to see something purely lexical go in,
but I'm now convinced that non-lexical has no
one right answer.

Russ

[gustavo_niemeyer.net, 2011-09-09 14:46:59]

> Assuming that Rel is a consistent operation
> is a recipe for trouble on modern systems.
> I'm willing to see something purely lexical go in,
> but I'm now convinced that non-lexical has no
> one right answer.

Sounds good. I'm not too bothered by these use cases either, given
that if/when one really wants them, it's trivial to convert the path
to absolute ahead of time before calling Rel.  This also makes Rob's
point more valid, since doing so would avoid having system calls on
every iteration of a potential loop.

I'll sort that out later today or over the weekend, thanks.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[borman, 2011-09-09 15:48:04]

On Fri, Sep 9, 2011 at 7:30 AM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote:

> 2) Converting an absolute path into one relative to cwd. E.g:
>
> a. This is a common trick in general to make paths easier
>    for humans to digest, avoiding line-long paths
>    in the terminal for local contexts.


Sadly this will produce relative paths that the user can't just say "cd
<paste>" to.  Some shells determine this lexicographically:

$ cd /tmp
$ mkdir -p a/b/c
$ ln -s a/b/c c


bash/ksh/zsh:

$ cd /tmp
$ cd c
$ pwd
/tmp/c
$ cd ..
$ pwd
/tmp


csh:

% cd /tmp
% cd c
% pwd
/tmp/c
% cd ..
% pwd
/tmp/a/b

$ mkdir a/b/d
$ touch a/b/d/f1
$ cd c
$ ls ../d/f1
f1
$ cd ../d
-ksh: cd: ../d: [No such file or directory]


This latter one does work in bash:

$ cd /tmp/c
$ cd ../d
$ pwd
/private/tmp/a/b/d


My point is there isn't a right answer to that problem, at least for humans
to read and use.

[niemeyer, 2011-09-19 23:50:01]

Hello golang-dev@googlegroups.com, rsc@golang.org, gustavo@niemeyer.net,
r@golang.org, borman@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[niemeyer, 2011-09-19 23:51:06]

Sorry, must complement the doc. Just a moment.

[niemeyer, 2011-09-20 00:01:15]

Hello golang-dev@googlegroups.com, rsc@golang.org, gustavo@niemeyer.net,
r@golang.org, borman@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2011-09-21 21:33:34]

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

http://codereview.appspot.com/4981049/diff/25001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:262: // be equivalent to path itself.
probably want to define join better. maybe, joined to path with an intervening
path separator. or give a code snippet, if that makes sense.
the semantics is worth making clear.

http://codereview.appspot.com/4981049/diff/25001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:264: // current working directory are supported.
this is an odd sentence. what in the API says anything about supported
operations?
i think what you're trying to say is that the relative path is constructed using
purely lexical operations on path and root.

http://codereview.appspot.com/4981049/diff/25001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:282: return "", os.NewError("can't make " + path +
" relative to " + root)
these errors should have a "Rel: " prefix.
they're also going to be really long but i guess that's ok

http://codereview.appspot.com/4981049/diff/25001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:284: // Position root[r0:ri] and path[p0:pi] on
the first differing elements.
s/on/at/

http://codereview.appspot.com/4981049/diff/25001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:312: b := []byte{'.', '.'}
[]byte("..") is easier to read

http://codereview.appspot.com/4981049/diff/25001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:315: b = append(b, '/', '.', '.')
[]byte("/..")...
(maybe)
or pull these out as vars
(maybe)

[niemeyer, 2011-09-22 03:38:36]

PTAL

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

http://codereview.appspot.com/4981049/diff/25001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:262: // be equivalent to path itself.
> probably want to define join better. maybe, joined to path 

Agreed. In hindsight I clearly struggled to explain due
to my choice of parameter names. Should be better now.

http://codereview.appspot.com/4981049/diff/25001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:264: // current working directory are supported.
> this is an odd sentence. what in the API says anything 

Reworded as well.

http://codereview.appspot.com/4981049/diff/25001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:282: return "", os.NewError("can't make " + path +
" relative to " + root)
> these errors should have a "Rel: " prefix.

Done.

http://codereview.appspot.com/4981049/diff/25001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:284: // Position root[r0:ri] and path[p0:pi] on
the first differing elements.
> s/on/at/

Done.

http://codereview.appspot.com/4981049/diff/25001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:312: b := []byte{'.', '.'}
> []byte("..") is easier to read

Done.

http://codereview.appspot.com/4981049/diff/25001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:315: b = append(b, '/', '.', '.')
> []byte("/..")...

Agreed it reads better, but doesn't feel bad enough to
have the side effects of the other choices. If you feel
strong about it I can pull out as a variable.

[niemeyer, 2011-10-03 13:41:57]

ping

[r, 2011-10-03 19:53:47]

lots of allocation in the ../ case. i think some refactoring could help, but
it's not a big deal.

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

http://codereview.appspot.com/4981049/diff/29001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:261: // Rel returns a relative path that is
equivalent to targpath when
s/equivalent/lexically &/

http://codereview.appspot.com/4981049/diff/29001/src/pkg/path/filepath/path_t...
File src/pkg/path/filepath/path_test.go (right):

http://codereview.appspot.com/4981049/diff/29001/src/pkg/path/filepath/path_t...
src/pkg/path/filepath/path_test.go:620: {"a/b", "a/b", "."},
you should include at least one example with elements longer than one character

[niemeyer, 2011-10-04 00:59:34]

> lots of allocation in the ../ case. i think
> some refactoring could help, but
> it's not a big deal.

Reduced to two allocations only (one to []byte
and one back to string).

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

http://codereview.appspot.com/4981049/diff/29001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:261: // Rel returns a relative path that is
equivalent to targpath when
> s/equivalent/lexically &/

Done.

http://codereview.appspot.com/4981049/diff/29001/src/pkg/path/filepath/path_t...
File src/pkg/path/filepath/path_test.go (right):

http://codereview.appspot.com/4981049/diff/29001/src/pkg/path/filepath/path_t...
src/pkg/path/filepath/path_test.go:620: {"a/b", "a/b", "."},
> you should include at least one example with elements longer than one
character

There were already entries in lines 625, 626, 636 and 637, but I've increased
the length of elements to make it more obvious anyway.

[niemeyer, 2011-10-04 01:00:36]

Hello golang-dev@googlegroups.com, rsc@golang.org, gustavo@niemeyer.net,
r@golang.org, borman@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2011-10-04 02:45:00]

LGTM
thanks for bearing with me

[niemeyer, 2011-10-04 14:27:15]

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

path/filepath: added Rel as the complement of Abs

R=golang-dev, rsc, gustavo, r, borman
CC=golang-dev
http://codereview.appspot.com/4981049

[niemeyer, 2011-10-04 14:37:09]

> LGTM
> thanks for bearing with me

Thanks for the patience as well, Rob.

Please note I did two trivial changes before committing. One
obvious simplification now that we're counting slashes on the
".." case, and one change in the test to attempt to make it
pass on Windows on a first try.

The two changes are seen in this small patch, for easier
consumption:

    http://paste.ubuntu.com/702271/

[r, 2011-10-04 14:58:08]

LGTM