Browse Source

Merge branch 'ojarjur/repo-changes' into ojarjur/detached-comments

Omar Jarjur 8 months ago
parent
commit
99c2b6cf20
3 changed files with 125 additions and 58 deletions
  1. 50 38
      repository/git.go
  2. 9 3
      repository/mock_repo.go
  3. 66 17
      repository/repo.go

+ 50 - 38
repository/git.go

@@ -32,8 +32,10 @@ import (
 )
 
 const (
-	branchRefPrefix   = "refs/heads/"
-	devtoolsRefPrefix = "refs/devtools/"
+	branchRefPrefix         = "refs/heads/"
+	notesRefPrefix          = "refs/notes/"
+	devtoolsRefPrefix       = "refs/devtools/"
+	remoteDevtoolsRefPrefix = "refs/remoteDevtools/"
 )
 
 // GitRepo represents an instance of a (local) git repository.
@@ -123,6 +125,20 @@ func (repo *GitRepo) HasRef(ref string) (bool, 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
@@ -518,14 +534,8 @@ func (repo *GitRepo) ListCommitsBetween(from, to string) ([]string, error) {
 }
 
 // StoreBlob writes the given file to the repository and returns its hash.
-func (repo *GitRepo) StoreBlob(b *Blob) (string, error) {
-	if b.savedHash != "" {
-		if _, existsErr := repo.runGitCommand("cat-file", "-e", b.savedHash); existsErr == nil {
-			// We verified the blob already exists
-			return b.savedHash, nil
-		}
-	}
-	stdin := strings.NewReader(b.Contents)
+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"}
@@ -538,15 +548,9 @@ func (repo *GitRepo) StoreBlob(b *Blob) (string, error) {
 }
 
 // StoreTree writes the given file tree to the repository and returns its hash.
-func (repo *GitRepo) StoreTree(t *Tree) (string, error) {
-	if t.savedHash != "" {
-		if _, existsErr := repo.runGitCommand("cat-file", "-e", t.savedHash); existsErr == nil {
-			// We verified the tree already exists
-			return t.savedHash, nil
-		}
-	}
+func (repo *GitRepo) StoreTree(contents map[string]TreeChild) (string, error) {
 	var lines []string
-	for path, obj := range t.contents {
+	for path, obj := range contents {
 		objHash, err := obj.Store(repo)
 		if err != nil {
 			return "", err
@@ -575,7 +579,7 @@ func (repo *GitRepo) readBlob(objHash string) (*Blob, error) {
 	if err != nil {
 		return nil, fmt.Errorf("failure reading the file contents of %q: %v", objHash, err)
 	}
-	return &Blob{Contents: out, savedHash: objHash}, nil
+	return &Blob{contents: out, savedHashes: map[Repo]string{repo: objHash}}, nil
 }
 
 func (repo *GitRepo) ReadTree(ref string) (*Tree, error) {
@@ -587,12 +591,11 @@ func (repo *GitRepo) readTreeWithHash(ref, hash string) (*Tree, error) {
 	if err != nil {
 		return nil, fmt.Errorf("failure listing the file contents of %q: %v", ref, err)
 	}
-	t := NewTree()
+	contents := make(map[string]TreeChild)
 	if len(out) == 0 {
 		// This is possible if the tree is empty
-		return t, nil
+		return NewTree(contents), nil
 	}
-	contents := t.Contents()
 	for _, line := range strings.Split(out, "\n") {
 		lineParts := strings.Split(line, "\t")
 		if len(lineParts) != 2 {
@@ -618,7 +621,8 @@ func (repo *GitRepo) readTreeWithHash(ref, hash string) (*Tree, error) {
 		}
 		contents[path] = child
 	}
-	t.savedHash = hash
+	t := NewTree(contents)
+	t.savedHashes[repo] = hash
 	return t, nil
 }
 
@@ -652,7 +656,7 @@ func (repo *GitRepo) CreateCommit(details *CommitDetails) (string, error) {
 
 // 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)
+	treeHash, err := repo.StoreTree(t.Contents())
 	if err != nil {
 		return "", fmt.Errorf("failure storing a tree: %v", err)
 	}
@@ -968,11 +972,10 @@ func (repo *GitRepo) Remotes() ([]string, error) {
 }
 
 // Fetch fetches from the given remote using the supplied refspecs.
-func (repo *GitRepo) Fetch(remote string, fetchSpecs []string) error {
+func (repo *GitRepo) Fetch(remote string, refspecs ...string) error {
 	args := []string{"fetch", remote}
-	args = append(args, fetchSpecs...)
-	_, err := repo.runGitCommand(args...)
-	return err
+	args = append(args, refspecs...)
+	return repo.runGitCommandInline(args...)
 }
 
 // PushNotes pushes git notes to a remote repo.
@@ -1022,13 +1025,22 @@ func (repo *GitRepo) getRefHashes(refPattern string) (map[string]string, error)
 }
 
 func getRemoteNotesRef(remote, localNotesRef string) string {
-	relativeNotesRef := strings.TrimPrefix(localNotesRef, "refs/notes/")
-	return "refs/notes/remotes/" + 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, "refs/notes/remotes/"+remote+"/")
-	return "refs/notes/" + relativeNotesRef
+	relativeNotesRef := strings.TrimPrefix(remoteNotesRef, notesRefPrefix+"remotes/"+remote+"/")
+	return notesRefPrefix + relativeNotesRef
 }
 
 // MergeNotes merges in the remote's state of the notes reference into the
@@ -1054,7 +1066,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
 	}
@@ -1063,13 +1075,13 @@ func (repo *GitRepo) PullNotes(remote, notesRefPattern string) error {
 }
 
 func getRemoteDevtoolsRef(remote, devtoolsRefPattern string) string {
-	relativeRef := strings.TrimPrefix(devtoolsRefPattern, "refs/devtools/")
-	return "refs/remoteDevtools/" + remote + "/" + relativeRef
+	relativeRef := strings.TrimPrefix(devtoolsRefPattern, devtoolsRefPrefix)
+	return remoteDevtoolsRefPrefix + remote + "/" + relativeRef
 }
 
 func getLocalDevtoolsRef(remote, remoteDevtoolsRef string) string {
-	relativeRef := strings.TrimPrefix(remoteDevtoolsRef, "refs/remoteDevtools/"+remote+"/")
-	return "refs/devtools/" + relativeRef
+	relativeRef := strings.TrimPrefix(remoteDevtoolsRef, remoteDevtoolsRefPrefix+remote+"/")
+	return devtoolsRefPrefix + relativeRef
 }
 
 // MergeArchives merges in the remote's state of the archives reference into
@@ -1115,7 +1127,7 @@ func (repo *GitRepo) FetchAndReturnNewReviewHashes(remote, notesRefPattern strin
 		return nil, fmt.Errorf("failure reading the existing ref hashes for the remote %q: %v", remote, err)
 	}
 
-	if err := repo.runGitCommandInline("fetch", remote, notesFetchRefSpec, devtoolsFetchRefSpec); err != nil {
+	if err := repo.Fetch(remote, notesFetchRefSpec, devtoolsFetchRefSpec); err != nil {
 		return nil, fmt.Errorf("failure fetching from the remote %q: %v", remote, err)
 	}
 

+ 9 - 3
repository/mock_repo.go

@@ -19,6 +19,7 @@ package repository
 import (
 	"crypto/sha1"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"strings"
 )
@@ -230,6 +231,11 @@ func (r *mockRepoForTest) HasRef(ref string) (bool, error) {
 	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 {
@@ -525,12 +531,12 @@ func (r *mockRepoForTest) ListCommitsBetween(from, to string) ([]string, error)
 }
 
 // StoreBlob writes the given file to the repository and returns its hash.
-func (r *mockRepoForTest) StoreBlob(b *Blob) (string, error) {
+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(t *Tree) (string, error) {
+func (r *mockRepoForTest) StoreTree(contents map[string]TreeChild) (string, error) {
 	return "", fmt.Errorf("not implemented")
 }
 
@@ -603,7 +609,7 @@ func (r *mockRepoForTest) Remotes() ([]string, error) {
 }
 
 // Fetch fetches from the given remote using the supplied refspecs.
-func (r *mockRepoForTest) Fetch(remote string, fetchSpecs []string) error { return nil }
+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 }

+ 66 - 17
repository/repo.go

@@ -47,15 +47,25 @@ type TreeChild interface {
 	// Type returns the type of the child object (e.g. "blob" vs. "tree").
 	Type() string
 
-	// Store writes the object to the given repository and returns its hash.
+	// 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 {
-	Contents string
+	savedHashes map[Repo]string
+	contents    string
+}
 
-	savedHash 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 {
@@ -63,17 +73,40 @@ func (b *Blob) Type() string {
 }
 
 func (b *Blob) Store(repo Repo) (string, error) {
-	return repo.StoreBlob(b)
+	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 {
-	contents  map[string]TreeChild
-	savedHash string
+	savedHashes map[Repo]string
+	contents    map[string]TreeChild
 }
 
-func NewTree() *Tree {
-	return &Tree{contents: make(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 {
@@ -81,13 +114,26 @@ func (t *Tree) Type() string {
 }
 
 func (t *Tree) Store(repo Repo) (string, error) {
-	return repo.StoreTree(t)
+	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 {
-	// Since the returned contents are mutable, we have to assume the hash could change.
-	t.savedHash = ""
-	return t.contents
+	result := make(map[string]TreeChild)
+	for k, v := range t.contents {
+		result[k] = v
+	}
+	return result
 }
 
 // Repo represents a source code repository.
@@ -117,6 +163,9 @@ type Repo interface {
 	// 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
 
@@ -225,11 +274,11 @@ 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 to the repository and returns its hash.
-	StoreBlob(b *Blob) (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 to the repository and returns its hash.
-	StoreTree(t *Tree) (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)
@@ -264,7 +313,7 @@ type Repo interface {
 	Remotes() ([]string, error)
 
 	// Fetch fetches from the given remote using the supplied refspecs.
-	Fetch(remote string, fetchSpecs []string) error
+	Fetch(remote string, refspecs ...string) error
 
 	// PushNotes pushes git notes to a remote repo.
 	PushNotes(remote, notesRefPattern string) error