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

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

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 {

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

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

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.