code review 5712044: path/filepath: note that SplitList is different from st...

path/filepath: note that SplitList is different from strings.Split

[rsc, 2012-02-29 20:35:42]

Hello golang-dev@googlegroups.com,

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

[r, 2012-02-29 20:41:45]

LGTM

[bradfitz, 2012-02-29 20:42:19]

LGTM

I guess asking for an Example here might not work too well with the \-vs-/
differences.

On Wed, Feb 29, 2012 at 12:35 PM, <rsc@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> path/filepath: note that SplitList is different from strings.Split
>
> Please review this at
http://codereview.appspot.com/**5712044/<http://codereview.appspot.com/5712044/>
>
> Affected files:
>  M src/pkg/path/filepath/path.go
>
>
> Index: src/pkg/path/filepath/path.go
> ==============================**==============================**=======
> --- a/src/pkg/path/filepath/path.**go
> +++ b/src/pkg/path/filepath/path.**go
> @@ -139,6 +139,7 @@
>
>  // SplitList splits a list of paths joined by the OS-specific
> ListSeparator,
>  // usually found in PATH or GOPATH environment variables.
> +// Unlike strings.Split, SplitList returns an empty slice when passed an
> empty string.
>  func SplitList(path string) []string {
>        if path == "" {
>                return []string{}
>
>
>

[gustavo_niemeyer.net, 2012-02-29 20:43:42]

> path/filepath: note that SplitList is different from strings.Split

On a vaguely related note, is the behavior of filepath.HasPrefix
intended?  I've been surprised before by the fact that it's just a
wrapper for strings.HasPrefix in Linux/MacOS/etc. I was expecting
something that took the path in consideration. For example, is
"/home/jo" a file path prefix for "/home/joe"?  Is "/home//joe" not a
prefix for "/home/joe/src"?  strings.HasPrefix felt counter-intuitive
in both at first.

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

-- I'm not absolutely sure of anything.

[rsc, 2012-02-29 20:50:16]

I have no idea what the point of filepath.HasPrefix is.
If it was a month ago I would delete it.

[rsc, 2012-02-29 20:50:49]

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

path/filepath: note that SplitList is different from strings.Split

R=golang-dev, r, bradfitz, gustavo
CC=golang-dev
http://codereview.appspot.com/5712044

[r2, 2012-02-29 20:53:12]

On 01/03/2012, at 7:50 AM, Russ Cox wrote:

> I have no idea what the point of filepath.HasPrefix is.
> If it was a month ago I would delete it.

it could have a 'this is pointless' comment even now

[rsc, 2012-02-29 20:59:11]

I started to write a comment about what it does that
would make clear when it makes sense to use
(and equally important, when it doesn't), but I couldn't
come up with any times it makes sense.  Maybe we
really should delete it.

The strikes against it are:

1. It does not take path boundaries into account.
2. It assumes that Windows==case-insensitive file system
and non-Windows==case-sensitive file system, neither of
which is always true.
3. Comparing ToLower against ToLower is not a correct
implementation of a case-insensitive string comparison.
4. If it returns true on Windows you still don't know how long
the matching prefix is in bytes, so you can't compute what
the suffix is.

Russ

[bradfitz, 2012-02-29 21:06:01]

I'd never even noticed that function before.  My small vote says kill it.

On Wed, Feb 29, 2012 at 12:59 PM, Russ Cox <rsc@golang.org> wrote:

> I started to write a comment about what it does that
> would make clear when it makes sense to use
> (and equally important, when it doesn't), but I couldn't
> come up with any times it makes sense.  Maybe we
> really should delete it.
>
> The strikes against it are:
>
> 1. It does not take path boundaries into account.
> 2. It assumes that Windows==case-insensitive file system
> and non-Windows==case-sensitive file system, neither of
> which is always true.
> 3. Comparing ToLower against ToLower is not a correct
> implementation of a case-insensitive string comparison.
> 4. If it returns true on Windows you still don't know how long
> the matching prefix is in bytes, so you can't compute what
> the suffix is.
>
> Russ
>

[gustavo_niemeyer.net, 2012-02-29 21:07:35]

On Wed, Feb 29, 2012 at 17:59, Russ Cox <rsc@golang.org> wrote:
> I started to write a comment about what it does that
> would make clear when it makes sense to use
> (and equally important, when it doesn't), but I couldn't
> come up with any times it makes sense.  Maybe we
> really should delete it.

+1.. its existence feels like a trap.

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

-- I'm not absolutely sure of anything.

[brainman, 2012-03-01 02:05:00]

On 2012/02/29 20:50:16, rsc wrote:
> I have no idea what the point of filepath.HasPrefix is.

The reason behind of filepath.HasPrefix is that we can compare paths on windows.
Is file c:\a\b lives in directory C:\a ? It does according to Windows. We were
using strings.HasPrefix before to answer that question, and that was not
acceptable. So I introduced filepath.HasPrefix so we could do that check without
care about GOOS.

Alex

[rsc, 2012-03-01 04:42:04]

On Wed, Feb 29, 2012 at 21:05,  <alex.brainman@gmail.com> wrote:
> The reason behind of filepath.HasPrefix is that we can compare paths on
> windows. Is file c:\a\b lives in directory C:\a ? It does according to
> Windows. We were using strings.HasPrefix before to answer that question,
> and that was not acceptable. So I introduced filepath.HasPrefix so we
> could do that check without care about GOOS.

That makes sense to me now; thanks.  As pointed out in the
other CL, I think it works better to have a separate utility for
canonicalizing paths, so that instead of having special ops
like this one you can write

strings.HasPrefix(canonical(path1), canonical(path2))

or even build more complex things, like HasSubdir.

For better or worse it appears that canonical is spelled EvalSymlinks.

Russ