cmd/gopherbot: don't set reviewers if they are the same as current
This change avoids calling SetReview with reviews that would have no
effect on the change. This is particularly relevant for changes imported
from PRs, as they require 2 reviewers. If there is only one owner for
the package, we should avoid continually setting the same reviewer.
This change is implemented by keeping track of all of the human
reviewers on a given change and comparing them to the proposed
reviewers. This is a little complicated, given that the current
reviewers can only be kept track of by their Gerrit IDs, whereas owners
are added by the emails associated with their Gerrit accounts.
Change-Id: I684c8c86fd5c0c2c411450ae48a332ba796f5d60
Reviewed-on: https://go-review.googlesource.com/c/build/+/236438
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
diff --git a/cmd/gopherbot/gopherbot.go b/cmd/gopherbot/gopherbot.go
index 2f7b264..023258a 100644
--- a/cmd/gopherbot/gopherbot.go
+++ b/cmd/gopherbot/gopherbot.go
@@ -19,6 +19,7 @@
"net/http"
"os"
"path/filepath"
+ "regexp"
"sort"
"strconv"
"strings"
@@ -1840,7 +1841,8 @@
return nil
}
- if b.humanReviewersOnChange(ctx, gc, cl) {
+ currentReviewers, ok := b.humanReviewersOnChange(ctx, gc, cl)
+ if ok {
return nil
}
@@ -1892,6 +1894,13 @@
for _, owner := range merged.Secondary {
review.Reviewers = append(review.Reviewers, gerrit.ReviewerInput{Reviewer: owner.GerritEmail, State: "CC"})
}
+
+ // If the reviewers that would be set are the same as the existing
+ // reviewers (minus the bots), there is no work to be done.
+ if sameReviewers(currentReviewers, review) {
+ log.Printf("Setting review %+v on %s would have no effect, continuing", review, changeURL)
+ return nil
+ }
if *dryRun {
log.Printf("[dry run] Would set review on %s: %+v", changeURL, review)
return nil
@@ -1908,6 +1917,38 @@
return nil
}
+func sameReviewers(reviewers []string, review gerrit.ReviewInput) bool {
+ if len(reviewers) != len(review.Reviewers) {
+ return false
+ }
+ sort.Strings(reviewers)
+ var people []*gophers.Person
+ for _, id := range reviewers {
+ p := gophers.GetPerson(fmt.Sprintf("%s@%s", id, gerritInstanceID))
+ // If an existing reviewer is not known to us, we have no way of
+ // checking if these reviewer lists are identical.
+ if p == nil {
+ return false
+ }
+ people = append(people, p)
+ }
+ sort.Slice(review.Reviewers, func(i, j int) bool {
+ return review.Reviewers[i].Reviewer < review.Reviewers[j].Reviewer
+ })
+ // Check if any of the person's emails match the expected reviewer email.
+outer:
+ for i, p := range people {
+ reviewerEmail := review.Reviewers[i].Reviewer
+ for _, email := range p.Emails {
+ if email == reviewerEmail {
+ continue outer
+ }
+ }
+ return false
+ }
+ return true
+}
+
// abandonScratchReviews abandons Gerrit CLs in the "scratch" project if they've been open for over a week.
func (b *gopherbot) abandonScratchReviews(ctx context.Context) error {
tooOld := time.Now().Add(-24 * time.Hour * 7)
@@ -1998,10 +2039,12 @@
return nil
}
-// humanReviewersOnChange returns true if there are (or were any) human reviewers in the given change.
-// The gerritChange passed must be used because it’s used as a key to deletedChanges and the ID returned
-// by cl.ChangeID() can be associated with multiple changes (cherry-picks, for example).
-func (b *gopherbot) humanReviewersOnChange(ctx context.Context, change gerritChange, cl *maintner.GerritCL) bool {
+// humanReviewersOnChange reports whether there is (or was) a sufficient number
+// of human reviewers in the given change. It also returns the IDs of the
+// current human reviewers. The given gerritChange must be used because it’s
+// used as a key to deletedChanges and the ID returned by cl.ChangeID() can be
+// associated with multiple changes (cherry-picks, for example).
+func (b *gopherbot) humanReviewersOnChange(ctx context.Context, change gerritChange, cl *maintner.GerritCL) ([]string, bool) {
const (
gobotID = 5976
gerritbotID = 12446
@@ -2017,8 +2060,8 @@
minHumans = 2
}
- if found := humanReviewersInMetas(cl.Metas, minHumans); found {
- return true
+ if reviewers, found := humanReviewersInMetas(cl.Metas, minHumans); found {
+ return reviewers, true
}
reviewers, err := b.gerrit.ListReviewers(ctx, change.ID())
@@ -2027,29 +2070,37 @@
b.deletedChanges[change] = true
}
log.Printf("Could not list reviewers on change %q: %v", change.ID(), err)
- return true
+ return nil, true
}
var count int
+ var ids []string
for _, r := range reviewers {
if r.NumericID != gobotID && r.NumericID != gerritbotID && r.NumericID != ownerID {
+ ids = append(ids, strconv.FormatInt(r.NumericID, 10))
count++
- if count == minHumans {
- return true
- }
}
}
- return false
+ return ids, count >= minHumans
}
+// reviewerRe extracts the reviewer's Gerrit ID from a line that looks like:
+//
+// Reviewer: Rebecca Stambler <16140@62eb7196-b449-3ce5-99f1-c037f21e1705>
+var reviewerRe = regexp.MustCompile(`.* <(?P<id>\d+)@.*>`)
+
+const gerritInstanceID = "@62eb7196-b449-3ce5-99f1-c037f21e1705"
+
// humanReviewersInMetas reports whether there are at least minHumans human
-// reviewers in the given metas.
-func humanReviewersInMetas(metas []*maintner.GerritMeta, minHumans int) bool {
+// reviewers in the given metas. It also returns the Gerrit IDs of all of the
+// human reviewers.
+func humanReviewersInMetas(metas []*maintner.GerritMeta, minHumans int) ([]string, bool) {
// Emails as they appear in maintner (<numeric ID>@<instance ID>)
var (
- gobotEmail = "5976@62eb7196-b449-3ce5-99f1-c037f21e1705"
- gerritbotEmail = "12446@62eb7196-b449-3ce5-99f1-c037f21e1705"
+ gobotEmail = "5976" + gerritInstanceID
+ gerritbotEmail = "12446" + gerritInstanceID
)
var count int
+ var ids []string
for _, m := range metas {
if !strings.Contains(m.Commit.Msg, "Reviewer:") && !strings.Contains(m.Commit.Msg, "CC:") {
continue
@@ -2060,20 +2111,30 @@
return nil
}
if !strings.Contains(ln, gobotEmail) && !strings.Contains(ln, gerritbotEmail) {
+ match := reviewerRe.FindStringSubmatch(ln)
+ if match == nil {
+ return nil
+ }
// A human is already on the change.
count++
- if count == minHumans {
- return errStopIteration
+ // Extract the human's Gerrit ID.
+ for i, name := range reviewerRe.SubexpNames() {
+ if name != "id" {
+ continue
+ }
+ if i < 0 || i > len(match) {
+ continue
+ }
+ ids = append(ids, match[i])
}
}
return nil
})
- if err != nil && err != errStopIteration {
+ if err != nil {
log.Printf("humanReviewersInMetas: got unexpected error from foreach.LineStr: %v", err)
- return count >= minHumans
}
}
- return count >= minHumans
+ return ids, count >= minHumans
}
func getCodeOwners(ctx context.Context, paths []string) ([]*owners.Entry, error) {
diff --git a/cmd/gopherbot/gopherbot_test.go b/cmd/gopherbot/gopherbot_test.go
index 5af4a08..cc08011 100644
--- a/cmd/gopherbot/gopherbot_test.go
+++ b/cmd/gopherbot/gopherbot_test.go
@@ -357,24 +357,28 @@
commitMsg string
hasHuman bool
atLeast int
+ wantIDs []string
}{
{`Patch-set: 6
Reviewer: Andrew Bonventre <22285@62eb7196-b449-3ce5-99f1-c037f21e1705>
`,
true,
1,
+ []string{"22285"},
},
{`Patch-set: 6
CC: Andrew Bonventre <22285@62eb7196-b449-3ce5-99f1-c037f21e1705>
`,
true,
1,
+ []string{"22285"},
},
{`Patch-set: 6
Reviewer: Gobot Gobot <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
`,
false,
1,
+ []string{},
},
{`Patch-set: 6
Reviewer: Gobot Gobot <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
@@ -382,6 +386,7 @@
`,
true,
1,
+ []string{"22285"},
},
{`Patch-set: 6
Reviewer: Gobot Gobot <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
@@ -389,6 +394,7 @@
`,
true,
1,
+ []string{"22285"},
},
{`Patch-set: 6
Reviewer: Gobot Gobot <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
@@ -396,6 +402,7 @@
`,
false,
2,
+ []string{"22285"},
},
{`Patch-set: 6
Reviewer: Gobot Gobot <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
@@ -404,6 +411,7 @@
`,
true,
2,
+ []string{"22285", "16140"},
},
}
@@ -411,8 +419,19 @@
metas := []*maintner.GerritMeta{
{Commit: &maintner.GitCommit{Msg: tc.commitMsg}},
}
- if got, want := humanReviewersInMetas(metas, tc.atLeast), tc.hasHuman; got != want {
+ ids, got := humanReviewersInMetas(metas, tc.atLeast)
+ if want := tc.hasHuman; got != want {
t.Errorf("Unexpected result for meta commit message: got %v; want %v for\n%s", got, want, tc.commitMsg)
+ continue
+ }
+ if len(ids) != len(tc.wantIDs) {
+ t.Errorf("Unexpected result for meta commit message: got %v reviewer IDs, want %v", len(ids), len(tc.wantIDs))
+ continue
+ }
+ for i, id := range tc.wantIDs {
+ if id != ids[i] {
+ t.Errorf("Unexpected ID at %d: got %v, want %v", i, ids[i], id)
+ }
}
}
}
