From e9188d1d18040bc6cb46065b2474664b8728a6df Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Wed, 12 Jun 2019 18:07:32 -0400 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Jay Conrod Reviewed-by: roger peppe --- src/cmd/go/internal/modfetch/cache.go | 42 ++++++++++++++++--- .../go/testdata/script/mod_pseudo_cache.txt | 29 +++++++++++++ 2 files changed, 66 insertions(+), 5 deletions(-) create mode 100644 src/cmd/go/testdata/script/mod_pseudo_cache.txt diff --git a/src/cmd/go/internal/modfetch/cache.go b/src/cmd/go/internal/modfetch/cache.go index 98d4806b61..b23776d874 100644 --- 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 index 0000000000..dd89614b9f --- /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",' -- 2.48.1