global storage interface refactor (#112)

* core: ObjectStorage, ReferenceStorage renamed to ObjectStorer and
ReferenceStorer

* rebase

* general, changes request by @alcortes

* general, changes request by @alcortes

7 files changed, 92 insertions, 172 deletions

diff --git a/storage/filesystem/config.go b/storage/filesystem/config.go
index b32265f..4b47937 100644
--- a/storage/filesystem/config.go
+++ b/storage/filesystem/config.go
@@ -18,70 +18,24 @@ type ConfigStorage struct {
 	dir *dotgit.DotGit
 }
 
-func (c *ConfigStorage) Remote(name string) (*config.RemoteConfig, error) {
-	cfg, err := c.read()
-	if err != nil {
-		return nil, err
-	}
-
-	s := cfg.Section(remoteSection)
-	if !s.HasSubsection(name) {
-		return nil, config.ErrRemoteConfigNotFound
-	}
+func (c *ConfigStorage) Config() (*config.Config, error) {
+	cfg := config.NewConfig()
 
-	return parseRemote(s.Subsection(name)), nil
-}
-
-func (c *ConfigStorage) Remotes() ([]*config.RemoteConfig, error) {
-	cfg, err := c.read()
+	ini, err := c.unmarshal()
 	if err != nil {
 		return nil, err
 	}
 
-	remotes := []*config.RemoteConfig{}
-	sect := cfg.Section(remoteSection)
+	sect := ini.Section(remoteSection)
 	for _, s := range sect.Subsections {
-		remotes = append(remotes, parseRemote(s))
-	}
-
-	return remotes, nil
-}
-
-func (c *ConfigStorage) SetRemote(r *config.RemoteConfig) error {
-	if err := r.Validate(); err != nil {
-		return err
+		r := c.unmarshalRemote(s)
+		cfg.Remotes[r.Name] = r
 	}
 
-	cfg, err := c.read()
-	if err != nil {
-		return err
-	}
-
-	s := cfg.Section(remoteSection).Subsection(r.Name)
-	s.Name = r.Name
-	if r.URL != "" {
-		s.SetOption(urlKey, r.URL)
-	}
-	s.RemoveOption(fetchKey)
-	for _, rs := range r.Fetch {
-		s.AddOption(fetchKey, rs.String())
-	}
-
-	return c.write(cfg)
-}
-
-func (c *ConfigStorage) DeleteRemote(name string) error {
-	cfg, err := c.read()
-	if err != nil {
-		return err
-	}
-
-	cfg = cfg.RemoveSubsection(remoteSection, name)
-
-	return c.write(cfg)
+	return cfg, nil
 }
 
-func (c *ConfigStorage) read() (*gitconfig.Config, error) {
+func (c *ConfigStorage) unmarshal() (*gitconfig.Config, error) {
 	cfg := gitconfig.New()
 
 	f, err := c.dir.Config()
@@ -103,23 +57,7 @@ func (c *ConfigStorage) read() (*gitconfig.Config, error) {
 	return cfg, nil
 }
 
-func (c *ConfigStorage) write(cfg *gitconfig.Config) error {
-	f, err := c.dir.ConfigWriter()
-	if err != nil {
-		return err
-	}
-
-	e := gitconfig.NewEncoder(f)
-	err = e.Encode(cfg)
-	if err != nil {
-		f.Close()
-		return err
-	}
-
-	return f.Close()
-}
-
-func parseRemote(s *gitconfig.Subsection) *config.RemoteConfig {
+func (c *ConfigStorage) unmarshalRemote(s *gitconfig.Subsection) *config.RemoteConfig {
 	fetch := []config.RefSpec{}
 	for _, f := range s.Options.GetAll(fetchKey) {
 		rs := config.RefSpec(f)
@@ -134,3 +72,47 @@ func parseRemote(s *gitconfig.Subsection) *config.RemoteConfig {
 		Fetch: fetch,
 	}
 }
+
+func (c *ConfigStorage) SetConfig(cfg *config.Config) error {
+	if err := cfg.Validate(); err != nil {
+		return err
+	}
+
+	ini, err := c.unmarshal()
+	if err != nil {
+		return err
+	}
+
+	s := ini.Section(remoteSection)
+	s.Subsections = make(gitconfig.Subsections, len(cfg.Remotes))
+
+	var i int
+	for _, r := range cfg.Remotes {
+		s.Subsections[i] = c.marshalRemote(r)
+		i++
+	}
+
+	return c.marshal(ini)
+}
+
+func (c *ConfigStorage) marshal(ini *gitconfig.Config) error {
+	f, err := c.dir.ConfigWriter()
+	if err != nil {
+		return err
+	}
+
+	defer f.Close()
+
+	e := gitconfig.NewEncoder(f)
+	return e.Encode(ini)
+}
+
+func (c *ConfigStorage) marshalRemote(r *config.RemoteConfig) *gitconfig.Subsection {
+	s := &gitconfig.Subsection{Name: r.Name}
+	s.AddOption(urlKey, r.URL)
+	for _, rs := range r.Fetch {
+		s.AddOption(fetchKey, rs.String())
+	}
+
+	return s
+}
diff --git a/storage/filesystem/config_test.go b/storage/filesystem/config_test.go
index 2eb0ab9..b86eaee 100644
--- a/storage/filesystem/config_test.go
+++ b/storage/filesystem/config_test.go
@@ -4,7 +4,6 @@ import (
 	"io/ioutil"
 	stdos "os"
 
-	"gopkg.in/src-d/go-git.v4/config"
 	"gopkg.in/src-d/go-git.v4/fixtures"
 	"gopkg.in/src-d/go-git.v4/storage/filesystem/internal/dotgit"
 	"gopkg.in/src-d/go-git.v4/utils/fs/os"
@@ -29,27 +28,20 @@ func (s *ConfigSuite) SetUpTest(c *C) {
 	s.path = tmp
 }
 
-func (s *ConfigSuite) TestSetRemote(c *C) {
-	cfg := &ConfigStorage{s.dir}
-	err := cfg.SetRemote(&config.RemoteConfig{Name: "foo", URL: "foo"})
-	c.Assert(err, IsNil)
-
-	remote, err := cfg.Remote("foo")
-	c.Assert(err, IsNil)
-	c.Assert(remote.Name, Equals, "foo")
-}
-
 func (s *ConfigSuite) TestRemotes(c *C) {
 	dir := dotgit.New(fixtures.Basic().ByTag(".git").One().DotGit())
-	cfg := &ConfigStorage{dir}
+	storer := &ConfigStorage{dir}
 
-	remotes, err := cfg.Remotes()
+	cfg, err := storer.Config()
 	c.Assert(err, IsNil)
+
+	remotes := cfg.Remotes
 	c.Assert(remotes, HasLen, 1)
-	c.Assert(remotes[0].Name, Equals, "origin")
-	c.Assert(remotes[0].URL, Equals, "https://github.com/git-fixtures/basic")
-	c.Assert(remotes[0].Fetch, HasLen, 1)
-	c.Assert(remotes[0].Fetch[0].String(), Equals, "+refs/heads/*:refs/remotes/origin/*")
+	remote := remotes["origin"]
+	c.Assert(remote.Name, Equals, "origin")
+	c.Assert(remote.URL, Equals, "https://github.com/git-fixtures/basic")
+	c.Assert(remote.Fetch, HasLen, 1)
+	c.Assert(remote.Fetch[0].String(), Equals, "+refs/heads/*:refs/remotes/origin/*")
 }
 
 func (s *ConfigSuite) TearDownTest(c *C) {
diff --git a/storage/filesystem/object.go b/storage/filesystem/object.go
index 6dbeae9..f780183 100644
--- a/storage/filesystem/object.go
+++ b/storage/filesystem/object.go
@@ -13,25 +13,17 @@ import (
 	"gopkg.in/src-d/go-git.v4/utils/fs"
 )
 
-// ObjectStorage is an implementation of core.ObjectStorage that stores
-// data on disk in the standard git format (this is, the .git directory).
-//
-// Zero values of this type are not safe to use, see the New function below.
-//
-// Currently only reads are supported, no writting.
-//
-// Also values from this type are not yet able to track changes on disk, this is,
-// Gitdir values will get outdated as soon as repositories change on disk.
 type ObjectStorage struct {
 	dir   *dotgit.DotGit
 	index map[core.Hash]index
 }
 
-func newObjectStorage(dir *dotgit.DotGit) (*ObjectStorage, error) {
-	s := &ObjectStorage{
+func newObjectStorage(dir *dotgit.DotGit) (ObjectStorage, error) {
+	s := ObjectStorage{
 		dir:   dir,
 		index: make(map[core.Hash]index, 0),
 	}
+
 	return s, s.loadIdxFiles()
 }
 
@@ -64,8 +56,7 @@ func (s *ObjectStorage) NewObject() core.Object {
 	return &core.MemoryObject{}
 }
 
-// Writer method not supported on Memory storage
-func (s *ObjectStorage) Writer() (io.WriteCloser, error) {
+func (s *ObjectStorage) PackfileWriter() (io.WriteCloser, error) {
 	w, err := s.dir.NewObjectPack()
 	if err != nil {
 		return nil, err
@@ -82,7 +73,7 @@ func (s *ObjectStorage) Writer() (io.WriteCloser, error) {
 }
 
 // Set adds a new object to the storage.
-func (s *ObjectStorage) Set(o core.Object) (core.Hash, error) {
+func (s *ObjectStorage) SetObject(o core.Object) (core.Hash, error) {
 	if o.Type() == core.OFSDeltaObject || o.Type() == core.REFDeltaObject {
 		return core.ZeroHash, core.ErrInvalidType
 	}
@@ -114,7 +105,7 @@ func (s *ObjectStorage) Set(o core.Object) (core.Hash, error) {
 
 // Get returns the object with the given hash, by searching for it in
 // the packfile and the git object directories.
-func (s *ObjectStorage) Get(t core.ObjectType, h core.Hash) (core.Object, error) {
+func (s *ObjectStorage) Object(t core.ObjectType, h core.Hash) (core.Object, error) {
 	obj, err := s.getFromUnpacked(h)
 	if err == core.ErrObjectNotFound {
 		obj, err = s.getFromPackfile(h)
@@ -183,7 +174,7 @@ func (s *ObjectStorage) getFromPackfile(h core.Hash) (core.Object, error) {
 	defer f.Close()
 
 	p := packfile.NewScanner(f)
-	d, err := packfile.NewDecoder(p, memory.NewStorage().ObjectStorage())
+	d, err := packfile.NewDecoder(p, memory.NewStorage())
 	if err != nil {
 		return nil, err
 	}
@@ -204,7 +195,7 @@ func (s *ObjectStorage) findObjectInPackfile(h core.Hash) (core.Hash, int64) {
 
 // Iter returns an iterator for all the objects in the packfile with the
 // given type.
-func (s *ObjectStorage) Iter(t core.ObjectType) (core.ObjectIter, error) {
+func (s *ObjectStorage) IterObjects(t core.ObjectType) (core.ObjectIter, error) {
 	objects, err := s.dir.Objects()
 	if err != nil {
 		return nil, err
@@ -251,20 +242,6 @@ func (s *ObjectStorage) buildPackfileIters(
 	return iters, nil
 }
 
-// Begin opens a new transaction. However, this implementation is not
-// transactional, so Commit and Rollback have no effect.
-func (o *ObjectStorage) Begin() core.TxObjectStorage {
-	return o
-}
-
-func (tx *ObjectStorage) Commit() error {
-	return nil
-}
-
-func (tx *ObjectStorage) Rollback() error {
-	return nil
-}
-
 type index map[core.Hash]int64
 
 func (i index) Decode(r io.Reader) error {
@@ -299,7 +276,7 @@ func newPackfileIter(f fs.File, t core.ObjectType, seen map[core.Hash]bool) (cor
 		return nil, err
 	}
 
-	d, err := packfile.NewDecoder(s, memory.NewStorage().ObjectStorage())
+	d, err := packfile.NewDecoder(s, memory.NewStorage())
 	if err != nil {
 		return nil, err
 	}
diff --git a/storage/filesystem/object_test.go b/storage/filesystem/object_test.go
index 0136e54..38e4b6d 100644
--- a/storage/filesystem/object_test.go
+++ b/storage/filesystem/object_test.go
@@ -20,7 +20,7 @@ func (s *FsSuite) TestGetFromObjectFile(c *C) {
 	c.Assert(err, IsNil)
 
 	expected := core.NewHash("f3dfe29d268303fc6e1bbce268605fc99573406e")
-	obj, err := o.Get(core.AnyObject, expected)
+	obj, err := o.Object(core.AnyObject, expected)
 	c.Assert(err, IsNil)
 	c.Assert(obj.Hash(), Equals, expected)
 }
@@ -32,7 +32,7 @@ func (s *FsSuite) TestGetFromPackfile(c *C) {
 		c.Assert(err, IsNil)
 
 		expected := core.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5")
-		obj, err := o.Get(core.AnyObject, expected)
+		obj, err := o.Object(core.AnyObject, expected)
 		c.Assert(err, IsNil)
 		c.Assert(obj.Hash(), Equals, expected)
 	})
@@ -60,7 +60,7 @@ func (s *FsSuite) TestIter(c *C) {
 		o, err := newObjectStorage(dotgit.New(fs))
 		c.Assert(err, IsNil)
 
-		iter, err := o.Iter(core.AnyObject)
+		iter, err := o.IterObjects(core.AnyObject)
 		c.Assert(err, IsNil)
 
 		var count int32
@@ -80,7 +80,7 @@ func (s *FsSuite) TestIterWithType(c *C) {
 		o, err := newObjectStorage(dotgit.New(fs))
 		c.Assert(err, IsNil)
 
-		iter, err := o.Iter(core.CommitObject)
+		iter, err := o.IterObjects(core.CommitObject)
 		c.Assert(err, IsNil)
 
 		err = iter.ForEach(func(o core.Object) error {
diff --git a/storage/filesystem/reference.go b/storage/filesystem/reference.go
index b2aa6d9..0015859 100644
--- a/storage/filesystem/reference.go
+++ b/storage/filesystem/reference.go
@@ -9,15 +9,15 @@ type ReferenceStorage struct {
 	dir *dotgit.DotGit
 }
 
-func (r *ReferenceStorage) Set(ref *core.Reference) error {
+func (r *ReferenceStorage) SetReference(ref *core.Reference) error {
 	return r.dir.SetRef(ref)
 }
 
-func (r *ReferenceStorage) Get(n core.ReferenceName) (*core.Reference, error) {
+func (r *ReferenceStorage) Reference(n core.ReferenceName) (*core.Reference, error) {
 	return r.dir.Ref(n)
 }
 
-func (r *ReferenceStorage) Iter() (core.ReferenceIter, error) {
+func (r *ReferenceStorage) IterReferences() (core.ReferenceIter, error) {
 	refs, err := r.dir.Refs()
 	if err != nil {
 		return nil, err
diff --git a/storage/filesystem/storage.go b/storage/filesystem/storage.go
index d39d6e4..7e05cd3 100644
--- a/storage/filesystem/storage.go
+++ b/storage/filesystem/storage.go
@@ -2,21 +2,20 @@
 package filesystem
 
 import (
-	"gopkg.in/src-d/go-git.v4/config"
-	"gopkg.in/src-d/go-git.v4/core"
 	"gopkg.in/src-d/go-git.v4/storage/filesystem/internal/dotgit"
 	"gopkg.in/src-d/go-git.v4/utils/fs"
 )
 
+// Storage is an implementation of git.Storer that stores data on disk in the
+// standard git format (this is, the .git directory). Zero values of this type
+// are not safe to use, see the NewStorage function below.
 type Storage struct {
-	dir *dotgit.DotGit
-	fs  fs.Filesystem
-
-	o *ObjectStorage
-	r *ReferenceStorage
-	c *ConfigStorage
+	ObjectStorage
+	ReferenceStorage
+	ConfigStorage
 }
 
+// NewStorage returns a new Storage backed by a given `fs.Filesystem`
 func NewStorage(fs fs.Filesystem) (*Storage, error) {
 	dir := dotgit.New(fs)
 	o, err := newObjectStorage(dir)
@@ -24,27 +23,9 @@ func NewStorage(fs fs.Filesystem) (*Storage, error) {
 		return nil, err
 	}
 
-	return &Storage{dir: dir, fs: fs, o: o}, nil
-}
-
-func (s *Storage) ObjectStorage() core.ObjectStorage {
-	return s.o
-}
-
-func (s *Storage) ReferenceStorage() core.ReferenceStorage {
-	if s.r != nil {
-		return s.r
-	}
-
-	s.r = &ReferenceStorage{dir: s.dir}
-	return s.r
-}
-
-func (s *Storage) ConfigStorage() config.ConfigStorage {
-	if s.c != nil {
-		return s.c
-	}
-
-	s.c = &ConfigStorage{dir: s.dir}
-	return s.c
+	return &Storage{
+		ObjectStorage:    o,
+		ReferenceStorage: ReferenceStorage{dir: dir},
+		ConfigStorage:    ConfigStorage{dir: dir},
+	}, nil
 }
diff --git a/storage/filesystem/storage_test.go b/storage/filesystem/storage_test.go
index f55452d..c24d5d5 100644
--- a/storage/filesystem/storage_test.go
+++ b/storage/filesystem/storage_test.go
@@ -18,20 +18,8 @@ type StorageSuite struct {
 var _ = Suite(&StorageSuite{})
 
 func (s *StorageSuite) SetUpTest(c *C) {
-	path := c.MkDir()
-	storage, err := NewStorage(os.New(path))
+	storage, err := NewStorage(os.New(c.MkDir()))
 	c.Assert(err, IsNil)
-	s.BaseStorageSuite = test.NewBaseStorageSuite(
-		storage.ObjectStorage(),
-		storage.ReferenceStorage(),
-		storage.ConfigStorage(),
-	)
-}
-
-func (s *StorageSuite) TestTxObjectStorageSetAndCommit(c *C) {
-	c.Skip("tx not supported")
-}
 
-func (s *StorageSuite) TestTxObjectStorageSetAndRollback(c *C) {
-	c.Skip("tx not supported")
+	s.BaseStorageSuite = test.NewBaseStorageSuite(storage)
 }