maintner/maintnerd/maintapi: factor out version parsing code

Create a new ./maintner/maintnerd/maintapi/version package and move
the version parsing code there. Change the implementation to use
strings.Split on "." rather than a regexp. Make it more precise at
rejecting invalid names and increase test coverage of edge cases.

Having this code be in a separate package makes it easier to use it
elsewhere when needed.

Change commit hash in TestSupportedGoReleases for -security branches,
to make the test failure more prominent.

Remove an unneeded blank line at the end of a function body.

Change-Id: I3168b0b25a6b6433d17f83b01580f08208532daf
Reviewed-on: https://go-review.googlesource.com/c/157559
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

maintner/maintnerd/maintapi/api.go

@@ -8,12 +8,8 @@ package maintapi
 import (
 	"context"
 	"errors"
-	"fmt"
-	"io"
 	"log"
-	"regexp"
 	"sort"
-	"strconv"
 	"strings"
 	"sync"
 	"time"
@@ -21,6 +17,7 @@ import (
 	"golang.org/x/build/gerrit"
 	"golang.org/x/build/maintner"
 	"golang.org/x/build/maintner/maintnerd/apipb"
+	"golang.org/x/build/maintner/maintnerd/maintapi/version"
 )
 
 // NewAPIService creates a gRPC Server that serves the Maintner API for the given corpus.
@@ -221,6 +218,31 @@ func (s apiService) GoFindTryWork(ctx context.Context, req *apipb.GoFindTryWorkR
 	return res, nil
 }
 
+// parseTagVersion parses the major-minor-patch version triplet
+// from goX, goX.Y, or goX.Y.Z tag names,
+// and reports whether the tag name is valid.
+//
+// Tags with suffixes like "go1.2beta3" or "go1.2rc1" are rejected.
+//
+// For example, "go1" is parsed as version 1.0.0,
+// "go1.2" is parsed as version 1.2.0,
+// and "go1.2.3" is parsed as version 1.2.3.
+func parseTagVersion(tagName string) (major, minor, patch int32, ok bool) {
+	maj, min, pat, ok := version.ParseTag(tagName)
+	return int32(maj), int32(min), int32(pat), ok
+}
+
+// parseReleaseBranchVersion parses the major-minor version pair
+// from release-branch.goX or release-branch.goX.Y release branch names,
+// and reports whether the release branch name is valid.
+//
+// For example, "release-branch.go1" is parsed as version 1.0,
+// and "release-branch.go1.2" is parsed as version 1.2.
+func parseReleaseBranchVersion(branchName string) (major, minor int32, ok bool) {
+	maj, min, ok := version.ParseReleaseBranch(branchName)
+	return int32(maj), int32(min), ok
+}
+
 // ListGoReleases lists Go releases. A release is considered to exist
 // if a tag for it exists.
 func (s apiService) ListGoReleases(ctx context.Context, req *apipb.ListGoReleasesRequest) (*apipb.ListGoReleasesResponse, error) {
@@ -246,10 +268,6 @@ type nonChangeRefLister interface {
 	ForeachNonChangeRef(fn func(ref string, hash maintner.GitHash) error) error
 }
 
-// releaseBranchRx matches things of the form "release-branch.go1.5" or "release-branch.go2",
-// but not things like "release-branch.go1.10-security".
-var releaseBranchRx = regexp.MustCompile(`^release-branch\.go(\d{1,2})(?:\.(\d{1,3}))?$`)
-
 // supportedGoReleases returns the latest patches of releases
 // that are considered supported per policy.
 func supportedGoReleases(goProj nonChangeRefLister) ([]*apipb.GoRelease, error) {
@@ -275,11 +293,8 @@ func supportedGoReleases(goProj nonChangeRefLister) ([]*apipb.GoRelease, error)
 		case strings.HasPrefix(ref, "refs/tags/go"):
 			// Tag.
 			tagName := ref[len("refs/tags/"):]
-			var major, minor, patch int32
-			_, err := fmt.Sscanf(tagName, "go%d.%d.%d", &major, &minor, &patch)
-			if err == io.ErrUnexpectedEOF {
-				// Do nothing.
-			} else if err != nil {
+			major, minor, patch, ok := parseTagVersion(tagName)
+			if !ok {
 				return nil
 			}
 			if t, ok := tags[majorMinor{major, minor}]; ok && patch <= t.Patch {
@@ -295,16 +310,11 @@ func supportedGoReleases(goProj nonChangeRefLister) ([]*apipb.GoRelease, error)
 		case strings.HasPrefix(ref, "refs/heads/release-branch.go"):
 			// Release branch.
 			branchName := ref[len("refs/heads/"):]
-			m := releaseBranchRx.FindStringSubmatch(branchName)
-			if m == nil {
+			major, minor, ok := parseReleaseBranchVersion(branchName)
+			if !ok {
 				return nil
 			}
-			var major, minor int
-			major, _ = strconv.Atoi(m[1])
-			if len(m) > 2 {
-				minor, _ = strconv.Atoi(m[2])
-			}
-			branches[majorMinor{int32(major), int32(minor)}] = branch{
+			branches[majorMinor{major, minor}] = branch{
 				Name:   branchName,
 				Commit: hash,
 			}

maintner/maintnerd/maintapi/api_test.go

@@ -115,7 +115,6 @@ func TestTryBotStatus(t *testing.T) {
 		}
 		*cl = old // restore
 	}
-
 }
 
 func TestTryWorkItem(t *testing.T) {
@@ -185,8 +184,8 @@ func TestSupportedGoReleases(t *testing.T) {
 					{"refs/heads/release-branch.go1.1", gitHash("1d6d8fca241bb611af51e265c1b5a2e9ae904702")},
 					{"refs/heads/release-branch.go1.10", gitHash("e97b7d68f107ff60152f5bd5701e0286f221ee93")},
 					{"refs/heads/release-branch.go1.11", gitHash("97781d2ed116d2cd9cb870d0b84fc0ec598c9abc")},
-					{"refs/heads/release-branch.go1.10-security", gitHash("e97b7d68f107ff60152f5bd5701e0286f221ee93")},
-					{"refs/heads/release-branch.go1.11-security", gitHash("97781d2ed116d2cd9cb870d0b84fc0ec598c9abc")},
+					{"refs/heads/release-branch.go1.10-security", gitHash("25ca8f49c3fc4a68daff7a23ab613e3453be5cda")},
+					{"refs/heads/release-branch.go1.11-security", gitHash("90c896448691b5edb0ab11110f37234f63cd28ed")},
 					{"refs/heads/release-branch.go1.2", gitHash("43d00b0942c1c6f43993ac71e1eea48e62e22b8d")},
 					{"refs/heads/release-branch.r59", gitHash("5d9765785dff74784bbdad43f7847b6825509032")},
 					{"refs/heads/release-branch.r60", gitHash("394b383a1ee0ac3fec5e453a7dbe590d3ce6d6b0")},

maintner/maintnerd/maintapi/version/version.go

@@ -0,0 +1,112 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package version implements logic to parse version
+// of Go tags and release branches.
+package version
+
+import (
+	"strings"
+)
+
+// ParseTag parses the major-minor-patch version triplet
+// from goX, goX.Y, or goX.Y.Z tag names,
+// and reports whether the tag name is valid.
+//
+// Tags with suffixes like "go1.2beta3" or "go1.2rc1"
+// are currently not supported, and get rejected.
+//
+// For example, "go1" is parsed as version 1.0.0,
+// "go1.2" is parsed as version 1.2.0,
+// and "go1.2.3" is parsed as version 1.2.3.
+func ParseTag(tagName string) (major, minor, patch int, ok bool) {
+	const prefix = "go"
+	if !strings.HasPrefix(tagName, prefix) {
+		return 0, 0, 0, false
+	}
+	if strings.HasSuffix(tagName, ".0") {
+		// Trailing zero version components must be omitted in Go tags,
+		// so reject if we see one.
+		return 0, 0, 0, false
+	}
+	v := strings.SplitN(tagName[len(prefix):], ".", 4)
+	if len(v) > 3 {
+		return 0, 0, 0, false
+	}
+	major, ok = parse0To999(v[0])
+	if !ok || major == 0 {
+		return 0, 0, 0, false
+	}
+	if len(v) == 2 || len(v) == 3 {
+		minor, ok = parse0To999(v[1])
+		if !ok {
+			return 0, 0, 0, false
+		}
+	}
+	if len(v) == 3 {
+		patch, ok = parse0To999(v[2])
+		if !ok {
+			return 0, 0, 0, false
+		}
+	}
+	return major, minor, patch, true
+}
+
+// ParseReleaseBranch parses the major-minor version pair
+// from release-branch.goX or release-branch.goX.Y release branch names,
+// and reports whether the release branch name is valid.
+//
+// For example, "release-branch.go1" is parsed as version 1.0,
+// and "release-branch.go1.2" is parsed as version 1.2.
+func ParseReleaseBranch(branchName string) (major, minor int, ok bool) {
+	const prefix = "release-branch.go"
+	if !strings.HasPrefix(branchName, prefix) {
+		return 0, 0, false
+	}
+	if strings.HasSuffix(branchName, ".0") {
+		// Trailing zero version components must be omitted in Go release branches,
+		// so reject if we see one.
+		return 0, 0, false
+	}
+	v := strings.SplitN(branchName[len(prefix):], ".", 3)
+	if len(v) > 2 {
+		return 0, 0, false
+	}
+	major, ok = parse0To999(v[0])
+	if !ok || major == 0 {
+		return 0, 0, false
+	}
+	if len(v) == 2 {
+		minor, ok = parse0To999(v[1])
+		if !ok {
+			return 0, 0, false
+		}
+	}
+	return major, minor, true
+}
+
+// parse0To999 converts the canonical ASCII string representation
+// of a number in the range [0, 999] to its integer form.
+// strconv.Itoa(n) will equal to s if and only if ok is true.
+//
+// It's similar to strconv.Atoi, except it doesn't permit
+// negative numbers, leading '+'/'-' signs, leading zeros,
+// or other potential valid but non-canonical string
+// representations of numbers.
+func parse0To999(s string) (n int, ok bool) {
+	if len(s) < 1 || 3 < len(s) {
+		return 0, false
+	}
+	if len(s) > 1 && s[0] == '0' {
+		// Leading zeros are rejected.
+		return 0, false
+	}
+	for _, c := range []byte(s) {
+		if c < '0' || '9' < c {
+			return 0, false
+		}
+		n = n*10 + int(c-'0')
+	}
+	return n, true
+}

maintner/maintnerd/maintapi/version/version_test.go

@@ -0,0 +1,160 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package version
+
+import (
+	"strconv"
+	"testing"
+	"testing/quick"
+)
+
+func TestParseTag(t *testing.T) {
+	tests := []struct {
+		tagName                         string
+		wantMajor, wantMinor, wantPatch int
+		wantOK                          bool
+	}{
+		{"go1", 1, 0, 0, true},
+		{"go1.2", 1, 2, 0, true},
+		{"go1.2.3", 1, 2, 3, true},
+		{"go23.45.67", 23, 45, 67, true},
+		{"not-go", 0, 0, 0, false},
+		{"go", 0, 0, 0, false},
+		{"go.", 0, 0, 0, false},
+		{"go1.", 0, 0, 0, false},
+		{"go1-bad", 0, 0, 0, false},
+		{"go1.2.", 0, 0, 0, false},
+		{"go1.2-bad", 0, 0, 0, false},
+		{"go1.2.3.", 0, 0, 0, false},
+		{"go1.2.3-bad", 0, 0, 0, false},
+		{"go1.2.3.4", 0, 0, 0, false},
+		{"go0.0.0", 0, 0, 0, false},
+		{"go-1", 0, 0, 0, false},
+		{"go1.-2", 0, 0, 0, false},
+		{"go1.2.-3", 0, 0, 0, false},
+		{"go+1", 0, 0, 0, false},
+		{"go01", 0, 0, 0, false},
+		{"go001", 0, 0, 0, false},
+		{"go1000", 0, 0, 0, false},
+		{"go1.0", 0, 0, 0, false},
+		{"go1.0.0", 0, 0, 0, false},
+		{"go1.2.0", 0, 0, 0, false},
+		{"go00", 0, 0, 0, false},
+		{"go00.2", 0, 0, 0, false},
+		{"go00.2.3", 0, 0, 0, false},
+		{"go1.00", 0, 0, 0, false},
+		{"go1.00.0", 0, 0, 0, false},
+		{"go1.00.3", 0, 0, 0, false},
+		{"go1.2.00", 0, 0, 0, false},
+	}
+	for i, tt := range tests {
+		major, minor, patch, ok := ParseTag(tt.tagName)
+		if got, want := ok, tt.wantOK; got != want {
+			t.Errorf("#%d %q: got ok = %v; want %v", i, tt.tagName, got, want)
+			continue
+		}
+		if !tt.wantOK {
+			continue
+		}
+		if got, want := major, tt.wantMajor; got != want {
+			t.Errorf("#%d %q: got major = %d; want %d", i, tt.tagName, got, want)
+		}
+		if got, want := minor, tt.wantMinor; got != want {
+			t.Errorf("#%d %q: got minor = %d; want %d", i, tt.tagName, got, want)
+		}
+		if got, want := patch, tt.wantPatch; got != want {
+			t.Errorf("#%d %q: got patch = %d; want %d", i, tt.tagName, got, want)
+		}
+	}
+}
+
+func TestParseReleaseBranch(t *testing.T) {
+	tests := []struct {
+		branchName           string
+		wantMajor, wantMinor int
+		wantOK               bool
+	}{
+		{"release-branch.go1", 1, 0, true},
+		{"release-branch.go1.2", 1, 2, true},
+		{"release-branch.go23.45", 23, 45, true},
+		{"not-release-branch", 0, 0, false},
+		{"release-branch.go", 0, 0, false},
+		{"release-branch.go.", 0, 0, false},
+		{"release-branch.go1.", 0, 0, false},
+		{"release-branch.go1-bad", 0, 0, false},
+		{"release-branch.go1.2.", 0, 0, false},
+		{"release-branch.go1.2-bad", 0, 0, false},
+		{"release-branch.go1.2.3", 0, 0, false},
+		{"release-branch.go0.0", 0, 0, false},
+		{"release-branch.go-1", 0, 0, false},
+		{"release-branch.go1.-2", 0, 0, false},
+		{"release-branch.go+1", 0, 0, false},
+		{"release-branch.go01", 0, 0, false},
+		{"release-branch.go001", 0, 0, false},
+		{"release-branch.go1000", 0, 0, false},
+		{"release-branch.go1.0", 0, 0, false},
+		{"release-branch.go00", 0, 0, false},
+		{"release-branch.go00.2", 0, 0, false},
+		{"release-branch.go1.00", 0, 0, false},
+	}
+	for i, tt := range tests {
+		major, minor, ok := ParseReleaseBranch(tt.branchName)
+		if got, want := ok, tt.wantOK; got != want {
+			t.Errorf("#%d %q: got ok = %v; want %v", i, tt.branchName, got, want)
+			continue
+		}
+		if !tt.wantOK {
+			continue
+		}
+		if got, want := major, tt.wantMajor; got != want {
+			t.Errorf("#%d %q: got major = %d; want %d", i, tt.branchName, got, want)
+		}
+		if got, want := minor, tt.wantMinor; got != want {
+			t.Errorf("#%d %q: got minor = %d; want %d", i, tt.branchName, got, want)
+		}
+	}
+}
+
+func TestParse0To999(t *testing.T) {
+	// The only accepted inputs are numbers in [0, 999] range
+	// in canonical string form. All other input should be rejected.
+	// Build a complete map of inputs to answers.
+	var golden = make(map[string]int) // input -> output
+	for n := 0; n <= 999; n++ {
+		golden[strconv.Itoa(n)] = n
+	}
+
+	// Numbers in [0, 999] range should be accepted.
+	for in, want := range golden {
+		got, ok := parse0To999(in)
+		if !ok {
+			t.Errorf("parse0To999(%q): got ok = false; want true", in)
+			continue
+		}
+		if got != want {
+			t.Errorf("parse0To999(%q): got n = %d; want %d", in, got, want)
+		}
+	}
+
+	// All other numbers should be rejected.
+	ints := func(x int) bool {
+		gotN, gotOK := parse0To999(strconv.Itoa(x))
+		wantN, wantOK := golden[strconv.Itoa(x)]
+		return gotOK == wantOK && gotN == wantN
+	}
+	if err := quick.Check(ints, nil); err != nil {
+		t.Error(err)
+	}
+
+	// All other strings should be rejected.
+	strings := func(x string) bool {
+		gotN, gotOK := parse0To999(x)
+		wantN, wantOK := golden[x]
+		return gotOK == wantOK && gotN == wantN
+	}
+	if err := quick.Check(strings, nil); err != nil {
+		t.Error(err)
+	}
+}