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 <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>

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.
 	//

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
 			}

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
 		}

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) {

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.