Submitting review ca43129

Support edited comments.

This change adds read-only support for comment edit actions. These
are represented in the JSON format using a new "original" field that
references the original, unedited comment.

By default, when displaying a comment thread that has been edited,
only the current version of comment is displayed. The full history of
the comment can be viewed by adding the `--json` flag to the
`git appraise show` command, to display the review in JSON format.

This change defines how edits are represented in git-notes and makes
the tool properly display edited comments, but does not add any support
to the tool for creating edits.

This fixes #67

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 + "  "

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.

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]

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) {

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"