]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go: query each path only once in 'go get'
authorBryan C. Mills <bcmills@google.com>
Thu, 8 Aug 2019 22:09:54 +0000 (18:09 -0400)
committerBryan C. Mills <bcmills@google.com>
Fri, 9 Aug 2019 15:52:43 +0000 (15:52 +0000)
If we don't know whether a path is a module path or a package path,
previously we would first try a module query for it, then fall back to
a package query.

If we are using a sequence of proxies with fallback (as will be the
default in Go 1.13), and the path is not actually a module path, that
initial module query will fail against the first proxy, then
immediately fall back to the next proxy in the sequence — even if the
query could have been satisfied by some other (prefix) module
available from the first proxy.

Instead, we now query the requested path as only one kind of path.
If we query it as a package path but it turns out to only exist as a
module, we can detect that as a PackageNotInModuleError with an
appropriate module path — we do not need to issue a second query to
classify it.

Fixes #31785

Change-Id: I581d44279196e41d1fed27ec25489e75d62654e3
Reviewed-on: https://go-review.googlesource.com/c/go/+/189517
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
src/cmd/go/internal/modfetch/repo.go
src/cmd/go/internal/modget/get.go
src/cmd/go/internal/modload/query.go
src/cmd/go/testdata/script/mod_get_fallback.txt [new file with mode: 0644]
src/cmd/go/testdata/script/mod_get_newcycle.txt
src/cmd/go/testdata/script/mod_sumdb.txt

