From 4d716e4d4a25526ba963a7cfb2b5208eb52e71c0 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Thu, 24 Feb 2022 16:12:34 -0500 Subject: [PATCH] cmd/go/internal/modfetch: simplify handling of weird version tags This fixes an obscure bug in 'go list -versions' if the repo contains a tag with an explicit "+incompatible" suffix. However, I've never seen such a repo in the wild; mostly it's an attempt to wrap my brain around the code and simplify things a bit for the future. Updates #51324 Updates #51312 Change-Id: I1b078b5db36470cf61aaa85b5244c99b5ee2c842 Reviewed-on: https://go-review.googlesource.com/c/go/+/387917 Run-TryBot: Bryan Mills Auto-Submit: Bryan Mills TryBot-Result: Gopher Robot Reviewed-by: Michael Matloob --- .../go/internal/modfetch/codehost/codehost.go | 2 +- src/cmd/go/internal/modfetch/codehost/git.go | 12 ++-- src/cmd/go/internal/modfetch/coderepo.go | 41 +++++++---- src/cmd/go/internal/modfetch/coderepo_test.go | 68 +++++++++++++++++++ .../go/testdata/script/mod_list_odd_tags.txt | 13 ++++ 5 files changed, 113 insertions(+), 23 deletions(-) create mode 100644 src/cmd/go/testdata/script/mod_list_odd_tags.txt diff --git a/src/cmd/go/internal/modfetch/codehost/codehost.go b/src/cmd/go/internal/modfetch/codehost/codehost.go index 31dc811752b83..e08a84b32c672 100644 --- a/src/cmd/go/internal/modfetch/codehost/codehost.go +++ b/src/cmd/go/internal/modfetch/codehost/codehost.go @@ -65,7 +65,7 @@ type Repo interface { // RecentTag returns the most recent tag on rev or one of its predecessors // with the given prefix. allowed may be used to filter out unwanted versions. - RecentTag(rev, prefix string, allowed func(string) bool) (tag string, err error) + RecentTag(rev, prefix string, allowed func(tag string) bool) (tag string, err error) // DescendsFrom reports whether rev or any of its ancestors has the given tag. // diff --git a/src/cmd/go/internal/modfetch/codehost/git.go b/src/cmd/go/internal/modfetch/codehost/git.go index 9c8fd42833cc7..853d43bc5b2ea 100644 --- a/src/cmd/go/internal/modfetch/codehost/git.go +++ b/src/cmd/go/internal/modfetch/codehost/git.go @@ -523,7 +523,7 @@ func (r *gitRepo) ReadFile(rev, file string, maxSize int64) ([]byte, error) { return out, nil } -func (r *gitRepo) RecentTag(rev, prefix string, allowed func(string) bool) (tag string, err error) { +func (r *gitRepo) RecentTag(rev, prefix string, allowed func(tag string) bool) (tag string, err error) { info, err := r.Stat(rev) if err != nil { return "", err @@ -553,15 +553,11 @@ func (r *gitRepo) RecentTag(rev, prefix string, allowed func(string) bool) (tag if !strings.HasPrefix(line, prefix) { continue } - - semtag := line[len(prefix):] - // Consider only tags that are valid and complete (not just major.minor prefixes). - // NOTE: Do not replace the call to semver.Compare with semver.Max. - // We want to return the actual tag, not a canonicalized version of it, - // and semver.Max currently canonicalizes (see golang.org/issue/32700). - if c := semver.Canonical(semtag); c == "" || !strings.HasPrefix(semtag, c) || !allowed(semtag) { + if !allowed(line) { continue } + + semtag := line[len(prefix):] if semver.Compare(semtag, highest) > 0 { highest = semtag } diff --git a/src/cmd/go/internal/modfetch/coderepo.go b/src/cmd/go/internal/modfetch/coderepo.go index dfaf16def61f2..ff1cea1d94caf 100644 --- a/src/cmd/go/internal/modfetch/coderepo.go +++ b/src/cmd/go/internal/modfetch/coderepo.go @@ -159,7 +159,18 @@ func (r *codeRepo) Versions(prefix string) ([]string, error) { if r.codeDir != "" { v = v[len(r.codeDir)+1:] } - if v == "" || v != module.CanonicalVersion(v) || module.IsPseudoVersion(v) { + if v == "" || v != semver.Canonical(v) { + // Ignore non-canonical tags: Stat rewrites those to canonical + // pseudo-versions. Note that we compare against semver.Canonical here + // instead of module.CanonicalVersion: revToRev strips "+incompatible" + // suffixes before looking up tags, so a tag like "v2.0.0+incompatible" + // would not resolve at all. (The Go version string "v2.0.0+incompatible" + // refers to the "v2.0.0" version tag, which we handle below.) + continue + } + if module.IsPseudoVersion(v) { + // Ignore tags that look like pseudo-versions: Stat rewrites those + // unambiguously to the underlying commit, and tagToVersion drops them. continue } @@ -540,23 +551,21 @@ func (r *codeRepo) convert(info *codehost.RevInfo, statVers string) (*RevInfo, e // major version and +incompatible constraints. Use that version as the // pseudo-version base so that the pseudo-version sorts higher. Ignore // retracted versions. - allowedMajor := func(major string) func(v string) bool { - return func(v string) bool { - return ((major == "" && canUseIncompatible(v)) || semver.Major(v) == major) && !isRetracted(v) + tagAllowed := func(tag string) bool { + v, _ := tagToVersion(tag) + if v == "" { + return false } + if !module.MatchPathMajor(v, r.pathMajor) && !canUseIncompatible(v) { + return false + } + return !isRetracted(v) } if pseudoBase == "" { - var tag string - if r.pseudoMajor != "" || canUseIncompatible("") { - tag, _ = r.code.RecentTag(info.Name, tagPrefix, allowedMajor(r.pseudoMajor)) - } else { - // Allow either v1 or v0, but not incompatible higher versions. - tag, _ = r.code.RecentTag(info.Name, tagPrefix, allowedMajor("v1")) - if tag == "" { - tag, _ = r.code.RecentTag(info.Name, tagPrefix, allowedMajor("v0")) - } + tag, _ := r.code.RecentTag(info.Name, tagPrefix, tagAllowed) + if tag != "" { + pseudoBase, _ = tagToVersion(tag) } - pseudoBase, _ = tagToVersion(tag) } return checkCanonical(module.PseudoVersion(r.pseudoMajor, pseudoBase, info.Time, info.Short)) @@ -920,6 +929,10 @@ func (r *codeRepo) retractedVersions() (func(string) bool, error) { for i, v := range versions { if strings.HasSuffix(v, "+incompatible") { + // We're looking for the latest release tag that may list retractions in a + // go.mod file. +incompatible versions necessarily do not, and they start + // at major version 2 — which is higher than any version that could + // validly contain a go.mod file. versions = versions[:i] break } diff --git a/src/cmd/go/internal/modfetch/coderepo_test.go b/src/cmd/go/internal/modfetch/coderepo_test.go index bb9268adb87c4..8d0eb00544ad5 100644 --- a/src/cmd/go/internal/modfetch/coderepo_test.go +++ b/src/cmd/go/internal/modfetch/coderepo_test.go @@ -506,6 +506,69 @@ var codeRepoTests = []codeRepoTest{ short: "80beb17a1603", time: time.Date(2022, 2, 22, 20, 55, 7, 0, time.UTC), }, + + // A version tag with explicit build metadata is valid but not canonical. + // It should resolve to a pseudo-version based on the same tag. + { + vcs: "git", + path: "vcs-test.golang.org/git/odd-tags.git", + rev: "v0.1.0+build-metadata", + version: "v0.1.1-0.20220223184835-9d863d525bbf", + name: "9d863d525bbfcc8eda09364738c4032393711a56", + short: "9d863d525bbf", + time: time.Date(2022, 2, 23, 18, 48, 35, 0, time.UTC), + }, + { + vcs: "git", + path: "vcs-test.golang.org/git/odd-tags.git", + rev: "9d863d525bbf", + version: "v0.1.1-0.20220223184835-9d863d525bbf", + name: "9d863d525bbfcc8eda09364738c4032393711a56", + short: "9d863d525bbf", + time: time.Date(2022, 2, 23, 18, 48, 35, 0, time.UTC), + }, + { + vcs: "git", + path: "vcs-test.golang.org/git/odd-tags.git", + rev: "latest", + version: "v0.1.1-0.20220223184835-9d863d525bbf", + name: "9d863d525bbfcc8eda09364738c4032393711a56", + short: "9d863d525bbf", + time: time.Date(2022, 2, 23, 18, 48, 35, 0, time.UTC), + }, + + // A version tag with an erroneous "+incompatible" suffix should resolve using + // only the prefix before the "+incompatible" suffix, not the "+incompatible" + // tag itself. (Otherwise, we would potentially have two different commits + // both named "v2.0.0+incompatible".) However, the tag is still valid semver + // and can still be used as the base for an unambiguous pseudo-version. + { + vcs: "git", + path: "vcs-test.golang.org/git/odd-tags.git", + rev: "v2.0.0+incompatible", + err: `unknown revision v2.0.0`, + }, + { + vcs: "git", + path: "vcs-test.golang.org/git/odd-tags.git", + rev: "12d19af20458", + version: "v2.0.1-0.20220223184802-12d19af20458+incompatible", + name: "12d19af204585b0db3d2a876ceddf5b9323f5a4a", + short: "12d19af20458", + time: time.Date(2022, 2, 23, 18, 48, 2, 0, time.UTC), + }, + + // Similarly, a pseudo-version must resolve to the named commit, even if a tag + // matching that pseudo-version is present on a *different* commit. + { + vcs: "git", + path: "vcs-test.golang.org/git/odd-tags.git", + rev: "v3.0.0-20220223184802-12d19af20458", + version: "v3.0.0-20220223184802-12d19af20458+incompatible", + name: "12d19af204585b0db3d2a876ceddf5b9323f5a4a", + short: "12d19af20458", + time: time.Date(2022, 2, 23, 18, 48, 2, 0, time.UTC), + }, } func TestCodeRepo(t *testing.T) { @@ -730,6 +793,11 @@ var codeRepoVersionsTests = []struct { path: "gopkg.in/natefinch/lumberjack.v2", versions: []string{"v2.0.0"}, }, + { + vcs: "git", + path: "vcs-test.golang.org/git/odd-tags.git", + versions: nil, + }, } func TestCodeRepoVersions(t *testing.T) { diff --git a/src/cmd/go/testdata/script/mod_list_odd_tags.txt b/src/cmd/go/testdata/script/mod_list_odd_tags.txt new file mode 100644 index 0000000000000..c1f40cdf3a312 --- /dev/null +++ b/src/cmd/go/testdata/script/mod_list_odd_tags.txt @@ -0,0 +1,13 @@ +[short] skip +[!exec:git] skip +[!net] skip + +env GOPROXY=direct + +go list -m vcs-test.golang.org/git/odd-tags.git@latest +stdout -count=1 '^.' +stdout '^vcs-test.golang.org/git/odd-tags.git v0.1.1-0.20220223184835-9d863d525bbf$' + +go list -m -versions vcs-test.golang.org/git/odd-tags.git +stdout -count=1 '^.' +stdout '^vcs-test.golang.org/git/odd-tags.git$' # No versions listed — the odd tags are filtered out.