From 0a9019b62919b77be7c3790d4569ee959acc5995 Mon Sep 17 00:00:00 2001
From: Edward Thomson <ethomson@github.com>
Date: Wed, 23 Mar 2016 12:18:16 -0400
Subject: [PATCH] GTTreeBuilder: don't add blob data lazily

Don't add the blob data lazily to the object database (during tree
creation), which would prevent strict object validity checking from
functioning.  In addition, this prevents us from hashing the file
twice (once to compute the OID, again when adding to the object
database) and reduces memory overhead.
---
 ObjectiveGit/GTTreeBuilder.m          | 46 +++------------------------
 ObjectiveGitTests/GTTreeBuilderSpec.m |  6 ++--
 2 files changed, 6 insertions(+), 46 deletions(-)

diff --git a/ObjectiveGit/GTTreeBuilder.m b/ObjectiveGit/GTTreeBuilder.m
index 6aa5da7a4..5ae85646a 100644
--- a/ObjectiveGit/GTTreeBuilder.m
+++ b/ObjectiveGit/GTTreeBuilder.m
@@ -43,13 +43,6 @@ @interface GTTreeBuilder ()
 @property (nonatomic, assign, readonly) git_treebuilder *git_treebuilder;
 @property (nonatomic, strong, readonly) GTRepository *repository;
 
-// Data to be written with the tree, keyed by the file name. This should only be
-// accessed while synchronized on self.
-//
-// This is needed because we don't want to add the entries to the object
-// database until the tree's been written.
-@property (nonatomic, strong, readonly) NSMutableDictionary *fileNameToPendingData;
-
 @end
 
 @implementation GTTreeBuilder
@@ -80,8 +73,6 @@ - (instancetype)initWithTree:(GTTree *)treeOrNil repository:(GTRepository *)repo
 	}
 
 	_repository = repository;
-	_fileNameToPendingData	= [NSMutableDictionary dictionary];
-
 	return self;
 }
 
@@ -122,12 +113,11 @@ - (GTTreeEntry *)addEntryWithData:(NSData *)data fileName:(NSString *)fileName f
 	NSParameterAssert(data != nil);
 	NSParameterAssert(fileName != nil);
 
-	GTOID *OID = [GTOID OIDByHashingData:data type:GTObjectTypeBlob error:error];
-	if (OID == nil) return nil;
+	GTObjectDatabase *odb = [self.repository objectDatabaseWithError:error];
+	if (odb == nil) return nil;
 
-	@synchronized (self) {
-		self.fileNameToPendingData[fileName] = data;
-	}
+	GTOID *OID = [odb writeData:data type:GTObjectTypeBlob error:error];
+	if (OID == nil) return nil;
 
 	return [self addEntryWithOID:OID fileName:fileName fileMode:fileMode error:error];
 }
@@ -153,38 +143,10 @@ - (BOOL)removeEntryWithFileName:(NSString *)fileName error:(NSError **)error {
 		if (error != NULL) *error = [NSError git_errorFor:status description:@"Failed to remove entry with name %@ from tree builder.", fileName];
 	}
 
-	@synchronized (self) {
-		[self.fileNameToPendingData removeObjectForKey:fileName];
-	}
-
 	return status == GIT_OK;
 }
 
-- (BOOL)writePendingData:(NSError **)error {
-	NSDictionary *copied;
-	@synchronized (self) {
-		copied = [self.fileNameToPendingData copy];
-		[self.fileNameToPendingData removeAllObjects];
-	}
-
-	if (copied.count != 0) {
-		GTObjectDatabase *odb = [self.repository objectDatabaseWithError:error];
-		if (odb == nil) return NO;
-
-		for (NSString *fileName in copied) {
-			NSData *data = copied[fileName];
-			GTOID *dataOID = [odb writeData:data type:GTObjectTypeBlob error:error];
-			if (dataOID == nil) return NO;
-		}
-	}
-
-	return YES;
-}
-
 - (GTTree *)writeTree:(NSError **)error {
-	BOOL success = [self writePendingData:error];
-	if (!success) return nil;
-
 	git_oid treeOid;
 	int status = git_treebuilder_write(&treeOid, self.git_treebuilder);
 	if (status != GIT_OK) {
diff --git a/ObjectiveGitTests/GTTreeBuilderSpec.m b/ObjectiveGitTests/GTTreeBuilderSpec.m
index c05b80637..547a9b149 100644
--- a/ObjectiveGitTests/GTTreeBuilderSpec.m
+++ b/ObjectiveGitTests/GTTreeBuilderSpec.m
@@ -103,19 +103,17 @@
 		expect(foundEntry.SHA).to(equal(entry.SHA));
 	});
 
-	it(@"should write new blobs when the tree is written", ^{
+	it(@"should be possible to write a blob with data", ^{
 		GTTreeEntry *entry = [builder addEntryWithData:[@"Hello, World!" dataUsingEncoding:NSUTF8StringEncoding] fileName:@"test.txt" fileMode:GTFileModeBlob error:NULL];
 		expect(entry).notTo(beNil());
 
 		GTObjectDatabase *database = [repo objectDatabaseWithError:NULL];
 		expect(database).notTo(beNil());
 
-		expect(@([database containsObjectWithOID:entry.OID])).to(beFalsy());
+		expect(@([database containsObjectWithOID:entry.OID])).to(beTruthy());
 
 		GTTree *tree = [builder writeTree:NULL];
 		expect(tree).notTo(beNil());
-
-		expect(@([database containsObjectWithOID:entry.OID])).to(beTruthy());
 	});
 
 	it(@"should be possible to write a builder to a repository", ^{