global storage interface refactor #112

mcuadros · 2016-11-04T00:30:26Z

Renames:

core.ObjectStorage => core.ObjectStorer
core.ReferenceStorage => core.ReferenceStorer
config.ConfigStorage => core.ConfigStorer
config.Storage => core.Storer
core.ObjectStorageWriter => core.PackfileWriter

All the inner methods now are compatible between all the *Storer interfaces. The main git.Storage now is just a group of all the others *Storer interfaces.

A new core.ObjectStorerTxwas create with the goal of having optional transactions in the storers

The config.Storer was simplified to only two methods SetConfig and Config

ReferenceStorer

alcortesm · 2016-11-04T16:41:29Z

formats/packfile/decoder.go

 }

-func (d *Decoder) readObjects(count uint32) error {
-	for i := 0; i < int(count); i++ {
+func (d *Decoder) readObjectsWithObjectStorer(count int) error {


It looks like the name should be readObjectsToObjectStorer, To instead of With.

That's because you have another readObjects without a s.o set (the core.ObjectStorer), I think is clear enough being in the context (is a deep one)

alcortesm · 2016-11-07T09:10:45Z

core/storage.go

@@ -11,41 +11,46 @@ var (
 	ErrNotImplemented = errors.New("method not-implemented")
 )

-// ObjectStorage generic storage of objects
-type ObjectStorage interface {
+// ObjectStorer generic storage of objects


Change the comment to include the word persist, as the other storers.

as we agreed, persistent is not a property intrinsic to a ObjectStorer

alcortesm · 2016-11-07T09:11:17Z

core/storage.go

-	// Iter returns a custom ObjectIter over all the object on the storage.
+	// TreeObject and AnyObject. If AnyObject is given, the object must be
+	// looked up regardless of its type.
+	GetObject(ObjectType, Hash) (Object, error)


Should this be called Object instead?

alcortesm · 2016-11-07T09:14:59Z

core/storage.go

+
+// ObjectStorerTx is a optional method for ObjectStorer, it enable transaction
+// base write and read operations in the storage
+type ObjectStorerTx interface {


We are going too far with the naming of this types, I suggest naming this Tx and move it to an store/object package.

having another package for a interface maybe is too much. Maybe just call the interface Transactional?

alcortesm · 2016-11-07T09:15:42Z

core/storage.go

-	// ErrNotImplemented error.
+// PackfileWriter is a optional method for ObjectStorer, it enable direct write
+// of packfile to the storage
+type PackfileWriter interface {


Interface and method with the same name: we need to rethink how to organize or code.

alcortesm · 2016-11-07T09:17:53Z

core/storage.go

-	Get(ObjectType, Hash) (Object, error)
+// TxObjectStorer is an in-progress storage transaction. A transaction must end
+// with a call to Commit or Rollback.
+type TxObjectStorer interface {


We have an ObjectStorerTx interface and a TxObjectStorer interface.

We must step back, think and organize the code.

Also we must split all this concepts in their own files.

alcortesm · 2016-11-07T09:22:55Z

storage/filesystem/storage.go

@@ -2,19 +2,14 @@
 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"
 )

 type Storage struct {


This needs comments, is this an implementation of Storer?

alcortesm · 2016-11-07T09:24:07Z

storage/memory/storage.go

@@ -12,94 +12,46 @@ var ErrUnsupportedObjectType = fmt.Errorf("unsupported object type")

 // Storage in memory storage system


This sentence doesn't have any verb.

alcortesm · 2016-11-07T09:25:34Z

storage/test/storage_suite.go


 	"gopkg.in/src-d/go-git.v4/config"
 	"gopkg.in/src-d/go-git.v4/core"

 	. "gopkg.in/check.v1"
 )

+type storer interface {


This needs a comment. Why is this type needed, how it is different than Storer?

This is part of the public suite test. If we make reference here to Storer at the main package we will get a cyclic dependency

alcortesm · 2016-11-07T09:26:49Z

common.go

-	ConfigStorage() config.ConfigStorage
-	ObjectStorage() core.ObjectStorage
-	ReferenceStorage() core.ReferenceStorage
+// Storer storage of objects, references and all the information that require


This sentence makes no sense, it has no verb.

Also, is this comment accurate?, does the memory implementation persist data?

@alcortes

* core: ObjectStorage, ReferenceStorage renamed to ObjectStorer and ReferenceStorer * rebase * general, changes request by @alcortes * general, changes request by @alcortes

mcuadros added 6 commits November 4, 2016 01:27

core: ObjectStorage, ReferenceStorage renamed to ObjectStorer and

d156d7f

ReferenceStorer

rebase

cbad394

rebase

dfe21c0

rebase

98bba04

rebase

6d421ea

Merge branch 'master' of github.com:src-d/go-git into storer

b494f4e

alcortesm suggested changes Nov 7, 2016

View reviewed changes

mcuadros added 2 commits November 7, 2016 16:36

general, changes request by @alcortes

7f2f4ff

general, changes request by @alcortes

2e2b5e1

mcuadros merged commit 5a72b12 into src-d:master Nov 7, 2016

mcuadros deleted the storer branch December 13, 2016 10:13

mcuadros added a commit that referenced this pull request Jan 31, 2017

global storage interface refactor (#112)

0ff9ef2

* core: ObjectStorage, ReferenceStorage renamed to ObjectStorer and ReferenceStorer * rebase * general, changes request by @alcortes * general, changes request by @alcortes

		@@ -12,94 +12,46 @@ var ErrUnsupportedObjectType = fmt.Errorf("unsupported object type")

		// Storage in memory storage system

global storage interface refactor #112

global storage interface refactor #112

Uh oh!

Conversation

mcuadros commented Nov 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcuadros Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcuadros Nov 7, 2016 •

edited

Loading