Browse Source

Submitting review 8ad8f69d5c8b

xAdd support for detached comments

This change adds support for "detached" comments; comments that are not
attached to a code review.

Instead, we anchor these comments at a file path relative to the repository.

In order to do that, we add the concept of a "well-known commit" which can be
deterministically recreated and corresponds to a specific path. Detached
comments are then attached to their corresponding well-known commit rather than
a review.

When we use a well-known commit for the first time, we add it to the archive
ref so that the attached git-notes do not get pruned. To prevent the archive
ref from growing indefinitely, this change also includes an enhancement where
trying to archive an already-archived object is a no-op.

This fixes #97
Omar Jarjur 8 months ago
parent
commit
d586cafb9a
6 changed files with 253 additions and 70 deletions
  1. 92 38
      commands/comment.go
  2. 2 9
      commands/list.go
  3. 54 12
      commands/output/output.go
  4. 29 3
      commands/show.go
  5. 6 0
      repository/git.go
  6. 70 8
      review/review.go

+ 92 - 38
commands/comment.go

@@ -33,13 +33,13 @@ var commentLocation = comment.Range{}
 
 var (
 	commentMessageFile = commentFlagSet.String("F", "", "Take the comment from the given file. Use - to read the message from the standard input")
-	commentMessage     = commentFlagSet.String("m", "", "Message to attach to the review")
+	commentMessage     = commentFlagSet.String("m", "", "Message body of the comment")
 	commentParent      = commentFlagSet.String("p", "", "Parent comment")
 	commentFile        = commentFlagSet.String("f", "", "File being commented upon")
+	commentDetached    = commentFlagSet.Bool("d", false, "Do not attach the comment to a review")
 	commentLgtm        = commentFlagSet.Bool("lgtm", false, "'Looks Good To Me'. Set this to express your approval. This cannot be combined with nmw")
 	commentNmw         = commentFlagSet.Bool("nmw", false, "'Needs More Work'. Set this to express your disapproval. This cannot be combined with lgtm")
-	commentSign        = commentFlagSet.Bool("S", false,
-		"Sign the contents of the comment")
+	commentSign        = commentFlagSet.Bool("S", false, "Sign the contents of the comment")
 )
 
 func init() {
@@ -65,71 +65,49 @@ func commentHashExists(hashToFind string, threads []review.CommentThread) bool {
 	return false
 }
 
-// commentOnReview adds a comment to the current code review.
-func commentOnReview(repo repository.Repo, args []string) error {
-	commentFlagSet.Parse(args)
-	args = commentFlagSet.Args()
-
-	var r *review.Review
-	var err error
-	if len(args) > 1 {
-		return errors.New("Only accepting a single review is supported.")
-	}
-
-	if len(args) == 1 {
-		r, err = review.Get(repo, args[0])
-	} else {
-		r, err = review.GetCurrent(repo)
-	}
-
-	if err != nil {
-		return fmt.Errorf("Failed to load the review: %v\n", err)
-	}
-	if r == nil {
-		return errors.New("There is no matching review.")
-	}
-
+func validateArgs(repo repository.Repo, args []string, threads []review.CommentThread) error {
 	if *commentLgtm && *commentNmw {
 		return errors.New("You cannot combine the flags -lgtm and -nmw.")
 	}
 	if commentLocation != (comment.Range{}) && *commentFile == "" {
 		return errors.New("Specifying a line number with the -l flag requires that you also specify a file name with the -f flag.")
 	}
-	if *commentParent != "" && !commentHashExists(*commentParent, r.Comments) {
+	if *commentParent != "" && !commentHashExists(*commentParent, threads) {
 		return errors.New("There is no matching parent comment.")
 	}
 
 	if *commentMessageFile != "" && *commentMessage == "" {
+		var err error
 		*commentMessage, err = input.FromFile(*commentMessageFile)
 		if err != nil {
 			return err
 		}
 	}
 	if *commentMessageFile == "" && *commentMessage == "" {
+		var err error
 		*commentMessage, err = input.LaunchEditor(repo, commentFilename)
 		if err != nil {
 			return err
 		}
 	}
+	return nil
+}
 
-	commentedUponCommit, err := r.GetHeadCommit()
-	if err != nil {
-		return err
-	}
+func buildCommentFromFlags(repo repository.Repo, commentedUponCommit string) (*comment.Comment, error) {
 	location := comment.Location{
 		Commit: commentedUponCommit,
 	}
 	if *commentFile != "" {
 		location.Path = *commentFile
 		location.Range = &commentLocation
-		if err := location.Check(r.Repo); err != nil {
-			return fmt.Errorf("Unable to comment on the given location: %v", err)
+		if err := location.Check(repo); err != nil {
+			return nil, fmt.Errorf("Unable to comment on the given location: %v", err)
 		}
 	}
 
 	userEmail, err := repo.GetUserEmail()
 	if err != nil {
-		return err
+		return nil, err
 	}
 	c := comment.New(userEmail, *commentMessage)
 	c.Location = &location
@@ -142,15 +120,86 @@ func commentOnReview(repo repository.Repo, args []string) error {
 	if *commentSign {
 		key, err := repo.GetUserSigningKey()
 		if err != nil {
-			return err
+			return nil, err
 		}
 		err = gpg.Sign(key, &c)
 		if err != nil {
-			return err
+			return nil, err
 		}
 	}
+	return &c, nil
+}
 
-	return r.AddComment(c)
+// commentOnReview adds a comment to the current code review.
+func commentOnReview(repo repository.Repo, args []string) error {
+	var r *review.Review
+	var err error
+
+	if len(args) > 1 {
+		return errors.New("Only commenting on a single review is supported.")
+	}
+	if len(args) == 1 {
+		r, err = review.Get(repo, args[0])
+	} else {
+		r, err = review.GetCurrent(repo)
+	}
+
+	if err != nil {
+		return fmt.Errorf("Failed to load the review: %v\n", err)
+	}
+	if r == nil {
+		return errors.New("There is no matching review.")
+	}
+
+	if err := validateArgs(repo, args, r.Comments); err != nil {
+		return err
+	}
+
+	commentedUponCommit, err := r.GetHeadCommit()
+	if err != nil {
+		return err
+	}
+
+	c, err := buildCommentFromFlags(r.Repo, commentedUponCommit)
+	if err != nil {
+		return err
+	}
+	return r.AddComment(*c)
+}
+
+// commentOnPath adds a comment about the given file without attaching it to a review.
+func commentOnPath(repo repository.Repo, args []string) error {
+	if *commentFile == "" {
+		return errors.New("You must specify the containing file for detached comments.")
+	}
+
+	if len(args) > 1 {
+		return errors.New("Only commenting on a single location is supported.")
+	}
+	var commentedUponRef string
+	if len(args) == 1 {
+		commentedUponRef = args[0]
+	} else {
+		commentedUponRef = "HEAD"
+	}
+	commentedUponCommit, err := repo.ResolveRefCommit(commentedUponRef)
+	if err != nil {
+		return fmt.Errorf("Failed to resolve the comment location: %v\n", err)
+	}
+
+	commentThreads, err := review.GetDetachedComments(repo, *commentFile)
+	if err != nil {
+		return err
+	}
+	if err := validateArgs(repo, args, commentThreads); err != nil {
+		return err
+	}
+
+	c, err := buildCommentFromFlags(repo, commentedUponCommit)
+	if err != nil {
+		return err
+	}
+	return review.AddDetachedComment(repo, c)
 }
 
 // commentCmd defines the "comment" subcommand.
@@ -160,6 +209,11 @@ var commentCmd = &Command{
 		commentFlagSet.PrintDefaults()
 	},
 	RunMethod: func(repo repository.Repo, args []string) error {
+		commentFlagSet.Parse(args)
+		args = commentFlagSet.Args()
+		if *commentDetached {
+			return commentOnPath(repo, args)
+		}
 		return commentOnReview(repo, args)
 	},
 }

+ 2 - 9
commands/list.go

@@ -20,6 +20,7 @@ import (
 	"encoding/json"
 	"flag"
 	"fmt"
+
 	"github.com/google/git-appraise/commands/output"
 	"github.com/google/git-appraise/repository"
 	"github.com/google/git-appraise/review"
@@ -39,14 +40,8 @@ func listReviews(repo repository.Repo, args []string) error {
 	var reviews []review.Summary
 	if *listAll {
 		reviews = review.ListAll(repo)
-		if !*listJSONOutput {
-			fmt.Printf("Loaded %d reviews:\n", len(reviews))
-		}
 	} else {
 		reviews = review.ListOpen(repo)
-		if !*listJSONOutput {
-			fmt.Printf("Loaded %d open reviews:\n", len(reviews))
-		}
 	}
 	if *listJSONOutput {
 		b, err := json.MarshalIndent(reviews, "", "  ")
@@ -56,9 +51,7 @@ func listReviews(repo repository.Repo, args []string) error {
 		fmt.Println(string(b))
 		return nil
 	}
-	for _, r := range reviews {
-		output.PrintSummary(&r)
-	}
+	output.PrintSummaries(reviews, *listAll)
 	return nil
 }
 

+ 54 - 12
commands/output/output.go

@@ -23,10 +23,20 @@ import (
 	"strings"
 	"time"
 
+	"github.com/google/git-appraise/repository"
 	"github.com/google/git-appraise/review"
 )
 
 const (
+	// Template for printing the summary of a list of reviews.
+	reviewListTemplate = `Loaded %d reviews:
+`
+	// Template for printing the summary of a list of open reviews.
+	openReviewListTemplate = `Loaded %d open reviews:
+`
+	// Template for printing the summary of a list of comment threads.
+	commentListTemplate = `Loaded %d comment threads:
+`
 	// Template for printing the summary of a code review.
 	reviewSummaryTemplate = `[%s] %.12s
   %s
@@ -77,6 +87,18 @@ func getStatusString(r *review.Summary) string {
 	return "rejected"
 }
 
+// PrintSummaries prints single-line summaries of a slice of reviews.
+func PrintSummaries(reviews []review.Summary, listAll bool) {
+	if listAll {
+		fmt.Printf(reviewListTemplate, len(reviews))
+	} else {
+		fmt.Printf(openReviewListTemplate, len(reviews))
+	}
+	for _, r := range reviews {
+		PrintSummary(&r)
+	}
+}
+
 // PrintSummary prints a single-line summary of a review.
 func PrintSummary(r *review.Summary) {
 	statusString := getStatusString(r)
@@ -99,16 +121,15 @@ func reformatTimestamp(timestamp string) string {
 }
 
 // showThread prints the detailed output for an entire comment thread.
-func showThread(r *review.Review, thread review.CommentThread) error {
+func showThread(repo repository.Repo, thread review.CommentThread, indent string) error {
 	comment := thread.Comment
-	indent := "    "
 	if comment.Location != nil && comment.Location.Path != "" && comment.Location.Range != nil && comment.Location.Range.StartLine > 0 {
-		contents, err := r.Repo.Show(comment.Location.Commit, comment.Location.Path)
+		contents, err := repo.Show(comment.Location.Commit, comment.Location.Path)
 		if err != nil {
 			return err
 		}
 		lines := strings.Split(contents, "\n")
-		err = comment.Location.Check(r.Repo)
+		err = comment.Location.Check(repo)
 		if err != nil {
 			return err
 		}
@@ -136,11 +157,11 @@ func showThread(r *review.Review, thread review.CommentThread) error {
 			fmt.Println(indent + "|" + strings.Join(lines[firstLine-1:lastLine], "\n"+indent+"|"))
 		}
 	}
-	return showSubThread(r, thread, indent)
+	return showSubThread(repo, thread, indent)
 }
 
 // showSubThread prints the given comment (sub)thread, indented by the given prefix string.
-func showSubThread(r *review.Review, thread review.CommentThread, indent string) error {
+func showSubThread(repo repository.Repo, thread review.CommentThread, indent string) error {
 	statusString := "fyi"
 	if thread.Resolved != nil {
 		if *thread.Resolved {
@@ -157,7 +178,7 @@ func showSubThread(r *review.Review, thread review.CommentThread, indent string)
 	indentedSummary := strings.Replace(commentSummary, "\n", "\n"+indent, -1)
 	fmt.Println(indentedSummary)
 	for _, child := range thread.Children {
-		err := showSubThread(r, child, indent)
+		err := showSubThread(repo, child, indent)
 		if err != nil {
 			return err
 		}
@@ -170,11 +191,10 @@ func printAnalyses(r *review.Review) {
 	fmt.Println("  analyses: ", r.GetAnalysesMessage())
 }
 
-// printComments prints all of the comments for the review, with snippets of the preceding source code.
-func printComments(r *review.Review) error {
-	fmt.Printf(commentSummaryTemplate, len(r.Comments))
-	for _, thread := range r.Comments {
-		err := showThread(r, thread)
+// printCommentsWithIndent prints all of the comment threads with the given indent before each line.
+func printCommentsWithIndent(repo repository.Repo, c []review.CommentThread, indent string) error {
+	for _, thread := range c {
+		err := showThread(repo, thread, indent)
 		if err != nil {
 			return err
 		}
@@ -182,6 +202,18 @@ func printComments(r *review.Review) error {
 	return nil
 }
 
+// PrintComments prints all of the given comment threads.
+func PrintComments(repo repository.Repo, c []review.CommentThread) error {
+	fmt.Printf(commentListTemplate, len(c))
+	return printCommentsWithIndent(repo, c, "  ")
+}
+
+// printComments prints all of the comments for the review, with snippets of the preceding source code.
+func printComments(r *review.Review) error {
+	fmt.Printf(commentSummaryTemplate, len(r.Comments))
+	return printCommentsWithIndent(r.Repo, r.Comments, "    ")
+}
+
 // PrintDetails prints a multi-line overview of a review, including all comments.
 func PrintDetails(r *review.Review) error {
 	PrintSummary(r.Summary)
@@ -195,6 +227,16 @@ func PrintDetails(r *review.Review) error {
 	return nil
 }
 
+// PrintCommentsJSON pretty prints the given review in JSON format.
+func PrintCommentsJSON(c []review.CommentThread) error {
+	json, err := review.GetCommentsJSON(c)
+	if err != nil {
+		return err
+	}
+	fmt.Println(json)
+	return nil
+}
+
 // PrintJSON pretty prints the given review in JSON format.
 func PrintJSON(r *review.Review) error {
 	json, err := r.GetJSON()

+ 29 - 3
commands/show.go

@@ -20,24 +20,45 @@ import (
 	"errors"
 	"flag"
 	"fmt"
+	"strings"
+
 	"github.com/google/git-appraise/commands/output"
 	"github.com/google/git-appraise/repository"
 	"github.com/google/git-appraise/review"
-	"strings"
 )
 
 var showFlagSet = flag.NewFlagSet("show", flag.ExitOnError)
 
 var (
+	showDetached    = showFlagSet.Bool("d", false, "Show the detached comments for the given path")
 	showJSONOutput  = showFlagSet.Bool("json", false, "Format the output as JSON")
 	showDiffOutput  = showFlagSet.Bool("diff", false, "Show the current diff for the review")
 	showDiffOptions = showFlagSet.String("diff-opts", "", "Options to pass to the diff tool; can only be used with the --diff option")
 )
 
+// showDetachedComments prints the current code review.
+func showDetachedComments(repo repository.Repo, args []string) error {
+	if *showDiffOptions != "" || *showDiffOutput {
+		return errors.New("The --diff and --diff-opts flags can not be combined with the -d flag.")
+	}
+	if len(args) > 1 {
+		return errors.New("Only showing comments for a single path is supported.")
+	} else if len(args) == 0 {
+		return errors.New("You must specify a path whose comments are to be shown.")
+	}
+	path := args[0]
+	comments, err := review.GetDetachedComments(repo, path)
+	if err != nil {
+		return fmt.Errorf("Failed to load the comments for %q: %v\n", path, err)
+	}
+	if *showJSONOutput {
+		return output.PrintCommentsJSON(comments)
+	}
+	return output.PrintComments(repo, comments)
+}
+
 // showReview prints the current code review.
 func showReview(repo repository.Repo, args []string) error {
-	showFlagSet.Parse(args)
-	args = showFlagSet.Args()
 	if *showDiffOptions != "" && !*showDiffOutput {
 		return errors.New("The --diff-opts flag can only be used if the --diff flag is set.")
 	}
@@ -80,6 +101,11 @@ var showCmd = &Command{
 		showFlagSet.PrintDefaults()
 	},
 	RunMethod: func(repo repository.Repo, args []string) error {
+		showFlagSet.Parse(args)
+		args = showFlagSet.Args()
+		if *showDetached {
+			return showDetachedComments(repo, args)
+		}
 		return showReview(repo, args)
 	},
 }

+ 6 - 0
repository/git.go

@@ -410,6 +410,12 @@ func (repo *GitRepo) ArchiveRef(ref, archive string) error {
 	if err != nil {
 		archiveHash = ""
 	} else {
+		if isAncestor, err := repo.IsAncestor(refHash, archiveHash); err != nil {
+			return err
+		} else if isAncestor {
+			// The ref has already been archived, so we have nothing to do
+			return nil
+		}
 		commitTreeArgs = append(commitTreeArgs, "-p", archiveHash)
 	}
 	commitTreeArgs = append(commitTreeArgs, "-p", refHash, "-m", fmt.Sprintf("Archive %s", refHash), refDetails.Tree)

+ 70 - 8
review/review.go

@@ -33,6 +33,8 @@ import (
 
 const archiveRef = "refs/devtools/archives/reviews"
 
+var emptyTree = repository.NewTree(map[string]repository.TreeChild{})
+
 // CommentThread represents the tree-based hierarchy of comments.
 //
 // The Resolved field represents the aggregate status of the entire thread. If
@@ -257,11 +259,13 @@ func buildCommentThreads(commentsByHash map[string]comment.Comment) []CommentThr
 	return threads
 }
 
-// loadComments reads in the log-structured sequence of comments for a review,
+// getCommentsFromNotes parses the log-structured sequence of comments for a commit,
 // and then builds the corresponding tree-structured comment threads.
-func (r *Summary) loadComments(commentNotes []repository.Note) []CommentThread {
+func getCommentsFromNotes(repo repository.Repo, revision string, commentNotes []repository.Note) ([]CommentThread, *bool) {
 	commentsByHash := comment.ParseAllValid(commentNotes)
-	return buildCommentThreads(commentsByHash)
+	comments := buildCommentThreads(commentsByHash)
+	resolved := updateThreadsStatus(comments)
+	return comments, resolved
 }
 
 func getSummaryFromNotes(repo repository.Repo, revision string, requestNotes, commentNotes []repository.Note) (*Summary, error) {
@@ -276,18 +280,23 @@ func getSummaryFromNotes(repo repository.Repo, revision string, requestNotes, co
 		Request:     requests[len(requests)-1],
 		AllRequests: requests,
 	}
-	reviewSummary.Comments = reviewSummary.loadComments(commentNotes)
-	reviewSummary.Resolved = updateThreadsStatus(reviewSummary.Comments)
+	comments, resolved := getCommentsFromNotes(repo, revision, commentNotes)
+	reviewSummary.Comments = comments
+	reviewSummary.Resolved = resolved
 	return &reviewSummary, nil
 }
 
+func GetComments(repo repository.Repo, revision string) ([]CommentThread, error) {
+	commentNotes := repo.GetNotes(comment.Ref, revision)
+	c, _ := getCommentsFromNotes(repo, revision, commentNotes)
+	return c, nil
+}
+
 // GetSummary returns the summary of the code review specified by its revision
 // and the references which contain that reviews summary and comments.
 //
 // If no review request exists, the returned review summary is nil.
-func GetSummaryViaRefs(repo repository.Repo, requestRef, commentRef,
-	revision string) (*Summary, error) {
-
+func GetSummaryViaRefs(repo repository.Repo, requestRef, commentRef, revision string) (*Summary, error) {
 	if err := repo.VerifyCommit(revision); err != nil {
 		return nil, fmt.Errorf("Could not find a commit named %q", revision)
 	}
@@ -522,6 +531,15 @@ func prettyPrintJSON(jsonBytes []byte) (string, error) {
 	return prettyBytes.String(), nil
 }
 
+// GetCommentsJSON returns the pretty printed JSON for a slice of comment threads.
+func GetCommentsJSON(cs []CommentThread) (string, error) {
+	jsonBytes, err := json.Marshal(cs)
+	if err != nil {
+		return "", err
+	}
+	return prettyPrintJSON(jsonBytes)
+}
+
 // GetJSON returns the pretty printed JSON for a review summary.
 func (r *Summary) GetJSON() (string, error) {
 	jsonBytes, err := json.Marshal(*r)
@@ -770,3 +788,47 @@ func (r *Review) RebaseAndSign(archivePrevious bool) error {
 	}
 	return r.Repo.AppendNote(request.Ref, r.Revision, newNote)
 }
+
+func wellKnownCommitForPath(repo repository.Repo, path string, archive bool) (string, error) {
+	commitDetails := &repository.CommitDetails{
+		Author:         "nobody",
+		AuthorEmail:    "nobody",
+		AuthorTime:     "100000000 +0000",
+		Committer:      "nobody",
+		CommitterEmail: "nobody",
+		Time:           "100000000 +0000",
+		Summary:        path,
+	}
+	commitHash, err := repo.CreateCommitWithTree(commitDetails, emptyTree)
+	if err != nil {
+		return "", err
+	}
+	if !archive {
+		return commitHash, nil
+	}
+	if err := repo.ArchiveRef(commitHash, archiveRef); err != nil {
+		return "", err
+	}
+	return commitHash, nil
+}
+
+func AddDetachedComment(repo repository.Repo, c *comment.Comment) error {
+	path := c.Location.Path
+	wellKnownCommit, err := wellKnownCommitForPath(repo, path, true)
+	if err != nil {
+		return fmt.Errorf("Failure finding the well-known commit for detached comments on %q: %v", path, err)
+	}
+	commentNote, err := c.Write()
+	if err != nil {
+		return err
+	}
+	return repo.AppendNote(comment.Ref, wellKnownCommit, commentNote)
+}
+
+func GetDetachedComments(repo repository.Repo, path string) ([]CommentThread, error) {
+	wellKnownCommit, err := wellKnownCommitForPath(repo, path, false)
+	if err != nil {
+		return nil, fmt.Errorf("Failure finding the well-known commit for detached comments on %q: %v", path, err)
+	}
+	return GetComments(repo, wellKnownCommit)
+}