diff --git a/commands/output/output.go b/commands/output/output.go index 81b05735..4613cd38 100644 --- a/commands/output/output.go +++ b/commands/output/output.go @@ -150,11 +150,7 @@ func showSubThread(r *review.Review, thread review.CommentThread, indent string) } } comment := thread.Comment - threadHash, err := comment.Hash() - if err != nil { - return err - } - + threadHash := thread.Hash timestamp := reformatTimestamp(comment.Timestamp) commentSummary := fmt.Sprintf(indent+commentTemplate, threadHash, comment.Author, timestamp, statusString, comment.Description) indent = indent + " " diff --git a/review/comment/comment.go b/review/comment/comment.go index 58bdbe21..c083fb1f 100644 --- a/review/comment/comment.go +++ b/review/comment/comment.go @@ -103,6 +103,8 @@ type Comment struct { // git-blame will become more and more expensive as the number of code reviews grows. Timestamp string `json:"timestamp,omitempty"` Author string `json:"author,omitempty"` + // If original is provided, then the comment is an updated version of another comment. + Original string `json:"original,omitempty"` // If parent is provided, then the comment is a response to another comment. Parent string `json:"parent,omitempty"` // If location is provided, then the comment is specific to that given location. diff --git a/review/review.go b/review/review.go index ab893a93..68434c6c 100644 --- a/review/review.go +++ b/review/review.go @@ -40,10 +40,13 @@ const archiveRef = "refs/devtools/archives/reviews" // then that means that there are no unaddressed comments, and that the root // comment has its resolved bit set to true. type CommentThread struct { - Hash string `json:"hash,omitempty"` - Comment comment.Comment `json:"comment"` - Children []CommentThread `json:"children,omitempty"` - Resolved *bool `json:"resolved,omitempty"` + Hash string `json:"hash,omitempty"` + Comment comment.Comment `json:"comment"` + Original *comment.Comment `json:"original,omitempty"` + Edits []*comment.Comment `json:"edits,omitempty"` + Children []CommentThread `json:"children,omitempty"` + Resolved *bool `json:"resolved,omitempty"` + Edited bool `json:"edited,omitempty"` } // Summary represents the high-level state of a code review. @@ -78,6 +81,15 @@ type Review struct { Analyses []analyses.Report `json:"analyses,omitempty"` } +type commentsByTimestamp []*comment.Comment + +// Interface methods for sorting comment threads by timestamp +func (cs commentsByTimestamp) Len() int { return len(cs) } +func (cs commentsByTimestamp) Swap(i, j int) { cs[i], cs[j] = cs[j], cs[i] } +func (cs commentsByTimestamp) Less(i, j int) bool { + return cs[i].Timestamp < cs[j].Timestamp +} + type byTimestamp []CommentThread // Interface methods for sorting comment threads by timestamp @@ -155,6 +167,7 @@ func (thread *CommentThread) updateResolvedStatus() { type mutableThread struct { Hash string Comment comment.Comment + Edits []*comment.Comment Children []*mutableThread } @@ -163,13 +176,27 @@ type mutableThread struct { // (fully constructed comment thread). func fixMutableThread(mutableThread *mutableThread) CommentThread { var children []CommentThread + edited := len(mutableThread.Edits) > 0 for _, mutableChild := range mutableThread.Children { - children = append(children, fixMutableThread(mutableChild)) + child := fixMutableThread(mutableChild) + if (!edited) && child.Edited { + edited = true + } + children = append(children, child) } + comment := &mutableThread.Comment + if len(mutableThread.Edits) > 0 { + sort.Stable(commentsByTimestamp(mutableThread.Edits)) + comment = mutableThread.Edits[len(mutableThread.Edits)-1] + } + return CommentThread{ Hash: mutableThread.Hash, - Comment: mutableThread.Comment, + Comment: *comment, + Original: &mutableThread.Comment, + Edits: mutableThread.Edits, Children: children, + Edited: edited, } } @@ -191,7 +218,12 @@ func buildCommentThreads(commentsByHash map[string]comment.Comment) []CommentThr } var rootHashes []string for hash, thread := range threadsByHash { - if thread.Comment.Parent == "" { + if thread.Comment.Original != "" { + original, ok := threadsByHash[thread.Comment.Original] + if ok { + original.Edits = append(original.Edits, &thread.Comment) + } + } else if thread.Comment.Parent == "" { rootHashes = append(rootHashes, hash) } else { parent, ok := threadsByHash[thread.Comment.Parent] diff --git a/review/review_test.go b/review/review_test.go index 13c5b3c7..af699afd 100644 --- a/review/review_test.go +++ b/review/review_test.go @@ -25,6 +25,39 @@ import ( ) func TestCommentSorting(t *testing.T) { + sampleComments := []*comment.Comment{ + &comment.Comment{ + Timestamp: "012400", + Description: "Fourth", + }, + &comment.Comment{ + Timestamp: "012400", + Description: "Fifth", + }, + &comment.Comment{ + Timestamp: "012346", + Description: "Second", + }, + &comment.Comment{ + Timestamp: "012345", + Description: "First", + }, + &comment.Comment{ + Timestamp: "012347", + Description: "Third", + }, + } + sort.Stable(commentsByTimestamp(sampleComments)) + descriptions := []string{} + for _, comment := range sampleComments { + descriptions = append(descriptions, comment.Description) + } + if !(descriptions[0] == "First" && descriptions[1] == "Second" && descriptions[2] == "Third" && descriptions[3] == "Fourth" && descriptions[4] == "Fifth") { + t.Fatalf("Comment ordering failed. Got %v", sampleComments) + } +} + +func TestThreadSorting(t *testing.T) { sampleThreads := []CommentThread{ CommentThread{ Comment: comment.Comment{ @@ -525,11 +558,18 @@ func TestBuildCommentThreads(t *testing.T) { } child := comment.Comment{ Timestamp: "012346", - Resolved: &rejected, + Resolved: nil, Parent: rootHash, Description: "child", } childHash, err := child.Hash() + updatedChild := comment.Comment{ + Timestamp: "012346", + Resolved: &rejected, + Original: childHash, + Description: "updated child", + } + updatedChildHash, err := updatedChild.Hash() if err != nil { t.Fatal(err) } @@ -544,9 +584,10 @@ func TestBuildCommentThreads(t *testing.T) { t.Fatal(err) } commentsByHash := map[string]comment.Comment{ - rootHash: root, - childHash: child, - leafHash: leaf, + rootHash: root, + childHash: child, + updatedChildHash: updatedChild, + leafHash: leaf, } threads := buildCommentThreads(commentsByHash) if len(threads) != 1 { @@ -556,12 +597,21 @@ func TestBuildCommentThreads(t *testing.T) { if rootThread.Comment.Description != "root" { t.Fatalf("Unexpected root thread: %v", rootThread) } + if !rootThread.Edited { + t.Fatalf("Unexpected root thread edited status: %v", rootThread) + } if len(rootThread.Children) != 1 { t.Fatalf("Unexpected root children: %v", rootThread.Children) } rootChild := rootThread.Children[0] - if rootChild.Comment.Description != "child" { - t.Fatalf("Unexpected child: %v", rootChild) + if rootChild.Comment.Description != "updated child" { + t.Fatalf("Unexpected updated child: %v", rootChild) + } + if rootChild.Original.Description != "child" { + t.Fatalf("Unexpected original child: %v", rootChild) + } + if len(rootChild.Edits) != 1 { + t.Fatalf("Unexpected child history: %v", rootChild.Edits) } if len(rootChild.Children) != 1 { t.Fatalf("Unexpected leaves: %v", rootChild.Children) @@ -573,6 +623,9 @@ func TestBuildCommentThreads(t *testing.T) { if len(threadLeaf.Children) != 0 { t.Fatalf("Unexpected leaf children: %v", threadLeaf.Children) } + if threadLeaf.Edited { + t.Fatalf("Unexpected leaf edited status: %v", threadLeaf) + } } func TestGetHeadCommit(t *testing.T) { diff --git a/schema/comment.json b/schema/comment.json index 8ffb7001..a39b1a2e 100644 --- a/schema/comment.json +++ b/schema/comment.json @@ -15,6 +15,11 @@ "type": "string" }, + "original": { + "description": "the SHA1 hash of another comment on the same revision, and it means this comment is an updated version of that comment", + "type": "string" + }, + "parent": { "description": "the SHA1 hash of another comment on the same revision, and it means this comment is a reply to that comment", "type": "string"