x/tools/go/vcs: fix FromDir returning bad root on Windows

Import path is a '/'-separated path. FromDir documentation says
on return, root is the import path corresponding to the root of
the repository. On Windows and other OSes where os.PathSeparator
is not '/', that wasn't true since root would contain characters
other than '/', and therefore it wasn't a valid import path
corresponding to the root of the repository. Fix that by using
filepath.ToSlash.

Add test coverage for root value returned from FromDir, it was
previously not tested.

Additionally, remove a dubious statement from the documentation
"(thus root is a prefix of importPath)". There is no variable
importPath that is being referred to. It's also redundant and
confusing. Without it, the description of root value matches
the documentation of RepoRoot.Root struct field:

	// Root is the import path corresponding to the root of the
	// repository.
	Root string

Fixes golang/go#7723.

Change-Id: If9f5f55b5751e01a7f88b79d9b039402af3e9312
Reviewed-on: https://go-review.googlesource.com/18461
Reviewed-by: Chris Manghane <cmang@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>

go/vcs/vcs.go

@@ -335,8 +335,7 @@ type vcsPath struct {
 // 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
+// corresponding to the root of the repository.
-// (thus root is a prefix of importPath).
 func FromDir(dir, srcRoot string) (vcs *Cmd, root string, err error) {
 	// Clean and double-check that dir is in (a subdirectory of) srcRoot.
 	dir = filepath.Clean(dir)
@@ -348,7 +347,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
 			}
 		}
 

go/vcs/vcs_test.go

@@ -7,6 +7,7 @@ package vcs
 import (
 	"io/ioutil"
 	"os"
+	pathpkg "path"
 	"path/filepath"
 	"reflect"
 	"runtime"
@@ -61,11 +62,11 @@ func TestRepoRootForImportPath(t *testing.T) {
 	}
 }
 
-// Test that FromDir correctly inspects a given directory and returns the right VCS.
+// Test that FromDir correctly inspects a given directory and returns the right VCS and root.
 func TestFromDir(t *testing.T) {
 	type testStruct struct {
 		path string
-		want *Cmd
+		want *RepoRoot
 	}
 
 	tests := make([]testStruct, len(vcsList))
@@ -77,16 +78,29 @@ func TestFromDir(t *testing.T) {
 
 	for i, vcs := range vcsList {
 		tests[i] = testStruct{
-			filepath.Join(tempDir, vcs.Name, "."+vcs.Cmd),
+			path: filepath.Join(tempDir, "example.com", vcs.Name, "."+vcs.Cmd),
-			vcs,
+			want: &RepoRoot{
+				VCS:  vcs,
+				Root: pathpkg.Join("example.com", vcs.Name),
+			},
 		}
 	}
 
 	for _, test := range tests {
 		os.MkdirAll(test.path, 0755)
-		got, _, _ := FromDir(test.path, tempDir)
+		var (
-		if got.Name != test.want.Name {
+			got = new(RepoRoot)
-			t.Errorf("FromDir(%q, %q) = %s, want %s", test.path, tempDir, got, test.want)
+			err error
+		)
+		got.VCS, got.Root, err = FromDir(test.path, tempDir)
+		if err != nil {
+			t.Errorf("FromDir(%q, %q): %v", test.path, tempDir, err)
+			os.RemoveAll(test.path)
+			continue
+		}
+		want := test.want
+		if got.VCS.Name != want.VCS.Name || got.Root != want.Root {
+			t.Errorf("FromDir(%q, %q) = VCS(%s) Root(%s), want VCS(%s) Root(%s)", test.path, tempDir, got.VCS, got.Root, want.VCS, want.Root)
 		}
 		os.RemoveAll(test.path)
 	}