internal/gerrit: add ChangeRevisions, no Comments error, various cleanups
This CL contains various cleanups while still working on the reviews dashboard:
- Canonicalize accounts in ChangeMessages.
- Rename ChangeReviewed to ChangeReviewers to reflect what it actually returns.
- Add ChangeRevisions.
- Stop returning an error from Comments to be consistent with other functions.
Also, canonicalize accounts.
- Add a test for ChangeCommentCounts.
- Change more slice fields in the Gerrit types to use struct pointers.
Change-Id: Iedb99765f542a5ad97b04f622a63a69abb8cb986
Reviewed-on: https://go-review.googlesource.com/c/oscar/+/617758
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Zvonimir Pavlinovic <zpavlinovic@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
diff --git a/internal/gerrit/change.go b/internal/gerrit/change.go
index 78ca091..be59b6d 100644
--- a/internal/gerrit/change.go
+++ b/internal/gerrit/change.go
@@ -5,6 +5,7 @@
package gerrit
import (
+ "cmp"
"encoding/json"
"fmt"
"slices"
@@ -135,12 +136,52 @@
// These are the top-level messages created by clicking on
// the top REPLY button when reviewing a change.
// Inline file comments are returned by [Client.Comments].
-func (c *Client) ChangeMessages(ch *Change) []ChangeMessageInfo {
+func (c *Client) ChangeMessages(ch *Change) []*ChangeMessageInfo {
var messages struct {
- Messages []ChangeMessageInfo `json:"messages"`
+ Messages []json.RawMessage `json:"messages"`
}
c.unmarshal(ch, "messages", &messages)
- return messages.Messages
+
+ // Unpack into a changeMessageInfo struct, and then convert to
+ // ChangeMessageInfo, so that we don't have to unpack the lengthy
+ // AccountInfo data each time.
+ type changeMessageInfo struct {
+ ID string `json:"id"`
+ Author json.RawMessage `json:"author,omitempty"`
+ RealAuthor json.RawMessage `json:"real_author,omitempty"`
+ Date TimeStamp `json:"date"`
+ Message string `json:"message"`
+ AccountsInMessage []json.RawMessage `json:"accounts_in_message,omitempty"`
+ Tag string `json:"tag,omitempty"`
+ RevisionNumber int `json:"_revision_number,omitempty"`
+ }
+
+ ret := make([]*ChangeMessageInfo, 0, len(messages.Messages))
+ for _, msg := range messages.Messages {
+ var cmi changeMessageInfo
+ if err := json.Unmarshal(msg, &cmi); err != nil {
+ c.slog.Error("gerrit message decode failure", "num", ch.num, "data", msg, "err", err)
+ c.db.Panic("gerrit message decode failure", "num", ch.num, "err", err)
+ }
+
+ var aims []*AccountInfo
+ for _, aim := range cmi.AccountsInMessage {
+ aims = append(aims, c.loadAccount(aim))
+ }
+ r := &ChangeMessageInfo{
+ ID: cmi.ID,
+ Author: c.loadAccount(cmi.Author),
+ RealAuthor: c.loadAccount(cmi.RealAuthor),
+ Date: cmi.Date,
+ Message: cmi.Message,
+ AccountsInMessage: aims,
+ Tag: cmi.Tag,
+ RevisionNumber: cmi.RevisionNumber,
+ }
+ ret = append(ret, r)
+ }
+
+ return ret
}
// ChangeDescription returns the current description of the change.
@@ -176,10 +217,11 @@
return workInProgress.WorkInProgress
}
-// ChangeReviewed returns a list of accounts that have reviewed this change.
+// ChangeReviewers returns a list of accounts that are listed as
+// reviewers of this change.
// Note that this is not identical to ChangeInfo.Reviewers,
// which includes both reviewers and people CC'ed.
-func (c *Client) ChangeReviewed(ch *Change) []*AccountInfo {
+func (c *Client) ChangeReviewers(ch *Change) []*AccountInfo {
var reviewers struct {
Reviewers map[string][]json.RawMessage `json:"reviewers"`
}
@@ -227,6 +269,9 @@
// unmarshalLabel unmarshals a LabelInfo.
func (c *Client) unmarshalLabel(ch *Change, input json.RawMessage) *LabelInfo {
+ // Unpack into approvalInfo and labelInfo structs, and then convert to
+ // ApprovalInfo and LabelInfo, so that we don't have to unpack the
+ // lengthy AccountInfo data each time.
type approvalInfo struct {
Value int `json:"value,omitempty"`
PermittedVotingRange VotingRangeInfo `json:"permitted_voting_range,omitempty"`
@@ -348,6 +393,72 @@
return counts.TotalCommentCount, counts.UnresolvedCommentCount
}
+// ChangeRevisions returns information about all the patch sets,
+// ordered by patch set number.
+func (c *Client) ChangeRevisions(ch *Change) []*RevisionInfo {
+ // Unpack into commitInfo and revisionInfo structs,
+ // and then convert to CommitInfo and RevisionInfo,
+ // so that we don't have to unpack the lengthy AccountInfo
+ // data each time. We also skip some CommitInfo fields.
+ type commitInfo struct {
+ Commit string `json:"commit,omitempty"`
+ Author *GitPersonInfo `json:"author,omitempty"`
+ Committer *GitPersonInfo `json:"committer,omitempty"`
+ Subject string `json:"subject"`
+ Message string `json:"message,omitempty"`
+ }
+ type revisionInfo struct {
+ Kind string `json:"kind"`
+ Number int `json:"_number"`
+ Created TimeStamp `json:"created"`
+ Uploader json.RawMessage `json:"uploader"`
+ RealUploader json.RawMessage `json:"real_uploader,omitempty"`
+ Ref string `json:"ref"`
+ Commit *commitInfo `json:"commit,omitempty"`
+ Branch string `json:"branch,omitempty"`
+ Description string `json:"description,omitempty"`
+ }
+ var revisions struct {
+ Revisions map[string]*revisionInfo `json:"revisions"`
+ }
+ c.unmarshal(ch, "revisions", &revisions)
+
+ toCommitInfo := func(from *commitInfo) *CommitInfo {
+ if from == nil {
+ return nil
+ }
+ return &CommitInfo{
+ Commit: from.Commit,
+ Author: from.Author,
+ Committer: from.Committer,
+ Subject: from.Subject,
+ Message: from.Message,
+ }
+ }
+
+ revs := make([]*RevisionInfo, 0, len(revisions.Revisions))
+ for _, rev := range revisions.Revisions {
+ ri := &RevisionInfo{
+ Kind: rev.Kind,
+ Number: rev.Number,
+ Created: rev.Created,
+ Uploader: c.loadAccount(rev.Uploader),
+ RealUploader: c.loadAccount(rev.RealUploader),
+ Ref: rev.Ref,
+ Commit: toCommitInfo(rev.Commit),
+ Branch: rev.Branch,
+ Description: rev.Description,
+ }
+ revs = append(revs, ri)
+ }
+
+ slices.SortFunc(revs, func(a, b *RevisionInfo) int {
+ return cmp.Compare(a.Number, b.Number)
+ })
+
+ return revs
+}
+
// unmarshal unmarshals ch.data into a value. If the unmarshal fails, it
// crashes with an error.
func (c *Client) unmarshal(ch *Change, msg string, val any) {
diff --git a/internal/gerrit/data.go b/internal/gerrit/data.go
index f78041d..595e772 100644
--- a/internal/gerrit/data.go
+++ b/internal/gerrit/data.go
@@ -56,21 +56,71 @@
// Comments returns the comments on a change, if any. These are the
// inline comments placed on files in the change. The top-level
-// replies are stored in a [Change] and are returned by [Change.Messages].
+// replies are stored in a [Change] and are returned by [Client.ChangeMessages].
//
// This returns a map from file names to a list of comments on each file.
-// Gerrit stores top-level comments on the file "/PATCHSET_LEVEL".
-// The result may be nil, with no error, if no comment information exists.
-func (c *Client) Comments(project string, changeNum int) (map[string][]*CommentInfo, error) {
+// The result is nil if no comment information exists.
+func (c *Client) Comments(project string, changeNum int) map[string][]*CommentInfo {
val, ok := c.db.Get(o(commentKind, c.instance, project, changeNum))
if !ok {
- return nil, nil
+ return nil
}
- var comments map[string][]*CommentInfo
+
+ // Unpack into a commentInfo struct, and then convert to CommentInfo,
+ // so that we don't have to unpack the lengthy AccountInfo data
+ // each time.
+ type commentInfo struct {
+ PatchSet int `json:"patch_set,omitempty"`
+ ID string `json:"id"`
+ Path string `json:"path,omitempty"`
+ Side string `json:"side,omitempty"`
+ Parent int `json:"parent,omitempty"`
+ Line int `json:"line,omitempty"`
+ Range *CommentRange `json:"range,omitempty"`
+ InReplyTo string `json:"in_reply_to,omitempty"`
+ Message string `json:"message,omitempty"`
+ Updated TimeStamp `json:"updated"`
+ Author json.RawMessage `json:"author,omitempty"`
+ Tag string `json:"tag,omitempty"`
+ Unresolved bool `json:"unresolved,omitempty"`
+ ChangeMessageID string `json:"change_message_id,omitempty"`
+ CommitID string `json:"commit_id,omitempty"`
+ FixSuggestions []*FixSuggestionInfo `json:"fix_suggestions,omitempty"`
+ }
+ var comments map[string][]*commentInfo
if err := json.Unmarshal(val, &comments); err != nil {
- return nil, fmt.Errorf("can't decode change %d comments: %v", changeNum, err)
+ c.slog.Error("gerrit comment decode failure", "num", changeNum, "data", val, "err", err)
+ c.db.Panic("gerrit comment decode failure", "num", changeNum, "err", err)
}
- return comments, nil
+
+ ret := make(map[string][]*CommentInfo, len(comments))
+ for key, val := range comments {
+ rcs := make([]*CommentInfo, 0, len(val))
+ for _, comment := range val {
+ rc := &CommentInfo{
+ PatchSet: comment.PatchSet,
+ ID: comment.ID,
+ Path: comment.Path,
+ Side: comment.Side,
+ Parent: comment.Parent,
+ Line: comment.Line,
+ Range: comment.Range,
+ InReplyTo: comment.InReplyTo,
+ Message: comment.Message,
+ Updated: comment.Updated,
+ Author: c.loadAccount(comment.Author),
+ Tag: comment.Tag,
+ Unresolved: comment.Unresolved,
+ ChangeMessageID: comment.ChangeMessageID,
+ CommitID: comment.CommitID,
+ FixSuggestions: comment.FixSuggestions,
+ }
+ rcs = append(rcs, rc)
+ }
+ ret[key] = rcs
+ }
+
+ return ret
}
// A ChangeEvent is a Gerrit CL change event returned by ChangeWatcher.
diff --git a/internal/gerrit/sync_test.go b/internal/gerrit/sync_test.go
index dfa1a1b..2058e31 100644
--- a/internal/gerrit/sync_test.go
+++ b/internal/gerrit/sync_test.go
@@ -227,7 +227,7 @@
"Patch Set 9:\n\nNaah, just roll forward and fix the test.",
},
func(got, want any) bool {
- g := got.([]ChangeMessageInfo)
+ g := got.([]*ChangeMessageInfo)
w := want.([]string)
for i, m := range g {
if m.Message != w[i] {
@@ -250,8 +250,8 @@
nil,
},
{
- "ChangeReviewed",
- wa(c.ChangeReviewed),
+ "ChangeReviewers",
+ wa(c.ChangeReviewers),
[]string{
"bradfitz@golang.org",
"iant@golang.org",
@@ -328,9 +328,32 @@
return slices.Equal(g, w)
},
},
+ {
+ "ChangeRevisions",
+ wa(c.ChangeRevisions),
+ "errgroup: add package",
+ func(got, want any) bool {
+ g := got.([]*RevisionInfo)
+ if len(g) != 9 {
+ return false
+ }
+ for _, r := range g {
+ if r.Commit.Subject != want {
+ return false
+ }
+ }
+ return true
+ },
+ },
}
testChangeTests(t, ch, tests)
+
+ total, unresolved := c.ChangeCommentCounts(ch)
+ wantTotal, wantUnresolved := 22, 0
+ if total != wantTotal || unresolved != wantUnresolved {
+ t.Errorf("CommentCounts = %d, %d; want %d, %d", total, unresolved, wantTotal, wantUnresolved)
+ }
}
// checkChange verifies that we can unpack CL information, and that it
@@ -343,11 +366,7 @@
}
// Fetch and unpack comments for the change.
- commentsInfo, err := c.Comments(project, changeNum)
- if err != nil {
- t.Error(err)
- return
- }
+ commentsInfo := c.Comments(project, changeNum)
if commentsInfo == nil {
t.Errorf("no comment information for CL %d", changeNum)
return
diff --git a/internal/gerrit/testing_test.go b/internal/gerrit/testing_test.go
index 2681160..d32841a 100644
--- a/internal/gerrit/testing_test.go
+++ b/internal/gerrit/testing_test.go
@@ -79,7 +79,7 @@
"message 2",
},
func(got, want any) bool {
- g := got.([]ChangeMessageInfo)
+ g := got.([]*ChangeMessageInfo)
w := want.([]string)
for i, m := range g {
if m.Message != w[i] {
diff --git a/internal/gerrit/types.go b/internal/gerrit/types.go
index 196b820..e7daa5a 100644
--- a/internal/gerrit/types.go
+++ b/internal/gerrit/types.go
@@ -205,7 +205,7 @@
UserName string `json:"username,omitempty"`
// List of [AvatarInfo] entities that provide information about
// avatar images of the account.
- Avatars []AvatarInfo `json:"avatars,omitempty"`
+ Avatars []*AvatarInfo `json:"avatars,omitempty"`
// Status message of the account.
Status string `json:"status,omitempty"`
// Whether the account is inactive.
@@ -265,7 +265,7 @@
// * label: the label name.
// * status:
// * appliedBy:
- Labels []struct {
+ Labels []*struct {
// The label name.
Label string `json:"label"`
// The label status: {OK, REJECT, MAY, NEED, IMPOSSIBLE}.
@@ -275,7 +275,7 @@
} `json:"labels,omitempty"`
// List of the requirements to be met before this change can
// be submitted.
- Requirements []Requirement `json:"requirements,omitempty"`
+ Requirements []*Requirement `json:"requirements,omitempty"`
// When status is RULE_ERROR this message provides some text
// describing the failure of the rule predicate.
ErrorMessage string `json:"error_message,omitempty"`
@@ -428,7 +428,7 @@
// branch name if the parent is a merged commit in the target
// branch. Otherwise, we include the change and patch-set
// numbers of the parent change.
- ParentsData []ParentInfo `json:"parents_data,omitempty"`
+ ParentsData []*ParentInfo `json:"parents_data,omitempty"`
// The name of the target branch that this revision is set to
// be merged into. Note that if the change is moved with the
// Move Change endpoint, this field can be different for
@@ -461,7 +461,7 @@
// The parent commits of this commit as a list of CommitInfo
// entities. In each parent only the commit and subject fields
// are populated.
- Parents []CommitInfo `json:"parents,omitempty"`
+ Parents []*CommitInfo `json:"parents,omitempty"`
// The author of the commit as a GitPersonInfo entity.
Author *GitPersonInfo `json:"author,omitempty"`
// The committer of the commit as a GitPersonInfo entity.
@@ -472,10 +472,10 @@
Message string `json:"message,omitempty"`
// Links to the patch set in external sites as a list of
// WebLinkInfo entities.
- WebLinks []WebLinkInfo `json:"web_links,omitempty"`
+ WebLinks []*WebLinkInfo `json:"web_links,omitempty"`
// Links to the commit in external sites for resolving
// conflicts as a list of WebLinkInfo entities.
- ResolveConflictsWebLinks []WebLinkInfo `json:"resolve_conflicts_web_links,omitempty"`
+ ResolveConflictsWebLinks []*WebLinkInfo `json:"resolve_conflicts_web_links,omitempty"`
}
// GitPersonInfo holds information about the author/committer of a commit.
@@ -579,7 +579,7 @@
// the patchset to which this comment applies.
CommitID string `json:"commit_id,omitempty"`
// Suggested fixes for this comment.
- FixSuggestions []FixSuggestionInfo `json:"fix_suggestions,omitempty"`
+ FixSuggestions []*FixSuggestionInfo `json:"fix_suggestions,omitempty"`
// The remaining fields are defined by Gerrit but we don't
// request their values.
@@ -598,7 +598,7 @@
// A list of FixReplacementInfo entities indicating how the
// content of one or several files should be modified. Within
// a file, they should refer to non-overlapping regions.
- Replacements []FixReplacementInfo `json:"replacements"`
+ Replacements []*FixReplacementInfo `json:"replacements"`
}
// FixReplacementInfo describes how the content of a file should be
diff --git a/internal/gerritdocs/sync.go b/internal/gerritdocs/sync.go
index de000cf..2fec090 100644
--- a/internal/gerritdocs/sync.go
+++ b/internal/gerritdocs/sync.go
@@ -146,10 +146,7 @@
// comments returns file comments for the gerrit change.
func comments(gr *gerrit.Client, c *changeInfo) ([]*gerrit.CommentInfo, error) {
var cmts []*gerrit.CommentInfo
- cmtsMap, err := gr.Comments(c.project, c.number)
- if err != nil {
- return nil, err
- }
+ cmtsMap := gr.Comments(c.project, c.number)
for _, cs := range cmtsMap { // we don't care about comment file locations
cmts = append(cmts, cs...)
}
