cmd/go/internal/modfetch: re-resolve commit hashes in readDiskStat

Previously, when we resolved a commit hash (not a complete version),
we always checked the contents of the module cache for any
pseudo-version matching that commit.

However, there are many possible names for a given commit. Generally
the semantically-highest valid name is the best, and that may change
over time as new tags are added, so if we are able to fetch a better
name from upstream we should do so. Otherwise, we should fall back to
the highest appropriate name found in the cache.

Fixes #27171
Updates #27173

Change-Id: Ib5c7d99eb463af84674e969813039cbbee7e395b
Reviewed-on: https://go-review.googlesource.com/c/go/+/182178
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: roger peppe <rogpeppe@gmail.com>

diff --git a/src/cmd/go/internal/modfetch/cache.go b/src/cmd/go/internal/modfetch/cache.go
index 98d4806b61cd75091db292f8b7b3850d72b3ea56..b23776d874d846a26130c8460cee4addb1c7b128 100644 (file)
--- a/src/cmd/go/internal/modfetch/cache.go
+++ b/src/cmd/go/internal/modfetch/cache.go
@@ -15,6 +15,7 @@ import (
        "strings"
 
        "cmd/go/internal/base"
+       "cmd/go/internal/cfg"
        "cmd/go/internal/lockedfile"
        "cmd/go/internal/modfetch/codehost"
        "cmd/go/internal/module"
@@ -385,8 +386,29 @@ var errNotCached = fmt.Errorf("not in cache")
 func readDiskStat(path, rev string) (file string, info *RevInfo, err error) {
        file, data, err := readDiskCache(path, rev, "info")
        if err != nil {
-               if file, info, err := readDiskStatByHash(path, rev); err == nil {
-                       return file, info, nil
+               // If the cache already contains a pseudo-version with the given hash, we
+               // would previously return that pseudo-version without checking upstream.
+               // However, that produced an unfortunate side-effect: if the author added a
+               // tag to the repository, 'go get' would not pick up the effect of that new
+               // tag on the existing commits, and 'go' commands that referred to those
+               // commits would use the previous name instead of the new one.
+               //
+               // That's especially problematic if the original pseudo-version starts with
+               // v0.0.0-, as was the case for all pseudo-versions during vgo development,
+               // since a v0.0.0- pseudo-version has lower precedence than pretty much any
+               // tagged version.
+               //
+               // In practice, we're only looking up by hash during initial conversion of a
+               // legacy config and during an explicit 'go get', and a little extra latency
+               // for those operations seems worth the benefit of picking up more accurate
+               // versions.
+               //
+               // Fall back to this resolution scheme only if the GOPROXY setting prohibits
+               // us from resolving upstream tags.
+               if cfg.GOPROXY == "off" {
+                       if file, info, err := readDiskStatByHash(path, rev); err == nil {
+                               return file, info, nil
+                       }
                }
                return file, nil, err
        }
@@ -436,13 +458,23 @@ func readDiskStatByHash(path, rev string) (file string, info *RevInfo, err error
        if err != nil {
                return "", nil, errNotCached
        }
+
+       // A given commit hash may map to more than one pseudo-version,
+       // depending on which tags are present on the repository.
+       // Take the highest such version.
+       var maxVersion string
        suffix := "-" + rev + ".info"
+       err = errNotCached
        for _, name := range names {
-               if strings.HasSuffix(name, suffix) && IsPseudoVersion(strings.TrimSuffix(name, ".info")) {
-                       return readDiskStat(path, strings.TrimSuffix(name, ".info"))
+               if strings.HasSuffix(name, suffix) {
+                       v := strings.TrimSuffix(name, ".info")
+                       if IsPseudoVersion(v) && semver.Max(maxVersion, v) == v {
+                               maxVersion = v
+                               file, info, err = readDiskStat(path, strings.TrimSuffix(name, ".info"))
+                       }
                }
        }
-       return "", nil, errNotCached
+       return file, info, err
 }
 
 // oldVgoPrefix is the prefix in the old auto-generated cached go.mod files.

diff --git a/src/cmd/go/testdata/script/mod_pseudo_cache.txt b/src/cmd/go/testdata/script/mod_pseudo_cache.txt
new file mode 100644 (file)
index 0000000..dd89614
--- /dev/null
+++ b/src/cmd/go/testdata/script/mod_pseudo_cache.txt
@@ -0,0 +1,29 @@
+[!net] skip
+[!exec:git] skip
+
+env GO111MODULE=on
+env GOPROXY=direct
+env GOSUMDB=off
+
+# Regression test for golang.org/issue/27171: after resolving an older
+# pseudo-version of a commit, future resolution of that commit by hash should
+# choose the highest appropriate pseudo-version instead of the cached one.
+
+go mod download -json golang.org/x/text@v0.0.0-20171215141712-a1b916ed6726
+stdout '"Version": "v0.0.0-20171215141712-a1b916ed6726",'
+
+# If GOPROXY is 'off', lookups should use whatever pseudo-version is available.
+env GOPROXY=off
+go mod download -json golang.org/x/text@a1b916ed6726
+stdout '"Version": "v0.0.0-20171215141712-a1b916ed6726",'
+
+# If we can re-resolve the commit to a pseudo-version, fetching the commit by
+# hash should use the highest such pseudo-version appropriate to the commit.
+env GOPROXY=direct
+go mod download -json golang.org/x/text@a1b916ed6726
+stdout '"Version": "v0.3.1-0.20171215141712-a1b916ed6726",'
+
+# If GOPROXY is 'off', lookups should use the highest pseudo-version in the cache.
+env GOPROXY=off
+go mod download -json golang.org/x/text@a1b916ed6726
+stdout '"Version": "v0.3.1-0.20171215141712-a1b916ed6726",'