Remove redundant blob loads in BlobCache

blob_cache.go

@@ -15,6 +15,7 @@ type BlobCache struct {
 	IgnoreMissingSubmodules bool
 
 	repository *git.Repository
+	cache map[plumbing.Hash]*object.Blob
 }
 
 func (cache *BlobCache) Name() string {
@@ -33,48 +34,66 @@ func (cache *BlobCache) Requires() []string {
 
 func (cache *BlobCache) Initialize(repository *git.Repository) {
 	cache.repository = repository
+	cache.cache = map[plumbing.Hash]*object.Blob{}
 }
 
 func (self *BlobCache) Consume(deps map[string]interface{}) (map[string]interface{}, error) {
 	commit := deps["commit"].(*object.Commit)
 	changes := deps["changes"].(object.Changes)
-	cache := make(map[plumbing.Hash]*object.Blob)
+	cache := map[plumbing.Hash]*object.Blob{}
+	newCache := map[plumbing.Hash]*object.Blob{}
 	for _, change := range changes {
 		action, err := change.Action()
 		if err != nil {
 			fmt.Fprintf(os.Stderr, "no action in %s\n", change.To.TreeEntry.Hash)
 			return nil, err
 		}
+		var exists bool
+		var blob *object.Blob
 		switch action {
 		case merkletrie.Insert:
-			cache[change.To.TreeEntry.Hash], err = self.getBlob(&change.To, commit.File)
+			blob, err = self.getBlob(&change.To, commit.File)
 			if err != nil {
 				fmt.Fprintf(os.Stderr, "file to %s %s\n", change.To.Name, change.To.TreeEntry.Hash)
+			} else {
+				cache[change.To.TreeEntry.Hash] = blob
+				newCache[change.To.TreeEntry.Hash] = blob
 			}
 		case merkletrie.Delete:
-			cache[change.From.TreeEntry.Hash], err = self.getBlob(&change.From, commit.File)
-			if err != nil {
-				if err.Error() != plumbing.ErrObjectNotFound.Error() {
-					fmt.Fprintf(os.Stderr, "file from %s %s\n", change.From.Name, change.From.TreeEntry.Hash)
-				} else {
-					cache[change.From.TreeEntry.Hash], err = createDummyBlob(
-						change.From.TreeEntry.Hash)
+			cache[change.From.TreeEntry.Hash], exists = self.cache[change.From.TreeEntry.Hash]
+			if !exists {
+				cache[change.From.TreeEntry.Hash], err = self.getBlob(&change.From, commit.File)
+				if err != nil {
+					if err.Error() != plumbing.ErrObjectNotFound.Error() {
+						fmt.Fprintf(os.Stderr, "file from %s %s\n", change.From.Name,
+							change.From.TreeEntry.Hash)
+					} else {
+						cache[change.From.TreeEntry.Hash], err = createDummyBlob(
+							change.From.TreeEntry.Hash)
+					}
 				}
 			}
 		case merkletrie.Modify:
-			cache[change.To.TreeEntry.Hash], err = self.getBlob(&change.To, commit.File)
+			blob, err = self.getBlob(&change.To, commit.File)
 			if err != nil {
 				fmt.Fprintf(os.Stderr, "file to %s\n", change.To.Name)
+			} else {
+				cache[change.To.TreeEntry.Hash] = blob
+				newCache[change.To.TreeEntry.Hash] = blob
 			}
-			cache[change.From.TreeEntry.Hash], err = self.getBlob(&change.From, commit.File)
-			if err != nil {
-				fmt.Fprintf(os.Stderr, "file from %s\n", change.From.Name)
+			cache[change.From.TreeEntry.Hash], exists = self.cache[change.From.TreeEntry.Hash]
+			if !exists {
+				cache[change.From.TreeEntry.Hash], err = self.getBlob(&change.From, commit.File)
+				if err != nil {
+					fmt.Fprintf(os.Stderr, "file from %s\n", change.From.Name)
+				}
 			}
 		}
 		if err != nil {
 			return nil, err
 		}
 	}
+	self.cache = newCache
 	return map[string]interface{}{"blob_cache": cache}, nil
 }
 

blob_cache_test.go

@@ -129,6 +129,35 @@ func TestBlobCacheConsumeNoAction(t *testing.T) {
 		"80fe25955b8e725feee25c08ea5759d74f8b670d"))
 	treeTo, _ := testRepository.TreeObject(plumbing.NewHash(
 		"63076fa0dfd93e94b6d2ef0fc8b1fdf9092f83c4"))
+	changes[0] = &object.Change{From: object.ChangeEntry{}, To: object.ChangeEntry{}}
+	deps := map[string]interface{}{}
+	deps["commit"] = commit
+	deps["changes"] = changes
+	result, err := fixtureBlobCache().Consume(deps)
+	assert.Nil(t, result)
+	assert.NotNil(t, err)
+	changes[0] = &object.Change{From: object.ChangeEntry{
+		Name:      "labours.py",
+		Tree:      treeFrom,
+		TreeEntry: object.TreeEntry{},
+	}, To: object.ChangeEntry{
+		Name:      "labours.py",
+		Tree:      treeTo,
+		TreeEntry: object.TreeEntry{},
+	}}
+	result, err = fixtureBlobCache().Consume(deps)
+	assert.Nil(t, result)
+	assert.NotNil(t, err)
+}
+
+func TestBlobCacheConsumeBadHashes(t *testing.T) {
+	commit, _ := testRepository.CommitObject(plumbing.NewHash(
+		"af2d8db70f287b52d2428d9887a69a10bc4d1f46"))
+	changes := make(object.Changes, 1)
+	treeFrom, _ := testRepository.TreeObject(plumbing.NewHash(
+		"80fe25955b8e725feee25c08ea5759d74f8b670d"))
+	treeTo, _ := testRepository.TreeObject(plumbing.NewHash(
+		"63076fa0dfd93e94b6d2ef0fc8b1fdf9092f83c4"))
 	changes[0] = &object.Change{From: object.ChangeEntry{
 		Name:      "labours.py",
 		Tree:      treeFrom,
@@ -144,6 +173,24 @@ func TestBlobCacheConsumeNoAction(t *testing.T) {
 	result, err := fixtureBlobCache().Consume(deps)
 	assert.Nil(t, result)
 	assert.NotNil(t, err)
+	changes[0] = &object.Change{From: object.ChangeEntry{
+		Name:      "labours.py",
+		Tree:      treeFrom,
+		TreeEntry: object.TreeEntry{},
+	}, To: object.ChangeEntry{}}
+	result, err = fixtureBlobCache().Consume(deps)
+	// Deleting a missing blob is fine
+	assert.NotNil(t, result)
+	assert.Nil(t, err)
+	changes[0] = &object.Change{From: object.ChangeEntry{},
+		To: object.ChangeEntry{
+		Name:      "labours.py",
+		Tree:      treeTo,
+		TreeEntry: object.TreeEntry{},
+	}}
+	result, err = fixtureBlobCache().Consume(deps)
+	assert.Nil(t, result)
+	assert.NotNil(t, err)
 }
 
 func TestBlobCacheConsumeInvalidHash(t *testing.T) {