From 3f7fbc6c49e50eb22e3de8a5143817fa7c0c54dd Mon Sep 17 00:00:00 2001
From: "Santiago M. Mola" <santi@mola.io>
Date: Thu, 3 Nov 2016 17:10:43 +0100
Subject: storage/filesystem: implement missing functionality. (#110)

* storage/filesystem: added ObjectStorage Set.
* storage/filesystem: now passes all tests, except those specific to transactions.
* formats/config: Encoder now encodes subsections with no options.
* formats/config: add HasSubsection on Section.
* dotgit: add Ref method to get specific reference.
---
 formats/config/common_test.go                     | 12 ++---
 formats/config/encoder.go                         | 65 ++++++++++++++++++-----
 formats/config/section.go                         | 10 ++++
 storage/filesystem/config.go                      | 19 ++++---
 storage/filesystem/config_test.go                 |  2 +-
 storage/filesystem/internal/dotgit/dotgit.go      | 21 ++++++++
 storage/filesystem/internal/dotgit/dotgit_test.go | 25 +++++++++
 storage/filesystem/object.go                      | 57 ++++++++++++--------
 storage/filesystem/reference.go                   | 40 ++------------
 storage/filesystem/storage_test.go                | 26 ++++++++-
 storage/test/storage_suite.go                     | 58 +++++++++++++++++---
 11 files changed, 241 insertions(+), 94 deletions(-)

diff --git a/formats/config/common_test.go b/formats/config/common_test.go
index 0bc4d2d..365b53f 100644
--- a/formats/config/common_test.go
+++ b/formats/config/common_test.go
@@ -13,7 +13,7 @@ type CommonSuite struct{}
 var _ = Suite(&CommonSuite{})
 
 func (s *CommonSuite) TestConfig_SetOption(c *C) {
-	obtained := New().SetOption("section", "", "key1", "value1")
+	obtained := New().SetOption("section", NoSubsection, "key1", "value1")
 	expected := &Config{
 		Sections: []*Section{
 			{
@@ -25,7 +25,7 @@ func (s *CommonSuite) TestConfig_SetOption(c *C) {
 		},
 	}
 	c.Assert(obtained, DeepEquals, expected)
-	obtained = obtained.SetOption("section", "", "key1", "value1")
+	obtained = obtained.SetOption("section", NoSubsection, "key1", "value1")
 	c.Assert(obtained, DeepEquals, expected)
 
 	obtained = New().SetOption("section", "subsection", "key1", "value1")
@@ -50,7 +50,7 @@ func (s *CommonSuite) TestConfig_SetOption(c *C) {
 }
 
 func (s *CommonSuite) TestConfig_AddOption(c *C) {
-	obtained := New().AddOption("section", "", "key1", "value1")
+	obtained := New().AddOption("section", NoSubsection, "key1", "value1")
 	expected := &Config{
 		Sections: []*Section{
 			{
@@ -66,10 +66,10 @@ func (s *CommonSuite) TestConfig_AddOption(c *C) {
 
 func (s *CommonSuite) TestConfig_RemoveSection(c *C) {
 	sect := New().
-		AddOption("section1", "", "key1", "value1").
-		AddOption("section2", "", "key1", "value1")
+		AddOption("section1", NoSubsection, "key1", "value1").
+		AddOption("section2", NoSubsection, "key1", "value1")
 	expected := New().
-		AddOption("section1", "", "key1", "value1")
+		AddOption("section1", NoSubsection, "key1", "value1")
 	c.Assert(sect.RemoveSection("other"), DeepEquals, sect)
 	c.Assert(sect.RemoveSection("section2"), DeepEquals, expected)
 }
diff --git a/formats/config/encoder.go b/formats/config/encoder.go
index 4a68e44..88bdf65 100644
--- a/formats/config/encoder.go
+++ b/formats/config/encoder.go
@@ -7,7 +7,7 @@ import (
 
 // An Encoder writes config files to an output stream.
 type Encoder struct {
-	io.Writer
+	w io.Writer
 }
 
 // NewEncoder returns a new encoder that writes to w.
@@ -18,21 +18,58 @@ func NewEncoder(w io.Writer) *Encoder {
 // Encode writes the config in git config format to the stream of the encoder.
 func (e *Encoder) Encode(cfg *Config) error {
 	for _, s := range cfg.Sections {
-		if len(s.Options) > 0 {
-			fmt.Fprintf(e, "[%s]\n", s.Name)
-			for _, o := range s.Options {
-				fmt.Fprintf(e, "\t%s = %s\n", o.Key, o.Value)
-			}
+		if err := e.encodeSection(s); err != nil {
+			return err
 		}
-		for _, ss := range s.Subsections {
-			if len(ss.Options) > 0 {
-				//TODO: escape
-				fmt.Fprintf(e, "[%s \"%s\"]\n", s.Name, ss.Name)
-				for _, o := range ss.Options {
-					fmt.Fprintf(e, "\t%s = %s\n", o.Key, o.Value)
-				}
-			}
+	}
+
+	return nil
+}
+
+func (e *Encoder) encodeSection(s *Section) error {
+	if len(s.Options) > 0 {
+		if err := e.printf("[%s]\n", s.Name); err != nil {
+			return err
+		}
+
+		if err := e.encodeOptions(s.Options); err != nil {
+			return err
+		}
+	}
+
+	for _, ss := range s.Subsections {
+		if err := e.encodeSubsection(s.Name, ss); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+func (e *Encoder) encodeSubsection(sectionName string, s *Subsection) error {
+	//TODO: escape
+	if err := e.printf("[%s \"%s\"]\n", sectionName, s.Name); err != nil {
+		return err
+	}
+
+	if err := e.encodeOptions(s.Options); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func (e *Encoder) encodeOptions(opts Options) error {
+	for _, o := range opts {
+		if err := e.printf("\t%s = %s\n", o.Key, o.Value); err != nil {
+			return err
 		}
 	}
+
 	return nil
 }
+
+func (e *Encoder) printf(msg string, args ...interface{}) error {
+	_, err := fmt.Fprintf(e.w, msg, args...)
+	return err
+}
diff --git a/formats/config/section.go b/formats/config/section.go
index 6d6a891..552ce74 100644
--- a/formats/config/section.go
+++ b/formats/config/section.go
@@ -74,3 +74,13 @@ func (s *Section) Subsection(name string) *Subsection {
 	s.Subsections = append(s.Subsections, ss)
 	return ss
 }
+
+func (s *Section) HasSubsection(name string) bool {
+	for _, ss := range s.Subsections {
+		if ss.IsName(name) {
+			return true
+		}
+	}
+
+	return false
+}
diff --git a/storage/filesystem/config.go b/storage/filesystem/config.go
index ba7e40b..b32265f 100644
--- a/storage/filesystem/config.go
+++ b/storage/filesystem/config.go
@@ -24,12 +24,12 @@ func (c *ConfigStorage) Remote(name string) (*config.RemoteConfig, error) {
 		return nil, err
 	}
 
-	s := cfg.Section(remoteSection).Subsection(name)
-	if s == nil {
+	s := cfg.Section(remoteSection)
+	if !s.HasSubsection(name) {
 		return nil, config.ErrRemoteConfigNotFound
 	}
 
-	return parseRemote(s), nil
+	return parseRemote(s.Subsection(name)), nil
 }
 
 func (c *ConfigStorage) Remotes() ([]*config.RemoteConfig, error) {
@@ -48,6 +48,10 @@ func (c *ConfigStorage) Remotes() ([]*config.RemoteConfig, error) {
 }
 
 func (c *ConfigStorage) SetRemote(r *config.RemoteConfig) error {
+	if err := r.Validate(); err != nil {
+		return err
+	}
+
 	cfg, err := c.read()
 	if err != nil {
 		return err
@@ -55,7 +59,9 @@ func (c *ConfigStorage) SetRemote(r *config.RemoteConfig) error {
 
 	s := cfg.Section(remoteSection).Subsection(r.Name)
 	s.Name = r.Name
-	s.SetOption(urlKey, r.URL)
+	if r.URL != "" {
+		s.SetOption(urlKey, r.URL)
+	}
 	s.RemoveOption(fetchKey)
 	for _, rs := range r.Fetch {
 		s.AddOption(fetchKey, rs.String())
@@ -103,15 +109,14 @@ func (c *ConfigStorage) write(cfg *gitconfig.Config) error {
 		return err
 	}
 
-	defer f.Close()
-
 	e := gitconfig.NewEncoder(f)
 	err = e.Encode(cfg)
 	if err != nil {
+		f.Close()
 		return err
 	}
 
-	return nil
+	return f.Close()
 }
 
 func parseRemote(s *gitconfig.Subsection) *config.RemoteConfig {
diff --git a/storage/filesystem/config_test.go b/storage/filesystem/config_test.go
index 7759f4e..b52e6a1 100644
--- a/storage/filesystem/config_test.go
+++ b/storage/filesystem/config_test.go
@@ -31,7 +31,7 @@ func (s *ConfigSuite) SetUpTest(c *C) {
 
 func (s *ConfigSuite) TestSetRemote(c *C) {
 	cfg := &ConfigStorage{s.dir}
-	err := cfg.SetRemote(&config.RemoteConfig{Name: "foo"})
+	err := cfg.SetRemote(&config.RemoteConfig{Name: "foo", URL: "foo"})
 	c.Assert(err, IsNil)
 
 	remote, err := cfg.Remote("foo")
diff --git a/storage/filesystem/internal/dotgit/dotgit.go b/storage/filesystem/internal/dotgit/dotgit.go
index ba293af..6606153 100644
--- a/storage/filesystem/internal/dotgit/dotgit.go
+++ b/storage/filesystem/internal/dotgit/dotgit.go
@@ -215,6 +215,27 @@ func (d *DotGit) Refs() ([]*core.Reference, error) {
 	return refs, nil
 }
 
+// Ref returns the reference for a given reference name.
+func (d *DotGit) Ref(name core.ReferenceName) (*core.Reference, error) {
+	ref, err := d.readReferenceFile(".", name.String())
+	if err == nil {
+		return ref, nil
+	}
+
+	refs, err := d.Refs()
+	if err != nil {
+		return nil, err
+	}
+
+	for _, ref := range refs {
+		if ref.Name() == name {
+			return ref, nil
+		}
+	}
+
+	return nil, core.ErrReferenceNotFound
+}
+
 func (d *DotGit) addRefsFromPackedRefs(refs *[]*core.Reference) (err error) {
 	f, err := d.fs.Open(packedRefsPath)
 	if err != nil {
diff --git a/storage/filesystem/internal/dotgit/dotgit_test.go b/storage/filesystem/internal/dotgit/dotgit_test.go
index 9e46639..a4cce09 100644
--- a/storage/filesystem/internal/dotgit/dotgit_test.go
+++ b/storage/filesystem/internal/dotgit/dotgit_test.go
@@ -44,6 +44,12 @@ func (s *SuiteDotGit) TestSetRefs(c *C) {
 
 	c.Assert(err, IsNil)
 
+	err = dir.SetRef(core.NewReferenceFromStrings(
+		"bar",
+		"e8d3ffab552895c19b9fcf7aa264d277cde33881",
+	))
+	c.Assert(err, IsNil)
+
 	refs, err := dir.Refs()
 	c.Assert(err, IsNil)
 	c.Assert(refs, HasLen, 2)
@@ -55,6 +61,25 @@ func (s *SuiteDotGit) TestSetRefs(c *C) {
 	ref = findReference(refs, "refs/heads/symbolic")
 	c.Assert(ref, NotNil)
 	c.Assert(ref.Target().String(), Equals, "refs/heads/foo")
+
+	ref = findReference(refs, "bar")
+	c.Assert(ref, IsNil)
+
+	ref, err = dir.Ref("refs/heads/foo")
+	c.Assert(err, IsNil)
+	c.Assert(ref, NotNil)
+	c.Assert(ref.Hash().String(), Equals, "e8d3ffab552895c19b9fcf7aa264d277cde33881")
+
+	ref, err = dir.Ref("refs/heads/symbolic")
+	c.Assert(err, IsNil)
+	c.Assert(ref, NotNil)
+	c.Assert(ref.Target().String(), Equals, "refs/heads/foo")
+
+	ref, err = dir.Ref("bar")
+	c.Assert(err, IsNil)
+	c.Assert(ref, NotNil)
+	c.Assert(ref.Hash().String(), Equals, "e8d3ffab552895c19b9fcf7aa264d277cde33881")
+
 }
 
 func (s *SuiteDotGit) TestRefsFromPackedRefs(c *C) {
diff --git a/storage/filesystem/object.go b/storage/filesystem/object.go
index f2f5351..6dbeae9 100644
--- a/storage/filesystem/object.go
+++ b/storage/filesystem/object.go
@@ -1,7 +1,6 @@
 package filesystem
 
 import (
-	"fmt"
 	"io"
 	"os"
 
@@ -82,11 +81,35 @@ func (s *ObjectStorage) Writer() (io.WriteCloser, error) {
 	return w, nil
 }
 
-// Set adds a new object to the storage. As this functionality is not
-// yet supported, this method always returns a "not implemented yet"
-// error an zero hash.
-func (s *ObjectStorage) Set(core.Object) (core.Hash, error) {
-	return core.ZeroHash, fmt.Errorf("set - not implemented yet")
+// Set adds a new object to the storage.
+func (s *ObjectStorage) Set(o core.Object) (core.Hash, error) {
+	if o.Type() == core.OFSDeltaObject || o.Type() == core.REFDeltaObject {
+		return core.ZeroHash, core.ErrInvalidType
+	}
+
+	ow, err := s.dir.NewObject()
+	if err != nil {
+		return core.ZeroHash, err
+	}
+
+	defer ow.Close()
+
+	or, err := o.Reader()
+	if err != nil {
+		return core.ZeroHash, err
+	}
+
+	defer or.Close()
+
+	if err := ow.WriteHeader(o.Type(), o.Size()); err != nil {
+		return core.ZeroHash, err
+	}
+
+	if _, err := io.Copy(ow, or); err != nil {
+		return core.ZeroHash, err
+	}
+
+	return o.Hash(), nil
 }
 
 // Get returns the object with the given hash, by searching for it in
@@ -228,26 +251,18 @@ 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 &TxObjectStorage{}
-}
-
-type TxObjectStorage struct{}
-
-func (tx *TxObjectStorage) Set(obj core.Object) (core.Hash, error) {
-	return core.ZeroHash, fmt.Errorf("tx.Set - not implemented yet")
-}
-
-func (tx *TxObjectStorage) Get(core.ObjectType, core.Hash) (core.Object, error) {
-	return nil, fmt.Errorf("tx.Get - not implemented yet")
+	return o
 }
 
-func (tx *TxObjectStorage) Commit() error {
-	return fmt.Errorf("tx.Commit - not implemented yet")
+func (tx *ObjectStorage) Commit() error {
+	return nil
 }
 
-func (tx *TxObjectStorage) Rollback() error {
-	return fmt.Errorf("tx.Rollback - not implemented yet")
+func (tx *ObjectStorage) Rollback() error {
+	return nil
 }
 
 type index map[core.Hash]int64
diff --git a/storage/filesystem/reference.go b/storage/filesystem/reference.go
index ae78efb..b2aa6d9 100644
--- a/storage/filesystem/reference.go
+++ b/storage/filesystem/reference.go
@@ -6,8 +6,7 @@ import (
 )
 
 type ReferenceStorage struct {
-	dir  *dotgit.DotGit
-	refs map[core.ReferenceName]*core.Reference
+	dir *dotgit.DotGit
 }
 
 func (r *ReferenceStorage) Set(ref *core.Reference) error {
@@ -15,45 +14,14 @@ func (r *ReferenceStorage) Set(ref *core.Reference) error {
 }
 
 func (r *ReferenceStorage) Get(n core.ReferenceName) (*core.Reference, error) {
-	if err := r.load(); err != nil {
-		return nil, err
-	}
-
-	ref, ok := r.refs[n]
-	if !ok {
-		return nil, core.ErrReferenceNotFound
-	}
-
-	return ref, nil
+	return r.dir.Ref(n)
 }
 
 func (r *ReferenceStorage) Iter() (core.ReferenceIter, error) {
-	if err := r.load(); err != nil {
-		return nil, err
-	}
-
-	var refs []*core.Reference
-	for _, ref := range r.refs {
-		refs = append(refs, ref)
-	}
-
-	return core.NewReferenceSliceIter(refs), nil
-}
-
-func (r *ReferenceStorage) load() error {
-	if len(r.refs) != 0 {
-		return nil
-	}
-
 	refs, err := r.dir.Refs()
 	if err != nil {
-		return err
-	}
-
-	r.refs = make(map[core.ReferenceName]*core.Reference, 0)
-	for _, ref := range refs {
-		r.refs[ref.Name()] = ref
+		return nil, err
 	}
 
-	return nil
+	return core.NewReferenceSliceIter(refs), nil
 }
diff --git a/storage/filesystem/storage_test.go b/storage/filesystem/storage_test.go
index f1f18f9..7d66205 100644
--- a/storage/filesystem/storage_test.go
+++ b/storage/filesystem/storage_test.go
@@ -3,11 +3,35 @@ package filesystem
 import (
 	"testing"
 
+	"gopkg.in/src-d/go-git.v4/storage/test"
+	"gopkg.in/src-d/go-git.v4/utils/fs/os"
+
 	. "gopkg.in/check.v1"
 )
 
 func Test(t *testing.T) { TestingT(t) }
 
-type StorageSuite struct{}
+type StorageSuite struct {
+	test.BaseStorageSuite
+}
 
 var _ = Suite(&StorageSuite{})
+
+func (s *StorageSuite) SetUpTest(c *C) {
+	path := c.MkDir()
+	storage, err := NewStorage(os.NewOS(path))
+	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")
+}
diff --git a/storage/test/storage_suite.go b/storage/test/storage_suite.go
index dc500bb..8686853 100644
--- a/storage/test/storage_suite.go
+++ b/storage/test/storage_suite.go
@@ -1,12 +1,17 @@
 package test
 
 import (
+	"encoding/hex"
+	"errors"
+	"fmt"
 	"io"
+	"io/ioutil"
 	"sort"
 
-	. "gopkg.in/check.v1"
 	"gopkg.in/src-d/go-git.v4/config"
 	"gopkg.in/src-d/go-git.v4/core"
+
+	. "gopkg.in/check.v1"
 )
 
 type TestObject struct {
@@ -67,11 +72,11 @@ func (s *BaseStorageSuite) TestObjectStorageSetAndGet(c *C) {
 
 		o, err := s.ObjectStorage.Get(to.Type, h)
 		c.Assert(err, IsNil)
-		c.Assert(o, Equals, to.Object)
+		c.Assert(objectEquals(o, to.Object), IsNil)
 
 		o, err = s.ObjectStorage.Get(core.AnyObject, h)
 		c.Assert(err, IsNil)
-		c.Assert(o, Equals, to.Object)
+		c.Assert(objectEquals(o, to.Object), IsNil)
 
 		for _, t := range s.validTypes {
 			if t == to.Type {
@@ -85,7 +90,7 @@ func (s *BaseStorageSuite) TestObjectStorageSetAndGet(c *C) {
 	}
 }
 
-func (s *BaseStorageSuite) TestObjectStorageGetIvalid(c *C) {
+func (s *BaseStorageSuite) TestObjectStorageGetInvalid(c *C) {
 	o := s.ObjectStorage.NewObject()
 	o.SetType(core.REFDeltaObject)
 
@@ -95,7 +100,9 @@ func (s *BaseStorageSuite) TestObjectStorageGetIvalid(c *C) {
 
 func (s *BaseStorageSuite) TestObjectStorageIter(c *C) {
 	for _, o := range s.testObjects {
-		s.ObjectStorage.Set(o.Object)
+		h, err := s.ObjectStorage.Set(o.Object)
+		c.Assert(err, IsNil)
+		c.Assert(h, Equals, o.Object.Hash())
 	}
 
 	for _, t := range s.validTypes {
@@ -105,7 +112,7 @@ func (s *BaseStorageSuite) TestObjectStorageIter(c *C) {
 
 		o, err := i.Next()
 		c.Assert(err, IsNil)
-		c.Assert(o, Equals, s.testObjects[t].Object, comment)
+		c.Assert(objectEquals(o, s.testObjects[t].Object), IsNil)
 
 		o, err = i.Next()
 		c.Assert(o, IsNil)
@@ -125,7 +132,7 @@ func (s *BaseStorageSuite) TestObjectStorageIter(c *C) {
 	for _, to := range s.testObjects {
 		found := false
 		for _, o := range foundObjects {
-			if to.Object == o {
+			if to.Object.Hash() == o.Hash() {
 				found = true
 				break
 			}
@@ -222,7 +229,7 @@ func (s *BaseStorageSuite) TestReferenceStorageGetNotFound(c *C) {
 
 func (s *BaseStorageSuite) TestReferenceStorageIter(c *C) {
 	err := s.ReferenceStorage.Set(
-		core.NewReferenceFromStrings("foo", "bc9968d75e48de59f0870ffb71f5e160bbbdcf52"),
+		core.NewReferenceFromStrings("refs/foo", "bc9968d75e48de59f0870ffb71f5e160bbbdcf52"),
 	)
 	c.Assert(err, IsNil)
 
@@ -283,3 +290,38 @@ func (s *BaseStorageSuite) TestConfigStorageRemotes(c *C) {
 	c.Assert(sorted[0], Equals, "bar")
 	c.Assert(sorted[1], Equals, "foo")
 }
+
+func objectEquals(a core.Object, b core.Object) error {
+	ha := a.Hash()
+	hb := b.Hash()
+	if ha != hb {
+		return fmt.Errorf("hashes do not match: %s != %s",
+			ha.String(), hb.String())
+	}
+
+	ra, err := a.Reader()
+	if err != nil {
+		return fmt.Errorf("can't get reader on b: %q", err)
+	}
+
+	rb, err := b.Reader()
+	if err != nil {
+		return fmt.Errorf("can't get reader on a: %q", err)
+	}
+
+	ca, err := ioutil.ReadAll(ra)
+	if err != nil {
+		return fmt.Errorf("error reading a: %q", err)
+	}
+
+	cb, err := ioutil.ReadAll(rb)
+	if err != nil {
+		return fmt.Errorf("error reading b: %q", err)
+	}
+
+	if hex.EncodeToString(ca) != hex.EncodeToString(cb) {
+		return errors.New("content does not match")
+	}
+
+	return nil
+}
-- 
cgit