From 82c7a306db75d083db50dbd41aebebd8bd55081b Mon Sep 17 00:00:00 2001 From: Javi Fontan Date: Thu, 20 Sep 2018 17:10:39 +0200 Subject: [PATCH 1/2] 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 --- 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 9eb085fd3..4aedacca9 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 bd4a94b40..407abf29c 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) } From a7b0102b83aa86fd299623d35666bfee93fee0c6 Mon Sep 17 00:00:00 2001 From: Javi Fontan Date: Fri, 21 Sep 2018 10:43:43 +0200 Subject: [PATCH 2/2] storage/filesystem: add more doc to NewPackfileIter Signed-off-by: Javi Fontan --- storage/filesystem/object.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/storage/filesystem/object.go b/storage/filesystem/object.go index 4aedacca9..68bd140fb 100644 --- a/storage/filesystem/object.go +++ b/storage/filesystem/object.go @@ -476,15 +476,18 @@ 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{} + + // 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,