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)