Merge pull request #942 from jfontan/improvement/maintain-packfiles-open

storage/dotgit: add KeepDescriptors option

5 files changed, 131 insertions, 2 deletions

diff --git a/storage/filesystem/dotgit/dotgit.go b/storage/filesystem/dotgit/dotgit.go
index 00dd2a4..df5cd10 100644
--- a/storage/filesystem/dotgit/dotgit.go
+++ b/storage/filesystem/dotgit/dotgit.go
@@ -62,6 +62,9 @@ type Options struct {
 	// ExclusiveAccess means that the filesystem is not modified externally
 	// while the repo is open.
 	ExclusiveAccess bool
+	// KeepDescriptors makes the file descriptors to be reused but they will
+	// need to be manually closed calling Close().
+	KeepDescriptors bool
 }
 
 // The DotGit type represents a local git repository on disk. This
@@ -78,6 +81,8 @@ type DotGit struct {
 	objectMap  map[plumbing.Hash]struct{}
 	packList   []plumbing.Hash
 	packMap    map[plumbing.Hash]struct{}
+
+	files map[string]billy.File
 }
 
 // New returns a DotGit value ready to be used. The path argument must
@@ -123,6 +128,28 @@ func (d *DotGit) Initialize() error {
 	return nil
 }
 
+// Close closes all opened files.
+func (d *DotGit) Close() error {
+	var firstError error
+	if d.files != nil {
+		for _, f := range d.files {
+			err := f.Close()
+			if err != nil && firstError == nil {
+				firstError = err
+				continue
+			}
+		}
+
+		d.files = nil
+	}
+
+	if firstError != nil {
+		return firstError
+	}
+
+	return nil
+}
+
 // ConfigWriter returns a file pointer for write to the config file
 func (d *DotGit) ConfigWriter() (billy.File, error) {
 	return d.fs.Create(configPath)
@@ -217,12 +244,22 @@ func (d *DotGit) objectPackPath(hash plumbing.Hash, extension string) string {
 }
 
 func (d *DotGit) objectPackOpen(hash plumbing.Hash, extension string) (billy.File, error) {
+	if d.files == nil {
+		d.files = make(map[string]billy.File)
+	}
+
 	err := d.hasPack(hash)
 	if err != nil {
 		return nil, err
 	}
 
-	pack, err := d.fs.Open(d.objectPackPath(hash, extension))
+	path := d.objectPackPath(hash, extension)
+	f, ok := d.files[path]
+	if ok {
+		return f, nil
+	}
+
+	pack, err := d.fs.Open(path)
 	if err != nil {
 		if os.IsNotExist(err) {
 			return nil, ErrPackfileNotFound
@@ -231,6 +268,10 @@ func (d *DotGit) objectPackOpen(hash plumbing.Hash, extension string) (billy.Fil
 		return nil, err
 	}
 
+	if d.options.KeepDescriptors && extension == "pack" {
+		d.files[path] = pack
+	}
+
 	return pack, nil
 }
 
diff --git a/storage/filesystem/dotgit/dotgit_test.go b/storage/filesystem/dotgit/dotgit_test.go
index c34543e..308c6b7 100644
--- a/storage/filesystem/dotgit/dotgit_test.go
+++ b/storage/filesystem/dotgit/dotgit_test.go
@@ -465,6 +465,45 @@ func (s *SuiteDotGit) TestObjectPack(c *C) {
 	c.Assert(filepath.Ext(pack.Name()), Equals, ".pack")
 }
 
+func (s *SuiteDotGit) TestObjectPackWithKeepDescriptors(c *C) {
+	f := fixtures.Basic().ByTag(".git").One()
+	fs := f.DotGit()
+	dir := NewWithOptions(fs, Options{KeepDescriptors: true})
+
+	pack, err := dir.ObjectPack(f.PackfileHash)
+	c.Assert(err, IsNil)
+	c.Assert(filepath.Ext(pack.Name()), Equals, ".pack")
+
+	// Move to an specific offset
+	pack.Seek(42, os.SEEK_SET)
+
+	pack2, err := dir.ObjectPack(f.PackfileHash)
+	c.Assert(err, IsNil)
+
+	// If the file is the same the offset should be the same
+	offset, err := pack2.Seek(0, os.SEEK_CUR)
+	c.Assert(err, IsNil)
+	c.Assert(offset, Equals, int64(42))
+
+	err = dir.Close()
+	c.Assert(err, IsNil)
+
+	pack2, err = dir.ObjectPack(f.PackfileHash)
+	c.Assert(err, IsNil)
+
+	// If the file is opened again its offset should be 0
+	offset, err = pack2.Seek(0, os.SEEK_CUR)
+	c.Assert(err, IsNil)
+	c.Assert(offset, Equals, int64(0))
+
+	err = pack2.Close()
+	c.Assert(err, IsNil)
+
+	err = dir.Close()
+	c.Assert(err, NotNil)
+
+}
+
 func (s *SuiteDotGit) TestObjectPackIdx(c *C) {
 	f := fixtures.Basic().ByTag(".git").One()
 	fs := f.DotGit()
diff --git a/storage/filesystem/object.go b/storage/filesystem/object.go
index 3519385..3545e27 100644
--- a/storage/filesystem/object.go
+++ b/storage/filesystem/object.go
@@ -74,6 +74,7 @@ func (s *ObjectStorage) loadIdxFile(h plumbing.Hash) (err error) {
 	}
 
 	defer ioutil.CheckClose(f, &err)
+
 	idxf := idxfile.NewMemoryIndex()
 	d := idxfile.NewDecoder(f)
 	if err = d.Decode(idxf); err != nil {
@@ -280,7 +281,9 @@ func (s *ObjectStorage) getFromPackfile(h plumbing.Hash, canBeDelta bool) (
 		return nil, err
 	}
 
-	defer ioutil.CheckClose(f, &err)
+	if !s.options.KeepDescriptors {
+		defer ioutil.CheckClose(f, &err)
+	}
 
 	idx := s.index[pack]
 	if canBeDelta {
@@ -423,6 +426,11 @@ func (s *ObjectStorage) buildPackfileIters(t plumbing.ObjectType, seen map[plumb
 	}, nil
 }
 
+// Close closes all opened files.
+func (s *ObjectStorage) Close() error {
+	return s.dir.Close()
+}
+
 type lazyPackfilesIter struct {
 	hashes []plumbing.Hash
 	open   func(h plumbing.Hash) (storer.EncodedObjectIter, error)
diff --git a/storage/filesystem/object_test.go b/storage/filesystem/object_test.go
index b1408b7..4a921a9 100644
--- a/storage/filesystem/object_test.go
+++ b/storage/filesystem/object_test.go
@@ -2,6 +2,7 @@ package filesystem
 
 import (
 	"io/ioutil"
+	"os"
 	"testing"
 
 	"gopkg.in/src-d/go-git.v4/plumbing"
@@ -48,6 +49,42 @@ func (s *FsSuite) TestGetFromPackfile(c *C) {
 	})
 }
 
+func (s *FsSuite) TestGetFromPackfileKeepDescriptors(c *C) {
+	fixtures.Basic().ByTag(".git").Test(c, func(f *fixtures.Fixture) {
+		fs := f.DotGit()
+		dg := dotgit.NewWithOptions(fs, dotgit.Options{KeepDescriptors: true})
+		o, err := NewObjectStorageWithOptions(dg, Options{KeepDescriptors: true})
+		c.Assert(err, IsNil)
+
+		expected := plumbing.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5")
+		obj, err := o.EncodedObject(plumbing.AnyObject, expected)
+		c.Assert(err, IsNil)
+		c.Assert(obj.Hash(), Equals, expected)
+
+		packfiles, err := dg.ObjectPacks()
+		c.Assert(err, IsNil)
+
+		pack1, err := dg.ObjectPack(packfiles[0])
+		c.Assert(err, IsNil)
+
+		pack1.Seek(42, os.SEEK_SET)
+
+		err = o.Close()
+		c.Assert(err, IsNil)
+
+		pack2, err := dg.ObjectPack(packfiles[0])
+		c.Assert(err, IsNil)
+
+		offset, err := pack2.Seek(0, os.SEEK_CUR)
+		c.Assert(err, IsNil)
+		c.Assert(offset, Equals, int64(0))
+
+		err = o.Close()
+		c.Assert(err, IsNil)
+
+	})
+}
+
 func (s *FsSuite) TestGetFromPackfileMultiplePackfiles(c *C) {
 	fs := fixtures.ByTag(".git").ByTag("multi-packfile").One().DotGit()
 	o, err := NewObjectStorage(dotgit.New(fs))
diff --git a/storage/filesystem/storage.go b/storage/filesystem/storage.go
index 25b3653..7fae789 100644
--- a/storage/filesystem/storage.go
+++ b/storage/filesystem/storage.go
@@ -27,6 +27,9 @@ type Options struct {
 	// ExclusiveAccess means that the filesystem is not modified externally
 	// while the repo is open.
 	ExclusiveAccess bool
+	// KeepDescriptors makes the file descriptors to be reused but they will
+	// need to be manually closed calling Close().
+	KeepDescriptors bool
 }
 
 // NewStorage returns a new Storage backed by a given `fs.Filesystem`
@@ -41,6 +44,7 @@ func NewStorageWithOptions(
 ) (*Storage, error) {
 	dirOps := dotgit.Options{
 		ExclusiveAccess: ops.ExclusiveAccess,
+		KeepDescriptors: ops.KeepDescriptors,
 	}
 
 	dir := dotgit.NewWithOptions(fs, dirOps)