Repositories

Modified .travis.yml

@@ -1,3 +1,7 @@
sudo: false
language: go
before_install: git config --global user.email "[email protected]"
before_install:
- git config --global user.email "[email protected]"
- git fetch --unshallow
- git fetch origin +refs/notes/devtools/*:refs/notes/devtools/*
- git fetch origin +refs/heads/master:refs/heads/master

Modified README.md

@@ -114,6 +114,9 @@ schema.
null,
0
]
},
"baseCommit": {
"type": "string"
}
},
"required": [

Modified commands/request.go

@@ -76,6 +76,7 @@ func requestReview(args []string) error {
}
repository.VerifyGitRefOrDie(r.TargetRef)
repository.VerifyGitRefOrDie(r.ReviewRef)
r.BaseCommit = repository.GetCommitHash(r.TargetRef)
reviewCommits := repository.ListCommitsBetween(r.TargetRef, r.ReviewRef)
if reviewCommits == nil {

Modified commands/show.go

@@ -25,6 +25,7 @@ import (
var showFlagSet = flag.NewFlagSet("show", flag.ExitOnError)
var showJsonOutput = showFlagSet.Bool("json", false, "Format the output as JSON")
var showDiffOutput = showFlagSet.Bool("diff", false, "Show the current diff for the review")
// showReview prints the current code review.
func showReview(args []string) error {
@@ -52,6 +53,9 @@ func showReview(args []string) error {
if *showJsonOutput {
return r.PrintJson()
}
if *showDiffOutput {
return r.PrintDiff()
}
return r.PrintDetails()
}

Modified git-appraise/git-appraise.go

@@ -27,10 +27,10 @@ package main
import (
"fmt"
"os"
"sort"
"github.com/google/git-appraise/commands"
"github.com/google/git-appraise/repository"
"os"
"sort"
"strings"
)

Modified repository/git.go

@@ -99,6 +99,12 @@ func HasUncommittedChanges() bool {
return false
}
// VerifyGitRef verifies that the supplied ref points to a known commit.
func VerifyGitRef(ref string) error {
_, err := runGitCommand("show-ref", "--verify", ref)
return err
}
// VerifyGitRefOrDie verifies that the supplied ref points to a known commit.
func VerifyGitRefOrDie(ref string) {
runGitCommandOrDie("show-ref", "--verify", ref)
@@ -114,12 +120,53 @@ func GetCommitHash(ref string) string {
return runGitCommandOrDie("show", "-s", "--format=%H", ref)
}
// ResolveRefCommit returns the commit pointed to by the given ref, which may be a remote ref.
//
// This differs from GetCommitHash which only works on exact matches, in that it will try to
// intelligently handle the scenario of a ref not existing locally, but being known to exist
// in a remote repo.
//
// This method should be used when a command may be performed by either the reviewer or the
// reviewee, while GetCommitHash should be used when the encompassing command should only be
// performed by the reviewee.
func ResolveRefCommit(ref string) (string, error) {
if err := VerifyGitRef(ref); err == nil {
return GetCommitHash(ref), nil
}
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 := runGitCommandOrDie("for-each-ref", "--format=%(refname)", pattern)
matchingRefs := strings.Split(matchingOutput, "\n")
if len(matchingRefs) == 1 && matchingRefs[0] != "" {
// There is exactly one match
return GetCommitHash(matchingRefs[0]), nil
}
return "", fmt.Errorf("Unable to find a git ref matching the pattern %q", pattern)
}
return "", fmt.Errorf("Unknown git ref %q", ref)
}
// GetCommitMessage returns the message stored in the commit pointed to by the given ref.
func GetCommitMessage(ref string) string {
return runGitCommandOrDie("show", "-s", "--format=%B", ref)
}
// IsAncestor determins if the first argument points to a commit that is an ancestor of the second.
// GetCommitTime returns the commit time of the commit pointed to by the given ref.
func GetCommitTime(ref string) string {
return runGitCommandOrDie("show", "-s", "--format=%ct", ref)
}
func GetLastParent(ref string) (string, error) {
return runGitCommand("rev-list", "--skip", "1", "-n", "1", ref)
}
// MergeBase determines if the first commit that is an ancestor of the two arguments.
func MergeBase(a, b string) string {
return runGitCommandOrDie("merge-base", a, b)
}
// IsAncestor determines if the first argument points to a commit that is an ancestor of the second.
func IsAncestor(ancestor, descendant string) bool {
_, err := runGitCommand("merge-base", "--is-ancestor", ancestor, descendant)
if err == nil {
@@ -132,6 +179,11 @@ func IsAncestor(ancestor, descendant string) bool {
return false
}
// Diff computes the diff between two given commits.
func Diff(left, right string) string {
return runGitCommandOrDie("diff", left, right)
}
// SwitchToRef changes the currently-checked-out ref.
func SwitchToRef(ref string) {
// If the ref starts with "refs/heads/", then we have to trim that prefix,

Modified review/request/request.go

@@ -45,6 +45,11 @@ type Request struct {
Description string `json:"description,omitempty"`
// Version represents the version of the metadata format.
Version int `json:"v,omitempty"`
// BaseCommit stores the commit ID of the target ref at the time the review was requested.
// This is optional, and only used for submitted reviews which were anchored at a merge commit.
// This allows someone viewing that submitted review to find the diff against which the
// code was reviewed.
BaseCommit string `json:"baseCommit,omitempty"`
}
// New returns a new request.

Modified review/review.go

@@ -21,11 +21,11 @@ import (
"bytes"
"encoding/json"
"fmt"
"sort"
"github.com/google/git-appraise/repository"
"github.com/google/git-appraise/review/ci"
"github.com/google/git-appraise/review/comment"
"github.com/google/git-appraise/review/request"
"sort"
"strconv"
"strings"
"time"
@@ -341,6 +341,100 @@ func (r *Review) PrintJson() error {
return nil
}
// findLastCommit returns the later (newest) commit from the union of the provided commit
// and all of the commits that are referenced in the given comment threads.
func findLastCommit(latestCommit string, commentThreads []CommentThread) string {
isLater := func(commit string) bool {
if repository.IsAncestor(latestCommit, commit) {
return true
}
if repository.IsAncestor(commit, latestCommit) {
return false
}
return repository.GetCommitTime(commit) > repository.GetCommitTime(latestCommit)
}
updateLatest := func(commit string) {
if commit == "" {
return
}
if isLater(commit) {
latestCommit = commit
}
}
for _, commentThread := range commentThreads {
comment := commentThread.Comment
if comment.Location != nil {
updateLatest(comment.Location.Commit)
}
updateLatest(findLastCommit(latestCommit, commentThread.Children))
}
return latestCommit
}
// GetHeadCommit returns the latest commit in a review.
func (r *Review) GetHeadCommit() (string, error) {
if r.Request.ReviewRef == "" {
return r.Revision, nil
}
targetRefHead, err := repository.ResolveRefCommit(r.Request.TargetRef)
if err != nil {
return "", err
}
if repository.IsAncestor(r.Revision, targetRefHead) {
// The review has already been submitted.
// Go through the list of comments and find the last commented upon commit.
return findLastCommit(r.Revision, r.Comments), nil
}
return repository.ResolveRefCommit(r.Request.ReviewRef)
}
// GetBaseCommit returns the commit against which a review should be compared.
func (r *Review) GetBaseCommit() (string, error) {
targetRefHead, err := repository.ResolveRefCommit(r.Request.TargetRef)
if err != nil {
return "", err
}
leftHandSide := targetRefHead
rightHandSide := r.Revision
if r.Request.ReviewRef == "" {
if reviewRefHead, err := repository.ResolveRefCommit(r.Request.ReviewRef); err == nil {
rightHandSide = reviewRefHead
}
}
if repository.IsAncestor(rightHandSide, leftHandSide) {
if r.Request.BaseCommit != "" {
return r.Request.BaseCommit, nil
}
// This means the review has been submitted, but did not specify a base commit.
// In this case, we have to treat the last parent commit as the base. This is
// usually what we want, since merging a target branch into a feature branch
// results in the previous commit to the feature branch being the first parent,
// and the latest commit to the target branch being the second parent.
return repository.GetLastParent(rightHandSide)
}
return repository.MergeBase(leftHandSide, rightHandSide), nil
}
// PrintDiff displays the diff for a review.
func (r *Review) PrintDiff() error {
var baseCommit, headCommit string
baseCommit, err := r.GetBaseCommit()
if err == nil {
headCommit, err = r.GetHeadCommit()
}
if err == nil {
fmt.Println(repository.Diff(baseCommit, headCommit))
}
return err
}
// AddComment adds the given comment to the review.
func (r *Review) AddComment(c comment.Comment) error {
commentNote, err := c.Write()

Modified review/review_test.go

@@ -17,8 +17,8 @@ limitations under the License.
package review
import (
"sort"
"github.com/google/git-appraise/review/comment"
"sort"
"testing"
)
@@ -533,3 +533,26 @@ func TestBuildCommentThreads(t *testing.T) {
t.Fatal("Unexpected leaf children: %v", threadLeaf.Children)
}
}
func TestGetHeadCommit(t *testing.T) {
// TODO(ojarjur): It's pretty terrible that this relies on running within the git repo of
// the tool and then using the tool's own review history as test data. We should change this
// to use a mock git repo.
submittedMergeReview := Get("fcc9b48925b8a880813275fa29b43426b5f1fccd")
submittedMergeReviewBase, err := submittedMergeReview.GetBaseCommit()
if err != nil {
t.Fatal("Unable to compute the base commit for a known review of a merge commit: ", err)
}
if submittedMergeReviewBase != "5c2b1d1e12eae76a85eb1b586c58d60e8c9ce388" {
t.Fatal("Unexpected base commit computed for a known review of a merge commit.")
}
submittedModifiedReview := Get("62f1f51aea3b59829071c58ad2189231b6505fd3")
submittedModifiedReviewBase, err := submittedModifiedReview.GetBaseCommit()
if err != nil {
t.Fatal("Unable to compute the base commit for a known, multi-commit review: ", err)
}
if submittedModifiedReviewBase != "b346936104f9bb4532d31abd085b531109e0b19c" {
t.Fatal("Unexpected base commit for a known, multi-commit review.")
}
}