cmd/go/internal/load: always use DefaultExecName to determine binary …

…name

It should produce equivalent results to split on the import path of the
package rather than its directory, in GOPATH mode. That means the common
code in DefaultExecName can be used.

We're in the middle of Go 1.12 cycle, so now is a good time to make it
happen for Go 1.13.

Modify isVersionElement to accept path elements like "v2", "v3", "v10",
rather than "/v2", "/v3", "/v10". Then use it in DefaultExecName instead
of the ad-hoc isVersion function. There is no change in behavior.

Add tests for DefaultExecName and isVersionElement.

Updates #26869

Change-Id: Ic6da2c92587459aa2b327385e994b72a6e183092
Reviewed-on: https://go-review.googlesource.com/c/go/+/168678
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>

src/cmd/go/internal/load/pkg.go

@@ -802,7 +802,7 @@ func findVersionElement(path string) (i, j int) {
 	j = len(path)
 	for i = len(path) - 1; i >= 0; i-- {
 		if path[i] == '/' {
-			if isVersionElement(path[i:j]) {
+			if isVersionElement(path[i+1 : j]) {
 				return i, j
 			}
 			j = i
@@ -814,10 +814,10 @@ func findVersionElement(path string) (i, j int) {
 // isVersionElement reports whether s is a well-formed path version element:
 // v2, v3, v10, etc, but not v0, v05, v1.
 func isVersionElement(s string) bool {
-	if len(s) < 3 || s[0] != '/' || s[1] != 'v' || s[2] == '0' || s[2] == '1' && len(s) == 3 {
+	if len(s) < 2 || s[0] != 'v' || s[1] == '0' || s[1] == '1' && len(s) == 2 {
 		return false
 	}
-	for i := 2; i < len(s); i++ {
+	for i := 1; i < len(s); i++ {
 		if s[i] < '0' || '9' < s[i] {
 			return false
 		}
@@ -1190,26 +1190,16 @@ var foldPath = make(map[string]string)
 // for a package with the import path importPath.
 //
 // The default executable name is the last element of the import path.
-// In module-aware mode, an additional rule is used. If the last element
-// is a vN path element specifying the major version, then the second last
-// element of the import path is used instead.
+// In module-aware mode, an additional rule is used on import paths
+// consisting of two or more path elements. If the last element is
+// a vN path element specifying the major version, then the
+// second last element of the import path is used instead.
 func DefaultExecName(importPath string) string {
 	_, elem := pathpkg.Split(importPath)
 	if cfg.ModulesEnabled {
-		// If this is example.com/mycmd/v2, it's more useful to install it as mycmd than as v2.
-		// See golang.org/issue/24667.
-		isVersion := func(v string) bool {
-			if len(v) < 2 || v[0] != 'v' || v[1] < '1' || '9' < v[1] {
-				return false
-			}
-			for i := 2; i < len(v); i++ {
-				if c := v[i]; c < '0' || '9' < c {
-					return false
-				}
-			}
-			return true
-		}
-		if isVersion(elem) {
+		// If this is example.com/mycmd/v2, it's more useful to
+		// install it as mycmd than as v2. See golang.org/issue/24667.
+		if elem != importPath && isVersionElement(elem) {
 			_, elem = pathpkg.Split(pathpkg.Dir(importPath))
 		}
 	}
@@ -1256,21 +1246,7 @@ func (p *Package) load(stk *ImportStack, bp *build.Package, err error) {
 			p.Error = &PackageError{Err: e}
 			return
 		}
-		_, elem := filepath.Split(p.Dir)
-		if cfg.ModulesEnabled {
-			// NOTE(rsc,dmitshur): Using p.ImportPath instead of p.Dir
-			// makes sure we install a package in the root of a
-			// cached module directory as that package name
-			// not name@v1.2.3.
-			// Using p.ImportPath instead of p.Dir
-			// is probably correct all the time,
-			// even for non-module-enabled code,
-			// but I'm not brave enough to change the
-			// non-module behavior this late in the
-			// release cycle. Can be done for Go 1.13.
-			// See golang.org/issue/26869.
-			elem = DefaultExecName(p.ImportPath)
-		}
+		elem := DefaultExecName(p.ImportPath)
 		full := cfg.BuildContext.GOOS + "_" + cfg.BuildContext.GOARCH + "/" + elem
 		if cfg.BuildContext.GOOS != base.ToolGOOS || cfg.BuildContext.GOARCH != base.ToolGOARCH {
 			// Install cross-compiled binaries to subdirectories of bin.

src/cmd/go/internal/load/pkg_test.go

@@ -0,0 +1,68 @@
+package load
+
+import (
+	"cmd/go/internal/cfg"
+	"testing"
+)
+
+func TestDefaultExecName(t *testing.T) {
+	oldModulesEnabled := cfg.ModulesEnabled
+	defer func() { cfg.ModulesEnabled = oldModulesEnabled }()
+	for _, tt := range []struct {
+		in         string
+		wantMod    string
+		wantGopath string
+	}{
+		{"example.com/mycmd", "mycmd", "mycmd"},
+		{"example.com/mycmd/v0", "v0", "v0"},
+		{"example.com/mycmd/v1", "v1", "v1"},
+		{"example.com/mycmd/v2", "mycmd", "v2"}, // Semantic import versioning, use second last element in module mode.
+		{"example.com/mycmd/v3", "mycmd", "v3"}, // Semantic import versioning, use second last element in module mode.
+		{"mycmd", "mycmd", "mycmd"},
+		{"mycmd/v0", "v0", "v0"},
+		{"mycmd/v1", "v1", "v1"},
+		{"mycmd/v2", "mycmd", "v2"}, // Semantic import versioning, use second last element in module mode.
+		{"v0", "v0", "v0"},
+		{"v1", "v1", "v1"},
+		{"v2", "v2", "v2"},
+	} {
+		{
+			cfg.ModulesEnabled = true
+			gotMod := DefaultExecName(tt.in)
+			if gotMod != tt.wantMod {
+				t.Errorf("DefaultExecName(%q) in module mode = %v; want %v", tt.in, gotMod, tt.wantMod)
+			}
+		}
+		{
+			cfg.ModulesEnabled = false
+			gotGopath := DefaultExecName(tt.in)
+			if gotGopath != tt.wantGopath {
+				t.Errorf("DefaultExecName(%q) in gopath mode = %v; want %v", tt.in, gotGopath, tt.wantGopath)
+			}
+		}
+	}
+}
+
+func TestIsVersionElement(t *testing.T) {
+	t.Parallel()
+	for _, tt := range []struct {
+		in   string
+		want bool
+	}{
+		{"v0", false},
+		{"v05", false},
+		{"v1", false},
+		{"v2", true},
+		{"v3", true},
+		{"v9", true},
+		{"v10", true},
+		{"v11", true},
+		{"v", false},
+		{"vx", false},
+	} {
+		got := isVersionElement(tt.in)
+		if got != tt.want {
+			t.Errorf("isVersionElement(%q) = %v; want %v", tt.in, got, tt.want)
+		}
+	}
+}