diff --git a/README.md b/README.md index eb046810..ea3474dd 100644 --- a/README.md +++ b/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 diff --git a/repository/mock_repo.go b/repository/mock_repo.go index 64b144d3..94396fb7 100644 --- a/repository/mock_repo.go +++ b/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}` diff --git a/review/review.go b/review/review.go index 3e283ca5..323c66f9 100644 --- a/review/review.go +++ b/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) diff --git a/review/review_test.go b/review/review_test.go index db961704..69cc67ec 100644 --- a/review/review_test.go +++ b/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" ) @@ -31,6 +32,12 @@ func TestCommentSorting(t *testing.T) { Description: "Fourth", }, }, + CommentThread{ + Comment: comment.Comment{ + Timestamp: "012400", + Description: "Fifth", + }, + }, CommentThread{ Comment: comment.Comment{ Timestamp: "012346", @@ -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) + } +}