Browse Source

Submitting review 38d9edc8c648

Cleanup and generalize the API for low-level repository operations.

This change cleans up the API for low-level repository operations
in the following ways:

1. Introduce more generic operations on the underlying repository
   DAG objects (i.e. blobs, trees, commits, and refs).
2. Fix a bug in the code for merging archives where we did not
   check if the local archives ref existed before trying to
   resolve it.

Previously, the API exposed by the `repository` interface was very
tailored to the tasks that the git-appraise needed to perform for
code reviews. That meant that adding new features which used the
repository in slightly different ways would require rather large
changes to the repository interface.

Under the new API, any feature that simply relies upon the
features of git should be able to use the API without changes.

This was originally part of the pull request to add support for
forks, but is now being split off into a stand-alone pull request
so that it can be reviewed in isolation, and so that it is
available for other work like the proposed change to support
detached comments (#97).

The bug-fix for the archives issue is being included in here
because the refactoring to clean up the repository code is what
made us catch the underlying bug. The issue with that bug is that
we tried to use a single command ('git show') to both check if
the local archives ref (`refs/devtools/archives/reviews`) exists
and to resolve what commit it points to. However, that command
returns an error if the ref does not exist.

This change fixes it by including an explicit check to see if
the ref exists before trying to resolve it.
Omar Jarjur 10 months ago
parent
commit
61c7434649
3 changed files with 577 additions and 167 deletions
  1. 368 155
      repository/git.go
  2. 61 5
      repository/mock_repo.go
  3. 148 7
      repository/repo.go

+ 368 - 155
repository/git.go

@@ -26,27 +26,39 @@ import (
 	"io"
 	"os"
 	"os/exec"
+	"sort"
 	"strconv"
 	"strings"
 )
 
-const branchRefPrefix = "refs/heads/"
+const (
+	branchRefPrefix         = "refs/heads/"
+	notesRefPrefix          = "refs/notes/"
+	devtoolsRefPrefix       = "refs/devtools/"
+	remoteDevtoolsRefPrefix = "refs/remoteDevtools/"
+)
 
 // GitRepo represents an instance of a (local) git repository.
 type GitRepo struct {
 	Path string
 }
 
-// Run the given git command with the given I/O reader/writers, returning an error if it fails.
-func (repo *GitRepo) runGitCommandWithIO(stdin io.Reader, stdout, stderr io.Writer, args ...string) error {
+// Run the given git command with the given I/O reader/writers and environment, returning an error if it fails.
+func (repo *GitRepo) runGitCommandWithIOAndEnv(stdin io.Reader, stdout, stderr io.Writer, env []string, args ...string) error {
 	cmd := exec.Command("git", args...)
 	cmd.Dir = repo.Path
 	cmd.Stdin = stdin
 	cmd.Stdout = stdout
 	cmd.Stderr = stderr
+	cmd.Env = env
 	return cmd.Run()
 }
 
+// Run the given git command with the given I/O reader/writers, returning an error if it fails.
+func (repo *GitRepo) runGitCommandWithIO(stdin io.Reader, stdout, stderr io.Writer, args ...string) error {
+	return repo.runGitCommandWithIOAndEnv(stdin, stdout, stderr, nil, args...)
+}
+
 // Run the given git command and return its stdout, or an error if the command fails.
 func (repo *GitRepo) runGitCommandRaw(args ...string) (string, string, error) {
 	var stdout bytes.Buffer
@@ -67,6 +79,21 @@ func (repo *GitRepo) runGitCommand(args ...string) (string, error) {
 	return stdout, err
 }
 
+// Run the given git command and return its stdout, or an error if the command fails.
+func (repo *GitRepo) runGitCommandWithEnv(env []string, args ...string) (string, error) {
+	var stdout bytes.Buffer
+	var stderr bytes.Buffer
+	err := repo.runGitCommandWithIOAndEnv(nil, &stdout, &stderr, env, args...)
+	if err != nil {
+		stderrStr := strings.TrimSpace(stderr.String())
+		if stderrStr == "" {
+			stderrStr = "Error running git command: " + strings.Join(args, " ")
+		}
+		err = fmt.Errorf(stderrStr)
+	}
+	return strings.TrimSpace(stdout.String()), err
+}
+
 // Run the given git command using the same stdin, stdout, and stderr as the review tool.
 func (repo *GitRepo) runGitCommandInline(args ...string) error {
 	return repo.runGitCommandWithIO(os.Stdin, os.Stdout, os.Stderr, args...)
@@ -86,6 +113,32 @@ func NewGitRepo(path string) (*GitRepo, error) {
 	return nil, err
 }
 
+func (repo *GitRepo) HasRef(ref string) (bool, error) {
+	_, _, err := repo.runGitCommandRaw("show-ref", "--verify", "--quiet", ref)
+	if err == nil {
+		return true, nil
+	}
+	if _, ok := err.(*exec.ExitError); ok {
+		return false, nil
+	}
+	// Got an unexpected error
+	return false, err
+}
+
+// HasObject returns whether or not the repo contains an object with the given hash.
+func (repo *GitRepo) HasObject(hash string) (bool, error) {
+	_, err := repo.runGitCommand("cat-file", "-e", hash)
+	if err == nil {
+		// We verified the object exists
+		return true, nil
+	}
+	if _, ok := err.(*exec.ExitError); ok {
+		return false, nil
+	}
+	// Got an unexpected error
+	return false, err
+}
+
 // GetPath returns the path to the repo.
 func (repo *GitRepo) GetPath() string {
 	return repo.Path
@@ -228,6 +281,8 @@ func (repo GitRepo) GetCommitDetails(ref string) (*CommitDetails, error) {
 	}
 	details.Author = show("%an")
 	details.AuthorEmail = show("%ae")
+	details.Committer = show("%cn")
+	details.CommitterEmail = show("%ce")
 	details.Summary = show("%s")
 	parentsString := show("%P")
 	details.Parents = strings.Split(parentsString, " ")
@@ -280,24 +335,32 @@ func (repo *GitRepo) SwitchToRef(ref string) error {
 
 // mergeArchives merges two archive refs.
 func (repo *GitRepo) mergeArchives(archive, remoteArchive string) error {
-	remoteHash, err := repo.GetCommitHash(remoteArchive)
+	hasRemote, err := repo.HasRef(remoteArchive)
 	if err != nil {
 		return err
 	}
-	if remoteHash == "" {
+	if !hasRemote {
 		// The remote archive does not exist, so we have nothing to do
 		return nil
 	}
+	remoteHash, err := repo.GetCommitHash(remoteArchive)
+	if err != nil {
+		return err
+	}
 
-	archiveHash, err := repo.GetCommitHash(archive)
+	hasLocal, err := repo.HasRef(archive)
 	if err != nil {
 		return err
 	}
-	if archiveHash == "" {
+	if !hasLocal {
 		// The local archive does not exist, so we merely need to set it
 		_, err := repo.runGitCommand("update-ref", archive, remoteHash)
 		return err
 	}
+	archiveHash, err := repo.GetCommitHash(archive)
+	if err != nil {
+		return err
+	}
 
 	isAncestor, err := repo.IsAncestor(archiveHash, remoteHash)
 	if err != nil {
@@ -464,6 +527,148 @@ func (repo *GitRepo) ListCommitsBetween(from, to string) ([]string, error) {
 	return strings.Split(out, "\n"), nil
 }
 
+// StoreBlob writes the given file to the repository and returns its hash.
+func (repo *GitRepo) StoreBlob(contents string) (string, error) {
+	stdin := strings.NewReader(contents)
+	var stdout bytes.Buffer
+	var stderr bytes.Buffer
+	args := []string{"hash-object", "-w", "-t", "blob", "--stdin"}
+	err := repo.runGitCommandWithIO(stdin, &stdout, &stderr, args...)
+	if err != nil {
+		message := strings.TrimSpace(stderr.String())
+		return "", fmt.Errorf("failure storing a git blob, %v: %q", err, message)
+	}
+	return strings.TrimSpace(stdout.String()), nil
+}
+
+// StoreTree writes the given file tree to the repository and returns its hash.
+func (repo *GitRepo) StoreTree(contents map[string]TreeChild) (string, error) {
+	var lines []string
+	for path, obj := range contents {
+		objHash, err := obj.Store(repo)
+		if err != nil {
+			return "", err
+		}
+		mode := "040000"
+		if obj.Type() == "blob" {
+			mode = "100644"
+		}
+		line := fmt.Sprintf("%s %s %s\t%s", mode, obj.Type(), objHash, path)
+		lines = append(lines, line)
+	}
+	stdin := strings.NewReader(strings.Join(lines, "\n"))
+	var stdout bytes.Buffer
+	var stderr bytes.Buffer
+	args := []string{"mktree"}
+	err := repo.runGitCommandWithIO(stdin, &stdout, &stderr, args...)
+	if err != nil {
+		message := strings.TrimSpace(stderr.String())
+		return "", fmt.Errorf("failure storing a git tree, %v: %q", err, message)
+	}
+	return strings.TrimSpace(stdout.String()), nil
+}
+
+func (repo *GitRepo) readBlob(objHash string) (*Blob, error) {
+	out, err := repo.runGitCommand("cat-file", "-p", objHash)
+	if err != nil {
+		return nil, fmt.Errorf("failure reading the file contents of %q: %v", objHash, err)
+	}
+	return &Blob{contents: out, savedHashes: map[Repo]string{repo: objHash}}, nil
+}
+
+func (repo *GitRepo) ReadTree(ref string) (*Tree, error) {
+	return repo.readTreeWithHash(ref, "")
+}
+
+func (repo *GitRepo) readTreeWithHash(ref, hash string) (*Tree, error) {
+	out, err := repo.runGitCommand("ls-tree", "--full-tree", ref)
+	if err != nil {
+		return nil, fmt.Errorf("failure listing the file contents of %q: %v", ref, err)
+	}
+	contents := make(map[string]TreeChild)
+	if len(out) == 0 {
+		// This is possible if the tree is empty
+		return NewTree(contents), nil
+	}
+	for _, line := range strings.Split(out, "\n") {
+		lineParts := strings.Split(line, "\t")
+		if len(lineParts) != 2 {
+			return nil, fmt.Errorf("malformed ls-tree output line: %q", line)
+		}
+		path := lineParts[1]
+		lineParts = strings.Split(lineParts[0], " ")
+		if len(lineParts) != 3 {
+			return nil, fmt.Errorf("malformed ls-tree output line: %q", line)
+		}
+		objType := lineParts[1]
+		objHash := lineParts[2]
+		var child TreeChild
+		if objType == "tree" {
+			child, err = repo.readTreeWithHash(objHash, objHash)
+		} else if objType == "blob" {
+			child, err = repo.readBlob(objHash)
+		} else {
+			return nil, fmt.Errorf("unrecognized tree object type: %q", objType)
+		}
+		if err != nil {
+			return nil, fmt.Errorf("failed to read a tree child object: %v", err)
+		}
+		contents[path] = child
+	}
+	t := NewTree(contents)
+	t.savedHashes[repo] = hash
+	return t, nil
+}
+
+// CreateCommit creates a commit object and returns its hash.
+func (repo *GitRepo) CreateCommit(details *CommitDetails) (string, error) {
+	args := []string{"commit-tree", details.Tree, "-m", details.Summary}
+	for _, parent := range details.Parents {
+		args = append(args, "-p", parent)
+	}
+	var env []string
+	if details.Author != "" {
+		env = append(env, fmt.Sprintf("GIT_AUTHOR_NAME=%s", details.Author))
+	}
+	if details.AuthorEmail != "" {
+		env = append(env, fmt.Sprintf("GIT_AUTHOR_EMAIL=%s", details.AuthorEmail))
+	}
+	if details.AuthorTime != "" {
+		env = append(env, fmt.Sprintf("GIT_AUTHOR_DATE=%s", details.AuthorTime))
+	}
+	if details.Committer != "" {
+		env = append(env, fmt.Sprintf("GIT_COMMITTER_NAME=%s", details.Committer))
+	}
+	if details.CommitterEmail != "" {
+		env = append(env, fmt.Sprintf("GIT_COMMITTER_EMAIL=%s", details.CommitterEmail))
+	}
+	if details.Time != "" {
+		env = append(env, fmt.Sprintf("GIT_COMMITTER_DATE=%s", details.Time))
+	}
+	return repo.runGitCommandWithEnv(env, args...)
+}
+
+// CreateCommitWithTree creates a commit object with the given tree and returns its hash.
+func (repo *GitRepo) CreateCommitWithTree(details *CommitDetails, t *Tree) (string, error) {
+	treeHash, err := repo.StoreTree(t.Contents())
+	if err != nil {
+		return "", fmt.Errorf("failure storing a tree: %v", err)
+	}
+	details.Tree = treeHash
+	return repo.CreateCommit(details)
+}
+
+// SetRef sets the commit pointed to by the specified ref to `newCommitHash`,
+// iff the ref currently points `previousCommitHash`.
+func (repo *GitRepo) SetRef(ref, newCommitHash, previousCommitHash string) error {
+	args := []string{"update-ref", ref, newCommitHash}
+	if previousCommitHash != "" {
+		args = append(args, previousCommitHash)
+	}
+	_, err := repo.runGitCommand(args...)
+	return err
+}
+
 // GetNotes uses the "git" command-line tool to read the notes from the given ref for a given revision.
 func (repo *GitRepo) GetNotes(notesRef, revision string) []Note {
 	var notes []Note
@@ -745,6 +950,28 @@ func (repo *GitRepo) ListNotedRevisions(notesRef string) []string {
 	return revisions
 }
 
+// Remotes returns a list of the remotes.
+func (repo *GitRepo) Remotes() ([]string, error) {
+	remotes, err := repo.runGitCommand("remote")
+	if err != nil {
+		return nil, err
+	}
+	remoteNames := strings.Split(remotes, "\n")
+	var result []string
+	for _, name := range remoteNames {
+		result = append(result, strings.TrimSpace(name))
+	}
+	sort.Strings(result)
+	return result, nil
+}
+
+// Fetch fetches from the given remote using the supplied refspecs.
+func (repo *GitRepo) Fetch(remote string, refspecs ...string) error {
+	args := []string{"fetch", remote}
+	args = append(args, refspecs...)
+	return repo.runGitCommandInline(args...)
+}
+
 // PushNotes pushes git notes to a remote repo.
 func (repo *GitRepo) PushNotes(remote, notesRefPattern string) error {
 	refspec := fmt.Sprintf("%s:%s", notesRefPattern, notesRefPattern)
@@ -769,27 +996,59 @@ func (repo *GitRepo) PushNotesAndArchive(remote, notesRefPattern, archiveRefPatt
 	return nil
 }
 
+func (repo *GitRepo) getRefHashes(refPattern string) (map[string]string, error) {
+	if !strings.HasSuffix(refPattern, "/*") {
+		return nil, fmt.Errorf("unsupported ref pattern %q", refPattern)
+	}
+	refPrefix := strings.TrimSuffix(refPattern, "*")
+	showRef, err := repo.runGitCommand("show-ref")
+	if err != nil {
+		return nil, err
+	}
+	refsMap := make(map[string]string)
+	for _, line := range strings.Split(showRef, "\n") {
+		lineParts := strings.Split(line, " ")
+		if len(lineParts) != 2 {
+			return nil, fmt.Errorf("unexpected line in output of `git show-ref`: %q", line)
+		}
+		if strings.HasPrefix(lineParts[1], refPrefix) {
+			refsMap[lineParts[1]] = lineParts[0]
+		}
+	}
+	return refsMap, nil
+}
+
 func getRemoteNotesRef(remote, localNotesRef string) string {
-	relativeNotesRef := strings.TrimPrefix(localNotesRef, "refs/notes/")
-	return "refs/notes/" + remote + "/" + relativeNotesRef
+	// Note: The pattern for remote notes deviates from that of remote heads and devtools,
+	// because the git command line tool requires all notes refs to be located under the
+	// "refs/notes/" prefix.
+	//
+	// Because of that, we make the remote refs a subset of the local refs instead of
+	// a parallel tree, which is the pattern used for heads and devtools.
+	//
+	// E.G. ("refs/notes/..." -> "refs/notes/remotes/<remote>/...")
+	//   versus ("refs/heads/..." -> "refs/remotes/<remote>/...")
+	relativeNotesRef := strings.TrimPrefix(localNotesRef, notesRefPrefix)
+	return notesRefPrefix + "remotes/" + remote + "/" + relativeNotesRef
+}
+
+func getLocalNotesRef(remote, remoteNotesRef string) string {
+	relativeNotesRef := strings.TrimPrefix(remoteNotesRef, notesRefPrefix+"remotes/"+remote+"/")
+	return notesRefPrefix + relativeNotesRef
 }
 
 // MergeNotes merges in the remote's state of the notes reference into the
 // local repository's.
 func (repo *GitRepo) MergeNotes(remote, notesRefPattern string) error {
-	remoteRefs, err := repo.runGitCommand("ls-remote", remote, notesRefPattern)
+	remoteRefPattern := getRemoteNotesRef(remote, notesRefPattern)
+	refsMap, err := repo.getRefHashes(remoteRefPattern)
 	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)
-			_, err := repo.runGitCommand("notes", "--ref", ref, "merge", remoteRef, "-s", "cat_sort_uniq")
-			if err != nil {
-				return err
-			}
+	for remoteRef := range refsMap {
+		localRef := getLocalNotesRef(remote, remoteRef)
+		if _, err := repo.runGitCommand("notes", "--ref", localRef, "merge", remoteRef, "-s", "cat_sort_uniq"); err != nil {
+			return err
 		}
 	}
 	return nil
@@ -801,7 +1060,7 @@ func (repo *GitRepo) MergeNotes(remote, notesRefPattern string) error {
 func (repo *GitRepo) PullNotes(remote, notesRefPattern string) error {
 	remoteNotesRefPattern := getRemoteNotesRef(remote, notesRefPattern)
 	fetchRefSpec := fmt.Sprintf("+%s:%s", notesRefPattern, remoteNotesRefPattern)
-	err := repo.runGitCommandInline("fetch", remote, fetchRefSpec)
+	err := repo.Fetch(remote, fetchRefSpec)
 	if err != nil {
 		return err
 	}
@@ -809,68 +1068,33 @@ func (repo *GitRepo) PullNotes(remote, notesRefPattern string) error {
 	return repo.MergeNotes(remote, notesRefPattern)
 }
 
-func getRemoteArchiveRef(remote, archiveRefPattern string) string {
-	relativeArchiveRef := strings.TrimPrefix(archiveRefPattern, "refs/devtools/archives/")
-	return "refs/devtools/remoteArchives/" + remote + "/" + relativeArchiveRef
+func getRemoteDevtoolsRef(remote, devtoolsRefPattern string) string {
+	relativeRef := strings.TrimPrefix(devtoolsRefPattern, devtoolsRefPrefix)
+	return remoteDevtoolsRefPrefix + remote + "/" + relativeRef
+}
+
+func getLocalDevtoolsRef(remote, remoteDevtoolsRef string) string {
+	relativeRef := strings.TrimPrefix(remoteDevtoolsRef, remoteDevtoolsRefPrefix+remote+"/")
+	return devtoolsRefPrefix + relativeRef
 }
 
 // MergeArchives merges in the remote's state of the archives reference into
 // the local repository's.
 func (repo *GitRepo) MergeArchives(remote, archiveRefPattern string) error {
-	remoteRefs, err := repo.runGitCommand("ls-remote", remote, archiveRefPattern)
+	remoteRefPattern := getRemoteDevtoolsRef(remote, archiveRefPattern)
+	refsMap, err := repo.getRefHashes(remoteRefPattern)
 	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 := getRemoteArchiveRef(remote, ref)
-			if err := repo.mergeArchives(ref, remoteRef); err != nil {
-				return err
-			}
+	for remoteRef := range refsMap {
+		localRef := getLocalDevtoolsRef(remote, remoteRef)
+		if err := repo.mergeArchives(localRef, remoteRef); err != nil {
+			return err
 		}
 	}
 	return nil
 }
 
-func (repo *GitRepo) fetchNotes(remote, notesRefPattern,
-	archiveRefPattern string) error {
-
-	remoteArchiveRef := getRemoteArchiveRef(remote, archiveRefPattern)
-	archiveFetchRefSpec := fmt.Sprintf("+%s:%s", archiveRefPattern, remoteArchiveRef)
-
-	remoteNotesRefPattern := getRemoteNotesRef(remote, notesRefPattern)
-	notesFetchRefSpec := fmt.Sprintf("+%s:%s", notesRefPattern, remoteNotesRefPattern)
-
-	return repo.runGitCommandInline("fetch", remote, notesFetchRefSpec, archiveFetchRefSpec)
-}
-
-// PullNotesAndArchive fetches the contents of the notes and archives refs from
-// a remote repo, and merges them with the corresponding local refs.
-//
-// For notes refs, we assume that every note can be automatically merged using
-// the 'cat_sort_uniq' strategy (the git-appraise schemas fit that requirement),
-// so we automatically merge the remote notes into the local notes.
-//
-// For "archive" refs, they are expected to be used solely for maintaining
-// reachability of commits that are part of the history of any reviews,
-// so we do not maintain any consistency with their tree objects. Instead,
-// we merely ensure that their history graph includes every commit that we
-// intend to keep.
-func (repo *GitRepo) PullNotesAndArchive(remote, notesRefPattern, archiveRefPattern string) error {
-	err := repo.fetchNotes(remote, notesRefPattern, archiveRefPattern)
-	if err != nil {
-		return err
-	}
-
-	err = repo.MergeNotes(remote, notesRefPattern)
-	if err != nil {
-		return err
-	}
-	return repo.MergeArchives(remote, archiveRefPattern)
-}
-
 // FetchAndReturnNewReviewHashes fetches the notes "branches" and then susses
 // out the IDs (the revision the review points to) of any new reviews, then
 // returns that list of IDs.
@@ -878,110 +1102,99 @@ func (repo *GitRepo) PullNotesAndArchive(remote, notesRefPattern, archiveRefPatt
 // This is accomplished by determining which files in the notes tree have
 // changed because the _names_ of these files correspond to the revisions they
 // point to.
-func (repo *GitRepo) FetchAndReturnNewReviewHashes(remote, notesRefPattern,
-	archiveRefPattern string) ([]string, error) {
-
-	// Record the current state of the reviews and comments refs.
-	var (
-		getAllRevs, getAllComs    bool
-		reviewsList, commentsList []string
-	)
-	reviewBeforeHash, err := repo.GetCommitHash(
-		"notes/" + remote + "/devtools/reviews")
-	getAllRevs = err != nil
+func (repo *GitRepo) FetchAndReturnNewReviewHashes(remote, notesRefPattern string, devtoolsRefPatterns ...string) ([]string, error) {
+	for _, refPattern := range devtoolsRefPatterns {
+		if !strings.HasPrefix(refPattern, devtoolsRefPrefix) {
+			return nil, fmt.Errorf("Unsupported devtools ref: %q", refPattern)
+		}
+	}
+	remoteNotesRefPattern := getRemoteNotesRef(remote, notesRefPattern)
+	notesFetchRefSpec := fmt.Sprintf("+%s:%s", notesRefPattern, remoteNotesRefPattern)
 
-	commentBeforeHash, err := repo.GetCommitHash(
-		"notes/" + remote + "/devtools/discuss")
-	getAllComs = err != nil
+	localDevtoolsRefPattern := devtoolsRefPrefix + "*"
+	remoteDevtoolsRefPattern := getRemoteDevtoolsRef(remote, localDevtoolsRefPattern)
+	devtoolsFetchRefSpec := fmt.Sprintf("+%s:%s", localDevtoolsRefPattern, remoteDevtoolsRefPattern)
 
-	// Update them from the remote.
-	err = repo.fetchNotes(remote, notesRefPattern, archiveRefPattern)
+	// Prior to fetching, record the current state of the remote notes refs
+	priorRefHashes, err := repo.getRefHashes(remoteNotesRefPattern)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failure reading the existing ref hashes for the remote %q: %v", remote, err)
 	}
 
-	// Now, if either of these are new refs, we just use the whole tree at that
-	// new ref. Otherwise we see which reviews or comments changed and collect
-	// them into a list.
-	if getAllRevs {
-		hash, err := repo.GetCommitHash(
-			"notes/" + remote + "/devtools/reviews")
-		// It is possible that even after we've pulled that this ref still
-		// isn't present (because there are no reviews yet).
-		if err == nil {
-			rvws, err := repo.runGitCommand("ls-tree", "-r", "--name-only",
-				hash)
-			if err != nil {
-				return nil, err
-			}
-			reviewsList = strings.Split(strings.Replace(rvws, "/", "", -1),
-				"\n")
-		}
-	} else {
-		reviewAfterHash, err := repo.GetCommitHash(
-			"notes/" + remote + "/devtools/reviews")
-		if err != nil {
-			return nil, err
-		}
+	if err := repo.Fetch(remote, notesFetchRefSpec, devtoolsFetchRefSpec); err != nil {
+		return nil, fmt.Errorf("failure fetching from the remote %q: %v", remote, err)
+	}
 
-		// Only run through this if the fetch fetched new revisions.
-		// Otherwise leave reviewsList as its default value, an empty slice
-		// of strings.
-		if reviewBeforeHash != reviewAfterHash {
-			newReviewsRaw, err := repo.runGitCommand("diff", "--name-only",
-				reviewBeforeHash, reviewAfterHash)
-			if err != nil {
-				return nil, err
-			}
-			reviewsList = strings.Split(strings.Replace(newReviewsRaw,
-				"/", "", -1), "\n")
-		}
+	// After fetching, record the updated state of the remote notes refs
+	updatedRefHashes, err := repo.getRefHashes(remoteNotesRefPattern)
+	if err != nil {
+		return nil, fmt.Errorf("failure reading the updated ref hashes for the remote %q: %v", remote, err)
 	}
 
-	if getAllComs {
-		hash, err := repo.GetCommitHash(
-			"notes/" + remote + "/devtools/discuss")
-		// It is possible that even after we've pulled that this ref still
-		// isn't present (because there are no comments yet).
-		if err == nil {
-			rvws, err := repo.runGitCommand("ls-tree", "-r", "--name-only",
-				hash)
-			if err != nil {
-				return nil, err
-			}
-			commentsList = strings.Split(strings.Replace(rvws, "/", "", -1),
-				"\n")
+	// Now that we have our two lists, we need to merge them.
+	updatedReviewSet := make(map[string]struct{})
+	for ref, hash := range updatedRefHashes {
+		priorHash, ok := priorRefHashes[ref]
+		if priorHash == hash {
+			// Nothing has changed for this ref
+			continue
+		}
+		var notes string
+		var err error
+		if !ok {
+			// This is a new ref, so include every noted object
+			notes, err = repo.runGitCommand("ls-tree", "-r", "--name-only", hash)
+		} else {
+			notes, err = repo.runGitCommand("diff", "--name-only", priorHash, hash)
 		}
-	} else {
-		commentAfterHash, err := repo.GetCommitHash(
-			"notes/" + remote + "/devtools/discuss")
 		if err != nil {
 			return nil, err
 		}
-
-		// Only run through this if the fetch fetched new revisions.
-		// Otherwise leave commentsList as its default value, an empty slice
-		// of strings.
-		if commentBeforeHash != commentAfterHash {
-			newCommentsRaw, err := repo.runGitCommand("diff", "--name-only",
-				commentBeforeHash, commentAfterHash)
-			if err != nil {
-				return nil, err
-			}
-			commentsList = strings.Split(strings.Replace(newCommentsRaw,
-				"/", "", -1), "\n")
+		// The name of the review matches the name of the notes tree entry, with slashes removed
+		reviews := strings.Split(strings.Replace(notes, "/", "", -1), "\n")
+		for _, review := range reviews {
+			updatedReviewSet[review] = struct{}{}
 		}
 	}
 
-	// Now that we have our two lists, we need to merge them.
-	updatedReviewSet := make(map[string]struct{})
-	for _, hash := range append(reviewsList, commentsList...) {
-		updatedReviewSet[hash] = struct{}{}
-	}
-
 	updatedReviews := make([]string, 0, len(updatedReviewSet))
 	for key, _ := range updatedReviewSet {
 		updatedReviews = append(updatedReviews, key)
 	}
 	return updatedReviews, nil
 }
+
+// PullNotesAndArchive fetches the contents of the notes and archives refs from
+// a remote repo, and merges them with the corresponding local refs.
+//
+// For notes refs, we assume that every note can be automatically merged using
+// the 'cat_sort_uniq' strategy (the git-appraise schemas fit that requirement),
+// so we automatically merge the remote notes into the local notes.
+//
+// For "archive" refs, they are expected to be used solely for maintaining
+// reachability of commits that are part of the history of any reviews,
+// so we do not maintain any consistency with their tree objects. Instead,
+// we merely ensure that their history graph includes every commit that we
+// intend to keep.
+func (repo *GitRepo) PullNotesAndArchive(remote, notesRefPattern, archiveRefPattern string) error {
+	if _, err := repo.FetchAndReturnNewReviewHashes(remote, notesRefPattern, archiveRefPattern); err != nil {
+		return fmt.Errorf("failure fetching from the remote %q: %v", remote, err)
+	}
+	if err := repo.MergeArchives(remote, archiveRefPattern); err != nil {
+		return fmt.Errorf("failure merging archives from the remote %q: %v", remote, err)
+	}
+	if err := repo.MergeNotes(remote, notesRefPattern); err != nil {
+		return fmt.Errorf("failure merging notes from the remote %q: %v", remote, err)
+	}
+	return nil
+}
+
+// Push pushes the given refs to a remote repo.
+func (repo *GitRepo) Push(remote string, refSpecs ...string) error {
+	pushArgs := append([]string{"push", remote}, refSpecs...)
+	err := repo.runGitCommandInline(pushArgs...)
+	if err != nil {
+		return fmt.Errorf("Failed to push the local refs to the remote '%s': %v", remote, err)
+	}
+	return nil
+}

+ 61 - 5
repository/mock_repo.go

@@ -19,6 +19,7 @@ package repository
 import (
 	"crypto/sha1"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"strings"
 )
@@ -222,6 +223,19 @@ func (r *mockRepoForTest) resolveLocalRef(ref string) (string, error) {
 	return "", fmt.Errorf("The ref %q does not exist", ref)
 }
 
+// HasRef checks whether the specified ref exists in the repo.
+func (r *mockRepoForTest) HasRef(ref string) (bool, error) {
+	if _, ok := r.Refs[ref]; !ok {
+		return false, nil
+	}
+	return true, nil
+}
+
+// HasObject reports whether or not the repo contains an object with the given hash
+func (r *mockRepoForTest) HasObject(hash string) (bool, error) {
+	return false, errors.New("Not implemented")
+}
+
 // VerifyCommit verifies that the supplied hash points to a known commit.
 func (r *mockRepoForTest) VerifyCommit(hash string) error {
 	if _, ok := r.Commits[hash]; !ok {
@@ -516,6 +530,37 @@ func (r *mockRepoForTest) ListCommitsBetween(from, to string) ([]string, error)
 	return commits, nil
 }
 
+// StoreBlob writes the given file to the repository and returns its hash.
+func (r *mockRepoForTest) StoreBlob(contents string) (string, error) {
+	return "", fmt.Errorf("not implemented")
+}
+
+// StoreTree writes the given file tree to the repository and returns its hash.
+func (r *mockRepoForTest) StoreTree(contents map[string]TreeChild) (string, error) {
+	return "", fmt.Errorf("not implemented")
+}
+
+// ReadTree reads the file tree pointed to by the given ref or hash from the repository.
+func (r *mockRepoForTest) ReadTree(ref string) (*Tree, error) {
+	return nil, fmt.Errorf("not implemented")
+}
+
+// CreateCommit creates a commit object and returns its hash.
+func (r *mockRepoForTest) CreateCommit(details *CommitDetails) (string, error) {
+	return "", fmt.Errorf("not implemented")
+}
+
+// CreateCommitWithTree creates a commit object with the given tree and returns its hash.
+func (r *mockRepoForTest) CreateCommitWithTree(details *CommitDetails, t *Tree) (string, error) {
+	return "", fmt.Errorf("not implemented")
+}
+
+// SetRef sets the commit pointed to by the specified ref to `newCommitHash`,
+// iff the ref currently points `previousCommitHash`.
+func (r *mockRepoForTest) SetRef(ref, newCommitHash, previousCommitHash string) error {
+	return fmt.Errorf("not implemented")
+}
+
 // GetNotes reads the notes from the given ref that annotate the given revision.
 func (r *mockRepoForTest) GetNotes(notesRef, revision string) []Note {
 	notesText := r.Notes[notesRef][revision]
@@ -558,6 +603,14 @@ func (r *mockRepoForTest) ListNotedRevisions(notesRef string) []string {
 	return revisions
 }
 
+// Remotes returns a list of the remotes.
+func (r *mockRepoForTest) Remotes() ([]string, error) {
+	return []string{"origin"}, nil
+}
+
+// Fetch fetches from the given remote using the supplied refspecs.
+func (r *mockRepoForTest) Fetch(remote string, refspecs ...string) error { return nil }
+
 // PushNotes pushes git notes to a remote repo.
 func (r *mockRepoForTest) PushNotes(remote, notesRefPattern string) error { return nil }
 
@@ -589,14 +642,13 @@ func (r *mockRepoForTest) PullNotesAndArchive(remote, notesRefPattern, archiveRe
 
 // MergeNotes merges in the remote's state of the archives reference into
 // the local repository's.
-func (repo *mockRepoForTest) MergeNotes(remote, notesRefPattern string) error {
+func (r *mockRepoForTest) MergeNotes(remote, notesRefPattern string) error {
 	return nil
 }
 
 // MergeArchives merges in the remote's state of the archives reference into
 // the local repository's.
-func (repo *mockRepoForTest) MergeArchives(remote,
-	archiveRefPattern string) error {
+func (r *mockRepoForTest) MergeArchives(remote, archiveRefPattern string) error {
 	return nil
 }
 
@@ -607,7 +659,11 @@ func (repo *mockRepoForTest) MergeArchives(remote,
 // This is accomplished by determining which files in the notes tree have
 // changed because the _names_ of these files correspond to the revisions they
 // point to.
-func (repo *mockRepoForTest) FetchAndReturnNewReviewHashes(remote, notesRefPattern,
-	archiveRefPattern string) ([]string, error) {
+func (r *mockRepoForTest) FetchAndReturnNewReviewHashes(remote, notesRefPattern string, devtoolsRefPatterns ...string) ([]string, error) {
 	return nil, nil
 }
+
+// Push pushes the given refs to a remote repo.
+func (r *mockRepoForTest) Push(remote string, refPattern ...string) error {
+	return nil
+}

+ 148 - 7
repository/repo.go

@@ -17,17 +17,123 @@ limitations under the License.
 // Package repository contains helper methods for working with a Git repo.
 package repository
 
+import (
+	"crypto/sha1"
+	"fmt"
+)
+
 // Note represents the contents of a git-note
 type Note []byte
 
+// Hash returns a hash of the given note
+func (n Note) Hash() string {
+	return fmt.Sprintf("%x", sha1.Sum([]byte(n)))
+}
+
 // CommitDetails represents the contents of a commit.
 type CommitDetails struct {
-	Author      string   `json:"author,omitempty"`
-	AuthorEmail string   `json:"authorEmail,omitempty"`
-	Tree        string   `json:"tree,omitempty"`
-	Time        string   `json:"time,omitempty"`
-	Parents     []string `json:"parents,omitempty"`
-	Summary     string   `json:"summary,omitempty"`
+	Author         string   `json:"author,omitempty"`
+	AuthorEmail    string   `json:"authorEmail,omitempty"`
+	AuthorTime     string   `json:"authorTime,omitempty"`
+	Committer      string   `json:"committer,omitempty"`
+	CommitterEmail string   `json:"committerEmail,omitempty"`
+	Tree           string   `json:"tree,omitempty"`
+	Time           string   `json:"time,omitempty"`
+	Parents        []string `json:"parents,omitempty"`
+	Summary        string   `json:"summary,omitempty"`
+}
+
+type TreeChild interface {
+	// Type returns the type of the child object (e.g. "blob" vs. "tree").
+	Type() string
+
+	// Store writes the object to the repository and returns its hash.
+	Store(repo Repo) (string, error)
+}
+
+// Blob represents a (non-directory) file stored in a repository.
+//
+// Blob objects are immutable.
+type Blob struct {
+	savedHashes map[Repo]string
+	contents    string
+}
+
+// NewBlob returns a new *Blob object tied to the given repo with the given contents.
+func NewBlob(contents string) *Blob {
+	savedHashes := make(map[Repo]string)
+	return &Blob{
+		savedHashes: savedHashes,
+		contents:    contents,
+	}
+}
+
+func (b *Blob) Type() string {
+	return "blob"
+}
+
+func (b *Blob) Store(repo Repo) (string, error) {
+	if savedHash := b.savedHashes[repo]; savedHash != "" {
+		return savedHash, nil
+	}
+	savedHash, err := repo.StoreBlob(b.Contents())
+	if err == nil && savedHash != "" {
+		b.savedHashes[repo] = savedHash
+	}
+	return savedHash, nil
+}
+
+// Contents returns the contents of the blob
+func (b *Blob) Contents() string {
+	return b.contents
+}
+
+// Tree represents a directory stored in a repository.
+//
+// Tree objects are immutable.
+type Tree struct {
+	savedHashes map[Repo]string
+	contents    map[string]TreeChild
+}
+
+// NewTree constructs a new *Tree object tied to the given repo with the given contents.
+func NewTree(contents map[string]TreeChild) *Tree {
+	immutableContents := make(map[string]TreeChild)
+	for k, v := range contents {
+		immutableContents[k] = v
+	}
+	savedHashes := make(map[Repo]string)
+	return &Tree{
+		savedHashes: savedHashes,
+		contents:    immutableContents,
+	}
+}
+
+func (t *Tree) Type() string {
+	return "tree"
+}
+
+func (t *Tree) Store(repo Repo) (string, error) {
+	if savedHash := t.savedHashes[repo]; savedHash != "" {
+		return savedHash, nil
+	}
+	savedHash, err := repo.StoreTree(t.Contents())
+	if err == nil && savedHash != "" {
+		t.savedHashes[repo] = savedHash
+	}
+	return savedHash, nil
+}
+
+// Contents returns a map of the child elements of the tree.
+//
+// The returned map is mutable, but changes made to it have no
+// effect on the underly Tree object.
+func (t *Tree) Contents() map[string]TreeChild {
+	result := make(map[string]TreeChild)
+	for k, v := range t.contents {
+		result[k] = v
+	}
+	return result
 }
 
 // Repo represents a source code repository.
@@ -54,6 +160,12 @@ type Repo interface {
 	// HasUncommittedChanges returns true if there are local, uncommitted changes.
 	HasUncommittedChanges() (bool, error)
 
+	// HasRef checks whether the specified ref exists in the repo.
+	HasRef(ref string) (bool, error)
+
+	// HasObject returns whether or not the repo contains an object with the given hash.
+	HasObject(hash string) (bool, error)
+
 	// VerifyCommit verifies that the supplied hash points to a known commit.
 	VerifyCommit(hash string) error
 
@@ -162,6 +274,25 @@ type Repo interface {
 	// The generated list is in chronological order (with the oldest commit first).
 	ListCommitsBetween(from, to string) ([]string, error)
 
+	// StoreBlob writes the given file contents to the repository and returns its hash.
+	StoreBlob(contents string) (string, error)
+
+	// StoreTree writes the given file tree contents to the repository and returns its hash.
+	StoreTree(contents map[string]TreeChild) (string, error)
+
+	// ReadTree reads the file tree pointed to by the given ref or hash from the repository.
+	ReadTree(ref string) (*Tree, error)
+
+	// CreateCommit creates a commit object and returns its hash.
+	CreateCommit(details *CommitDetails) (string, error)
+
+	// CreateCommitWithTree creates a commit object with the given tree and returns its hash.
+	CreateCommitWithTree(details *CommitDetails, t *Tree) (string, error)
+
+	// SetRef sets the commit pointed to by the specified ref to `newCommitHash`,
+	// iff the ref currently points `previousCommitHash`.
+	SetRef(ref, newCommitHash, previousCommitHash string) error
+
 	// GetNotes reads the notes from the given ref that annotate the given revision.
 	GetNotes(notesRef, revision string) []Note
 
@@ -178,6 +309,12 @@ type Repo interface {
 	// ListNotedRevisions returns the collection of revisions that are annotated by notes in the given ref.
 	ListNotedRevisions(notesRef string) []string
 
+	// Remotes returns a list of the remotes.
+	Remotes() ([]string, error)
+
+	// Fetch fetches from the given remote using the supplied refspecs.
+	Fetch(remote string, refspecs ...string) error
+
 	// PushNotes pushes git notes to a remote repo.
 	PushNotes(remote, notesRefPattern string) error
 
@@ -206,6 +343,7 @@ type Repo interface {
 	// MergeNotes merges in the remote's state of the archives reference into
 	// the local repository's.
 	MergeNotes(remote, notesRefPattern string) error
+
 	// MergeArchives merges in the remote's state of the archives reference
 	// into the local repository's.
 	MergeArchives(remote, archiveRefPattern string) error
@@ -217,5 +355,8 @@ type Repo interface {
 	// This is accomplished by determining which files in the notes tree have
 	// changed because the _names_ of these files correspond to the revisions
 	// they point to.
-	FetchAndReturnNewReviewHashes(remote, notesRefPattern, archiveRefPattern string) ([]string, error)
+	FetchAndReturnNewReviewHashes(remote, notesRefPattern string, devtoolsRefPatterns ...string) ([]string, error)
+
+	// Push pushes the given refs to a remote repo.
+	Push(remote string, refPattern ...string) error
 }