Skip to content

Commit

Permalink
Fixed a bug in reading reviews where we would not discount commented-…
Browse files Browse the repository at this point in the history
…upon commits that had been garbage collected, and as a result the tool would exit due to a git command failing unexpectedly
  • Loading branch information
ojarjur committed Nov 20, 2015
1 parent cf28570 commit dcfe4f8
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 0 deletions.
13 changes: 13 additions & 0 deletions repository/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,19 @@ func (repo *GitRepo) HasUncommittedChanges() bool {
return false
}

// VerifyCommit verifies that the supplied hash points to a known commit.
func (repo *GitRepo) VerifyCommit(hash string) error {
out, err := repo.runGitCommand("cat-file", "-t", hash)
if err != nil {
return err
}
objectType := strings.TrimSpace(string(out))
if objectType != "commit" {
return fmt.Errorf("Hash %q points to a non-commit object of type %q", hash, objectType)
}
return nil
}

// VerifyGitRef verifies that the supplied ref points to a known commit.
func (repo *GitRepo) VerifyGitRef(ref string) error {
_, err := repo.runGitCommand("show-ref", "--verify", ref)
Expand Down
8 changes: 8 additions & 0 deletions repository/mock_repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,14 @@ func (r mockRepoForTest) resolveLocalRef(ref string) (string, error) {
return "", fmt.Errorf("The ref %q does not exist", ref)
}

// VerifyCommit verifies that the supplied hash points to a known commit.
func (r mockRepoForTest) VerifyCommit(hash string) error {
if _, ok := r.Commits[hash]; !ok {
return fmt.Errorf("The given hash %q is not a known commit", hash)
}
return nil
}

// VerifyGitRef verifies that the supplied ref points to a known commit.
func (r mockRepoForTest) VerifyGitRef(ref string) error {
_, err := r.resolveLocalRef(ref)
Expand Down
3 changes: 3 additions & 0 deletions repository/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ type Repo interface {
// HasUncommittedChanges returns true if there are local, uncommitted changes.
HasUncommittedChanges() bool

// VerifyCommit verifies that the supplied hash points to a known commit.
VerifyCommit(hash string) error

// VerifyGitRef verifies that the supplied ref points to a known commit.
VerifyGitRef(ref string) error

Expand Down
3 changes: 3 additions & 0 deletions review/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,9 @@ func (r *Review) GetJson() (string, error) {
// and all of the commits that are referenced in the given comment threads.
func (r *Review) findLastCommit(latestCommit string, commentThreads []CommentThread) string {
isLater := func(commit string) bool {
if err := r.Repo.VerifyCommit(commit); err != nil {
return false
}
if r.Repo.IsAncestor(latestCommit, commit) {
return true
}
Expand Down

0 comments on commit dcfe4f8

Please sign in to comment.