Merge pull request #30 from scjalliance/consistent-iterators

Improved consistency of Tree iterators

Author: mcuadros

commit_test.go

@@ -41,7 +41,7 @@ func (s *SuiteCommit) SetUpSuite(c *C) {
 	}
 }
 
-var iterTests = []struct {
+var commitIterTests = []struct {
 	repo    string   // the repo name in the test suite's map of fixtures
 	commits []string // the commit hashes to iterate over in the test
 }{
@@ -59,15 +59,15 @@ var iterTests = []struct {
 }
 
 func (s *SuiteCommit) TestIterSlice(c *C) {
-	for i, t := range iterTests {
+	for i, t := range commitIterTests {
 		r := s.repos[t.repo]
 		iter := NewCommitIter(r, core.NewObjectSliceIter(makeObjectSlice(t.commits, r.Storage)))
 		s.checkIter(c, r, i, iter, t.commits)
 	}
 }
 
 func (s *SuiteCommit) TestIterLookup(c *C) {
-	for i, t := range iterTests {
+	for i, t := range commitIterTests {
 		r := s.repos[t.repo]
 		iter := NewCommitIter(r, core.NewObjectLookupIter(r.Storage, makeHashSlice(t.commits)))
 		s.checkIter(c, r, i, iter, t.commits)
@@ -85,15 +85,15 @@ func (s *SuiteCommit) checkIter(c *C, r *Repository, subtest int, iter *CommitIt
 }
 
 func (s *SuiteCommit) TestIterSliceClose(c *C) {
-	for i, t := range iterTests {
+	for i, t := range commitIterTests {
 		r := s.repos[t.repo]
 		iter := NewCommitIter(r, core.NewObjectSliceIter(makeObjectSlice(t.commits, r.Storage)))
 		s.checkIterClose(c, i, iter)
 	}
 }
 
 func (s *SuiteCommit) TestIterLookupClose(c *C) {
-	for i, t := range iterTests {
+	for i, t := range commitIterTests {
 		r := s.repos[t.repo]
 		iter := NewCommitIter(r, core.NewObjectLookupIter(r.Storage, makeHashSlice(t.commits)))
 		s.checkIterClose(c, i, iter)

file.go

@@ -33,3 +33,34 @@ func (f *File) Lines() []string {
 	}
 	return splits
 }
+
+type FileIter struct {
+	w TreeWalker
+}
+
+func NewFileIter(r *Repository, t *Tree) *FileIter {
+	return &FileIter{w: *NewTreeWalker(r, t)}
+}
+
+func (iter *FileIter) Next() (*File, error) {
+	for {
+		name, entry, obj, err := iter.w.Next()
+		if err != nil {
+			return nil, err
+		}
+
+		if obj.Type() != core.BlobObject {
+			// Skip non-blob objects
+			continue
+		}
+
+		blob := &Blob{}
+		blob.Decode(obj)
+
+		return &File{Name: name, Reader: blob.Reader(), Hash: entry.Hash}, nil
+	}
+}
+
+func (iter *FileIter) Close() {
+	iter.w.Close()
+}

file_test.go

@@ -1,6 +1,7 @@
 package git
 
 import (
+	"io"
 	"os"
 
 	"gopkg.in/src-d/go-git.v3/core"
@@ -41,6 +42,49 @@ func (s *SuiteFile) SetUpSuite(c *C) {
 	}
 }
 
+type fileIterExpectedEntry struct {
+	Name string
+	Hash string
+}
+
+var fileIterTests = []struct {
+	repo   string // the repo name as in localRepos
+	commit string // the commit to search for the file
+	files  []fileIterExpectedEntry
+}{
+	// https://api.github.com/repos/tyba/git-fixture/git/trees/6ecf0ef2c2dffb796033e5a02219af86ec6584e5
+	{"https://github.com/tyba/git-fixture.git", "6ecf0ef2c2dffb796033e5a02219af86ec6584e5", []fileIterExpectedEntry{
+		{".gitignore", "32858aad3c383ed1ff0a0f9bdf231d54a00c9e88"},
+		{"CHANGELOG", "d3ff53e0564a9f87d8e84b6e28e5060e517008aa"},
+		{"LICENSE", "c192bd6a24ea1ab01d78686e417c8bdc7c3d197f"},
+		{"binary.jpg", "d5c0f4ab811897cadf03aec358ae60d21f91c50d"},
+		{"go/example.go", "880cd14280f4b9b6ed3986d6671f907d7cc2a198"},
+		{"json/long.json", "49c6bb89b17060d7b4deacb7b338fcc6ea2352a9"},
+		{"json/short.json", "c8f1d8c61f9da76f4cb49fd86322b6e685dba956"},
+		{"php/crappy.php", "9a48f23120e880dfbe41f7c9b7b708e9ee62a492"},
+		{"vendor/foo.go", "9dea2395f5403188298c1dabe8bdafe562c491e3"},
+	}},
+}
+
+func (s *SuiteFile) TestIter(c *C) {
+	for i, t := range fileIterTests {
+		r := s.repos[t.repo]
+		commit, err := r.Commit(core.NewHash(t.commit))
+		c.Assert(err, IsNil, Commentf("subtest %d: %v (%s)", i, err, t.commit))
+
+		iter := NewFileIter(r, commit.Tree())
+		for k := 0; k < len(t.files); k++ {
+			expected := t.files[k]
+			file, err := iter.Next()
+			c.Assert(err, IsNil, Commentf("subtest %d, iter %d, err=%v", i, k, err))
+			c.Assert(file.Name, Equals, expected.Name, Commentf("subtest %d, iter %d, name=%s, expected=%s", i, k, file.Name, expected.Hash))
+			c.Assert(file.Hash.String(), Equals, expected.Hash, Commentf("subtest %d, iter %d, hash=%v, expected=%s", i, k, file.Hash.String(), expected.Hash))
+		}
+		_, err = iter.Next()
+		c.Assert(err, Equals, io.EOF)
+	}
+}
+
 var contentsTests = []struct {
 	repo     string // the repo name as in localRepos
 	commit   string // the commit to search for the file
@@ -154,7 +198,9 @@ func (s *SuiteFile) TestIgnoreEmptyDirEntries(c *C) {
 		commit, err := s.repos[t.repo].Commit(core.NewHash(t.commit))
 		c.Assert(err, IsNil, Commentf("subtest %d: %v (%s)", i, err, t.commit))
 
-		for file := range commit.Tree().Files() {
+		iter := commit.Tree().Files()
+		defer iter.Close()
+		for file, err := iter.Next(); err == nil; file, err = iter.Next() {
 			_ = file.Contents()
 			// this would probably panic if we are not ignoring empty dirs
 		}

formats/packfile/reader_test.go

@@ -10,7 +10,7 @@ import (
 	"time"
 
 	"gopkg.in/src-d/go-git.v3/core"
-	"gopkg.in/src-d/go-git.v3/storages/memory"
+	"gopkg.in/src-d/go-git.v3/storage/memory"
 
 	"github.com/dustin/go-humanize"
 	. "gopkg.in/check.v1"

objects_test.go

@@ -6,7 +6,7 @@ import (
 
 	. "gopkg.in/check.v1"
 	"gopkg.in/src-d/go-git.v3/core"
-	"gopkg.in/src-d/go-git.v3/storages/memory"
+	"gopkg.in/src-d/go-git.v3/storage/memory"
 )
 
 type ObjectsSuite struct {
@@ -54,13 +54,17 @@ func (s *ObjectsSuite) TestParseTree(c *C) {
 	c.Assert(err, IsNil)
 
 	c.Assert(tree.Entries, HasLen, 8)
-	c.Assert(tree.Entries[".gitignore"].Name, Equals, ".gitignore")
-	c.Assert(tree.Entries[".gitignore"].Mode.String(), Equals, "-rw-r--r--")
-	c.Assert(tree.Entries[".gitignore"].Hash.String(), Equals, "32858aad3c383ed1ff0a0f9bdf231d54a00c9e88")
+
+	tree.buildMap()
+	c.Assert(tree.m, HasLen, 8)
+	c.Assert(tree.m[".gitignore"].Name, Equals, ".gitignore")
+	c.Assert(tree.m[".gitignore"].Mode.String(), Equals, "-rw-r--r--")
+	c.Assert(tree.m[".gitignore"].Hash.String(), Equals, "32858aad3c383ed1ff0a0f9bdf231d54a00c9e88")
 
 	count := 0
-	ch := tree.Files()
-	for f := range ch {
+	iter := tree.Files()
+	defer iter.Close()
+	for f, err := iter.Next(); err == nil; f, err = iter.Next() {
 		count++
 		if f.Name == "go/example.go" {
 			content, _ := ioutil.ReadAll(f)

remote_test.go

@@ -3,7 +3,7 @@ package git
 import (
 	"gopkg.in/src-d/go-git.v3/clients/http"
 	"gopkg.in/src-d/go-git.v3/formats/packfile"
-	"gopkg.in/src-d/go-git.v3/storages/memory"
+	"gopkg.in/src-d/go-git.v3/storage/memory"
 
 	. "gopkg.in/check.v1"
 )

repository.go

@@ -7,7 +7,7 @@ import (
 	"gopkg.in/src-d/go-git.v3/clients/common"
 	"gopkg.in/src-d/go-git.v3/core"
 	"gopkg.in/src-d/go-git.v3/formats/packfile"
-	"gopkg.in/src-d/go-git.v3/storages/memory"
+	"gopkg.in/src-d/go-git.v3/storage/memory"
 )
 
 var (

tree.go

@@ -5,20 +5,30 @@ import (
 	"errors"
 	"io"
 	"os"
-	"path"
 	"strconv"
 	"strings"
 
 	"gopkg.in/src-d/go-git.v3/core"
 )
 
+const (
+	maxTreeDepth = 1024
+)
+
+// New errors defined by this package.
+var (
+	ErrMaxTreeDepth = errors.New("maximum tree depth exceeded")
+	ErrFileNotFound = errors.New("file not found")
+)
+
 // Tree is basically like a directory - it references a bunch of other trees
 // and/or blobs (i.e. files and sub-directories)
 type Tree struct {
-	Entries map[string]TreeEntry
+	Entries []TreeEntry
 	Hash    core.Hash
 
 	r *Repository
+	m map[string]*TreeEntry
 }
 
 // TreeEntry represents a file
@@ -28,9 +38,6 @@ type TreeEntry struct {
 	Hash core.Hash
 }
 
-// New errors defined by this package.
-var ErrFileNotFound = errors.New("file not found")
-
 // File returns the hash of the file identified by the `path` argument.
 // The path is interpreted as relative to the tree receiver.
 func (t *Tree) File(path string) (*File, error) {
@@ -105,48 +112,19 @@ func (t *Tree) dir(baseName string) (*Tree, error) {
 var errEntryNotFound = errors.New("entry not found")
 
 func (t *Tree) entry(baseName string) (*TreeEntry, error) {
-	entry, ok := t.Entries[baseName]
+	if t.m == nil {
+		t.buildMap()
+	}
+	entry, ok := t.m[baseName]
 	if !ok {
 		return nil, errEntryNotFound
 	}
 
-	return &entry, nil
+	return entry, nil
 }
 
-func (t *Tree) Files() chan *File {
-	ch := make(chan *File, 1)
-
-	go func() {
-		defer func() { close(ch) }()
-		t.walkEntries("", ch)
-	}()
-
-	return ch
-}
-
-func (t *Tree) walkEntries(base string, ch chan *File) {
-	for _, entry := range t.Entries {
-		obj, err := t.r.Storage.Get(entry.Hash)
-		if err != nil {
-			if err == core.ObjectNotFoundErr {
-				continue // ignore entries without hash (= submodule dirs)
-			}
-			//FIXME: Refactor this function to return an error. Ideally this would be
-			//       moved into a FileIter type.
-		}
-
-		if obj.Type() == core.TreeObject {
-			tree := &Tree{r: t.r}
-			tree.Decode(obj)
-			tree.walkEntries(path.Join(base, entry.Name), ch)
-			continue
-		}
-
-		blob := &Blob{}
-		blob.Decode(obj)
-
-		ch <- &File{Name: path.Join(base, entry.Name), Reader: blob.Reader(), Hash: entry.Hash}
-	}
+func (t *Tree) Files() *FileIter {
+	return NewFileIter(t.r, t)
 }
 
 // Decode transform an core.Object into a Tree struct
@@ -160,7 +138,8 @@ func (t *Tree) Decode(o core.Object) error {
 		return nil
 	}
 
-	t.Entries = make(map[string]TreeEntry)
+	t.Entries = nil
+	t.m = nil
 
 	r := bufio.NewReader(o.Reader())
 	for {
@@ -190,31 +169,69 @@ func (t *Tree) Decode(o core.Object) error {
 		}
 
 		baseName := name[:len(name)-1]
-		t.Entries[baseName] = TreeEntry{
+		t.Entries = append(t.Entries, TreeEntry{
 			Hash: hash,
 			Mode: os.FileMode(fm),
 			Name: baseName,
-		}
+		})
 	}
 
 	return nil
 }
 
+func (t *Tree) buildMap() {
+	t.m = make(map[string]*TreeEntry)
+	for i := 0; i < len(t.Entries); i++ {
+		t.m[t.Entries[i].Name] = &t.Entries[i]
+	}
+}
+
+// TreeEntryIter facilitates iterating through the TreeEntry objects in a Tree.
+type TreeEntryIter struct {
+	t   *Tree
+	pos int
+}
+
+func NewTreeEntryIter(t *Tree) *TreeEntryIter {
+	return &TreeEntryIter{t, 0}
+}
+
+func (iter *TreeEntryIter) Next() (TreeEntry, error) {
+	if iter.pos >= len(iter.t.Entries) {
+		return TreeEntry{}, io.EOF
+	}
+	iter.pos++
+	return iter.t.Entries[iter.pos-1], nil
+}
+
+// TreeEntryIter facilitates iterating through the descendent subtrees of a
+// Tree.
 type TreeIter struct {
-	core.ObjectIter
-	r *Repository
+	w TreeWalker
 }
 
-func NewTreeIter(r *Repository, iter core.ObjectIter) *TreeIter {
-	return &TreeIter{iter, r}
+func NewTreeIter(r *Repository, t *Tree) *TreeIter {
+	return &TreeIter{
+		w: *NewTreeWalker(r, t),
+	}
 }
 
 func (iter *TreeIter) Next() (*Tree, error) {
-	obj, err := iter.ObjectIter.Next()
-	if err != nil {
-		return nil, err
+	for {
+		_, _, obj, err := iter.w.Next()
+		if err != nil {
+			return nil, err
+		}
+
+		if obj.Type() != core.TreeObject {
+			// Skip non-tree objects
+			continue
+		}
+
+		return iter.w.Tree(), nil
 	}
+}
 
-	tree := &Tree{r: iter.r}
-	return tree, tree.Decode(obj)
+func (iter *TreeIter) Close() {
+	iter.w.Close()
 }