Merge pull request #962 from jfontan/fix/do-not-close-files-in-iterator

storage/filesystem: keep packs open in PackfileIter

Author: mcuadros

storage/filesystem/object.go

@@ -396,7 +396,10 @@ func (s *ObjectStorage) IterEncodedObjects(t plumbing.ObjectType) (storer.Encode
 	return storer.NewMultiEncodedObjectIter(iters), nil
 }
 
-func (s *ObjectStorage) buildPackfileIters(t plumbing.ObjectType, seen map[plumbing.Hash]struct{}) (storer.EncodedObjectIter, error) {
+func (s *ObjectStorage) buildPackfileIters(
+	t plumbing.ObjectType,
+	seen map[plumbing.Hash]struct{},
+) (storer.EncodedObjectIter, error) {
 	if err := s.requireIndex(); err != nil {
 		return nil, err
 	}
@@ -412,7 +415,10 @@ func (s *ObjectStorage) buildPackfileIters(t plumbing.ObjectType, seen map[plumb
 			if err != nil {
 				return nil, err
 			}
-			return newPackfileIter(s.dir.Fs(), pack, t, seen, s.index[h], s.deltaBaseCache)
+			return newPackfileIter(
+				s.dir.Fs(), pack, t, seen, s.index[h],
+				s.deltaBaseCache, s.options.KeepDescriptors,
+			)
 		},
 	}, nil
 }
@@ -473,16 +479,21 @@ type packfileIter struct {
 	pack billy.File
 	iter storer.EncodedObjectIter
 	seen map[plumbing.Hash]struct{}
+
+	// tells whether the pack file should be left open after iteration or not
+	keepPack bool
 }
 
 // NewPackfileIter returns a new EncodedObjectIter for the provided packfile
 // and object type. Packfile and index file will be closed after they're
-// used.
+// used. If keepPack is true the packfile won't be closed after the iteration
+// finished.
 func NewPackfileIter(
 	fs billy.Filesystem,
 	f billy.File,
 	idxFile billy.File,
 	t plumbing.ObjectType,
+	keepPack bool,
 ) (storer.EncodedObjectIter, error) {
 	idx := idxfile.NewMemoryIndex()
 	if err := idxfile.NewDecoder(idxFile).Decode(idx); err != nil {
@@ -493,7 +504,8 @@ func NewPackfileIter(
 		return nil, err
 	}
 
-	return newPackfileIter(fs, f, t, make(map[plumbing.Hash]struct{}), idx, nil)
+	seen := make(map[plumbing.Hash]struct{})
+	return newPackfileIter(fs, f, t, seen, idx, nil, keepPack)
 }
 
 func newPackfileIter(
@@ -503,6 +515,7 @@ func newPackfileIter(
 	seen map[plumbing.Hash]struct{},
 	index idxfile.Index,
 	cache cache.Object,
+	keepPack bool,
 ) (storer.EncodedObjectIter, error) {
 	var p *packfile.Packfile
 	if cache != nil {
@@ -517,9 +530,10 @@ func newPackfileIter(
 	}
 
 	return &packfileIter{
-		pack: f,
-		iter: iter,
-		seen: seen,
+		pack:     f,
+		iter:     iter,
+		seen:     seen,
+		keepPack: keepPack,
 	}, nil
 }
 
@@ -557,7 +571,9 @@ func (iter *packfileIter) ForEach(cb func(plumbing.EncodedObject) error) error {
 
 func (iter *packfileIter) Close() {
 	iter.iter.Close()
-	_ = iter.pack.Close()
+	if !iter.keepPack {
+		_ = iter.pack.Close()
+	}
 }
 
 type objectsIter struct {

storage/filesystem/object_test.go

@@ -153,18 +153,54 @@ func (s *FsSuite) TestPackfileIter(c *C) {
 				idxf, err := dg.ObjectPackIdx(h)
 				c.Assert(err, IsNil)
 
-				iter, err := NewPackfileIter(fs, f, idxf, t)
+				iter, err := NewPackfileIter(fs, f, idxf, t, false)
 				c.Assert(err, IsNil)
+
 				err = iter.ForEach(func(o plumbing.EncodedObject) error {
 					c.Assert(o.Type(), Equals, t)
 					return nil
 				})
-
 				c.Assert(err, IsNil)
 			}
 		}
 	})
+}
+
+func (s *FsSuite) TestPackfileIterKeepDescriptors(c *C) {
+	fixtures.ByTag(".git").Test(c, func(f *fixtures.Fixture) {
+		fs := f.DotGit()
+		ops := dotgit.Options{KeepDescriptors: true}
+		dg := dotgit.NewWithOptions(fs, ops)
+
+		for _, t := range objectTypes {
+			ph, err := dg.ObjectPacks()
+			c.Assert(err, IsNil)
+
+			for _, h := range ph {
+				f, err := dg.ObjectPack(h)
+				c.Assert(err, IsNil)
+
+				idxf, err := dg.ObjectPackIdx(h)
+				c.Assert(err, IsNil)
 
+				iter, err := NewPackfileIter(fs, f, idxf, t, true)
+				c.Assert(err, IsNil)
+
+				err = iter.ForEach(func(o plumbing.EncodedObject) error {
+					c.Assert(o.Type(), Equals, t)
+					return nil
+				})
+				c.Assert(err, IsNil)
+
+				// test twice to check that packfiles are not closed
+				err = iter.ForEach(func(o plumbing.EncodedObject) error {
+					c.Assert(o.Type(), Equals, t)
+					return nil
+				})
+				c.Assert(err, IsNil)
+			}
+		}
+	})
 }
 
 func BenchmarkPackfileIter(b *testing.B) {
@@ -201,7 +237,7 @@ func BenchmarkPackfileIter(b *testing.B) {
 							b.Fatal(err)
 						}
 
-						iter, err := NewPackfileIter(fs, f, idxf, t)
+						iter, err := NewPackfileIter(fs, f, idxf, t, false)
 						if err != nil {
 							b.Fatal(err)
 						}
@@ -257,7 +293,7 @@ func BenchmarkPackfileIterReadContent(b *testing.B) {
 							b.Fatal(err)
 						}
 
-						iter, err := NewPackfileIter(fs, f, idxf, t)
+						iter, err := NewPackfileIter(fs, f, idxf, t, false)
 						if err != nil {
 							b.Fatal(err)
 						}