Repositories

Previously, we treated some errors as fatal if they were unrecoverable, and used log.Fatal(...) to kill the program.

That old behavior was desirable when the code was simply being used by the command-line tool, as it allowed us to catch unexpected errors very easily. However, now that this code is being reused as a library, we should let the caller decide how to treat unrecoverable errors.

Modified commands/accept.go

@@ -43,7 +43,7 @@ func acceptReview(repo repository.Repo, args []string) error {
}
if len(args) == 1 {
r = review.Get(repo, args[0])
r, err = review.Get(repo, args[0])
} else {
r, err = review.GetCurrent(repo)
}
@@ -63,7 +63,11 @@ func acceptReview(repo repository.Repo, args []string) error {
Commit: acceptedCommit,
}
resolved := true
c := comment.New(repo.GetUserEmail(), *acceptMessage)
userEmail, err := repo.GetUserEmail()
if err != nil {
return err
}
c := comment.New(userEmail, *acceptMessage)
c.Location = &location
c.Resolved = &resolved
return r.AddComment(c)

Modified commands/comment.go

@@ -54,7 +54,7 @@ func commentOnReview(repo repository.Repo, args []string) error {
}
if len(args) == 1 {
r = review.Get(repo, args[0])
r, err = review.Get(repo, args[0])
} else {
r, err = review.GetCurrent(repo)
}
@@ -82,7 +82,11 @@ func commentOnReview(repo repository.Repo, args []string) error {
}
}
c := comment.New(repo.GetUserEmail(), *commentMessage)
userEmail, err := repo.GetUserEmail()
if err != nil {
return err
}
c := comment.New(userEmail, *commentMessage)
c.Location = &location
c.Parent = *commentParent
if *commentLgtm || *commentNmw {

Modified commands/request.go

@@ -65,26 +65,53 @@ func requestReview(repo repository.Repo, args []string) error {
if !*requestAllowUncommitted {
// Requesting a code review with uncommited local changes is usually a mistake, so
// we want to report that to the user instead of creating the request.
if repo.HasUncommittedChanges() {
hasUncommitted, err := repo.HasUncommittedChanges()
if err != nil {
return err
}
if hasUncommitted {
return errors.New("You have uncommitted or untracked files. Use --allow-uncommitted to ignore those.")
}
}
r := buildRequestFromFlags(repo.GetUserEmail())
userEmail, err := repo.GetUserEmail()
if err != nil {
return err
}
r := buildRequestFromFlags(userEmail)
if r.ReviewRef == "HEAD" {
r.ReviewRef = repo.GetHeadRef()
headRef, err := repo.GetHeadRef()
if err != nil {
return err
}
r.ReviewRef = headRef
}
if err := repo.VerifyGitRef(r.TargetRef); err != nil {
return err
}
if err := repo.VerifyGitRef(r.ReviewRef); err != nil {
return err
}
base, err := repo.GetCommitHash(r.TargetRef)
if err != nil {
return err
}
repo.VerifyGitRefOrDie(r.TargetRef)
repo.VerifyGitRefOrDie(r.ReviewRef)
r.BaseCommit = repo.GetCommitHash(r.TargetRef)
r.BaseCommit = base
reviewCommits := repo.ListCommitsBetween(r.TargetRef, r.ReviewRef)
reviewCommits, err := repo.ListCommitsBetween(r.TargetRef, r.ReviewRef)
if err != nil {
return err
}
if reviewCommits == nil {
return errors.New("There are no commits included in the review request")
}
if r.Description == "" {
r.Description = repo.GetCommitMessage(reviewCommits[0])
description, err := repo.GetCommitMessage(reviewCommits[0])
if err != nil {
return err
}
r.Description = description
}
note, err := r.Write()

Modified commands/show.go

@@ -46,7 +46,7 @@ func showReview(repo repository.Repo, args []string) error {
}
if len(args) == 1 {
r = review.Get(repo, args[0])
r, err = review.Get(repo, args[0])
} else {
r, err = review.GetCurrent(repo)
}

Modified commands/submit.go

@@ -56,22 +56,31 @@ func submitReview(repo repository.Repo, args []string) error {
target := r.Request.TargetRef
source := r.Request.ReviewRef
repo.VerifyGitRefOrDie(target)
repo.VerifyGitRefOrDie(source)
if err := repo.VerifyGitRef(target); err != nil {
return err
}
if err := repo.VerifyGitRef(source); err != nil {
return err
}
if !repo.IsAncestor(target, source) {
isAncestor, err := repo.IsAncestor(target, source)
if err != nil {
return err
}
if !isAncestor {
return errors.New("Refusing to submit a non-fast-forward review. First merge the target ref.")
}
repo.SwitchToRef(target)
if err := repo.SwitchToRef(target); err != nil {
return err
}
if *submitMerge {
repo.MergeRef(source, false)
return repo.MergeRef(source, false)
} else if *submitRebase {
repo.RebaseRef(source)
return repo.RebaseRef(source)
} else {
repo.MergeRef(source, true)
return repo.MergeRef(source, true)
}
return nil
}
// submitCmd defines the "submit" subcommand.

Modified repository/git.go

@@ -21,7 +21,6 @@ import (
"crypto/sha1"
"encoding/json"
"fmt"
"log"
"os"
"os/exec"
"strings"
@@ -52,25 +51,6 @@ func (repo *GitRepo) runGitCommandInline(args ...string) error {
return cmd.Run()
}
// Run the given git command using the same stdin, stdout, and stderr as the review tool.
func (repo *GitRepo) runGitCommandInlineOrDie(args ...string) {
err := repo.runGitCommandInline(args...)
if err != nil {
log.Print("git", args)
log.Fatal(err)
}
}
// Run the given git command and return its stdout.
func (repo *GitRepo) runGitCommandOrDie(args ...string) string {
out, err := repo.runGitCommand(args...)
if err != nil {
log.Print("git", args)
log.Fatal(out)
}
return out
}
// NewGitRepo determines if the given working directory is inside of a git repository,
// and returns the corresponding GitRepo instance if it is.
func NewGitRepo(path string) (*GitRepo, error) {
@@ -82,7 +62,6 @@ func NewGitRepo(path string) (*GitRepo, error) {
if _, ok := err.(*exec.ExitError); ok {
return nil, err
}
log.Fatal(err)
return nil, err
}
@@ -92,23 +71,26 @@ func (repo *GitRepo) GetPath() string {
}
// GetRepoStateHash returns a hash which embodies the entire current state of a repository.
func (repo *GitRepo) GetRepoStateHash() string {
stateSummary := repo.runGitCommandOrDie("show-ref")
return fmt.Sprintf("%x", sha1.Sum([]byte(stateSummary)))
func (repo *GitRepo) GetRepoStateHash() (string, error) {
stateSummary, error := repo.runGitCommand("show-ref")
return fmt.Sprintf("%x", sha1.Sum([]byte(stateSummary))), error
}
// GetUserEmail returns the email address that the user has used to configure git.
func (repo *GitRepo) GetUserEmail() string {
return repo.runGitCommandOrDie("config", "user.email")
func (repo *GitRepo) GetUserEmail() (string, error) {
return repo.runGitCommand("config", "user.email")
}
// HasUncommittedChanges returns true if there are local, uncommitted changes.
func (repo *GitRepo) HasUncommittedChanges() bool {
out := repo.runGitCommandOrDie("status", "--porcelain")
func (repo *GitRepo) HasUncommittedChanges() (bool, error) {
out, err := repo.runGitCommand("status", "--porcelain")
if err != nil {
return false, err
}
if len(out) > 0 {
return true
return true, nil
}
return false
return false, nil
}
// VerifyCommit verifies that the supplied hash points to a known commit.
@@ -130,19 +112,14 @@ func (repo *GitRepo) VerifyGitRef(ref string) error {
return err
}
// VerifyGitRefOrDie verifies that the supplied ref points to a known commit.
func (repo *GitRepo) VerifyGitRefOrDie(ref string) {
repo.runGitCommandOrDie("show-ref", "--verify", ref)
}
// GetHeadRef returns the ref that is the current HEAD.
func (repo *GitRepo) GetHeadRef() string {
return repo.runGitCommandOrDie("symbolic-ref", "HEAD")
func (repo *GitRepo) GetHeadRef() (string, error) {
return repo.runGitCommand("symbolic-ref", "HEAD")
}
// GetCommitHash returns the hash of the commit pointed to by the given ref.
func (repo *GitRepo) GetCommitHash(ref string) string {
return repo.runGitCommandOrDie("show", "-s", "--format=%H", ref)
func (repo *GitRepo) GetCommitHash(ref string) (string, error) {
return repo.runGitCommand("show", "-s", "--format=%H", ref)
}
// ResolveRefCommit returns the commit pointed to by the given ref, which may be a remote ref.
@@ -156,16 +133,19 @@ func (repo *GitRepo) GetCommitHash(ref string) string {
// performed by the reviewee.
func (repo *GitRepo) ResolveRefCommit(ref string) (string, error) {
if err := repo.VerifyGitRef(ref); err == nil {
return repo.GetCommitHash(ref), nil
return repo.GetCommitHash(ref)
}
if strings.HasPrefix(ref, "refs/heads/") {
// The ref is a branch. Check if it exists in exactly one remote
pattern := strings.Replace(ref, "refs/heads", "**", 1)
matchingOutput := repo.runGitCommandOrDie("for-each-ref", "--format=%(refname)", pattern)
matchingOutput, err := repo.runGitCommand("for-each-ref", "--format=%(refname)", pattern)
if err != nil {
return "", err
}
matchingRefs := strings.Split(matchingOutput, "\n")
if len(matchingRefs) == 1 && matchingRefs[0] != "" {
// There is exactly one match
return repo.GetCommitHash(matchingRefs[0]), nil
return repo.GetCommitHash(matchingRefs[0])
}
return "", fmt.Errorf("Unable to find a git ref matching the pattern %q", pattern)
}
@@ -173,13 +153,13 @@ func (repo *GitRepo) ResolveRefCommit(ref string) (string, error) {
}
// GetCommitMessage returns the message stored in the commit pointed to by the given ref.
func (repo *GitRepo) GetCommitMessage(ref string) string {
return repo.runGitCommandOrDie("show", "-s", "--format=%B", ref)
func (repo *GitRepo) GetCommitMessage(ref string) (string, error) {
return repo.runGitCommand("show", "-s", "--format=%B", ref)
}
// GetCommitTime returns the commit time of the commit pointed to by the given ref.
func (repo *GitRepo) GetCommitTime(ref string) string {
return repo.runGitCommandOrDie("show", "-s", "--format=%ct", ref)
func (repo *GitRepo) GetCommitTime(ref string) (string, error) {
return repo.runGitCommand("show", "-s", "--format=%ct", ref)
}
// GetLastParent returns the last parent of the given commit (as ordered by git).
@@ -220,29 +200,28 @@ func (repo GitRepo) GetCommitDetails(ref string) (*CommitDetails, error) {
}
// MergeBase determines if the first commit that is an ancestor of the two arguments.
func (repo *GitRepo) MergeBase(a, b string) string {
return repo.runGitCommandOrDie("merge-base", a, b)
func (repo *GitRepo) MergeBase(a, b string) (string, error) {
return repo.runGitCommand("merge-base", a, b)
}
// IsAncestor determines if the first argument points to a commit that is an ancestor of the second.
func (repo *GitRepo) IsAncestor(ancestor, descendant string) bool {
func (repo *GitRepo) IsAncestor(ancestor, descendant string) (bool, error) {
_, err := repo.runGitCommand("merge-base", "--is-ancestor", ancestor, descendant)
if err == nil {
return true
return true, nil
}
if _, ok := err.(*exec.ExitError); ok {
return false
return false, nil
}
log.Fatal(err)
return false
return false, fmt.Errorf("Error while trying to determine commit ancestry: %v", err)
}
// Diff computes the diff between two given commits.
func (repo *GitRepo) Diff(left, right string, diffArgs ...string) string {
func (repo *GitRepo) Diff(left, right string, diffArgs ...string) (string, error) {
args := []string{"diff"}
args = append(args, diffArgs...)
args = append(args, fmt.Sprintf("%s..%s", left, right))
return repo.runGitCommandOrDie(args...)
return repo.runGitCommand(args...)
}
// Show returns the contents of the given file at the given commit.
@@ -251,20 +230,21 @@ func (repo *GitRepo) Show(commit, path string) (string, error) {
}
// SwitchToRef changes the currently-checked-out ref.
func (repo *GitRepo) SwitchToRef(ref string) {
func (repo *GitRepo) SwitchToRef(ref string) error {
// If the ref starts with "refs/heads/", then we have to trim that prefix,
// or else we will wind up in a detached HEAD state.
if strings.HasPrefix(ref, branchRefPrefix) {
ref = ref[len(branchRefPrefix):]
}
repo.runGitCommandOrDie("checkout", ref)
_, err := repo.runGitCommand("checkout", ref)
return err
}
// MergeRef merges the given ref into the current one.
//
// The ref argument is the ref to merge, and fastForward indicates that the
// current ref should only move forward, as opposed to creating a bubble merge.
func (repo *GitRepo) MergeRef(ref string, fastForward bool) {
func (repo *GitRepo) MergeRef(ref string, fastForward bool) error {
args := []string{"merge"}
if fastForward {
args = append(args, "--ff", "--ff-only")
@@ -272,12 +252,12 @@ func (repo *GitRepo) MergeRef(ref string, fastForward bool) {
args = append(args, "--no-ff")
}
args = append(args, ref)
repo.runGitCommandInlineOrDie(args...)
return repo.runGitCommandInline(args...)
}
// RebaseRef rebases the given ref into the current one.
func (repo *GitRepo) RebaseRef(ref string) {
repo.runGitCommandInlineOrDie("rebase", "-i", ref)
func (repo *GitRepo) RebaseRef(ref string) error {
return repo.runGitCommandInline("rebase", "-i", ref)
}
// ListCommitsBetween returns the list of commits between the two given revisions.
@@ -288,12 +268,15 @@ func (repo *GitRepo) RebaseRef(ref string) {
// merge base of the two is used as the starting point.
//
// The generated list is in chronological order (with the oldest commit first).
func (repo *GitRepo) ListCommitsBetween(from, to string) []string {
out := repo.runGitCommandOrDie("rev-list", "--reverse", "--ancestry-path", from+".."+to)
func (repo *GitRepo) ListCommitsBetween(from, to string) ([]string, error) {
out, err := repo.runGitCommand("rev-list", "--reverse", "--ancestry-path", from+".."+to)
if err != nil {
return nil, err
}
if out == "" {
return nil
return nil, nil
}
return strings.Split(out, "\n")
return strings.Split(out, "\n"), nil
}
// GetNotes uses the "git" command-line tool to read the notes from the given ref for a given revision.
@@ -311,14 +294,19 @@ func (repo *GitRepo) GetNotes(notesRef, revision string) []Note {
}
// AppendNote appends a note to a revision under the given ref.
func (repo *GitRepo) AppendNote(notesRef, revision string, note Note) {
repo.runGitCommandOrDie("notes", "--ref", notesRef, "append", "-m", string(note), revision)
func (repo *GitRepo) AppendNote(notesRef, revision string, note Note) error {
_, err := repo.runGitCommand("notes", "--ref", notesRef, "append", "-m", string(note), revision)
return err
}
// ListNotedRevisions returns the collection of revisions that are annotated by notes in the given ref.
func (repo *GitRepo) ListNotedRevisions(notesRef string) []string {
var revisions []string
notesList := strings.Split(repo.runGitCommandOrDie("notes", "--ref", notesRef, "list"), "\n")
notesListOut, err := repo.runGitCommand("notes", "--ref", notesRef, "list")
if err != nil {
return nil
}
notesList := strings.Split(notesListOut, "\n")
for _, notePair := range notesList {
noteParts := strings.SplitN(notePair, " ", 2)
if len(noteParts) == 2 {
@@ -355,18 +343,28 @@ func getRemoteNotesRef(remote, localNotesRef string) string {
// PullNotes fetches the contents of the given notes ref from a remote repo,
// and then merges them with the corresponding local notes using the
// "cat_sort_uniq" strategy.
func (repo *GitRepo) PullNotes(remote, notesRefPattern string) {
func (repo *GitRepo) PullNotes(remote, notesRefPattern string) error {
remoteNotesRefPattern := getRemoteNotesRef(remote, notesRefPattern)
fetchRefSpec := fmt.Sprintf("+%s:%s", notesRefPattern, remoteNotesRefPattern)
repo.runGitCommandInlineOrDie("fetch", remote, fetchRefSpec)
err := repo.runGitCommandInline("fetch", remote, fetchRefSpec)
if err != nil {
return err
}
remoteRefs := repo.runGitCommandOrDie("ls-remote", remote, notesRefPattern)
remoteRefs, err := repo.runGitCommand("ls-remote", remote, notesRefPattern)
if err != nil {
return err
}
for _, line := range strings.Split(remoteRefs, "\n") {
lineParts := strings.Split(line, "\t")
if len(lineParts) == 2 {
ref := lineParts[1]
remoteRef := getRemoteNotesRef(remote, ref)
repo.runGitCommandOrDie("notes", "--ref", ref, "merge", remoteRef, "-s", "cat_sort_uniq")
_, err := repo.runGitCommand("notes", "--ref", ref, "merge", remoteRef, "-s", "cat_sort_uniq")
if err != nil {
return err
}
}
}
return nil
}

Modified repository/mock_repo.go

@@ -20,7 +20,6 @@ import (
"crypto/sha1"
"encoding/json"
"fmt"
"log"
"strings"
)
@@ -162,19 +161,19 @@ func NewMockRepoForTest() Repo {
func (r mockRepoForTest) GetPath() string { return "~/mockRepo/" }
// GetRepoStateHash returns a hash which embodies the entire current state of a repository.
func (r mockRepoForTest) GetRepoStateHash() string {
func (r mockRepoForTest) GetRepoStateHash() (string, error) {
repoJson, err := json.Marshal(r)
if err != nil {
log.Fatal(err)
return "", err
}
return fmt.Sprintf("%x", sha1.Sum([]byte(repoJson)))
return fmt.Sprintf("%x", sha1.Sum([]byte(repoJson))), nil
}
// GetUserEmail returns the email address that the user has used to configure git.
func (r mockRepoForTest) GetUserEmail() string { return "[email protected]" }
func (r mockRepoForTest) GetUserEmail() (string, error) { return "[email protected]", nil }
// HasUncommittedChanges returns true if there are local, uncommitted changes.
func (r mockRepoForTest) HasUncommittedChanges() bool { return false }
func (r mockRepoForTest) HasUncommittedChanges() (bool, error) { return false, nil }
func (r mockRepoForTest) resolveLocalRef(ref string) (string, error) {
if commit, ok := r.Refs[ref]; ok {
@@ -200,20 +199,16 @@ func (r mockRepoForTest) VerifyGitRef(ref string) error {
return err
}
// VerifyGitRefOrDie verifies that the supplied ref points to a known commit.
func (r mockRepoForTest) VerifyGitRefOrDie(ref string) {
if err := r.VerifyGitRef(ref); err != nil {
log.Fatal(err)
}
}
// GetHeadRef returns the ref that is the current HEAD.
func (r mockRepoForTest) GetHeadRef() string { return r.Head }
func (r mockRepoForTest) GetHeadRef() (string, error) { return r.Head, nil }
// GetCommitHash returns the hash of the commit pointed to by the given ref.
func (r mockRepoForTest) GetCommitHash(ref string) string {
r.VerifyGitRefOrDie(ref)
return r.Refs[ref]
func (r mockRepoForTest) GetCommitHash(ref string) (string, error) {
err := r.VerifyGitRef(ref)
if err != nil {
return "", err
}
return r.Refs[ref], nil
}
// ResolveRefCommit returns the commit pointed to by the given ref, which may be a remote ref.
@@ -237,22 +232,22 @@ func (r mockRepoForTest) getCommit(ref string) (mockCommit, error) {
return r.Commits[commit], err
}
func (r mockRepoForTest) getCommitOrDie(ref string) mockCommit {
// GetCommitMessage returns the message stored in the commit pointed to by the given ref.
func (r mockRepoForTest) GetCommitMessage(ref string) (string, error) {
commit, err := r.getCommit(ref)
if err != nil {
log.Fatal(err)
return "", err
}
return commit
}
// GetCommitMessage returns the message stored in the commit pointed to by the given ref.
func (r mockRepoForTest) GetCommitMessage(ref string) string {
return r.getCommitOrDie(ref).Message
return commit.Message, nil
}
// GetCommitTime returns the commit time of the commit pointed to by the given ref.
func (r mockRepoForTest) GetCommitTime(ref string) string {
return r.getCommitOrDie(ref).Time
func (r mockRepoForTest) GetCommitTime(ref string) (string, error) {
commit, err := r.getCommit(ref)
if err != nil {
return "", err
}
return commit.Time, nil
}
// GetLastParent returns the last parent of the given commit (as ordered by git).
@@ -280,47 +275,59 @@ func (r mockRepoForTest) GetCommitDetails(ref string) (*CommitDetails, error) {
}
// ancestors returns the breadth-first traversal of a commit's ancestors
func (r mockRepoForTest) ancestors(commit string) []string {
func (r mockRepoForTest) ancestors(commit string) ([]string, error) {
queue := []string{commit}
var ancestors []string
for queue != nil {
var nextQueue []string
for _, c := range queue {
parents := r.getCommitOrDie(c).Parents
commit, err := r.getCommit(c)
if err != nil {
return nil, err
}
parents := commit.Parents
nextQueue = append(nextQueue, parents...)
ancestors = append(ancestors, parents...)
}
queue = nextQueue
}
return ancestors
return ancestors, nil
}
// IsAncestor determines if the first argument points to a commit that is an ancestor of the second.
func (r mockRepoForTest) IsAncestor(ancestor, descendant string) bool {
func (r mockRepoForTest) IsAncestor(ancestor, descendant string) (bool, error) {
if ancestor == descendant {
return true
return true, nil
}
descendantCommit, err := r.getCommit(descendant)
if err != nil {
return false, err
}
for _, parent := range r.getCommitOrDie(descendant).Parents {
if r.IsAncestor(ancestor, parent) {
return true
for _, parent := range descendantCommit.Parents {
if t, e := r.IsAncestor(ancestor, parent); e == nil && t {
return true, nil
}
}
return false
return false, nil
}
// MergeBase determines if the first commit that is an ancestor of the two arguments.
func (r mockRepoForTest) MergeBase(a, b string) string {
for _, ancestor := range r.ancestors(a) {
if r.IsAncestor(ancestor, b) {
return ancestor
func (r mockRepoForTest) MergeBase(a, b string) (string, error) {
ancestors, err := r.ancestors(a)
if err != nil {
return "", err
}
for _, ancestor := range ancestors {
if t, e := r.IsAncestor(ancestor, b); e == nil && t {
return ancestor, nil
}
}
return ""
return "", nil
}
// Diff computes the diff between two given commits.
func (r mockRepoForTest) Diff(left, right string, diffArgs ...string) string {
return fmt.Sprintf("Diff between %q and %q", left, right)
func (r mockRepoForTest) Diff(left, right string, diffArgs ...string) (string, error) {
return fmt.Sprintf("Diff between %q and %q", left, right), nil
}
// Show returns the contents of the given file at the given commit.
@@ -329,18 +336,19 @@ func (r mockRepoForTest) Show(commit, path string) (string, error) {
}
// SwitchToRef changes the currently-checked-out ref.
func (r mockRepoForTest) SwitchToRef(ref string) {
func (r mockRepoForTest) SwitchToRef(ref string) error {
r.Head = ref
return nil
}
// MergeRef merges the given ref into the current one.
//
// The ref argument is the ref to merge, and fastForward indicates that the
// current ref should only move forward, as opposed to creating a bubble merge.
func (r mockRepoForTest) MergeRef(ref string, fastForward bool) {}
func (r mockRepoForTest) MergeRef(ref string, fastForward bool) error { return nil }
// RebaseRef rebases the given ref into the current one.
func (r mockRepoForTest) RebaseRef(ref string) {}
func (r mockRepoForTest) RebaseRef(ref string) error { return nil }
// ListCommitsBetween returns the list of commits between the two given revisions.
//
@@ -350,7 +358,7 @@ func (r mockRepoForTest) RebaseRef(ref string) {}
// merge base of the two is used as the starting point.
//
// The generated list is in chronological order (with the oldest commit first).
func (r mockRepoForTest) ListCommitsBetween(from, to string) []string { return nil }
func (r mockRepoForTest) ListCommitsBetween(from, to string) ([]string, error) { return nil, nil }
// GetNotes reads the notes from the given ref that annotate the given revision.
func (r mockRepoForTest) GetNotes(notesRef, revision string) []Note {
@@ -363,10 +371,11 @@ func (r mockRepoForTest) GetNotes(notesRef, revision string) []Note {
}
// AppendNote appends a note to a revision under the given ref.
func (r mockRepoForTest) AppendNote(ref, revision string, note Note) {
func (r mockRepoForTest) AppendNote(ref, revision string, note Note) error {
existingNotes := r.Notes[ref][revision]
newNotes := existingNotes + "\n" + string(note)
r.Notes[ref][revision] = newNotes
return nil
}
// ListNotedRevisions returns the collection of revisions that are annotated by notes in the given ref.
@@ -386,4 +395,4 @@ func (r mockRepoForTest) PushNotes(remote, notesRefPattern string) error { retur
// PullNotes fetches the contents of the given notes ref from a remote repo,
// and then merges them with the corresponding local notes using the
// "cat_sort_uniq" strategy.
func (r mockRepoForTest) PullNotes(remote, notesRefPattern string) {}
func (r mockRepoForTest) PullNotes(remote, notesRefPattern string) error { return nil }

Modified repository/repo.go

@@ -35,13 +35,13 @@ type Repo interface {
GetPath() string
// GetRepoStateHash returns a hash which embodies the entire current state of a repository.
GetRepoStateHash() string
GetRepoStateHash() (string, error)
// GetUserEmail returns the email address that the user has used to configure git.
GetUserEmail() string
GetUserEmail() (string, error)
// HasUncommittedChanges returns true if there are local, uncommitted changes.
HasUncommittedChanges() bool
HasUncommittedChanges() (bool, error)
// VerifyCommit verifies that the supplied hash points to a known commit.
VerifyCommit(hash string) error
@@ -49,14 +49,11 @@ type Repo interface {
// VerifyGitRef verifies that the supplied ref points to a known commit.
VerifyGitRef(ref string) error
// VerifyGitRefOrDie verifies that the supplied ref points to a known commit.
VerifyGitRefOrDie(ref string)
// GetHeadRef returns the ref that is the current HEAD.
GetHeadRef() string
GetHeadRef() (string, error)
// GetCommitHash returns the hash of the commit pointed to by the given ref.
GetCommitHash(ref string) string
GetCommitHash(ref string) (string, error)
// ResolveRefCommit returns the commit pointed to by the given ref, which may be a remote ref.
//
@@ -70,10 +67,10 @@ type Repo interface {
ResolveRefCommit(ref string) (string, error)
// GetCommitMessage returns the message stored in the commit pointed to by the given ref.
GetCommitMessage(ref string) string
GetCommitMessage(ref string) (string, error)
// GetCommitTime returns the commit time of the commit pointed to by the given ref.
GetCommitTime(ref string) string
GetCommitTime(ref string) (string, error)
// GetLastParent returns the last parent of the given commit (as ordered by git).
GetLastParent(ref string) (string, error)
@@ -82,28 +79,28 @@ type Repo interface {
GetCommitDetails(ref string) (*CommitDetails, error)
// MergeBase determines if the first commit that is an ancestor of the two arguments.
MergeBase(a, b string) string
MergeBase(a, b string) (string, error)
// IsAncestor determines if the first argument points to a commit that is an ancestor of the second.
IsAncestor(ancestor, descendant string) bool
IsAncestor(ancestor, descendant string) (bool, error)
// Diff computes the diff between two given commits.
Diff(left, right string, diffArgs ...string) string
Diff(left, right string, diffArgs ...string) (string, error)
// Show returns the contents of the given file at the given commit.
Show(commit, path string) (string, error)
// SwitchToRef changes the currently-checked-out ref.
SwitchToRef(ref string)
SwitchToRef(ref string) error
// MergeRef merges the given ref into the current one.
//
// The ref argument is the ref to merge, and fastForward indicates that the
// current ref should only move forward, as opposed to creating a bubble merge.
MergeRef(ref string, fastForward bool)
MergeRef(ref string, fastForward bool) error
// RebaseRef rebases the given ref into the current one.
RebaseRef(ref string)
RebaseRef(ref string) error
// ListCommitsBetween returns the list of commits between the two given revisions.
//
@@ -113,13 +110,13 @@ type Repo interface {
// merge base of the two is used as the starting point.
//
// The generated list is in chronological order (with the oldest commit first).
ListCommitsBetween(from, to string) []string
ListCommitsBetween(from, to string) ([]string, error)
// GetNotes reads the notes from the given ref that annotate the given revision.
GetNotes(notesRef, revision string) []Note
// AppendNote appends a note to a revision under the given ref.
AppendNote(ref, revision string, note Note)
AppendNote(ref, revision string, note Note) error
// ListNotedRevisions returns the collection of revisions that are annotated by notes in the given ref.
ListNotedRevisions(notesRef string) []string
@@ -130,5 +127,5 @@ type Repo interface {
// PullNotes fetches the contents of the given notes ref from a remote repo,
// and then merges them with the corresponding local notes using the
// "cat_sort_uniq" strategy.
PullNotes(remote, notesRefPattern string)
PullNotes(remote, notesRefPattern string) error
}

Modified review/review.go

@@ -181,11 +181,11 @@ func (r *Review) loadComments() []CommentThread {
// Get returns the specified code review.
//
// If no review request exists, the returned review is nil.
func Get(repo repository.Repo, revision string) *Review {
func Get(repo repository.Repo, revision string) (*Review, error) {
requestNotes := repo.GetNotes(request.Ref, revision)
requests := request.ParseAllValid(requestNotes)
if requests == nil {
return nil
return nil, nil
}
review := Review{
Repo: repo,
@@ -194,21 +194,25 @@ func Get(repo repository.Repo, revision string) *Review {
}
review.Comments = review.loadComments()
review.Resolved = updateThreadsStatus(review.Comments)
review.Submitted = repo.IsAncestor(revision, review.Request.TargetRef)
submitted, err := repo.IsAncestor(revision, review.Request.TargetRef)
if err != nil {
return nil, err
}
review.Submitted = submitted
currentCommit, err := review.GetHeadCommit()
if err == nil {
review.Reports = ci.ParseAllValid(repo.GetNotes(ci.Ref, currentCommit))
review.Analyses = analyses.ParseAllValid(repo.GetNotes(analyses.Ref, currentCommit))
}
return &review
return &review, nil
}
// ListAll returns all reviews stored in the git-notes.
func ListAll(repo repository.Repo) []Review {
var reviews []Review
for _, revision := range repo.ListNotedRevisions(request.Ref) {
review := Get(repo, revision)
if review != nil {
review, err := Get(repo, revision)
if err == nil && review != nil {
reviews = append(reviews, *review)
}
}
@@ -230,7 +234,10 @@ func ListOpen(repo repository.Repo) []Review {
//
// If there are multiple matching reviews, then an error is returned.
func GetCurrent(repo repository.Repo) (*Review, error) {
reviewRef := repo.GetHeadRef()
reviewRef, err := repo.GetHeadRef()
if err != nil {
return nil, err
}
var matchingReviews []Review
for _, review := range ListOpen(repo) {
if review.Request.ReviewRef == reviewRef {
@@ -303,13 +310,21 @@ func (r *Review) findLastCommit(latestCommit string, commentThreads []CommentThr
if err := r.Repo.VerifyCommit(commit); err != nil {
return false
}
if r.Repo.IsAncestor(latestCommit, commit) {
if t, e := r.Repo.IsAncestor(latestCommit, commit); e == nil && t {
return true
}
if r.Repo.IsAncestor(commit, latestCommit) {
if t, e := r.Repo.IsAncestor(commit, latestCommit); e == nil && t {
return false
}
return r.Repo.GetCommitTime(commit) > r.Repo.GetCommitTime(latestCommit)
ct, err := r.Repo.GetCommitTime(commit)
if err != nil {
return false
}
lt, err := r.Repo.GetCommitTime(latestCommit)
if err != nil {
return true
}
return ct > lt
}
updateLatest := func(commit string) {
if commit == "" {
@@ -335,12 +350,7 @@ func (r *Review) GetHeadCommit() (string, error) {
return r.Revision, nil
}
targetRefHead, err := r.Repo.ResolveRefCommit(r.Request.TargetRef)
if err != nil {
return "", err
}
if r.Repo.IsAncestor(r.Revision, targetRefHead) {
if r.Submitted {
// The review has already been submitted.
// Go through the list of comments and find the last commented upon commit.
return r.findLastCommit(r.Revision, r.Comments), nil
@@ -351,13 +361,7 @@ func (r *Review) GetHeadCommit() (string, error) {
// GetBaseCommit returns the commit against which a review should be compared.
func (r *Review) GetBaseCommit() (string, error) {
targetRefHead, err := r.Repo.ResolveRefCommit(r.Request.TargetRef)
if err != nil {
return "", err
}
leftHandSide := targetRefHead
if r.Repo.IsAncestor(r.Revision, leftHandSide) {
if r.Submitted {
if r.Request.BaseCommit != "" {
return r.Request.BaseCommit, nil
}
@@ -370,6 +374,11 @@ func (r *Review) GetBaseCommit() (string, error) {
return r.Repo.GetLastParent(r.Revision)
}
targetRefHead, err := r.Repo.ResolveRefCommit(r.Request.TargetRef)
if err != nil {
return "", err
}
leftHandSide := targetRefHead
rightHandSide := r.Revision
if r.Request.ReviewRef != "" {
if reviewRefHead, err := r.Repo.ResolveRefCommit(r.Request.ReviewRef); err == nil {
@@ -377,7 +386,7 @@ func (r *Review) GetBaseCommit() (string, error) {
}
}
return r.Repo.MergeBase(leftHandSide, rightHandSide), nil
return r.Repo.MergeBase(leftHandSide, rightHandSide)
}
// GetDiff returns the diff for a review.
@@ -388,7 +397,7 @@ func (r *Review) GetDiff(diffArgs ...string) (string, error) {
headCommit, err = r.GetHeadCommit()
}
if err == nil {
return r.Repo.Diff(baseCommit, headCommit, diffArgs...), nil
return r.Repo.Diff(baseCommit, headCommit, diffArgs...)
}
return "", err
}

Modified review/review_test.go

@@ -538,7 +538,10 @@ func TestBuildCommentThreads(t *testing.T) {
func TestGetHeadCommit(t *testing.T) {
repo := repository.NewMockRepoForTest()
submittedSimpleReview := Get(repo, repository.TestCommitB)
submittedSimpleReview, err := Get(repo, repository.TestCommitB)
if err != nil {
t.Fatal(err)
}
submittedSimpleReviewHead, err := submittedSimpleReview.GetHeadCommit()
if err != nil {
t.Fatal("Unable to compute the head commit for a known review of a simple commit: ", err)
@@ -547,7 +550,10 @@ func TestGetHeadCommit(t *testing.T) {
t.Fatal("Unexpected head commit computed for a known review of a simple commit.")
}
submittedModifiedReview := Get(repo, repository.TestCommitD)
submittedModifiedReview, err := Get(repo, repository.TestCommitD)
if err != nil {
t.Fatal(err)
}
submittedModifiedReviewHead, err := submittedModifiedReview.GetHeadCommit()
if err != nil {
t.Fatal("Unable to compute the head commit for a known, multi-commit review: ", err)
@@ -556,7 +562,10 @@ func TestGetHeadCommit(t *testing.T) {
t.Fatal("Unexpected head commit for a known, multi-commit review.")
}
pendingReview := Get(repo, repository.TestCommitG)
pendingReview, err := Get(repo, repository.TestCommitG)
if err != nil {
t.Fatal(err)
}
pendingReviewHead, err := pendingReview.GetHeadCommit()
if err != nil {
t.Fatal("Unable to compute the head commit for a known review of a merge commit: ", err)
@@ -569,7 +578,10 @@ func TestGetHeadCommit(t *testing.T) {
func TestGetBaseCommit(t *testing.T) {
repo := repository.NewMockRepoForTest()
submittedSimpleReview := Get(repo, repository.TestCommitB)
submittedSimpleReview, err := Get(repo, repository.TestCommitB)
if err != nil {
t.Fatal(err)
}
submittedSimpleReviewBase, err := submittedSimpleReview.GetBaseCommit()
if err != nil {
t.Fatal("Unable to compute the base commit for a known review of a simple commit: ", err)
@@ -578,7 +590,10 @@ func TestGetBaseCommit(t *testing.T) {
t.Fatal("Unexpected base commit computed for a known review of a simple commit.")
}
submittedMergeReview := Get(repo, repository.TestCommitD)
submittedMergeReview, err := Get(repo, repository.TestCommitD)
if err != nil {
t.Fatal(err)
}
submittedMergeReviewBase, err := submittedMergeReview.GetBaseCommit()
if err != nil {
t.Fatal("Unable to compute the base commit for a known review of a merge commit: ", err)
@@ -587,7 +602,10 @@ func TestGetBaseCommit(t *testing.T) {
t.Fatal("Unexpected base commit computed for a known review of a merge commit.")
}
pendingReview := Get(repo, repository.TestCommitG)
pendingReview, err := Get(repo, repository.TestCommitG)
if err != nil {
t.Fatal(err)
}
pendingReviewBase, err := pendingReview.GetBaseCommit()
if err != nil {
t.Fatal("Unable to compute the base commit for a known review of a merge commit: ", err)