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>

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.

src/cmd/go/testdata/script/mod_pseudo_cache.txt

+[!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",'