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>
2018-12-06 00:16:32 +00:00 · 2018-12-06 00:16:32 +00:00 · 228e5d2805
--- a/maintner/gerrit.go
+++ b/maintner/gerrit.go
@ -364,7 +364,7 @@ func (cl *GerritCL) Branch() string { return cl.branch }
 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 @@ func (cl *GerritCL) updateBranch() {
 	}
 }

-// 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 @@ func lineValue(all, prefix string) (value, rest string) {
 		} 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 @@ func (cl *GerritCL) Footer(key string) string {
 		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 @@ func (gp *GerritProject) check() error {
 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 @@ func (m *GerritMeta) Footer() string {
 	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 @@ func (m *GerritMeta) HashtagEdits() (added, removed GerritHashtags, ok bool) {
 	// 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 @@ func (m *GerritMeta) LabelVotes() map[string]map[string]int8 {
 		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 @@ func (m *GerritMeta) LabelVotes() map[string]map[string]int8 {
 		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 != "" {
--- a/maintner/git.go
+++ b/maintner/git.go
@ -364,7 +364,7 @@ func (c *Corpus) processGitCommit(commit *maintpb.GitCommit) (*GitCommit, error)
 	// 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}
 	}

--- a/maintner/godata/godata_test.go
+++ b/maintner/godata/godata_test.go
@ -8,10 +8,14 @@ import (
 	"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 @@ removed "bar" = "quux,blarf"
 		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, ", ")
+}