storage/filesystem: check file object before using cache

If the cache is shared between several repositories getFromUnpacked can
erroneously return an object from other repository.

This decreases performance a little bit as there's an extra fs operation
when the object is in the cache but is correct when the cache is shared.

Signed-off-by: Javi Fontan <[email protected]>

storage/filesystem/object.go

@@ -307,10 +307,6 @@ func (s *ObjectStorage) DeltaObject(t plumbing.ObjectType,
 }
 
 func (s *ObjectStorage) getFromUnpacked(h plumbing.Hash) (obj plumbing.EncodedObject, err error) {
-	if cacheObj, found := s.objectCache.Get(h); found {
-		return cacheObj, nil
-	}
-
 	f, err := s.dir.Object(h)
 	if err != nil {
 		if os.IsNotExist(err) {
@@ -319,9 +315,12 @@ func (s *ObjectStorage) getFromUnpacked(h plumbing.Hash) (obj plumbing.EncodedOb
 
 		return nil, err
 	}
-
 	defer ioutil.CheckClose(f, &err)
 
+	if cacheObj, found := s.objectCache.Get(h); found {
+		return cacheObj, nil
+	}
+
 	obj = s.NewEncodedObject()
 	r, err := objfile.NewReader(f)
 	if err != nil {

storage/filesystem/object_test.go

@@ -297,6 +297,23 @@ func (s *FsSuite) TestPackfileIterKeepDescriptors(c *C) {
 	})
 }
 
+func (s *FsSuite) TestGetFromObjectFileSharedCache(c *C) {
+	f1 := fixtures.ByTag("worktree").One().DotGit()
+	f2 := fixtures.ByTag("worktree").ByTag("submodule").One().DotGit()
+
+	ch := cache.NewObjectLRUDefault()
+	o1 := NewObjectStorage(dotgit.New(f1), ch)
+	o2 := NewObjectStorage(dotgit.New(f2), ch)
+
+	expected := plumbing.NewHash("af2d6a6954d532f8ffb47615169c8fdf9d383a1a")
+	obj, err := o1.EncodedObject(plumbing.CommitObject, expected)
+	c.Assert(err, IsNil)
+	c.Assert(obj.Hash(), Equals, expected)
+
+	obj, err = o2.EncodedObject(plumbing.CommitObject, expected)
+	c.Assert(err, Equals, plumbing.ErrObjectNotFound)
+}
+
 func BenchmarkPackfileIter(b *testing.B) {
 	if err := fixtures.Init(); err != nil {
 		b.Fatal(err)