From 82c7a306db75d083db50dbd41aebebd8bd55081b Mon Sep 17 00:00:00 2001
From: Javi Fontan <jfontan@gmail.com>
Date: Thu, 20 Sep 2018 17:10:39 +0200
Subject: 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>
---
 storage/filesystem/object.go      | 33 ++++++++++++++++++++---------
 storage/filesystem/object_test.go | 44 +++++++++++++++++++++++++++++++++++----
 2 files changed, 63 insertions(+), 14 deletions(-)

diff --git a/storage/filesystem/object.go b/storage/filesystem/object.go
index 9eb085f..4aedacc 100644
--- a/storage/filesystem/object.go
+++ b/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 {
diff --git a/storage/filesystem/object_test.go b/storage/filesystem/object_test.go
index bd4a94b..407abf2 100644
--- a/storage/filesystem/object_test.go
+++ b/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)
 						}
-- 
cgit