Repositories

This includes adding a new field to review requests for saving the commit against which to review.

Normally, that new field isn't necessary, but it removes ambiguity for the case of a submitted review that is anchored on a merge commit.

Modified .travis.yml

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

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 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)
@@ -119,7 +125,16 @@ 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.
func GetFirstParent(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 +147,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

@@ -341,6 +341,69 @@ func (r *Review) PrintJson() error {
return nil
}
// GetHeadCommit returns the latest commit in a review.
func (r *Review) GetHeadCommit() (string, error) {
if r.Request.ReviewRef == "" {
return r.Revision, nil
}
if err := repository.VerifyGitRef(r.Request.TargetRef); err != nil {
return "", err
}
if repository.IsAncestor(r.Revision, r.Request.TargetRef) {
// The review has already been submitted.
// TODO(ojarjur): Go through the list of comments and find the last commented upon commit.
return r.Revision, nil
}
if err := repository.VerifyGitRef(r.Request.ReviewRef); err != nil {
return "", fmt.Errorf("Local copy of the review ref not found; run 'git checkout %s' to create it", r.Request.ReviewRef)
}
return repository.GetCommitHash(r.Request.ReviewRef), nil
}
// GetBaseCommit returns the commit against which a review should be compared.
func (r *Review) GetBaseCommit() (string, error) {
if r.Request.BaseCommit != "" {
return r.Request.BaseCommit, nil
}
if err := repository.VerifyGitRef(r.Request.TargetRef); err != nil {
return "", err
}
leftHandSide := repository.GetCommitHash(r.Request.TargetRef)
rightHandSide := r.Revision
if r.Request.ReviewRef == "" {
if err := repository.VerifyGitRef(r.Request.ReviewRef); err == nil {
rightHandSide = repository.GetCommitHash(r.Request.ReviewRef)
}
}
if repository.IsAncestor(rightHandSide, leftHandSide) {
// This means the review has been submitted, but did not specify a base commit.
// In this case, we have to treat the first parent commit as the base.
return repository.GetFirstParent(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

@@ -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.")
}
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.")
}
if submittedModifiedReviewBase != "b346936104f9bb4532d31abd085b531109e0b19c" {
t.Fatal("Unexpected base commit for a known, multi-commit review.")
}
}