x/tools/go/vcs: FromDir documentation and root return value

[kr]

func FromDir(dir, srcRoot string) (vcs *Cmd, root string, err error)

"FromDir inspects dir and its parents to determine the version control
system and code repository to use. On return, root is the import path
corresponding to the root of the repository (thus root is a prefix of
importPath)."

- importPath isn't part of the function's signature. Copy-paste error
from somewhere?

- the value returned in root starts with "src/"; it is not a prefix of
any typical import path. Doc bug or code bug?

- the returned root is a filesystem path, but documenting it as an
"import path" suggests it'll always be slash-separated.
Doc bug or code bug?

[robpike]

Comment 1:

Easy doc fix. Should do for 1.3

Labels changed: added release-go1.3, repo-tools.

Owner changed to @robpike.

Status changed to Accepted.

[kr]

Comment 2:

I think part of this confusion is from me misunderstanding the purpose of srcRoot. I had
been using the Root field from 'go list' directly, but it looks like it should be
filepath.Join(pkg.Root, "src")

[rsc]

Comment 3:

Not part of the release.

Labels changed: removed release-go1.3.

[paranoiacblack]

Comment 5:

I have a couple of follow-up questions, to make sure I get this right.  The `importPath`
comment is erroneous as you state, but I'm wondering if you could provide some context
about the other two points you make.  Perhaps an example of a directory layout for a
given repository and a mismatch between what you expect from FromDir and what it returns
instead.
"- the returned root is a filesystem path, but documenting it as an
"import path" suggests it'll always be slash-separated.
Doc bug or code bug?"
Can you clarify this, as well?  What was the import path separated by if not by slashes?

[kr]

Comment 6:

On Windows, the root parameter contains backslashes.
But an import path is conventionally slash-separated,
not a filepath as implemented here. (It's a slice of dir.)
My middle point above was due to my confusion about
what to provide for srcRoot. For a dir of $GOPATH/src/foo/bar,
I had been providing $GOPATH and getting back src/foo.
Providing $GOPATH/src yields just foo, as documented.
A sentence documenting the expected form of srcRoot would
have saved me a little time.

[rsc]

Comment 7:

Labels changed: added release-none.

[dmitshur]

I also ran into the invalid root value on Windows bug and was going to file an issue, but I found this existing one.

I think part of this confusion is from me misunderstanding the purpose of srcRoot. I had
been using the Root field from 'go list' directly, but it looks like it should be
filepath.Join(pkg.Root, "src")

It should be pkg.SrcRoot, which is already set to that value. /cc @kr

• importPath isn't part of the function's signature. Copy-paste error
  from somewhere?

You're right, importPath is confusing because it makes it sound like a variable, but there is no such variable anywhere. Perhaps it meant to say "thus root is a prefix of import path", meaning root is a prefix of the import path of the package that is in in dir.

On Windows, the root parameter contains backslashes.
But an import path is conventionally slash-separated,
not a filepath as implemented here. (It's a slice of dir.)

I can confirm, and I think it's a bug. Here's the rationale. The documentation says:

On return, root is the import path corresponding to the root of the repository

If I understand correctly, an import path is defined as a /-separated path (source here).

However, as @kr said, on Windows, there's a bug in that the returned root value uses \ as separators.

Consider this snippet being executed on Windows:

pkg, err := build.Import("example.com/repo/subpackage", "", build.FindOnly)
if err != nil {
    panic(err)
}
_, root, err := FromDir(pkg.Dir, pkg.SrcRoot)
if err != nil {
    panic(err)
}
fmt.Println(root)

On Windows, that snippet will print example.com\repo, which is incorrect (it is not an import path corresponding to repo root, it has a wrong \ character). On OS X and Linux it will print example.com/repo correctly.

The fix is simple:

diff --git a/go/vcs/vcs.go b/go/vcs/vcs.go
index a702496..b615508 100644
--- a/go/vcs/vcs.go
+++ b/go/vcs/vcs.go
@@ -348,7 +348,7 @@ func FromDir(dir, srcRoot string) (vcs *Cmd, root string, err error) {
    for len(dir) > len(srcRoot) {
        for _, vcs := range vcsList {
            if fi, err := os.Stat(filepath.Join(dir, "."+vcs.Cmd)); err == nil && fi.IsDir() {
-               return vcs, dir[len(srcRoot)+1:], nil
+               return vcs, filepath.ToSlash(dir[len(srcRoot)+1:]), nil
            }
        }

I can make a CL that fixes this issue.

[dmitshur]

I've created https://golang.org/cl/18461 that resolves this issue.

[gopherbot]

CL https://golang.org/cl/18461 mentions this issue.

[gopherbot]

CL https://golang.org/cl/21345 mentions this issue.