storage/filesystem: keep packs open in PackfileIter

PackfileIter was not taking into account the option KeepDescriptors
and was always closing the file. This caused "file already closed"
errors when iterating packfiles in with KeepDescriptors active.

Signed-off-by: Javi Fontan <jfontan@gmail.com>

Author: jfontan

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
 }
@@ -470,9 +476,10 @@ func (it *lazyPackfilesIter) Close() {
 }
 
 type packfileIter struct {
-	pack billy.File
-	iter storer.EncodedObjectIter
-	seen map[plumbing.Hash]struct{}
+	pack     billy.File
+	iter     storer.EncodedObjectIter
+	seen     map[plumbing.Hash]struct{}
+	keepPack bool
 }
 
 // NewPackfileIter returns a new EncodedObjectIter for the provided packfile
@@ -483,6 +490,7 @@ func NewPackfileIter(
 	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 +501,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 +512,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 +527,10 @@ func newPackfileIter(
 	}
 
 	return &packfileIter{
-		pack: f,
-		iter: iter,
-		seen: seen,
+		pack:     f,
+		iter:     iter,
+		seen:     seen,
+		keepPack: keepPack,
 	}, nil
 }
 
@@ -557,7 +568,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)
 						}