Repositories

This, in turn, will allow us to build a mock git repo that we can use for testing, as opposed to the current test behavior, which relies on this repos review history.

To keep this change manageable, it only includes the injecting of the repo. The updates to the tests will come later.

Modified commands/accept.go

@@ -20,6 +20,7 @@ import (
"errors"
"flag"
"fmt"
"github.com/google/git-appraise/repository"
"github.com/google/git-appraise/review"
"github.com/google/git-appraise/review/comment"
)
@@ -31,7 +32,7 @@ var (
)
// acceptReview adds an LGTM comment to the current code review.
func acceptReview(args []string) error {
func acceptReview(repo repository.Repo, args []string) error {
acceptFlagSet.Parse(args)
args = acceptFlagSet.Args()
@@ -42,9 +43,9 @@ func acceptReview(args []string) error {
}
if len(args) == 1 {
r = review.Get(args[0])
r = review.Get(repo, args[0])
} else {
r, err = review.GetCurrent()
r, err = review.GetCurrent(repo)
}
if err != nil {
@@ -62,7 +63,7 @@ func acceptReview(args []string) error {
Commit: acceptedCommit,
}
resolved := true
c := comment.New(*acceptMessage)
c := comment.New(repo.GetUserEmail(), *acceptMessage)
c.Location = &location
c.Resolved = &resolved
return r.AddComment(c)
@@ -74,7 +75,7 @@ var acceptCmd = &Command{
fmt.Printf("Usage: %s accept <option>... (<commit>)\n\nOptions:\n", arg0)
acceptFlagSet.PrintDefaults()
},
RunMethod: func(args []string) error {
return acceptReview(args)
RunMethod: func(repo repository.Repo, args []string) error {
return acceptReview(repo, args)
},
}

Modified commands/commands.go

@@ -17,20 +17,24 @@ limitations under the License.
// Package commands contains the assorted sub commands supported by the git-appraise tool.
package commands
import (
"github.com/google/git-appraise/repository"
)
const notesRefPattern = "refs/notes/devtools/*"
// Command represents the definition of a single command.
type Command struct {
Usage func(string)
RunMethod func([]string) error
RunMethod func(repository.Repo, []string) error
}
// Run executes a command, given its arguments.
//
// The args parameter is all of the command line args that followed the
// subcommand.
func (cmd *Command) Run(args []string) error {
return cmd.RunMethod(args)
func (cmd *Command) Run(repo repository.Repo, args []string) error {
return cmd.RunMethod(repo, args)
}
// CommandMap defines all of the available (sub)commands.

Modified commands/comment.go

@@ -36,14 +36,14 @@ var (
)
// commentOnReview adds a comment to the current code review.
func commentOnReview(args []string) error {
func commentOnReview(repo repository.Repo, args []string) error {
commentFlagSet.Parse(args)
args = commentFlagSet.Args()
if *lgtm && *nmw {
return errors.New("You cannot combine the flags -lgtm and -nmw.")
}
r, err := review.GetCurrent()
r, err := review.GetCurrent(repo)
if err != nil {
return fmt.Errorf("Failed to load the current review: %v\n", err)
}
@@ -51,7 +51,7 @@ func commentOnReview(args []string) error {
return errors.New("There is no current review.")
}
commentedUponCommit := repository.GetCommitHash(r.Request.ReviewRef)
commentedUponCommit := repo.GetCommitHash(r.Request.ReviewRef)
location := comment.Location{
Commit: commentedUponCommit,
}
@@ -68,7 +68,7 @@ func commentOnReview(args []string) error {
}
}
c := comment.New(*commentMessage)
c := comment.New(repo.GetUserEmail(), *commentMessage)
c.Location = &location
c.Parent = *parent
if *lgtm || *nmw {
@@ -84,7 +84,7 @@ var commentCmd = &Command{
fmt.Printf("Usage: %s comment <option>... [<file> [<line>]]\n\nOptions:\n", arg0)
commentFlagSet.PrintDefaults()
},
RunMethod: func(args []string) error {
return commentOnReview(args)
RunMethod: func(repo repository.Repo, args []string) error {
return commentOnReview(repo, args)
},
}

Modified commands/list.go

@@ -18,16 +18,17 @@ package commands
import (
"fmt"
"github.com/google/git-appraise/repository"
"github.com/google/git-appraise/review"
)
// listReviews lists all extant reviews.
// TODO(ojarjur): Add flags for filtering the output (e.g. to just open reviews).
func listReviews(args []string) {
reviews := review.ListAll()
func listReviews(repo repository.Repo, args []string) {
reviews := review.ListAll(repo)
fmt.Printf("Loaded %d reviews:\n", len(reviews))
for _, review := range review.ListAll() {
review.PrintSummary()
for _, r := range reviews {
r.PrintSummary()
}
}
@@ -36,8 +37,8 @@ var listCmd = &Command{
Usage: func(arg0 string) {
fmt.Printf("Usage: %s list\n", arg0)
},
RunMethod: func(args []string) error {
listReviews(args)
RunMethod: func(repo repository.Repo, args []string) error {
listReviews(repo, args)
return nil
},
}

Modified commands/pull.go

@@ -23,7 +23,7 @@ import (
)
// pull updates the local git-notes used for reviews with those from a remote repo.
func pull(args []string) error {
func pull(repo repository.Repo, args []string) error {
if len(args) > 1 {
return errors.New("Only pulling from one remote at a time is supported.")
}
@@ -33,7 +33,7 @@ func pull(args []string) error {
remote = args[0]
}
repository.PullNotes(remote, notesRefPattern)
repo.PullNotes(remote, notesRefPattern)
return nil
}
@@ -41,7 +41,7 @@ var pullCmd = &Command{
Usage: func(arg0 string) {
fmt.Printf("Usage: %s pull [<remote>]", arg0)
},
RunMethod: func(args []string) error {
return pull(args)
RunMethod: func(repo repository.Repo, args []string) error {
return pull(repo, args)
},
}

Modified commands/push.go

@@ -23,7 +23,7 @@ import (
)
// push pushes the local git-notes used for reviews to a remote repo.
func push(args []string) error {
func push(repo repository.Repo, args []string) error {
if len(args) > 1 {
return errors.New("Only pushing to one remote at a time is supported.")
}
@@ -33,14 +33,14 @@ func push(args []string) error {
remote = args[0]
}
return repository.PushNotes(remote, notesRefPattern)
return repo.PushNotes(remote, notesRefPattern)
}
var pushCmd = &Command{
Usage: func(arg0 string) {
fmt.Printf("Usage: %s push [<remote>]", arg0)
},
RunMethod: func(args []string) error {
return push(args)
RunMethod: func(repo repository.Repo, args []string) error {
return push(repo, args)
},
}

Modified commands/request.go

@@ -45,7 +45,7 @@ var (
)
// Build the template review request based solely on the parsed flag values.
func buildRequestFromFlags() request.Request {
func buildRequestFromFlags(requester string) request.Request {
var reviewers []string
if len(*requestReviewers) > 0 {
for _, reviewer := range strings.Split(*requestReviewers, ",") {
@@ -53,45 +53,45 @@ func buildRequestFromFlags() request.Request {
}
}
return request.New(reviewers, *requestSource, *requestTarget, *requestMessage)
return request.New(requester, reviewers, *requestSource, *requestTarget, *requestMessage)
}
// Create a new code review request.
//
// The "args" parameter is all of the command line arguments that followed the subcommand.
func requestReview(args []string) error {
func requestReview(repo repository.Repo, args []string) error {
requestFlagSet.Parse(args)
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 repository.HasUncommittedChanges() {
if repo.HasUncommittedChanges() {
return errors.New("You have uncommitted or untracked files. Use --allow-uncommitted to ignore those.")
}
}
r := buildRequestFromFlags()
r := buildRequestFromFlags(repo.GetUserEmail())
if r.ReviewRef == "HEAD" {
r.ReviewRef = repository.GetHeadRef()
r.ReviewRef = repo.GetHeadRef()
}
repository.VerifyGitRefOrDie(r.TargetRef)
repository.VerifyGitRefOrDie(r.ReviewRef)
r.BaseCommit = repository.GetCommitHash(r.TargetRef)
repo.VerifyGitRefOrDie(r.TargetRef)
repo.VerifyGitRefOrDie(r.ReviewRef)
r.BaseCommit = repo.GetCommitHash(r.TargetRef)
reviewCommits := repository.ListCommitsBetween(r.TargetRef, r.ReviewRef)
reviewCommits := repo.ListCommitsBetween(r.TargetRef, r.ReviewRef)
if reviewCommits == nil {
return errors.New("There are no commits included in the review request")
}
if r.Description == "" {
r.Description = repository.GetCommitMessage(reviewCommits[0])
r.Description = repo.GetCommitMessage(reviewCommits[0])
}
note, err := r.Write()
if err != nil {
return err
}
repository.AppendNote(request.Ref, reviewCommits[0], note)
repo.AppendNote(request.Ref, reviewCommits[0], note)
if !*requestQuiet {
fmt.Printf(requestSummaryTemplate, reviewCommits[0], r.TargetRef, r.ReviewRef, r.Description)
}
@@ -104,7 +104,7 @@ var requestCmd = &Command{
fmt.Printf("Usage: %s request <option>...\n\nOptions:\n", arg0)
requestFlagSet.PrintDefaults()
},
RunMethod: func(args []string) error {
return requestReview(args)
RunMethod: func(repo repository.Repo, args []string) error {
return requestReview(repo, args)
},
}

Modified commands/request_test.go

@@ -23,7 +23,7 @@ import (
func TestBuildRequestFromFlags(t *testing.T) {
args := []string{"-m", "Request message", "-r", "Me, Myself, \nAnd I "}
requestFlagSet.Parse(args)
r := buildRequestFromFlags()
r := buildRequestFromFlags("[email protected]")
if r.Description != "Request message" {
t.Fatalf("Unexpected request description: '%s'", r.Description)
}

Modified commands/show.go

@@ -20,6 +20,7 @@ import (
"errors"
"flag"
"fmt"
"github.com/google/git-appraise/repository"
"github.com/google/git-appraise/review"
)
@@ -28,7 +29,7 @@ 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 {
func showReview(repo repository.Repo, args []string) error {
showFlagSet.Parse(args)
args = showFlagSet.Args()
@@ -39,9 +40,9 @@ func showReview(args []string) error {
}
if len(args) == 1 {
r = review.Get(args[0])
r = review.Get(repo, args[0])
} else {
r, err = review.GetCurrent()
r, err = review.GetCurrent(repo)
}
if err != nil {
@@ -64,7 +65,7 @@ var showCmd = &Command{
Usage: func(arg0 string) {
fmt.Printf("Usage: %s show (<commit>)\n", arg0)
},
RunMethod: func(args []string) error {
return showReview(args)
RunMethod: func(repo repository.Repo, args []string) error {
return showReview(repo, args)
},
}

Modified commands/submit.go

@@ -35,14 +35,14 @@ var (
// Submit the current code review request.
//
// The "args" parameter contains all of the command line arguments that followed the subcommand.
func submitReview(args []string) error {
func submitReview(repo repository.Repo, args []string) error {
submitFlagSet.Parse(args)
if *submitMerge && *submitRebase {
return errors.New("Only one of --merge or --rebase is allowed.")
}
r, err := review.GetCurrent()
r, err := review.GetCurrent(repo)
if err != nil {
return err
}
@@ -56,20 +56,20 @@ func submitReview(args []string) error {
target := r.Request.TargetRef
source := r.Request.ReviewRef
repository.VerifyGitRefOrDie(target)
repository.VerifyGitRefOrDie(source)
repo.VerifyGitRefOrDie(target)
repo.VerifyGitRefOrDie(source)
if !repository.IsAncestor(target, source) {
if !repo.IsAncestor(target, source) {
return errors.New("Refusing to submit a non-fast-forward review. First merge the target ref.")
}
repository.SwitchToRef(target)
repo.SwitchToRef(target)
if *submitMerge {
repository.MergeRef(source, false)
repo.MergeRef(source, false)
} else if *submitRebase {
repository.RebaseRef(source)
repo.RebaseRef(source)
} else {
repository.MergeRef(source, true)
repo.MergeRef(source, true)
}
return nil
}
@@ -80,7 +80,7 @@ var submitCmd = &Command{
fmt.Printf("Usage: %s submit <option>...\n\nOptions:\n", arg0)
submitFlagSet.PrintDefaults()
},
RunMethod: func(args []string) error {
return submitReview(args)
RunMethod: func(repo repository.Repo, args []string) error {
return submitReview(repo, args)
},
}

Modified git-appraise/git-appraise.go

@@ -76,17 +76,23 @@ func main() {
help()
return
}
if !repository.IsGitRepo() {
fmt.Printf("%s must be run from within a git repo.", os.Args[0])
cwd, err := os.Getwd()
if err != nil {
fmt.Printf("Unable to get the current working directory: %q\n", err)
return
}
repo, err := repository.NewGitRepo(cwd)
if err != nil {
fmt.Printf("%s must be run from within a git repo.\n", os.Args[0])
return
}
subcommand, ok := commands.CommandMap[os.Args[1]]
if !ok {
fmt.Printf("Unknown command %q", os.Args[1])
fmt.Printf("Unknown command: %q\n", os.Args[1])
usage()
return
}
if err := subcommand.Run(os.Args[2:]); err != nil {
if err := subcommand.Run(repo, os.Args[2:]); err != nil {
fmt.Println(err.Error())
os.Exit(1)
}

Modified repository/git.go

@@ -28,19 +28,23 @@ import (
const branchRefPrefix = "refs/heads/"
// Note represents the contents of a git-note
type Note []byte
// GitRepo represents an instance of a (local) git repository.
type GitRepo struct {
Path string
}
// Run the given git command and return its stdout, or an error if the command fails.
func runGitCommand(args ...string) (string, error) {
func (repo *GitRepo) runGitCommand(args ...string) (string, error) {
cmd := exec.Command("git", args...)
cmd.Dir = repo.Path
out, err := cmd.Output()
return strings.Trim(string(out), "\n"), err
}
// Run the given git command using the same stdin, stdout, and stderr as the review tool.
func runGitCommandInline(args ...string) error {
func (repo *GitRepo) runGitCommandInline(args ...string) error {
cmd := exec.Command("git", args...)
cmd.Dir = repo.Path
cmd.Stdin = os.Stdin
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
@@ -48,8 +52,8 @@ func runGitCommandInline(args ...string) error {
}
// Run the given git command using the same stdin, stdout, and stderr as the review tool.
func runGitCommandInlineOrDie(args ...string) {
err := runGitCommandInline(args...)
func (repo *GitRepo) runGitCommandInlineOrDie(args ...string) {
err := repo.runGitCommandInline(args...)
if err != nil {
log.Print("git", args)
log.Fatal(err)
@@ -57,8 +61,8 @@ func runGitCommandInlineOrDie(args ...string) {
}
// Run the given git command and return its stdout.
func runGitCommandOrDie(args ...string) string {
out, err := runGitCommand(args...)
func (repo *GitRepo) runGitCommandOrDie(args ...string) string {
out, err := repo.runGitCommand(args...)
if err != nil {
log.Print("git", args)
log.Fatal(out)
@@ -66,33 +70,40 @@ func runGitCommandOrDie(args ...string) string {
return out
}
// IsGitRepo determines if the current working directory is inside of a git repository.
func IsGitRepo() bool {
_, err := runGitCommand("rev-parse")
// 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) {
repo := &GitRepo{Path: path}
_, err := repo.runGitCommand("rev-parse")
if err == nil {
return true
return repo, nil
}
if _, ok := err.(*exec.ExitError); ok {
return false
return nil, err
}
log.Fatal(err)
return false
return nil, err
}
// GetPath returns the path to the repo.
func (repo *GitRepo) GetPath() string {
return repo.Path
}
// GetRepoStateHash returns a hash which embodies the entire current state of a repository.
func GetRepoStateHash() string {
stateSummary := runGitCommandOrDie("show-ref")
func (repo *GitRepo) GetRepoStateHash() string {
stateSummary := repo.runGitCommandOrDie("show-ref")
return fmt.Sprintf("%x", sha1.Sum([]byte(stateSummary)))
}
// GetUserEmail returns the email address that the user has used to configure git.
func GetUserEmail() string {
return runGitCommandOrDie("config", "user.email")
func (repo *GitRepo) GetUserEmail() string {
return repo.runGitCommandOrDie("config", "user.email")
}
// HasUncommittedChanges returns true if there are local, uncommitted changes.
func HasUncommittedChanges() bool {
out := runGitCommandOrDie("status", "--porcelain")
func (repo *GitRepo) HasUncommittedChanges() bool {
out := repo.runGitCommandOrDie("status", "--porcelain")
if len(out) > 0 {
return true
}
@@ -100,24 +111,24 @@ func HasUncommittedChanges() bool {
}
// VerifyGitRef verifies that the supplied ref points to a known commit.
func VerifyGitRef(ref string) error {
_, err := runGitCommand("show-ref", "--verify", ref)
func (repo *GitRepo) VerifyGitRef(ref string) error {
_, err := repo.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)
func (repo *GitRepo) VerifyGitRefOrDie(ref string) {
repo.runGitCommandOrDie("show-ref", "--verify", ref)
}
// GetHeadRef returns the ref that is the current HEAD.
func GetHeadRef() string {
return runGitCommandOrDie("symbolic-ref", "HEAD")
func (repo *GitRepo) GetHeadRef() string {
return repo.runGitCommandOrDie("symbolic-ref", "HEAD")
}
// GetCommitHash returns the hash of the commit pointed to by the given ref.
func GetCommitHash(ref string) string {
return runGitCommandOrDie("show", "-s", "--format=%H", ref)
func (repo *GitRepo) GetCommitHash(ref string) string {
return repo.runGitCommandOrDie("show", "-s", "--format=%H", ref)
}
// ResolveRefCommit returns the commit pointed to by the given ref, which may be a remote ref.
@@ -129,18 +140,18 @@ func GetCommitHash(ref string) string {
// 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
func (repo *GitRepo) ResolveRefCommit(ref string) (string, error) {
if err := repo.VerifyGitRef(ref); err == nil {
return repo.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)
matchingOutput := repo.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 repo.GetCommitHash(matchingRefs[0]), nil
}
return "", fmt.Errorf("Unable to find a git ref matching the pattern %q", pattern)
}
@@ -148,27 +159,27 @@ func ResolveRefCommit(ref string) (string, error) {
}
// 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)
func (repo *GitRepo) GetCommitMessage(ref string) string {
return repo.runGitCommandOrDie("show", "-s", "--format=%B", ref)
}
// 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 (repo *GitRepo) GetCommitTime(ref string) string {
return repo.runGitCommandOrDie("show", "-s", "--format=%ct", ref)
}
func GetLastParent(ref string) (string, error) {
return runGitCommand("rev-list", "--skip", "1", "-n", "1", ref)
func (repo *GitRepo) GetLastParent(ref string) (string, error) {
return repo.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)
func (repo *GitRepo) MergeBase(a, b string) string {
return repo.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)
func (repo *GitRepo) IsAncestor(ancestor, descendant string) bool {
_, err := repo.runGitCommand("merge-base", "--is-ancestor", ancestor, descendant)
if err == nil {
return true
}
@@ -180,25 +191,25 @@ func IsAncestor(ancestor, descendant string) bool {
}
// Diff computes the diff between two given commits.
func Diff(left, right string) string {
return runGitCommandOrDie("diff", left, right)
func (repo *GitRepo) Diff(left, right string, diffArgs ...string) string {
return repo.runGitCommandOrDie("diff", left, right)
}
// SwitchToRef changes the currently-checked-out ref.
func SwitchToRef(ref string) {
func (repo *GitRepo) SwitchToRef(ref string) {
// 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):]
}
runGitCommandOrDie("checkout", ref)
repo.runGitCommandOrDie("checkout", ref)
}
// 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 MergeRef(ref string, fastForward bool) {
func (repo *GitRepo) MergeRef(ref string, fastForward bool) {
args := []string{"merge"}
if fastForward {
args = append(args, "--ff", "--ff-only")
@@ -206,12 +217,12 @@ func MergeRef(ref string, fastForward bool) {
args = append(args, "--no-ff")
}
args = append(args, ref)
runGitCommandInlineOrDie(args...)
repo.runGitCommandInlineOrDie(args...)
}
// RebaseRef rebases the given ref into the current one.
func RebaseRef(ref string) {
runGitCommandInlineOrDie("rebase", "-i", ref)
func (repo *GitRepo) RebaseRef(ref string) {
repo.runGitCommandInlineOrDie("rebase", "-i", ref)
}
// ListCommitsBetween returns the list of commits between the two given revisions.
@@ -222,8 +233,8 @@ func 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 ListCommitsBetween(from, to string) []string {
out := runGitCommandOrDie("rev-list", "--reverse", "--ancestry-path", from+".."+to)
func (repo *GitRepo) ListCommitsBetween(from, to string) []string {
out := repo.runGitCommandOrDie("rev-list", "--reverse", "--ancestry-path", from+".."+to)
if out == "" {
return nil
}
@@ -231,9 +242,9 @@ func ListCommitsBetween(from, to string) []string {
}
// GetNotes uses the "git" command-line tool to read the notes from the given ref for a given revision.
func GetNotes(notesRef, revision string) []Note {
func (repo *GitRepo) GetNotes(notesRef, revision string) []Note {
var notes []Note
rawNotes, err := runGitCommand("notes", "--ref", notesRef, "show", revision)
rawNotes, err := repo.runGitCommand("notes", "--ref", notesRef, "show", revision)
if err != nil {
// We just assume that this means there are no notes
return nil
@@ -245,19 +256,19 @@ func GetNotes(notesRef, revision string) []Note {
}
// AppendNote appends a note to a revision under the given ref.
func AppendNote(notesRef, revision string, note Note) {
runGitCommandOrDie("notes", "--ref", notesRef, "append", "-m", string(note), revision)
func (repo *GitRepo) AppendNote(notesRef, revision string, note Note) {
repo.runGitCommandOrDie("notes", "--ref", notesRef, "append", "-m", string(note), revision)
}
// ListNotedRevisions returns the collection of revisions that are annotated by notes in the given ref.
func ListNotedRevisions(notesRef string) []string {
func (repo *GitRepo) ListNotedRevisions(notesRef string) []string {
var revisions []string
notesList := strings.Split(runGitCommandOrDie("notes", "--ref", notesRef, "list"), "\n")
notesList := strings.Split(repo.runGitCommandOrDie("notes", "--ref", notesRef, "list"), "\n")
for _, notePair := range notesList {
noteParts := strings.SplitN(notePair, " ", 2)
if len(noteParts) == 2 {
objHash := noteParts[1]
objType, err := runGitCommand("cat-file", "-t", objHash)
objType, err := repo.runGitCommand("cat-file", "-t", objHash)
// If a note points to an object that we do not know about (yet), then err will not
// be nil. We can safely just ignore those notes.
if err == nil && objType == "commit" {
@@ -269,12 +280,12 @@ func ListNotedRevisions(notesRef string) []string {
}
// PushNotes pushes git notes to a remote repo.
func PushNotes(remote, notesRefPattern string) error {
func (repo *GitRepo) PushNotes(remote, notesRefPattern string) error {
refspec := fmt.Sprintf("%s:%s", notesRefPattern, notesRefPattern)
// The push is liable to fail if the user forgot to do a pull first, so
// we treat errors as user errors rather than fatal errors.
err := runGitCommandInline("push", remote, refspec)
err := repo.runGitCommandInline("push", remote, refspec)
if err != nil {
return fmt.Errorf("Failed to push to the remote '%s': %v", remote, err)
}
@@ -289,18 +300,18 @@ 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 PullNotes(remote, notesRefPattern string) {
func (repo *GitRepo) PullNotes(remote, notesRefPattern string) {
remoteNotesRefPattern := getRemoteNotesRef(remote, notesRefPattern)
fetchRefSpec := fmt.Sprintf("+%s:%s", notesRefPattern, remoteNotesRefPattern)
runGitCommandInlineOrDie("fetch", remote, fetchRefSpec)
repo.runGitCommandInlineOrDie("fetch", remote, fetchRefSpec)
remoteRefs := runGitCommandOrDie("ls-remote", remote, notesRefPattern)
remoteRefs := repo.runGitCommandOrDie("ls-remote", remote, notesRefPattern)
for _, line := range strings.Split(remoteRefs, "\n") {
lineParts := strings.Split(line, "\t")
if len(lineParts) == 2 {
ref := lineParts[1]
remoteRef := getRemoteNotesRef(remote, ref)
runGitCommandOrDie("notes", "--ref", ref, "merge", remoteRef, "-s", "cat_sort_uniq")
repo.runGitCommandOrDie("notes", "--ref", ref, "merge", remoteRef, "-s", "cat_sort_uniq")
}
}
}

Added repository/repo.go

@@ -0,0 +1,115 @@
/*
Copyright 2015 Google Inc. All rights reserved.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
// Package repository contains helper methods for working with a Git repo.
package repository
// Note represents the contents of a git-note
type Note []byte
// Repo represents a source code repository.
type Repo interface {
// GetPath returns the path to the repo.
GetPath() string
// GetRepoStateHash returns a hash which embodies the entire current state of a repository.
GetRepoStateHash() string
// GetUserEmail returns the email address that the user has used to configure git.
GetUserEmail() string
// HasUncommittedChanges returns true if there are local, uncommitted changes.
HasUncommittedChanges() bool
// 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
// GetCommitHash returns the hash of the commit pointed to by the given ref.
GetCommitHash(ref string) string
// 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.
ResolveRefCommit(ref string) (string, error)
// GetCommitMessage returns the message stored in the commit pointed to by the given ref.
GetCommitMessage(ref string) string
// GetCommitTime returns the commit time of the commit pointed to by the given ref.
GetCommitTime(ref string) string
GetLastParent(ref string) (string, error)
// MergeBase determines if the first commit that is an ancestor of the two arguments.
MergeBase(a, b string) string
// IsAncestor determines if the first argument points to a commit that is an ancestor of the second.
IsAncestor(ancestor, descendant string) bool
// Diff computes the diff between two given commits.
Diff(left, right string, diffArgs ...string) string
// SwitchToRef changes the currently-checked-out ref.
SwitchToRef(ref string)
// 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)
// RebaseRef rebases the given ref into the current one.
RebaseRef(ref string)
// ListCommitsBetween returns the list of commits between the two given revisions.
//
// The "from" parameter is the starting point (exclusive), and the "to" parameter
// is the ending point (inclusive). If the commit pointed to by the "from" parameter
// is not an ancestor of the commit pointed to by the "to" parameter, then the
// 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
// 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)
// ListNotedRevisions returns the collection of revisions that are annotated by notes in the given ref.
ListNotedRevisions(notesRef string) []string
// PushNotes pushes git notes to a remote repo.
PushNotes(remote, notesRefPattern string) error
// 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)
}

Modified review/comment/comment.go

@@ -75,10 +75,10 @@ type Comment struct {
// New returns a new comment with the given description message.
//
// The Timestamp and Author fields are automatically filled in with the current time and user.
func New(description string) Comment {
func New(author string, description string) Comment {
return Comment{
Timestamp: strconv.FormatInt(time.Now().Unix(), 10),
Author: repository.GetUserEmail(),
Author: author,
Description: description,
}
}

Modified review/request/request.go

@@ -55,10 +55,10 @@ type Request struct {
// New returns a new request.
//
// The Timestamp and Requester fields are automatically filled in with the current time and user.
func New(reviewers []string, reviewRef, targetRef, description string) Request {
func New(requester string, reviewers []string, reviewRef, targetRef, description string) Request {
return Request{
Timestamp: strconv.FormatInt(time.Now().Unix(), 10),
Requester: repository.GetUserEmail(),
Requester: requester,
Reviewers: reviewers,
ReviewRef: reviewRef,
TargetRef: targetRef,

Modified review/review.go

@@ -67,6 +67,7 @@ type CommentThread struct {
// correspond to either the current commit in the review ref (for pending
// reviews), or to the last commented-upon commit (for submitted reviews).
type Review struct {
Repo repository.Repo `json:"-"`
Revision string `json:"revision"`
Request request.Request `json:"request"`
Comments []CommentThread `json:"comments,omitempty"`
@@ -184,7 +185,7 @@ func buildCommentThreads(commentsByHash map[string]comment.Comment) []CommentThr
// loadComments reads in the log-structured sequence of comments for a review,
// and then builds the corresponding tree-structured comment threads.
func (r *Review) loadComments() []CommentThread {
commentNotes := repository.GetNotes(comment.Ref, r.Revision)
commentNotes := r.Repo.GetNotes(comment.Ref, r.Revision)
commentsByHash := comment.ParseAllValid(commentNotes)
return buildCommentThreads(commentsByHash)
}
@@ -192,29 +193,30 @@ func (r *Review) loadComments() []CommentThread {
// Get returns the specified code review.
//
// If no review request exists, the returned review is nil.
func Get(revision string) *Review {
requestNotes := repository.GetNotes(request.Ref, revision)
func Get(repo repository.Repo, revision string) *Review {
requestNotes := repo.GetNotes(request.Ref, revision)
requests := request.ParseAllValid(requestNotes)
if requests == nil {
return nil
}
review := Review{
Repo: repo,
Revision: revision,
Request: requests[len(requests)-1],
}
review.Comments = review.loadComments()
review.Resolved = updateThreadsStatus(review.Comments)
review.Submitted = repository.IsAncestor(revision, review.Request.TargetRef)
review.Submitted = repo.IsAncestor(revision, review.Request.TargetRef)
// TODO(ojarjur): Optionally fetch the CI status of the last commit
// in the review for which there are comments.
return &review
}
// ListAll returns all reviews stored in the git-notes.
func ListAll() []Review {
func ListAll(repo repository.Repo) []Review {
var reviews []Review
for _, revision := range repository.ListNotedRevisions(request.Ref) {
review := Get(revision)
for _, revision := range repo.ListNotedRevisions(request.Ref) {
review := Get(repo, revision)
if review != nil {
reviews = append(reviews, *review)
}
@@ -223,9 +225,9 @@ func ListAll() []Review {
}
// ListOpen returns all reviews that are not yet incorporated into their target refs.
func ListOpen() []Review {
func ListOpen(repo repository.Repo) []Review {
var openReviews []Review
for _, review := range ListAll() {
for _, review := range ListAll(repo) {
if !review.Submitted {
openReviews = append(openReviews, review)
}
@@ -236,11 +238,11 @@ func ListOpen() []Review {
// GetCurrent returns the current, open code review.
//
// If there are multiple matching reviews, then an error is returned.
func GetCurrent() (*Review, error) {
reviewRef := repository.GetHeadRef()
currentCommit := repository.GetCommitHash(reviewRef)
func GetCurrent(repo repository.Repo) (*Review, error) {
reviewRef := repo.GetHeadRef()
currentCommit := repo.GetCommitHash(reviewRef)
var matchingReviews []Review
for _, review := range ListOpen() {
for _, review := range ListOpen(repo) {
if review.Request.ReviewRef == reviewRef {
matchingReviews = append(matchingReviews, review)
}
@@ -252,7 +254,7 @@ func GetCurrent() (*Review, error) {
return nil, fmt.Errorf("There are %d open reviews for the ref \"%s\"", len(matchingReviews), reviewRef)
}
r := &matchingReviews[0]
reports := ci.ParseAllValid(repository.GetNotes(ci.Ref, currentCommit))
reports := ci.ParseAllValid(repo.GetNotes(ci.Ref, currentCommit))
r.Reports = reports
return r, nil
}
@@ -343,15 +345,15 @@ func (r *Review) PrintJson() error {
// 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 {
func (r *Review) findLastCommit(latestCommit string, commentThreads []CommentThread) string {
isLater := func(commit string) bool {
if repository.IsAncestor(latestCommit, commit) {
if r.Repo.IsAncestor(latestCommit, commit) {
return true
}
if repository.IsAncestor(commit, latestCommit) {
if r.Repo.IsAncestor(commit, latestCommit) {
return false
}
return repository.GetCommitTime(commit) > repository.GetCommitTime(latestCommit)
return r.Repo.GetCommitTime(commit) > r.Repo.GetCommitTime(latestCommit)
}
updateLatest := func(commit string) {
if commit == "" {
@@ -366,7 +368,7 @@ func findLastCommit(latestCommit string, commentThreads []CommentThread) string
if comment.Location != nil {
updateLatest(comment.Location.Commit)
}
updateLatest(findLastCommit(latestCommit, commentThread.Children))
updateLatest(r.findLastCommit(latestCommit, commentThread.Children))
}
return latestCommit
}
@@ -377,23 +379,23 @@ func (r *Review) GetHeadCommit() (string, error) {
return r.Revision, nil
}
targetRefHead, err := repository.ResolveRefCommit(r.Request.TargetRef)
targetRefHead, err := r.Repo.ResolveRefCommit(r.Request.TargetRef)
if err != nil {
return "", err
}
if repository.IsAncestor(r.Revision, targetRefHead) {
if r.Repo.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 r.findLastCommit(r.Revision, r.Comments), nil
}
return repository.ResolveRefCommit(r.Request.ReviewRef)
return r.Repo.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)
targetRefHead, err := r.Repo.ResolveRefCommit(r.Request.TargetRef)
if err != nil {
return "", err
}
@@ -401,12 +403,12 @@ func (r *Review) GetBaseCommit() (string, error) {
leftHandSide := targetRefHead
rightHandSide := r.Revision
if r.Request.ReviewRef == "" {
if reviewRefHead, err := repository.ResolveRefCommit(r.Request.ReviewRef); err == nil {
if reviewRefHead, err := r.Repo.ResolveRefCommit(r.Request.ReviewRef); err == nil {
rightHandSide = reviewRefHead
}
}
if repository.IsAncestor(rightHandSide, leftHandSide) {
if r.Repo.IsAncestor(rightHandSide, leftHandSide) {
if r.Request.BaseCommit != "" {
return r.Request.BaseCommit, nil
}
@@ -416,10 +418,10 @@ func (r *Review) GetBaseCommit() (string, error) {
// 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 r.Repo.GetLastParent(rightHandSide)
}
return repository.MergeBase(leftHandSide, rightHandSide), nil
return r.Repo.MergeBase(leftHandSide, rightHandSide), nil
}
// PrintDiff displays the diff for a review.
@@ -430,7 +432,7 @@ func (r *Review) PrintDiff() error {
headCommit, err = r.GetHeadCommit()
}
if err == nil {
fmt.Println(repository.Diff(baseCommit, headCommit))
fmt.Println(r.Repo.Diff(baseCommit, headCommit))
}
return err
}
@@ -442,6 +444,6 @@ func (r *Review) AddComment(c comment.Comment) error {
return err
}
repository.AppendNote(comment.Ref, r.Revision, commentNote)
r.Repo.AppendNote(comment.Ref, r.Revision, commentNote)
return nil
}

Modified review/review_test.go

@@ -17,7 +17,9 @@ limitations under the License.
package review
import (
"github.com/google/git-appraise/repository"
"github.com/google/git-appraise/review/comment"
"os"
"sort"
"testing"
)
@@ -538,7 +540,15 @@ 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")
cwd, err := os.Getwd()
if err != nil {
t.Fatal(err)
}
selfRepo, err := repository.NewGitRepo(cwd)
if err != nil {
t.Fatal(err)
}
submittedMergeReview := Get(selfRepo, "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)
@@ -547,7 +557,7 @@ func TestGetHeadCommit(t *testing.T) {
t.Fatal("Unexpected base commit computed for a known review of a merge commit.")
}
submittedModifiedReview := Get("62f1f51aea3b59829071c58ad2189231b6505fd3")
submittedModifiedReview := Get(selfRepo, "62f1f51aea3b59829071c58ad2189231b6505fd3")
submittedModifiedReviewBase, err := submittedModifiedReview.GetBaseCommit()
if err != nil {
t.Fatal("Unable to compute the base commit for a known, multi-commit review: ", err)