maintner: fix GerritMeta.Hashtags to look at earlier meta parents for answer
The Gerrit meta commit graph is a linear history. The most recent meta
with a "Hashtags: " footer line has the complete set. We just have to
go back and look for it.
Fixes golang/go#28318
Updates golang/go#28510 (fixes after gopherbot re-deployed)
Updates golang/go#28320 (fixes after gopherbot re-deployed)
Change-Id: I43705075800ae3d353c1c8f60ab7685883ea5602
Reviewed-on: https://go-review.googlesource.com/c/152779
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
diff --git a/maintner/gerrit.go b/maintner/gerrit.go
index 88d03f0..ffd03ca 100644
--- a/maintner/gerrit.go
+++ b/maintner/gerrit.go
@@ -364,7 +364,7 @@
func (cl *GerritCL) updateBranch() {
for i := len(cl.Metas) - 1; i >= 0; i-- {
mc := cl.Metas[i]
- branch, _ := lineValue(mc.Commit.Msg, "Branch:")
+ branch := lineValue(mc.Commit.Msg, "Branch:")
if branch != "" {
cl.branch = strings.TrimPrefix(branch, "refs/heads/")
return
@@ -372,19 +372,21 @@
}
}
-// lineValue extracts a value from an RFC 822-style "key: value" series of lines.
+// lineValueOK extracts a value from an RFC 822-style "key: value" series of lines.
// If all is,
// foo: bar
// bar: baz
// lineValue(all, "foo:") returns "bar". It trims any whitespace.
// The prefix is case sensitive and must include the colon.
-func lineValue(all, prefix string) (value, rest string) {
+// The ok value reports whether a line with such a prefix is found, even if its
+// value is empty. If ok is true, the rest value contains the subsequent lines.
+func lineValueOK(all, prefix string) (value, rest string, ok bool) {
orig := all
consumed := 0
for {
i := strings.Index(all, prefix)
if i == -1 {
- return "", ""
+ return "", "", false
}
if i > 0 && all[i-1] != '\n' && all[i-1] != '\r' {
all = all[i+len(prefix):]
@@ -399,17 +401,26 @@
} else {
consumed = len(orig)
}
- return strings.TrimSpace(val), orig[consumed:]
+ return strings.TrimSpace(val), orig[consumed:], true
}
}
+func lineValue(all, prefix string) string {
+ value, _, _ := lineValueOK(all, prefix)
+ return value
+}
+
+func lineValueRest(all, prefix string) (value, rest string) {
+ value, rest, _ = lineValueOK(all, prefix)
+ return
+}
+
// WorkInProgress reports whether the CL has its Work-in-progress bit set, per
// https://gerrit-review.googlesource.com/Documentation/intro-user.html#wip
func (cl *GerritCL) WorkInProgress() bool {
var wip bool
for _, m := range cl.Metas {
- v, _ := lineValue(m.Commit.Msg, "Work-in-progress:")
- switch v {
+ switch lineValue(m.Commit.Msg, "Work-in-progress:") {
case "true":
wip = true
case "false":
@@ -438,8 +449,7 @@
panic("Footer key does not end in colon")
}
// TODO: git footers are treated as multimaps. Account for this.
- v, _ := lineValue(cl.Commit.Msg, key)
- return v
+ return lineValue(cl.Commit.Msg, key)
}
// OwnerID returns the ID of the CL’s owner. It will return -1 on error.
@@ -1292,7 +1302,6 @@
type GerritMeta struct {
// Commit points up to the git commit for this Gerrit NoteDB meta commit.
Commit *GitCommit
-
// CL is the Gerrit CL this metadata is for.
CL *GerritCL
@@ -1324,17 +1333,39 @@
return m.Commit.Msg[i+2:]
}
-// Hashtags returns the current set of hashtags.
+// Hashtags returns the set of hashtags on m's CL as of the time of m.
func (m *GerritMeta) Hashtags() GerritHashtags {
- tags, _ := lineValue(m.Footer(), "Hashtags: ")
- return GerritHashtags(tags)
+ // If this GerritMeta set hashtags, use it.
+ tags, _, ok := lineValueOK(m.Footer(), "Hashtags: ")
+ if ok {
+ return GerritHashtags(tags)
+ }
+
+ // Otherwise, look at older metas (from most recent to oldest)
+ // to find most recent value. Ignore anything that's newer
+ // than m.
+ sawThisMeta := false // whether we've seen 'm'
+ metas := m.CL.Metas
+ for i := len(metas) - 1; i >= 0; i-- {
+ mp := metas[i]
+ if mp.Commit.Hash == m.Commit.Hash {
+ sawThisMeta = true
+ continue
+ }
+ if !sawThisMeta {
+ continue
+ }
+ if tags, _, ok := lineValueOK(mp.Footer(), "Hashtags: "); ok {
+ return GerritHashtags(tags)
+ }
+ }
+ return ""
}
// ActionTag returns the Gerrit "Tag" value from the meta commit.
// These are of the form "autogenerated:gerrit:setHashtag".
func (m *GerritMeta) ActionTag() string {
- v, _ := lineValue(m.Footer(), "Tag: ")
- return v
+ return lineValue(m.Footer(), "Tag: ")
}
// HashtagEdits returns the hashtags added and removed by this meta commit,
@@ -1354,7 +1385,7 @@
// Hashtag added: bar
// Hashtags added: foo, bar
for len(msg) > 0 {
- value, rest := lineValue(msg, "Hash")
+ value, rest := lineValueRest(msg, "Hash")
msg = rest
colon := strings.IndexByte(value, ':')
if colon != -1 {
@@ -1419,8 +1450,7 @@
isNew := strings.Contains(footer, "\nTag: autogenerated:gerrit:newPatchSet\n")
email := mc.Commit.Author.Email()
if isNew {
- commit, _ := lineValue(footer, "Commit: ")
- if commit != "" {
+ if commit := lineValue(footer, "Commit: "); commit != "" {
// TODO: implement Gerrit's vote copying. For example,
// label.Label-Name.copyAllScoresIfNoChange defaults to true (as it is with Go's server)
// https://gerrit-review.googlesource.com/Documentation/config-labels.html#label_copyAllScoresIfNoChange
@@ -1447,7 +1477,7 @@
remain := footer
for len(remain) > 0 {
var labelEqVal string
- labelEqVal, remain = lineValue(remain, "Label: ")
+ labelEqVal, remain = lineValueRest(remain, "Label: ")
if labelEqVal != "" {
label, value, whose := parseGerritLabelValue(labelEqVal)
if label != "" {
diff --git a/maintner/git.go b/maintner/git.go
index 1e65249..3d67010 100644
--- a/maintner/git.go
+++ b/maintner/git.go
@@ -364,7 +364,7 @@
// Patch-set: 1
// Reviewer: Ian Lance Taylor <5206@62eb7196-b449-3ce5-99f1-c037f21e1705>
// Label: Code-Review=+2
- if reviewer, _ := lineValue(c.strb(msg), "Reviewer: "); reviewer != "" {
+ if reviewer := lineValue(c.strb(msg), "Reviewer: "); reviewer != "" {
gc.Reviewer = &GitPerson{Str: reviewer}
}
diff --git a/maintner/godata/godata_test.go b/maintner/godata/godata_test.go
index 2152338..edd18b0 100644
--- a/maintner/godata/godata_test.go
+++ b/maintner/godata/godata_test.go
@@ -8,10 +8,14 @@
"bytes"
"context"
"fmt"
+ "os"
+ "sort"
"strings"
"sync"
"testing"
+ "cloud.google.com/go/compute/metadata"
+ "golang.org/x/build/gerrit"
"golang.org/x/build/maintner"
)
@@ -272,3 +276,84 @@
t.Errorf("got:\n%s\n\nwant prefix:\n%s", got, want)
}
}
+
+func getGerritAuth() (username string, password string, err error) {
+ var slurp string
+ if metadata.OnGCE() {
+ for _, key := range []string{"gopherbot-gerrit-token", "maintner-gerrit-token", "gobot-password"} {
+ slurp, err = metadata.ProjectAttributeValue(key)
+ if err != nil || slurp == "" {
+ continue
+ }
+ break
+ }
+ }
+ if slurp == "" {
+ slurp = os.Getenv("TEST_GERRIT_AUTH")
+ }
+ f := strings.SplitN(strings.TrimSpace(slurp), ":", 2)
+ if len(f) == 1 {
+ // assume the whole thing is the token
+ return "git-gobot.golang.org", f[0], nil
+ }
+ if len(f) != 2 || f[0] == "" || f[1] == "" {
+ return "", "", fmt.Errorf("Expected Gerrit token %q to be of form <git-email>:<token>", slurp)
+ }
+ return f[0], f[1], nil
+}
+
+// Hit the Gerrit API and compare its computation of CLs' hashtags against what maintner thinks.
+// Off by default unless $TEST_GERRIT_AUTH is defined with "user:token", or we're running in the
+// prod project.
+func TestGerritHashtags(t *testing.T) {
+ if testing.Short() {
+ t.Skip("skipping in short mode")
+ }
+ c := getGoData(t)
+ user, pass, err := getGerritAuth()
+ if err != nil {
+ t.Skipf("no Gerrit auth defined, skipping: %v", err)
+ }
+ gc := gerrit.NewClient("https://go-review.googlesource.com", gerrit.BasicAuth(user, pass))
+ ctx := context.Background()
+ more := true
+ n := 0
+ for more {
+ // We search Gerrit for "hashtag", which seems to also
+ // search auto-generated gerrit meta (notedb) texts,
+ // so this has the effect of searching for all Gerrit
+ // changes that have ever had hashtags added or
+ // removed:
+ cis, err := gc.QueryChanges(ctx, "hashtag", gerrit.QueryChangesOpt{
+ Start: n,
+ })
+ if err != nil {
+ t.Fatal(err)
+ }
+ for _, ci := range cis {
+ n++
+ cl := c.Gerrit().Project("go.googlesource.com", ci.Project).CL(int32(ci.ChangeNumber))
+ if cl == nil {
+ t.Logf("Ignoring not-in-maintner %s/%v", ci.Project, ci.ChangeNumber)
+ continue
+ }
+ sort.Strings(ci.Hashtags)
+ want := strings.Join(ci.Hashtags, ", ")
+ got := canonicalTagList(string(cl.Meta.Hashtags()))
+ if got != want {
+ t.Errorf("ci: https://golang.org/cl/%d (%s) -- maintner = %q; want gerrit value %q", ci.ChangeNumber, ci.Project, got, want)
+ }
+ more = ci.MoreChanges
+ }
+ }
+ t.Logf("N = %v", n)
+}
+
+func canonicalTagList(s string) string {
+ var sl []string
+ for _, v := range strings.Split(s, ",") {
+ sl = append(sl, strings.TrimSpace(v))
+ }
+ sort.Strings(sl)
+ return strings.Join(sl, ", ")
+}
