plumbing/revlist: input as a slice of hashes instead of commits

- Now the input of the method Objects inside revlist package is a slice of hashes instead of commits. Hashes can be from Blobs, Trees and Commits objects.
- ObjectStorer now is used to obtain the object content using hashes slice.
- This PR fix #222. Now a test into upload_pack_test.go file is not skipped anymore.
- Remove code from remote.go and server.go that is not necessary.

5 files changed, 144 insertions, 98 deletions

diff --git a/plumbing/revlist/revlist.go b/plumbing/revlist/revlist.go
index f1d66b4..5731474 100644
--- a/plumbing/revlist/revlist.go
+++ b/plumbing/revlist/revlist.go
@@ -3,6 +3,7 @@
 package revlist
 
 import (
+	"fmt"
 	"io"
 
 	"srcd.works/go-git.v4/plumbing"
@@ -11,29 +12,26 @@ import (
 )
 
 // Objects applies a complementary set. It gets all the hashes from all
-// the reachable objects from the given commits. Ignore param are object hashes
-// that we want to ignore on the result. It is a list because is
-// easier to interact with other porcelain elements, but internally it is
-// converted to a map. All that objects must be accessible from the object
-// storer.
+// the reachable objects from the given objects. Ignore param are object hashes
+// that we want to ignore on the result. All that objects must be accessible
+// from the object storer.
 func Objects(
 	s storer.EncodedObjectStorer,
-	commits []*object.Commit,
+	objects []plumbing.Hash,
 	ignore []plumbing.Hash) ([]plumbing.Hash, error) {
 
 	seen := hashListToSet(ignore)
 	result := make(map[plumbing.Hash]bool)
-	for _, c := range commits {
-		err := reachableObjects(s, c, seen, func(h plumbing.Hash) error {
-			if !seen[h] {
-				result[h] = true
-				seen[h] = true
-			}
 
-			return nil
-		})
+	walkerFunc := func(h plumbing.Hash) {
+		if !seen[h] {
+			result[h] = true
+			seen[h] = true
+		}
+	}
 
-		if err != nil {
+	for _, h := range objects {
+		if err := processObject(s, h, seen, walkerFunc); err != nil {
 			return nil, err
 		}
 	}
@@ -41,56 +39,76 @@ func Objects(
 	return hashSetToList(result), nil
 }
 
+// processObject obtains the object using the hash an process it depending of its type
+func processObject(
+	s storer.EncodedObjectStorer,
+	h plumbing.Hash,
+	seen map[plumbing.Hash]bool,
+	walkerFunc func(h plumbing.Hash),
+) error {
+	o, err := s.EncodedObject(plumbing.AnyObject, h)
+	if err != nil {
+		return err
+	}
+
+	do, err := object.DecodeObject(s, o)
+	if err != nil {
+		return err
+	}
+
+	switch do := do.(type) {
+	case *object.Commit:
+		return reachableObjects(do, seen, walkerFunc)
+	case *object.Tree:
+		return iterateCommitTrees(seen, do, walkerFunc)
+	case *object.Tag:
+		walkerFunc(do.Hash)
+		return processObject(s, do.Target, seen, walkerFunc)
+	case *object.Blob:
+		walkerFunc(do.Hash)
+	default:
+		return fmt.Errorf("object type not valid: %s. "+
+			"Object reference: %s", o.Type(), o.Hash())
+	}
+
+	return nil
+}
+
 // reachableObjects returns, using the callback function, all the reachable
 // objects from the specified commit. To avoid to iterate over seen commits,
 // if a commit hash is into the 'seen' set, we will not iterate all his trees
 // and blobs objects.
 func reachableObjects(
-	s storer.EncodedObjectStorer,
 	commit *object.Commit,
 	seen map[plumbing.Hash]bool,
-	cb func(h plumbing.Hash) error) error {
-
-	return iterateCommits(commit, func(commit *object.Commit) error {
+	cb func(h plumbing.Hash)) error {
+	return object.WalkCommitHistory(commit, func(commit *object.Commit) error {
 		if seen[commit.Hash] {
 			return nil
 		}
 
-		if err := cb(commit.Hash); err != nil {
+		cb(commit.Hash)
+
+		tree, err := commit.Tree()
+		if err != nil {
 			return err
 		}
 
-		return iterateCommitTrees(s, commit, func(h plumbing.Hash) error {
-			return cb(h)
-		})
-	})
-}
-
-// iterateCommits iterate all reachable commits from the given one
-func iterateCommits(commit *object.Commit, cb func(c *object.Commit) error) error {
-	if err := cb(commit); err != nil {
-		return err
-	}
-
-	return object.WalkCommitHistory(commit, func(c *object.Commit) error {
-		return cb(c)
+		return iterateCommitTrees(seen, tree, cb)
 	})
 }
 
 // iterateCommitTrees iterate all reachable trees from the given commit
 func iterateCommitTrees(
-	s storer.EncodedObjectStorer,
-	commit *object.Commit,
-	cb func(h plumbing.Hash) error) error {
-
-	tree, err := commit.Tree()
-	if err != nil {
-		return err
-	}
-	if err := cb(tree.Hash); err != nil {
-		return err
+	seen map[plumbing.Hash]bool,
+	tree *object.Tree,
+	cb func(h plumbing.Hash)) error {
+	if seen[tree.Hash] {
+		return nil
 	}
 
+	cb(tree.Hash)
+
 	treeWalker := object.NewTreeWalker(tree, true)
 
 	for {
@@ -101,9 +119,12 @@ func iterateCommitTrees(
 		if err != nil {
 			return err
 		}
-		if err := cb(e.Hash); err != nil {
-			return err
+
+		if seen[e.Hash] {
+			continue
 		}
+
+		cb(e.Hash)
 	}
 
 	return nil
diff --git a/plumbing/revlist/revlist_test.go b/plumbing/revlist/revlist_test.go
index d7361d9..ca69975 100644
--- a/plumbing/revlist/revlist_test.go
+++ b/plumbing/revlist/revlist_test.go
@@ -76,13 +76,65 @@ func (s *RevListSuite) TestRevListObjects(c *C) {
 		"d3ff53e0564a9f87d8e84b6e28e5060e517008aa": true, // CHANGELOG
 	}
 
-	initCommit := s.commit(c, plumbing.NewHash(initialCommit))
-	secondCommit := s.commit(c, plumbing.NewHash(secondCommit))
+	localHist, err := Objects(s.Storer,
+		[]plumbing.Hash{plumbing.NewHash(initialCommit)}, nil)
+	c.Assert(err, IsNil)
+
+	remoteHist, err := Objects(s.Storer,
+		[]plumbing.Hash{plumbing.NewHash(secondCommit)}, localHist)
+	c.Assert(err, IsNil)
+
+	for _, h := range remoteHist {
+		c.Assert(revList[h.String()], Equals, true)
+	}
+	c.Assert(len(remoteHist), Equals, len(revList))
+}
+
+func (s *RevListSuite) TestRevListObjectsTagObject(c *C) {
+	sto, err := filesystem.NewStorage(
+		fixtures.ByTag("tags").
+			ByURL("https://github.com/git-fixtures/tags.git").One().DotGit())
+	c.Assert(err, IsNil)
+
+	expected := map[string]bool{
+		"70846e9a10ef7b41064b40f07713d5b8b9a8fc73": true,
+		"e69de29bb2d1d6434b8b29ae775ad8c2e48c5391": true,
+		"ad7897c0fb8e7d9a9ba41fa66072cf06095a6cfc": true,
+		"f7b877701fbf855b44c0a9e86f3fdce2c298b07f": true,
+	}
+
+	hist, err := Objects(sto, []plumbing.Hash{plumbing.NewHash("ad7897c0fb8e7d9a9ba41fa66072cf06095a6cfc")}, nil)
+	c.Assert(err, IsNil)
+
+	for _, h := range hist {
+		c.Assert(expected[h.String()], Equals, true)
+	}
+
+	c.Assert(len(hist), Equals, len(expected))
+}
+
+// ---
+// | |\
+// | | * b8e471f Creating changelog
+// | |/
+// * | 35e8510 binary file
+// |/
+// * b029517 Initial commit
+func (s *RevListSuite) TestRevListObjectsWithBlobsAndTrees(c *C) {
+	revList := map[string]bool{
+		"b8e471f58bcbca63b07bda20e428190409c2db47": true, // second commit
+	}
 
-	localHist, err := Objects(s.Storer, []*object.Commit{initCommit}, nil)
+	localHist, err := Objects(s.Storer,
+		[]plumbing.Hash{
+			plumbing.NewHash(initialCommit),
+			plumbing.NewHash("c2d30fa8ef288618f65f6eed6e168e0d514886f4"),
+			plumbing.NewHash("d3ff53e0564a9f87d8e84b6e28e5060e517008aa"),
+		}, nil)
 	c.Assert(err, IsNil)
 
-	remoteHist, err := Objects(s.Storer, []*object.Commit{secondCommit}, localHist)
+	remoteHist, err := Objects(s.Storer,
+		[]plumbing.Hash{plumbing.NewHash(secondCommit)}, localHist)
 	c.Assert(err, IsNil)
 
 	for _, h := range remoteHist {
@@ -92,25 +144,25 @@ func (s *RevListSuite) TestRevListObjects(c *C) {
 }
 
 func (s *RevListSuite) TestRevListObjectsReverse(c *C) {
-	initCommit := s.commit(c, plumbing.NewHash(initialCommit))
-	secondCommit := s.commit(c, plumbing.NewHash(secondCommit))
 
-	localHist, err := Objects(s.Storer, []*object.Commit{secondCommit}, nil)
+	localHist, err := Objects(s.Storer,
+		[]plumbing.Hash{plumbing.NewHash(secondCommit)}, nil)
 	c.Assert(err, IsNil)
 
-	remoteHist, err := Objects(s.Storer, []*object.Commit{initCommit}, localHist)
+	remoteHist, err := Objects(s.Storer,
+		[]plumbing.Hash{plumbing.NewHash(initialCommit)}, localHist)
 	c.Assert(err, IsNil)
 
 	c.Assert(len(remoteHist), Equals, 0)
 }
 
 func (s *RevListSuite) TestRevListObjectsSameCommit(c *C) {
-	commit := s.commit(c, plumbing.NewHash(secondCommit))
-
-	localHist, err := Objects(s.Storer, []*object.Commit{commit}, nil)
+	localHist, err := Objects(s.Storer,
+		[]plumbing.Hash{plumbing.NewHash(secondCommit)}, nil)
 	c.Assert(err, IsNil)
 
-	remoteHist, err := Objects(s.Storer, []*object.Commit{commit}, localHist)
+	remoteHist, err := Objects(s.Storer,
+		[]plumbing.Hash{plumbing.NewHash(secondCommit)}, localHist)
 	c.Assert(err, IsNil)
 
 	c.Assert(len(remoteHist), Equals, 0)
@@ -122,15 +174,14 @@ func (s *RevListSuite) TestRevListObjectsSameCommit(c *C) {
 // * 918c48b some code
 // -----
 func (s *RevListSuite) TestRevListObjectsNewBranch(c *C) {
-	someCommit := s.commit(c, plumbing.NewHash(someCommit))
-	someCommitBranch := s.commit(c, plumbing.NewHash(someCommitBranch))
-	someCommitOtherBranch := s.commit(c, plumbing.NewHash(someCommitOtherBranch))
-
-	localHist, err := Objects(s.Storer, []*object.Commit{someCommit}, nil)
+	localHist, err := Objects(s.Storer,
+		[]plumbing.Hash{plumbing.NewHash(someCommit)}, nil)
 	c.Assert(err, IsNil)
 
 	remoteHist, err := Objects(
-		s.Storer, []*object.Commit{someCommitBranch, someCommitOtherBranch}, localHist)
+		s.Storer, []plumbing.Hash{
+			plumbing.NewHash(someCommitBranch),
+			plumbing.NewHash(someCommitOtherBranch)}, localHist)
 	c.Assert(err, IsNil)
 
 	revList := map[string]bool{
diff --git a/plumbing/transport/server/server.go b/plumbing/transport/server/server.go
index 79b64a1..457240f 100644
--- a/plumbing/transport/server/server.go
+++ b/plumbing/transport/server/server.go
@@ -9,7 +9,6 @@ import (
 
 	"srcd.works/go-git.v4/plumbing"
 	"srcd.works/go-git.v4/plumbing/format/packfile"
-	"srcd.works/go-git.v4/plumbing/object"
 	"srcd.works/go-git.v4/plumbing/protocol/packp"
 	"srcd.works/go-git.v4/plumbing/protocol/packp/capability"
 	"srcd.works/go-git.v4/plumbing/revlist"
@@ -153,26 +152,12 @@ func (s *upSession) UploadPack(req *packp.UploadPackRequest) (*packp.UploadPackR
 }
 
 func (s *upSession) objectsToUpload(req *packp.UploadPackRequest) ([]plumbing.Hash, error) {
-	commits, err := s.commitsToUpload(req.Wants)
+	haves, err := revlist.Objects(s.storer, req.Haves, nil)
 	if err != nil {
 		return nil, err
 	}
 
-	return revlist.Objects(s.storer, commits, req.Haves)
-}
-
-func (s *upSession) commitsToUpload(wants []plumbing.Hash) ([]*object.Commit, error) {
-	var commits []*object.Commit
-	for _, h := range wants {
-		c, err := object.GetCommit(s.storer, h)
-		if err != nil {
-			return nil, err
-		}
-
-		commits = append(commits, c)
-	}
-
-	return commits, nil
+	return revlist.Objects(s.storer, req.Wants, haves)
 }
 
 func (*upSession) setSupportedCapabilities(c *capability.List) error {
diff --git a/plumbing/transport/server/upload_pack_test.go b/plumbing/transport/server/upload_pack_test.go
index d127522..4fc3be2 100644
--- a/plumbing/transport/server/upload_pack_test.go
+++ b/plumbing/transport/server/upload_pack_test.go
@@ -38,8 +38,3 @@ func (s *UploadPackSuite) TestAdvertisedReferencesNotExists(c *C) {
 	c.Assert(err, Equals, transport.ErrRepositoryNotFound)
 	c.Assert(r, IsNil)
 }
-
-// TODO revList implementation is returning more objects than expected.
-func (s *UploadPackSuite) TestUploadPackPartial(c *C) {
-	c.Skip("Fix revList implementation")
-}
diff --git a/remote.go b/remote.go
index a51946b..fe7993c 100644
--- a/remote.go
+++ b/remote.go
@@ -8,7 +8,6 @@ import (
 	"srcd.works/go-git.v4/config"
 	"srcd.works/go-git.v4/plumbing"
 	"srcd.works/go-git.v4/plumbing/format/packfile"
-	"srcd.works/go-git.v4/plumbing/object"
 	"srcd.works/go-git.v4/plumbing/protocol/packp"
 	"srcd.works/go-git.v4/plumbing/protocol/packp/capability"
 	"srcd.works/go-git.v4/plumbing/protocol/packp/sideband"
@@ -97,7 +96,7 @@ func (r *Remote) Push(o *PushOptions) (err error) {
 		return NoErrAlreadyUpToDate
 	}
 
-	commits, err := commitsToPush(r.s, req.Commands)
+	objects, err := objectsToPush(req.Commands)
 	if err != nil {
 		return err
 	}
@@ -107,7 +106,7 @@ func (r *Remote) Push(o *PushOptions) (err error) {
 		return err
 	}
 
-	hashesToPush, err := revlist.Objects(r.s, commits, haves)
+	hashesToPush, err := revlist.Objects(r.s, objects, haves)
 	if err != nil {
 		return err
 	}
@@ -476,22 +475,17 @@ func (r *Remote) buildFetchedTags(refs storer.ReferenceStorer) error {
 	})
 }
 
-func commitsToPush(s storer.EncodedObjectStorer, commands []*packp.Command) ([]*object.Commit, error) {
-	var commits []*object.Commit
+func objectsToPush(commands []*packp.Command) ([]plumbing.Hash, error) {
+	var objects []plumbing.Hash
 	for _, cmd := range commands {
 		if cmd.New == plumbing.ZeroHash {
 			continue
 		}
 
-		c, err := object.GetCommit(s, cmd.New)
-		if err != nil {
-			return nil, err
-		}
-
-		commits = append(commits, c)
+		objects = append(objects, cmd.New)
 	}
 
-	return commits, nil
+	return objects, nil
 }
 
 func referencesToHashes(refs storer.ReferenceStorer) ([]plumbing.Hash, error) {