From 7dd2de1e3fc7df4dafc66c0d181402cd03f35f48 Mon Sep 17 00:00:00 2001 From: Omar Jarjur Date: Thu, 21 Jan 2016 17:25:55 -0800 Subject: [PATCH] Formalize how to handle multiple review requests on a commit. 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. --- README.md | 9 +++++++ repository/mock_repo.go | 6 ++++- review/review.go | 34 +++++++++++++++++-------- review/review_test.go | 55 +++++++++++++++++++++++++++++++++++++++-- 4 files changed, 91 insertions(+), 13 deletions(-) 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) + } +}