cmd/go/internal/modfetch/codehost: replace a dubious call to semver.Max

Bryan C. Mills · Bryan C. Mills · commit 3f51350c706c · 2019-12-20T15:34:39.000Z
The documentation for RecentTag indicates that it returns an actual tag, not a canonicalized prefix+version blob equivalent to a tag, so the canonicalization due to semver.Max seems like a bug here. Fortunately, RecentTag is not currently ever actually used as a tag, so the removal of metadata does not result in a user-facing bug. Nonetheless, it may be a subtle source of confusion for maintainers in the future. Updates golang#32700 Change-Id: I525423c1c0c7ec7c36c09e53b180034474f74e5a Reviewed-on: https://go-review.googlesource.com/c/go/+/212202 Run-TryBot: Bryan C. Mills <bcmills@google.com> Reviewed-by: Jay Conrod <jayconrod@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>
diff --git a/src/cmd/go/internal/modfetch/codehost/git.go b/src/cmd/go/internal/modfetch/codehost/git.go
@@ -682,8 +682,11 @@ func (r *gitRepo) RecentTag(rev, prefix, major string) (tag string, err error) {
 
 			semtag := line[len(prefix):]
 			// Consider only tags that are valid and complete (not just major.minor prefixes).
-			if c := semver.Canonical(semtag); c != "" && strings.HasPrefix(semtag, c) && (major == "" || semver.Major(c) == major) {
-				highest = semver.Max(highest, semtag)
+			// 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) && (major == "" || semver.Major(c) == major) && semver.Compare(semtag, highest) > 0 {
+				highest = semtag
 			}
 		}
 
diff --git a/src/cmd/go/internal/modload/query_test.go b/src/cmd/go/internal/modload/query_test.go
@@ -64,7 +64,7 @@ var queryTests = []struct {
 		git add go.mod
 		git commit -m v1 go.mod
 		git tag start
-		for i in v0.0.0-pre1 v0.0.0 v0.0.1 v0.0.2 v0.0.3 v0.1.0 v0.1.1 v0.1.2 v0.3.0 v1.0.0 v1.1.0 v1.9.0 v1.9.9 v1.9.10-pre1 v1.9.10-pre2+metadata; do
+		for i in v0.0.0-pre1 v0.0.0 v0.0.1 v0.0.2 v0.0.3 v0.1.0 v0.1.1 v0.1.2 v0.3.0 v1.0.0 v1.1.0 v1.9.0 v1.9.9 v1.9.10-pre1 v1.9.10-pre2+metadata unversioned; do
 			echo before $i >status
 			git add status
 			git commit -m "before $i" status
@@ -107,6 +107,7 @@ var queryTests = []struct {
 	{path: queryRepo, query: "v0.2", err: `no matching versions for query "v0.2"`},
 	{path: queryRepo, query: "v0.0", vers: "v0.0.3"},
 	{path: queryRepo, query: "v1.9.10-pre2+metadata", vers: "v1.9.10-pre2.0.20190513201126-42abcb6df8ee"},
+	{path: queryRepo, query: "ed5ffdaa", vers: "v1.9.10-pre2.0.20191220134614-ed5ffdaa1f5e"},
 
 	// golang.org/issue/29262: The major version for for a module without a suffix
 	// should be based on the most recent tag (v1 as appropriate, not v0

Original file line number	Diff line number	Diff line change
`@@ -682,8 +682,11 @@ func (r *gitRepo) RecentTag(rev, prefix, major string) (tag string, err error) {`
`682`	`682`
`683`	`683`	`semtag := line[len(prefix):]`
`684`	`684`	`// Consider only tags that are valid and complete (not just major.minor prefixes).`
`685`		`- if c := semver.Canonical(semtag); c != "" && strings.HasPrefix(semtag, c) && (major == "" \|\| semver.Major(c) == major) {`
`686`		`- highest = semver.Max(highest, semtag)`
	`685`	`+ // NOTE: Do not replace the call to semver.Compare with semver.Max.`
	`686`	`+ // We want to return the actual tag, not a canonicalized version of it,`
	`687`	`+ // and semver.Max currently canonicalizes (see golang.org/issue/32700).`
	`688`	`+ if c := semver.Canonical(semtag); c != "" && strings.HasPrefix(semtag, c) && (major == "" \|\| semver.Major(c) == major) && semver.Compare(semtag, highest) > 0 {`
	`689`	`+ highest = semtag`
`687`	`690`	`}`
`688`	`691`	`}`
`689`	`692`