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>
diff --git a/maintner/maintnerd/maintapi/api.go b/maintner/maintnerd/maintapi/api.go
index f8ccd4e..62b52ee 100644
--- a/maintner/maintnerd/maintapi/api.go
+++ b/maintner/maintnerd/maintapi/api.go
@@ -8,12 +8,8 @@
import (
"context"
"errors"
- "fmt"
- "io"
"log"
- "regexp"
"sort"
- "strconv"
"strings"
"sync"
"time"
@@ -21,6 +17,7 @@
"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 @@
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 @@
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 @@
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 @@
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,
}
diff --git a/maintner/maintnerd/maintapi/api_test.go b/maintner/maintnerd/maintapi/api_test.go
index 435700a..53c9335 100644
--- a/maintner/maintnerd/maintapi/api_test.go
+++ b/maintner/maintnerd/maintapi/api_test.go
@@ -115,7 +115,6 @@
}
*cl = old // restore
}
-
}
func TestTryWorkItem(t *testing.T) {
@@ -185,8 +184,8 @@
{"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")},
diff --git a/maintner/maintnerd/maintapi/version/version.go b/maintner/maintnerd/maintapi/version/version.go
new file mode 100644
index 0000000..f71f02e
--- /dev/null
+++ b/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
+}
diff --git a/maintner/maintnerd/maintapi/version/version_test.go b/maintner/maintnerd/maintapi/version/version_test.go
new file mode 100644
index 0000000..b4d0357
--- /dev/null
+++ b/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)
+ }
+}
