Submitting review 7dd2de1

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
#33.

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

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}`

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)

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: ", pendingReview.AllRequests, pendingReview.Request)
+	}
+}