cmd/go/internal/get: move wildcard-trimming to before CheckImportPath

Previously, RepoRootForImportPath trimmed certain "..." wildcards from
package patterns (even though its name suggests that the argument must
be an actual import path). It trimmed at the first path element that
was literally "..." (although wildcards in general may appear within a
larger path element), and relied on a subsequent check in
RepoRootForImportPath to catch confusing resolutions.

However, that causes 'go get' with wildcard patterns in fresh paths to
fail as of CL 154101: a wildcard pattern is not a valid import path,
and fails the path check. (The existing Test{Vendor,Go}Get* packages
in go_test.go and vendor_test.go catch the failure, but they are all
skipped when the "-short" flag is set — including in all.bash — and we
had forgotten to run them separately.)

We now trim the path before any element that contains a wildcard, and
perform the path check (and repo resolution) on only that prefix. It
is possible that the expanded path after fetching the repo will be
invalid, but a repository can contain directories that are not valid
import paths in general anyway.

Fixes #29241

Change-Id: I70fb2f7fc6603b7d339fd6c02e8cdeacfc93fc4b
Reviewed-on: https://go-review.googlesource.com/c/154108
Reviewed-by: Russ Cox <rsc@golang.org>

diff --git a/src/cmd/go/internal/get/get.go b/src/cmd/go/internal/get/get.go
index f4b969fcb22c6c26b07e3b1494e0dcbaf2432f2d..a314c57160b396203d8db3638c150d31ea908392 100644 (file)
--- a/src/cmd/go/internal/get/get.go
+++ b/src/cmd/go/internal/get/get.go
@@ -232,7 +232,7 @@ var downloadCache = map[string]bool{}
 var downloadRootCache = map[string]bool{}
 
 // download runs the download half of the get command
-// for the package named by the argument.
+// for the package or pattern named by the argument.
 func download(arg string, parent *load.Package, stk *load.ImportStack, mode int) {
        if mode&load.ResolveImport != 0 {
                // Caller is responsible for expanding vendor paths.
@@ -402,7 +402,20 @@ func downloadPackage(p *load.Package) error {
                security = web.Insecure
        }
 
-       if err := CheckImportPath(p.ImportPath); err != nil {
+       // p can be either a real package, or a pseudo-package whose “import path” is
+       // actually a wildcard pattern.
+       // Trim the path at the element containing the first wildcard,
+       // and hope that it applies to the wildcarded parts too.
+       // This makes 'go get rsc.io/pdf/...' work in a fresh GOPATH.
+       importPrefix := p.ImportPath
+       if i := strings.Index(importPrefix, "..."); i >= 0 {
+               slash := strings.LastIndexByte(importPrefix[:i], '/')
+               if slash < 0 {
+                       return fmt.Errorf("cannot expand ... in %q", p.ImportPath)
+               }
+               importPrefix = importPrefix[:slash]
+       }
+       if err := CheckImportPath(importPrefix); err != nil {
                return fmt.Errorf("%s: invalid import path: %v", p.ImportPath, err)
        }
 
@@ -425,7 +438,7 @@ func downloadPackage(p *load.Package) error {
                        }
                        repo = remote
                        if !*getF && err == nil {
-                               if rr, err := RepoRootForImportPath(p.ImportPath, IgnoreMod, security); err == nil {
+                               if rr, err := RepoRootForImportPath(importPrefix, IgnoreMod, security); err == nil {
                                        repo := rr.Repo
                                        if rr.vcs.resolveRepo != nil {
                                                resolved, err := rr.vcs.resolveRepo(rr.vcs, dir, repo)
@@ -442,7 +455,7 @@ func downloadPackage(p *load.Package) error {
        } else {
                // Analyze the import path to determine the version control system,
                // repository, and the import path for the root of the repository.
-               rr, err := RepoRootForImportPath(p.ImportPath, IgnoreMod, security)
+               rr, err := RepoRootForImportPath(importPrefix, IgnoreMod, security)
                if err != nil {
                        return err
                }

diff --git a/src/cmd/go/internal/get/vcs.go b/src/cmd/go/internal/get/vcs.go
index 052c82b7b564d692296a317cc0258756babb8d84..a7a2ba32cc33cbe0baebb7152cbe78a646abf8ae 100644 (file)
--- a/src/cmd/go/internal/get/vcs.go
+++ b/src/cmd/go/internal/get/vcs.go
@@ -647,14 +647,7 @@ const (
 func RepoRootForImportPath(importPath string, mod ModuleMode, security web.SecurityMode) (*RepoRoot, error) {
        rr, err := repoRootFromVCSPaths(importPath, "", security, vcsPaths)
        if err == errUnknownSite {
-               // If there are wildcards, look up the thing before the wildcard,
-               // hoping it applies to the wildcarded parts too.
-               // This makes 'go get rsc.io/pdf/...' work in a fresh GOPATH.
-               lookup := strings.TrimSuffix(importPath, "/...")
-               if i := strings.Index(lookup, "/.../"); i >= 0 {
-                       lookup = lookup[:i]
-               }
-               rr, err = repoRootForImportDynamic(lookup, mod, security)
+               rr, err = repoRootForImportDynamic(importPath, mod, security)
                if err != nil {
                        err = fmt.Errorf("unrecognized import path %q (%v)", importPath, err)
                }
@@ -667,6 +660,7 @@ func RepoRootForImportPath(importPath string, mod ModuleMode, security web.Secur
                }
        }
 
+       // Should have been taken care of above, but make sure.
        if err == nil && strings.Contains(importPath, "...") && strings.Contains(rr.Root, "...") {
                // Do not allow wildcards in the repo root.
                rr = nil