code review 3146041: godoc: goroot must be clean, w/o trailing '/' (bug fix)

godoc: bug fix in relativePath

This fixes a problem with relativePath, where
a prefix was not recognized because it ended
in "//" as opposed to just "/".

Also: Minor unrelated cleanup of a declaration.

[gri, 2010-11-16 19:48:52]

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

I'd like you to review this change.

[rsc, 2010-11-16 19:52:32]

i don't think this is correct.
path.Clean("/") = "/"

[gri, 2010-11-16 22:23:03]

I think this change is fine.

a) The Clean call will make sure goroot is sanitized.
b) The fix in ToRelative makes sure that the prefix ends in a "/"
independent of whether GOROOT is "/" or not.

These changes will fix the problem at hand (ToRelative not working
correctly) and will keep the rest running as before.

I am reluctant to change the invariant for goroot such that it always ends
in "/" as the code so far has run with it not ending in "/". This may be the
right thing to do, but I don't want to chase down problems related to that
change while I am working on another larger and unrelated change.

Added a TODO and slightly reformulated the code in ToRelative. Also changed
the CL description to be less misleading.

PTAL.

- gri

On Tue, Nov 16, 2010 at 11:52 AM, Russ Cox <rsc@golang.org> wrote:

> i don't think this is correct.
> path.Clean("/") = "/"
>

[rsc1, 2010-11-17 18:22:56]

Can path.Clean be dropped now?

I am just a little worried about the fact that
it does not preserve Unix path name semantics
when .. and symlinks are involved.  For presentation
using path.Clean makes perfect sense but for
computing names to use with system calls, it is
usually less surprising if avoided.

If we don't want to figure out why godoc can't
work with a trailing slash and a cleaner is needed,
a safer one would be

func clean(s string) string {
    if len(s) > 1 && s[len(s)-1] == '/' {
        return s[0:len(s)-1]
    }
    return s
}

[gri, 2010-11-17 18:28:17]

sure.
PTAL.
- gri

On Wed, Nov 17, 2010 at 10:22 AM, <rsc@google.com> wrote:

> Can path.Clean be dropped now?
>
> I am just a little worried about the fact that
> it does not preserve Unix path name semantics
> when .. and symlinks are involved.  For presentation
> using path.Clean makes perfect sense but for
> computing names to use with system calls, it is
> usually less surprising if avoided.
>
> If we don't want to figure out why godoc can't
> work with a trailing slash and a cleaner is needed,
> a safer one would be
>
> func clean(s string) string {
>    if len(s) > 1 && s[len(s)-1] == '/' {
>        return s[0:len(s)-1]
>    }
>    return s
> }
>
>
> http://codereview.appspot.com/3146041/
>

[rsc1, 2010-11-17 18:47:16]

LGTM

[gri, 2010-11-17 19:03:39]

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

godoc: bug fix in relativePath

This fixes a problem with relativePath, where
a prefix was not recognized because it ended
in "//" as opposed to just "/".

Also: Minor unrelated cleanup of a declaration.

R=rsc
CC=golang-dev
http://codereview.appspot.com/3146041