index 95351269dbec8c79b82e8e0c12a7249740b5f01a..be52a8dc11f977fe357b04bfe4cb2b3a84aadb8c 100644 (file)
@@ -240,7 +240,7 @@ func lookup(proxy, path string) (r Repo, err error) {
 
 var (
        errModVendor       = errors.New("module lookup disabled by -mod=vendor")
-       errProxyOff        = errors.New("module lookup disabled by GOPROXY=off")
+       errProxyOff        = notExistError("module lookup disabled by GOPROXY=off")
        errNoproxy   error = notExistError("disabled by GOPRIVATE/GONOPROXY")
        errUseProxy  error = notExistError("path does not match GOPRIVATE/GONOPROXY")
 )
index 84b1ac1b019b54b7039bbf70a226c3dc80d2b044..1cae311c4c157b825164f2fe36fbd559fe2ebeeb 100644 (file)
@@ -735,7 +735,7 @@ func runQueries(cache map[querySpec]*query, queries []*query, modOnly map[string
        return byPath
 }
 
-// getQuery evaluates the given package path, version pair
+// getQuery evaluates the given (package or module) path and version
 // to determine the underlying module version being requested.
 // If forceModulePath is set, getQuery must interpret path
 // as a module path.
@@ -753,40 +753,51 @@ func getQuery(path, vers string, prevM module.Version, forceModulePath bool) (mo
                base.Fatalf("go get: internal error: prevM may be set if and only if forceModulePath is set")
        }
 
-       if forceModulePath || !strings.Contains(path, "...") {
+       // If the query must be a module path, try only that module path.
+       if forceModulePath {
                if path == modload.Target.Path {
                        if vers != "latest" {
                                return module.Version{}, fmt.Errorf("can't get a specific version of the main module")
                        }
                }
 
-               // If the path doesn't contain a wildcard, try interpreting it as a module path.
                info, err := modload.Query(path, vers, prevM.Version, modload.Allowed)
                if err == nil {
                        return module.Version{Path: path, Version: info.Version}, nil
                }
 
-               // If the query fails, and the path must be a real module, report the query error.
-               if forceModulePath {
-                       // If the query was "upgrade" or "patch" and the current version has been
-                       // replaced, check to see whether the error was for that same version:
-                       // if so, the version was probably replaced because it is invalid,
-                       // and we should keep that replacement without complaining.
-                       if vers == "upgrade" || vers == "patch" {
-                               var vErr *module.InvalidVersionError
-                               if errors.As(err, &vErr) && vErr.Version == prevM.Version && modload.Replacement(prevM).Path != "" {
-                                       return prevM, nil
-                               }
+               // If the query was "upgrade" or "patch" and the current version has been
+               // replaced, check to see whether the error was for that same version:
+               // if so, the version was probably replaced because it is invalid,
+               // and we should keep that replacement without complaining.
+               if vers == "upgrade" || vers == "patch" {
+                       var vErr *module.InvalidVersionError
+                       if errors.As(err, &vErr) && vErr.Version == prevM.Version && modload.Replacement(prevM).Path != "" {
+                               return prevM, nil
                        }
-                       return module.Version{}, err
                }
+
+               return module.Version{}, err
        }
 
-       // Otherwise, try a package path or pattern.
+       // If the query may be either a package or a module, try it as a package path.
+       // If it turns out to only exist as a module, we can detect the resulting
+       // PackageNotInModuleError and avoid a second round-trip through (potentially)
+       // all of the configured proxies.
        results, err := modload.QueryPattern(path, vers, modload.Allowed)
        if err != nil {
+               // If the path doesn't contain a wildcard, check whether it was actually a
+               // module path instead. If so, return that.
+               if !strings.Contains(path, "...") {
+                       var modErr *modload.PackageNotInModuleError
+                       if errors.As(err, &modErr) && modErr.Mod.Path == path {
+                               return modErr.Mod, nil
+                       }
+               }
+
                return module.Version{}, err
        }
+
        return results[0].Mod, nil
 }
 
index 8c5fdc8bf9ba5986eba5564c9cc2e19d6bd18903..602bf47275db8bb7af45f682ef20c601d60888a8 100644 (file)
@@ -380,10 +380,10 @@ func QueryPattern(pattern, query string, allowed func(module.Version) bool) ([]Q
                        }
                        r.Packages = match(r.Mod, root, isLocal)
                        if len(r.Packages) == 0 {
-                               return r, &packageNotInModuleError{
-                                       mod:     r.Mod,
-                                       query:   query,
-                                       pattern: pattern,
+                               return r, &PackageNotInModuleError{
+                                       Mod:     r.Mod,
+                                       Query:   query,
+                                       Pattern: pattern,
                                }
                        }
                        return r, nil
@@ -446,30 +446,31 @@ func queryPrefixModules(candidateModules []string, queryModule func(path string)
        wg.Wait()
 
        // Classify the results. In case of failure, identify the error that the user
-       // is most likely to find helpful.
+       // is most likely to find helpful: the most useful class of error at the
+       // longest matching path.
        var (
+               noPackage   *PackageNotInModuleError
                noVersion   *NoMatchingVersionError
-               noPackage   *packageNotInModuleError
                notExistErr error
        )
        for _, r := range results {
                switch rErr := r.err.(type) {
                case nil:
                        found = append(found, r.QueryResult)
+               case *PackageNotInModuleError:
+                       if noPackage == nil {
+                               noPackage = rErr
+                       }
                case *NoMatchingVersionError:
                        if noVersion == nil {
                                noVersion = rErr
                        }
-               case *packageNotInModuleError:
-                       if noPackage == nil {
-                               noPackage = rErr
-                       }
                default:
                        if errors.Is(rErr, os.ErrNotExist) {
                                if notExistErr == nil {
                                        notExistErr = rErr
                                }
-                       } else {
+                       } else if err == nil {
                                err = r.err
                        }
                }
@@ -515,31 +516,31 @@ func (e *NoMatchingVersionError) Error() string {
        return fmt.Sprintf("no matching versions for query %q", e.query) + currentSuffix
 }
 
-// A packageNotInModuleError indicates that QueryPattern found a candidate
+// A PackageNotInModuleError indicates that QueryPattern found a candidate
 // module at the requested version, but that module did not contain any packages
 // matching the requested pattern.
 //
-// NOTE: packageNotInModuleError MUST NOT implement Is(os.ErrNotExist).
+// NOTE: PackageNotInModuleError MUST NOT implement Is(os.ErrNotExist).
 //
 // If the module came from a proxy, that proxy had to return a successful status
 // code for the versions it knows about, and thus did not have the opportunity
 // to return a non-400 status code to suppress fallback.
-type packageNotInModuleError struct {
-       mod     module.Version
-       query   string
-       pattern string
+type PackageNotInModuleError struct {
+       Mod     module.Version
+       Query   string
+       Pattern string
 }
 
-func (e *packageNotInModuleError) Error() string {
+func (e *PackageNotInModuleError) Error() string {
        found := ""
-       if e.query != e.mod.Version {
-               found = fmt.Sprintf(" (%s)", e.mod.Version)
+       if e.Query != e.Mod.Version {
+               found = fmt.Sprintf(" (%s)", e.Mod.Version)
        }
 
-       if strings.Contains(e.pattern, "...") {
-               return fmt.Sprintf("module %s@%s%s found, but does not contain packages matching %s", e.mod.Path, e.query, found, e.pattern)
+       if strings.Contains(e.Pattern, "...") {
+               return fmt.Sprintf("module %s@%s%s found, but does not contain packages matching %s", e.Mod.Path, e.Query, found, e.Pattern)
        }
-       return fmt.Sprintf("module %s@%s%s found, but does not contain package %s", e.mod.Path, e.query, found, e.pattern)
+       return fmt.Sprintf("module %s@%s%s found, but does not contain package %s", e.Mod.Path, e.Query, found, e.Pattern)
 }
 
 // ModuleHasRootPackage returns whether module m contains a package m.Path.
diff --git a/src/cmd/go/testdata/script/mod_get_fallback.txt b/src/cmd/go/testdata/script/mod_get_fallback.txt
new file mode 100644 (file)
index 0000000..a9834a3
--- /dev/null
@@ -0,0 +1,10 @@
+env GO111MODULE=on
+
+[!net] skip
+
+env GOPROXY=https://proxy.golang.org,direct
+env GOSUMDB=off
+
+go get -x -v -d golang.org/x/tools/cmd/goimports
+stderr '# get https://proxy.golang.org/golang.org/x/tools/@latest'
+! stderr '# get https://golang.org'
index 9616863383dae74cedc7f4a230a9d5066dffa7bd..b1838f824a6353aa3e6f0c89cd2c614bf8a71c5c 100644 (file)
@@ -1,6 +1,7 @@
 env GO111MODULE=on
 
 # Download modules to avoid stderr chatter
+go mod download example.com@v1.0.0
 go mod download example.com/newcycle/a@v1.0.0
 go mod download example.com/newcycle/a@v1.0.1
 go mod download example.com/newcycle/b@v1.0.0
@@ -10,5 +11,6 @@ go mod init m
 cmp stderr stderr-expected
 
 -- stderr-expected --
+go: finding example.com/newcycle v1.0.0
 go get: inconsistent versions:
        example.com/newcycle/a@v1.0.0 requires example.com/newcycle/a@v1.0.1 (not example.com/newcycle/a@v1.0.0)
index 8e1f3d7a7b364db6c8897cd39fb3ae23ee8be2ca..641b9e73bcc67f565c9945f5d0359bc6a9ceae9e 100644 (file)
@@ -9,8 +9,8 @@ env dbname=localhost.localdev/sumdb
 cp go.mod.orig go.mod
 env GOSUMDB=$sumdb' '$proxy/sumdb-wrong
 ! go get -d rsc.io/quote
-stderr 'verifying rsc.io/quote@v1.5.2/go.mod: checksum mismatch'
-stderr 'downloaded: h1:LzX7'
+stderr 'verifying rsc.io/quote@v1.5.2: checksum mismatch'
+stderr 'downloaded: h1:3fEy'
 stderr 'localhost.localdev/sumdb: h1:wrong'
 stderr 'SECURITY ERROR\nThis download does NOT match the one reported by the checksum server.'
 ! go get -d rsc.io/sampler