Browse Source

Add support for detached comments

Omar Jarjur 11 months ago
parent
commit
8ad8f69d5c
7 changed files with 263 additions and 77 deletions
  1. 92 38
      commands/comment.go
  2. 27 12
      commands/output/output.go
  3. 30 3
      commands/show.go
  4. 54 13
      repository/git.go
  5. 3 3
      repository/mock_repo.go
  6. 4 3
      repository/repo.go
  7. 53 5
      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)
 	},
 }

+ 27 - 12
commands/output/output.go

@@ -23,6 +23,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/google/git-appraise/repository"
 	"github.com/google/git-appraise/review"
 )
 
@@ -48,6 +49,9 @@ status: %s
 %s`
 	// Template for displaying the summary of the comment threads for a review
 	commentSummaryTemplate = `  comments (%d threads):
+`
+	// Template for displaying the summary of the detached comment threads for a path
+	detachedCommentSummaryTemplate = `Comments (%d threads):
 `
 	// Number of lines of context to print for inline comments
 	contextLineCount = 5
@@ -99,16 +103,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 +139,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 +160,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 +173,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 +184,19 @@ 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(detachedCommentSummaryTemplate, 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))
+	indent := "    "
+	return printCommentsWithIndent(r.Repo, r.Comments, indent)
+}
+
 // PrintDetails prints a multi-line overview of a review, including all comments.
 func PrintDetails(r *review.Review) error {
 	PrintSummary(r.Summary)

+ 30 - 3
commands/show.go

@@ -20,24 +20,46 @@ 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 errors.New("Not yet implemented")
+		//return output.PrintJSON(r)
+	}
+	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 +102,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)
 	},
 }

+ 54 - 13
repository/git.go

@@ -41,16 +41,22 @@ type GitRepo struct {
 	Path string
 }
 
-// Run the given git command with the given I/O reader/writers, returning an error if it fails.
-func (repo *GitRepo) runGitCommandWithIO(stdin io.Reader, stdout, stderr io.Writer, args ...string) error {
+// Run the given git command with the given I/O reader/writers and environment, returning an error if it fails.
+func (repo *GitRepo) runGitCommandWithIOAndEnv(stdin io.Reader, stdout, stderr io.Writer, env []string, args ...string) error {
 	cmd := exec.Command("git", args...)
 	cmd.Dir = repo.Path
 	cmd.Stdin = stdin
 	cmd.Stdout = stdout
 	cmd.Stderr = stderr
+	cmd.Env = env
 	return cmd.Run()
 }
 
+// Run the given git command with the given I/O reader/writers, returning an error if it fails.
+func (repo *GitRepo) runGitCommandWithIO(stdin io.Reader, stdout, stderr io.Writer, args ...string) error {
+	return repo.runGitCommandWithIOAndEnv(stdin, stdout, stderr, nil, args...)
+}
+
 // Run the given git command and return its stdout, or an error if the command fails.
 func (repo *GitRepo) runGitCommandRaw(args ...string) (string, string, error) {
 	var stdout bytes.Buffer
@@ -71,6 +77,21 @@ func (repo *GitRepo) runGitCommand(args ...string) (string, error) {
 	return stdout, err
 }
 
+// Run the given git command and return its stdout, or an error if the command fails.
+func (repo *GitRepo) runGitCommandWithEnv(env []string, args ...string) (string, error) {
+	var stdout bytes.Buffer
+	var stderr bytes.Buffer
+	err := repo.runGitCommandWithIOAndEnv(nil, &stdout, &stderr, env, args...)
+	if err != nil {
+		stderrStr := strings.TrimSpace(stderr.String())
+		if stderrStr == "" {
+			stderrStr = "Error running git command: " + strings.Join(args, " ")
+		}
+		err = fmt.Errorf(stderrStr)
+	}
+	return strings.TrimSpace(stdout.String()), err
+}
+
 // Run the given git command using the same stdin, stdout, and stderr as the review tool.
 func (repo *GitRepo) runGitCommandInline(args ...string) error {
 	return repo.runGitCommandWithIO(os.Stdin, os.Stdout, os.Stderr, args...)
@@ -596,21 +617,41 @@ func (repo *GitRepo) readTreeWithHash(ref, hash string) (*Tree, error) {
 }
 
 // CreateCommit creates a commit object and returns its hash.
-func (repo *GitRepo) CreateCommit(t *Tree, parents []string, message string) (string, error) {
-	treeHash, err := repo.StoreTree(t)
-	if err != nil {
-		return "", fmt.Errorf("failure storing a tree: %v", err)
+func (repo *GitRepo) CreateCommit(details *CommitDetails) (string, error) {
+	args := []string{"commit-tree", details.Tree, "-m", details.Summary}
+	for _, parent := range details.Parents {
+		args = append(args, "-p", parent)
+	}
+	var env []string
+	if details.Author != "" {
+		env = append(env, fmt.Sprintf("GIT_AUTHOR_NAME=%s", details.Author))
+	}
+	if details.AuthorEmail != "" {
+		env = append(env, fmt.Sprintf("GIT_AUTHOR_EMAIL=%s", details.AuthorEmail))
+	}
+	if details.AuthorTime != "" {
+		env = append(env, fmt.Sprintf("GIT_AUTHOR_DATE=%s", details.AuthorTime))
+	}
+	if details.Committer != "" {
+		env = append(env, fmt.Sprintf("GIT_COMMITTER_NAME=%s", details.Committer))
 	}
-	return repo.CreateCommitFromTreeHash(treeHash, parents, message)
+	if details.CommitterEmail != "" {
+		env = append(env, fmt.Sprintf("GIT_COMMITTER_EMAIL=%s", details.CommitterEmail))
+	}
+	if details.Time != "" {
+		env = append(env, fmt.Sprintf("GIT_COMMITTER_DATE=%s", details.Time))
+	}
+	return repo.runGitCommandWithEnv(env, args...)
 }
 
-// CreateCommitFromTreeHash creates a commit object and returns its hash.
-func (repo *GitRepo) CreateCommitFromTreeHash(treeHash string, parents []string, message string) (string, error) {
-	args := []string{"commit-tree", treeHash, "-m", message}
-	for _, parent := range parents {
-		args = append(args, "-p", parent)
+// CreateCommitWithTree creates a commit object with the given tree and returns its hash.
+func (repo *GitRepo) CreateCommitWithTree(details *CommitDetails, t *Tree) (string, error) {
+	treeHash, err := repo.StoreTree(t)
+	if err != nil {
+		return "", fmt.Errorf("failure storing a tree: %v", err)
 	}
-	return repo.runGitCommand(args...)
+	details.Tree = treeHash
+	return repo.CreateCommit(details)
 }
 
 // SetRef sets the commit pointed to by the specified ref to `newCommitHash`,

+ 3 - 3
repository/mock_repo.go

@@ -540,12 +540,12 @@ func (r *mockRepoForTest) ReadTree(ref string) (*Tree, error) {
 }
 
 // CreateCommit creates a commit object and returns its hash.
-func (r *mockRepoForTest) CreateCommit(t *Tree, parents []string, message string) (string, error) {
+func (r *mockRepoForTest) CreateCommit(details *CommitDetails) (string, error) {
 	return "", fmt.Errorf("not implemented")
 }
 
-// CreateCommitFromTreeHash creates a commit object and returns its hash.
-func (r *mockRepoForTest) CreateCommitFromTreeHash(treeHash string, parents []string, message string) (string, error) {
+// CreateCommitWithTree creates a commit object with the given tree and returns its hash.
+func (r *mockRepoForTest) CreateCommitWithTree(details *CommitDetails, t *Tree) (string, error) {
 	return "", fmt.Errorf("not implemented")
 }
 

+ 4 - 3
repository/repo.go

@@ -34,6 +34,7 @@ func (n Note) Hash() string {
 type CommitDetails struct {
 	Author         string   `json:"author,omitempty"`
 	AuthorEmail    string   `json:"authorEmail,omitempty"`
+	AuthorTime     string   `json:"authorTime,omitempty"`
 	Committer      string   `json:"committer,omitempty"`
 	CommitterEmail string   `json:"committerEmail,omitempty"`
 	Tree           string   `json:"tree,omitempty"`
@@ -234,10 +235,10 @@ type Repo interface {
 	ReadTree(ref string) (*Tree, error)
 
 	// CreateCommit creates a commit object and returns its hash.
-	CreateCommit(t *Tree, parents []string, message string) (string, error)
+	CreateCommit(details *CommitDetails) (string, error)
 
-	// CreateCommitFromTreeHash creates a commit object and returns its hash.
-	CreateCommitFromTreeHash(treeHash string, parents []string, message string) (string, error)
+	// CreateCommitWithTree creates a commit object with the given tree and returns its hash.
+	CreateCommitWithTree(details *CommitDetails, t *Tree) (string, error)
 
 	// SetRef sets the commit pointed to by the specified ref to `newCommitHash`,
 	// iff the ref currently points `previousCommitHash`.

+ 53 - 5
review/review.go

@@ -33,6 +33,8 @@ import (
 
 const archiveRef = "refs/devtools/archives/reviews"
 
+var emptyTree = repository.NewTree()
+
 // CommentThread represents the tree-based hierarchy of comments.
 //
 // The Resolved field represents the aggregate status of the entire thread. If
@@ -264,6 +266,13 @@ func (r *Summary) loadComments(commentNotes []repository.Note) []CommentThread {
 	return buildCommentThreads(commentsByHash)
 }
 
+func getCommentsFromNotes(repo repository.Repo, revision string, commentNotes []repository.Note) ([]CommentThread, *bool) {
+	commentsByHash := comment.ParseAllValid(commentNotes)
+	comments := buildCommentThreads(commentsByHash)
+	resolved := updateThreadsStatus(comments)
+	return comments, resolved
+}
+
 func getSummaryFromNotes(repo repository.Repo, revision string, requestNotes, commentNotes []repository.Note) (*Summary, error) {
 	requests := request.ParseAllValid(requestNotes)
 	if requests == nil {
@@ -276,18 +285,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)
 	}
@@ -770,3 +784,37 @@ func (r *Review) RebaseAndSign(archivePrevious bool) error {
 	}
 	return r.Repo.AppendNote(request.Ref, r.Revision, newNote)
 }
+
+func wellKnownCommitForPath(repo repository.Repo, path string) (string, error) {
+	commitDetails := &repository.CommitDetails{
+		Author:         "nobody",
+		AuthorEmail:    "nobody",
+		AuthorTime:     "100000000 +0000",
+		Committer:      "nobody",
+		CommitterEmail: "nobody",
+		Time:           "100000000 +0000",
+		Summary:        path,
+	}
+	return repo.CreateCommitWithTree(commitDetails, emptyTree)
+}
+
+func AddDetachedComment(repo repository.Repo, c *comment.Comment) error {
+	path := c.Location.Path
+	wellKnownCommit, err := wellKnownCommitForPath(repo, path)
+	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)
+	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)
+}