From d9f03ea9eaa7a36b7d3116a7a48cc88e8dbb164c Mon Sep 17 00:00:00 2001
From: "ojarjur@google.com" <ojarjur@google.com>
Date: Wed, 2 Dec 2015 16:13:12 -0800
Subject: [PATCH] Changed the logic that calls into git to always cascade
 errors upwards.

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.
---
 commands/accept.go      |   8 ++-
 commands/comment.go     |   8 ++-
 commands/request.go     |  43 +++++++++---
 commands/show.go        |   2 +-
 commands/submit.go      |  25 ++++---
 repository/git.go       | 142 ++++++++++++++++++++--------------------
 repository/mock_repo.go | 107 ++++++++++++++++--------------
 repository/repo.go      |  35 +++++-----
 review/review.go        |  59 ++++++++++-------
 review/review_test.go   |  30 +++++++--
 10 files changed, 267 insertions(+), 192 deletions(-)

diff --git a/commands/accept.go b/commands/accept.go
index dd441438..74c06a54 100644
--- a/commands/accept.go
+++ b/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)
diff --git a/commands/comment.go b/commands/comment.go
index a9e56c56..00075f48 100644
--- a/commands/comment.go
+++ b/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 {
diff --git a/commands/request.go b/commands/request.go
index cbb2524b..3ddddf58 100644
--- a/commands/request.go
+++ b/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()
diff --git a/commands/show.go b/commands/show.go
index 8ad60458..130991a7 100644
--- a/commands/show.go
+++ b/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)
 	}
diff --git a/commands/submit.go b/commands/submit.go
index 69773163..150b8456 100644
--- a/commands/submit.go
+++ b/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.
diff --git a/repository/git.go b/repository/git.go
index c17a33ed..0ffb2f4f 100644
--- a/repository/git.go
+++ b/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
 }
diff --git a/repository/mock_repo.go b/repository/mock_repo.go
index 54f7e91d..1bd617c8 100644
--- a/repository/mock_repo.go
+++ b/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 "user@example.com" }
+func (r mockRepoForTest) GetUserEmail() (string, error) { return "user@example.com", 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 }
diff --git a/repository/repo.go b/repository/repo.go
index 24ed2929..641f4e7f 100644
--- a/repository/repo.go
+++ b/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
 }
diff --git a/review/review.go b/review/review.go
index 82a9235d..703d8fcd 100644
--- a/review/review.go
+++ b/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
 }
diff --git a/review/review_test.go b/review/review_test.go
index e0cbdece..5c22d356 100644
--- a/review/review_test.go
+++ b/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)