From 228e5d2805f40118cda3326147d5610cef56d67a Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Thu, 6 Dec 2018 00:16:32 +0000 Subject: [PATCH] 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 --- maintner/gerrit.go | 68 +++++++++++++++++++-------- maintner/git.go | 2 +- maintner/godata/godata_test.go | 85 ++++++++++++++++++++++++++++++++++ 3 files changed, 135 insertions(+), 20 deletions(-) diff --git a/maintner/gerrit.go b/maintner/gerrit.go index 88d03f0c..ffd03ca0 100644 --- 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 != "" { diff --git a/maintner/git.go b/maintner/git.go index 1e652490..3d670104 100644 --- 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} } diff --git a/maintner/godata/godata_test.go b/maintner/godata/godata_test.go index 2152338e..edd18b02 100644 --- 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 :", 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, ", ") +}