Repositories

Previously, the tool was taking the last request in a note as being the current one, and relying on the combination of appending notes and merging them using "cat_sort_uniq" to make that correspond to the request with the latest timestamp.

However, that was neither as robust as it could be, nor was it documented in our spec.

This change makes the tool pick the last request by timestamp, and updates the spec to indicate that this is how the notes should be interpreted.

This change also makes the sorting of both requests and comments stable, and extends the Summary struct to store all of the review requests in addition to storing the current one.

This change is a (partial) response to feedback in https://github.com/google/git-appraise/issues/33.

Modified README.md

@@ -147,6 +147,15 @@ The "reviewRef" field is used to specify a git ref that tracks the current
revision under review, and the "targetRef" field is used to specify the git ref
that should be updated once the review is approved.
If there are multiple requests for a single commit, then they are sorted by
timestamp and the final request is treated as the current one. This sorting
should be done in a stable manner, so that if there are multiple requests
with the same timestamp, then the last such request in the note is treated
as the current one.
This design allows a user to update a review request by re-running the
`git appraise request` command.
### Continuous Integration Status
Continuous integration build and test results are stored in the

Modified repository/mock_repo.go

@@ -54,7 +54,11 @@ const (
TestRequestB = `{"timestamp": "0000000001", "reviewRef": "refs/heads/ojarjur/mychange", "targetRef": "refs/heads/master", "requester": "ojarjur", "reviewers": ["ojarjur"], "description": "B"}`
TestRequestD = `{"timestamp": "0000000002", "reviewRef": "refs/heads/ojarjur/mychange", "targetRef": "refs/heads/master", "requester": "ojarjur", "reviewers": ["ojarjur"], "description": "D"}`
TestRequestG = `{"timestamp": "0000000004", "reviewRef": "refs/heads/ojarjur/mychange", "targetRef": "refs/heads/master", "requester": "ojarjur", "reviewers": ["ojarjur"], "description": "G"}`
TestRequestG = `{"timestamp": "0000000004", "reviewRef": "refs/heads/ojarjur/mychange", "targetRef": "refs/heads/master", "requester": "ojarjur", "reviewers": ["ojarjur"], "description": "G"}
{"timestamp": "0000000005", "reviewRef": "refs/heads/ojarjur/mychange", "targetRef": "refs/heads/master", "requester": "ojarjur", "reviewers": ["ojarjur"], "description": "Updated description of G"}
{"timestamp": "0000000005", "reviewRef": "refs/heads/ojarjur/mychange", "targetRef": "refs/heads/master", "requester": "ojarjur", "reviewers": ["ojarjur"], "description": "Final description of G"}`
TestDiscussB = `{"timestamp": "0000000001", "author": "ojarjur", "location": {"commit": "B"}, "resolved": true}`
TestDiscussD = `{"timestamp": "0000000003", "author": "ojarjur", "location": {"commit": "E"}, "resolved": true}`

Modified review/review.go

@@ -55,12 +55,13 @@ type CommentThread struct {
// 1. Resolved indicates if a reviewer has accepted or rejected the change.
// 2. Submitted indicates if the change has been incorporated into the target.
type Summary struct {
Repo repository.Repo `json:"-"`
Revision string `json:"revision"`
Request request.Request `json:"request"`
Comments []CommentThread `json:"comments,omitempty"`
Resolved *bool `json:"resolved,omitempty"`
Submitted bool `json:"submitted"`
Repo repository.Repo `json:"-"`
Revision string `json:"revision"`
Request request.Request `json:"request"`
AllRequests []request.Request `json:"-"`
Comments []CommentThread `json:"comments,omitempty"`
Resolved *bool `json:"resolved,omitempty"`
Submitted bool `json:"submitted"`
}
// Review represents the entire state of a code review.
@@ -84,13 +85,24 @@ func (threads byTimestamp) Less(i, j int) bool {
return threads[i].Comment.Timestamp < threads[j].Comment.Timestamp
}
type requestsByTimestamp []request.Request
// Interface methods for sorting review requests by timestamp
func (requests requestsByTimestamp) Len() int { return len(requests) }
func (requests requestsByTimestamp) Swap(i, j int) {
requests[i], requests[j] = requests[j], requests[i]
}
func (requests requestsByTimestamp) Less(i, j int) bool {
return requests[i].Timestamp < requests[j].Timestamp
}
// updateThreadsStatus calculates the aggregate status of a sequence of comment threads.
//
// The aggregate status is the conjunction of all of the non-nil child statuses.
//
// This has the side-effect of setting the "Resolved" field of all descendant comment threads.
func updateThreadsStatus(threads []CommentThread) *bool {
sort.Sort(byTimestamp(threads))
sort.Stable(byTimestamp(threads))
noUnresolved := true
var result *bool
for i := range threads {
@@ -199,10 +211,12 @@ func GetSummary(repo repository.Repo, revision string) (*Summary, error) {
if requests == nil {
return nil, nil
}
sort.Stable(requestsByTimestamp(requests))
reviewSummary := Summary{
Repo: repo,
Revision: revision,
Request: requests[len(requests)-1],
Repo: repo,
Revision: revision,
Request: requests[len(requests)-1],
AllRequests: requests,
}
reviewSummary.Comments = reviewSummary.loadComments()
reviewSummary.Resolved = updateThreadsStatus(reviewSummary.Comments)

Modified review/review_test.go

@@ -19,6 +19,7 @@ package review
import (
"github.com/google/git-appraise/repository"
"github.com/google/git-appraise/review/comment"
"github.com/google/git-appraise/review/request"
"sort"
"testing"
)
@@ -33,6 +34,12 @@ func TestCommentSorting(t *testing.T) {
},
CommentThread{
Comment: comment.Comment{
Timestamp: "012400",
Description: "Fifth",
},
},
CommentThread{
Comment: comment.Comment{
Timestamp: "012346",
Description: "Second",
},
@@ -50,16 +57,49 @@ func TestCommentSorting(t *testing.T) {
},
},
}
sort.Sort(byTimestamp(sampleThreads))
sort.Stable(byTimestamp(sampleThreads))
descriptions := []string{}
for _, thread := range sampleThreads {
descriptions = append(descriptions, thread.Comment.Description)
}
if !(descriptions[0] == "First" && descriptions[1] == "Second" && descriptions[2] == "Third" && descriptions[3] == "Fourth") {
if !(descriptions[0] == "First" && descriptions[1] == "Second" && descriptions[2] == "Third" && descriptions[3] == "Fourth" && descriptions[4] == "Fifth") {
t.Fatalf("Comment thread ordering failed. Got %v", sampleThreads)
}
}
func TestRequestSorting(t *testing.T) {
sampleRequests := []request.Request{
request.Request{
Timestamp: "012400",
Description: "Fourth",
},
request.Request{
Timestamp: "012400",
Description: "Fifth",
},
request.Request{
Timestamp: "012346",
Description: "Second",
},
request.Request{
Timestamp: "012345",
Description: "First",
},
request.Request{
Timestamp: "012347",
Description: "Third",
},
}
sort.Stable(requestsByTimestamp(sampleRequests))
descriptions := []string{}
for _, r := range sampleRequests {
descriptions = append(descriptions, r.Description)
}
if !(descriptions[0] == "First" && descriptions[1] == "Second" && descriptions[2] == "Third" && descriptions[3] == "Fourth" && descriptions[4] == "Fifth") {
t.Fatalf("Review request ordering failed. Got %v", sampleRequests)
}
}
func validateUnresolved(t *testing.T, resolved *bool) {
if resolved != nil {
t.Fatalf("Expected resolved status to be unset, but instead it was %v", *resolved)
@@ -614,3 +654,14 @@ func TestGetBaseCommit(t *testing.T) {
t.Fatal("Unexpected base commit computed for a pending review.")
}
}
func TestGetRequests(t *testing.T) {
repo := repository.NewMockRepoForTest()
pendingReview, err := Get(repo, repository.TestCommitG)
if err != nil {
t.Fatal(err)
}
if len(pendingReview.AllRequests) != 3 || pendingReview.Request.Description != "Final description of G" {
t.Fatal("Unexpected requests for a pending review: %v %v", pendingReview.AllRequests, pendingReview.Request)
}
}