refactor diff calls to be memory safe

- use existing `withGitObject` method
- introduce extended version that can handle multiple git objects for the purpose of handling multiple parents

SwiftGit2/Diffs.swift

@@ -5,6 +5,7 @@
 //  Created by Jake Van Alstyne on 8/20/17.
 //  Copyright © 2017 GitHub, Inc. All rights reserved.
 //
+import Foundation
 import libgit2
 
 public struct StatusEntry {
@@ -26,6 +27,26 @@ public struct StatusEntry {
 }
 
 public struct Diff {
+
+	/// The set of deltas.
+	public var deltas = [Delta]()
+
+	public struct Delta {
+		public static let type = GIT_OBJ_REF_DELTA
+
+		public var status: Status
+		public var flags: Flags
+		public var oldFile: File?
+		public var newFile: File?
+
+		public init(_ delta: git_diff_delta) {
+			self.status = Status(rawValue: UInt32(git_diff_status_char(delta.status)))
+			self.flags = Flags(rawValue: delta.flags)
+			self.oldFile = File(delta.old_file)
+			self.newFile = File(delta.new_file)
+		}
+	}
+
 	public struct File {
 		public var oid: OID
 		public var path: String
@@ -79,17 +100,12 @@ public struct Diff {
 		public static let exists     = Flags(rawValue: 1 << 2)
 	}
 
-	public struct Delta {
-		public var status: Status
-		public var flags: Flags
-		public var oldFile: File?
-		public var newFile: File?
-
-		public init(_ delta: git_diff_delta) {
-			self.status = Status(rawValue: delta.status.rawValue)
-			self.flags = Flags(rawValue: delta.flags)
-			self.oldFile = File(delta.old_file)
-			self.newFile = File(delta.new_file)
+	/// Create an instance with a libgit2 `git_diff`.
+	public init(_ pointer: OpaquePointer) {
+		for i in 0..<git_diff_num_deltas(pointer) {
+			if let delta = git_diff_get_delta(pointer, i) {
+				deltas.append(Diff.Delta(delta.pointee))
+			}
 		}
 	}
 }

SwiftGit2/Repository.swift

@@ -225,6 +225,27 @@ final public class Repository {
 		return withGitObject(oid, type: type) { Result.success(transform($0)) }
 	}
 
+	private func withGitObjects<T>(_ oids: [OID], type: git_otype, transform: ([OpaquePointer]) -> Result<T, NSError>) -> Result<T, NSError> {
+		var pointers = [OpaquePointer]()
+		for oid in oids {
+			var pointer: OpaquePointer? = nil
+			var oid = oid.oid
+			let result = git_object_lookup(&pointer, self.pointer, &oid, type)
+
+			guard result == GIT_OK.rawValue else {
+				return Result.failure(NSError(gitError: result, pointOfFailure: "git_object_lookup"))
+			}
+
+			pointers.append(pointer!)
+		}
+
+		let value = transform(pointers)
+		for pointer in pointers {
+			git_object_free(pointer)
+		}
+		return value
+	}
+
 	/// Loads the object with the given OID.
 	///
 	/// oid - The OID of the blob to look up.
@@ -561,24 +582,26 @@ final public class Repository {
 
 	// MARK: - Diffs
 
-	public func diff(for commit: Commit) -> Result<[Diff.Delta], NSError> {
+	public func diff(for commit: Commit) -> Result<Diff, NSError> {
+		typealias Delta = Diff.Delta
+
 		guard !commit.parents.isEmpty else {
 			// Initial commit in a repository
-			let diff = self.diff(from: nil, to: commit.oid)
-			if let value = diff.value {
-				return self.processDiffDeltas(value)
-			}
-			return Result<[Diff.Delta], NSError>.failure(diff.error!)
+			return self.diff(from: nil, to: commit.oid)
 		}
 
+		var diffs = [OpaquePointer]()
 		var mergeDiff: OpaquePointer? = nil
 		for parent in commit.parents {
-			let diff = self.diff(from: parent.oid, to: commit.oid)
+			let diff: Result<OpaquePointer, NSError> = self.diff(from: parent.oid, to: commit.oid)
 			guard diff.error == nil else {
-				return Result<[Diff.Delta], NSError>.failure(diff.error!)
+				return Result<Diff, NSError>.failure(diff.error!)
 			}
+
+			diffs.append(diff.value!)
+
 			if mergeDiff == nil {
-				mergeDiff = diff.value
+				mergeDiff = diff.value!
 			} else {
 				let mergeResult = git_diff_merge(mergeDiff, diff.value)
 				guard mergeResult == GIT_OK.rawValue else {
@@ -586,64 +609,132 @@ final public class Repository {
 				}
 			}
 		}
-		return self.processDiffDeltas(mergeDiff!)
+
+		for diff in diffs {
+			git_object_free(diff)
+		}
+
+		return .success(Diff(mergeDiff!))
 	}
 
-	private func diff(from oldOid: OID?,
-	                  to newOid: OID) -> Result<OpaquePointer, NSError> {
-		var oldPointer: OpaquePointer? = nil
-		if oldOid != nil {
-			let result = self.treePointer(from: oldOid!)
-			if let value = result.value {
-				oldPointer = value
-			} else {
-				return result
+	/// Caller is responsible to free returned git_diff with git_object_free
+	private func diff(from oldCommitOid: OID?, to newCommitOid: OID?) -> Result<OpaquePointer, NSError> {
+		assert(oldCommitOid != nil || newCommitOid != nil, "It is an error to pass nil for both the oldOid and newOid")
+
+		var oldTree: OpaquePointer? = nil
+		if let oid = oldCommitOid {
+			let result = unsafeTreeForCommitId(oid)
+			guard result.error == nil else {
+				return Result.failure(result.error!)
 			}
+
+			oldTree = result.value
 		}
 
-		var newPointer: OpaquePointer? = nil
-		let newResult = self.treePointer(from: newOid)
-		if let value = newResult.value {
-			newPointer = value
-		} else {
-			return newResult
+		var newTree: OpaquePointer? = nil
+		if let oid = newCommitOid {
+			let result = unsafeTreeForCommitId(oid)
+			guard result.error == nil else {
+				return Result.failure(result.error!)
+			}
+
+			newTree = result.value
 		}
 
-		var unsafeDiff: OpaquePointer? = nil
-		let diffResult = git_diff_tree_to_tree(&unsafeDiff,
+		var diff: OpaquePointer? = nil
+		let diffResult = git_diff_tree_to_tree(&diff,
 																					 self.pointer,
-																					 oldPointer,
-																					 newPointer,
+																					 oldTree,
+																					 newTree,
 																					 nil)
-		guard diffResult == GIT_OK.rawValue,
-			let unwrapDiffResult = unsafeDiff else {
+
+		git_object_free(oldTree)
+		git_object_free(newTree)
+
+		guard diffResult == GIT_OK.rawValue else {
 			return .failure(NSError(gitError: diffResult,
 															pointOfFailure: "git_diff_tree_to_tree"))
 		}
 
-		return .success(unwrapDiffResult)
+		return Result<OpaquePointer, NSError>.success(diff!)
 	}
 
-	private func treePointer(from commitOid: OID) -> Result<OpaquePointer, NSError> {
-		var commit: OpaquePointer? = nil
-		var _oid = commitOid.oid
-		let commitResult = git_object_lookup(&commit, self.pointer, &_oid, GIT_OBJ_COMMIT)
-		guard commitResult == GIT_OK.rawValue else {
-			return .failure(NSError(gitError: commitResult,
-															pointOfFailure: "git_object_lookup"))
+	/// Memory safe
+	private func diff(from oldCommitOid: OID?, to newCommitOid: OID?) -> Result<Diff, NSError> {
+		assert(oldCommitOid != nil || newCommitOid != nil, "It is an error to pass nil for both the oldOid and newOid")
+
+		var oldTree: Tree? = nil
+		if oldCommitOid != nil {
+			let result = safeTreeForCommitId(oldCommitOid!)
+			guard result.error == nil else {
+				return Result<Diff, NSError>.failure(result.error!)
+			}
+			oldTree = result.value
 		}
 
-		var tree: OpaquePointer? = nil
-		let treeResult = git_commit_tree(&tree, commit)
-		guard treeResult == GIT_OK.rawValue else {
-			return .failure(NSError(gitError: treeResult,
-															pointOfFailure: "git_object_lookup"))
+		var newTree: Tree? = nil
+		if newCommitOid != nil {
+			let result = self.safeTreeForCommitId(newCommitOid!)
+			guard result.error == nil else {
+				return Result<Diff, NSError>.failure(result.error!)
+			}
+			newTree = result.value!
 		}
 
-		let result = Result<OpaquePointer, NSError>.success(tree!)
-		git_tree_free(tree)
-		git_commit_free(commit)
-		return result
+		if oldTree != nil && newTree != nil {
+			return withGitObjects([oldTree!.oid, newTree!.oid], type: GIT_OBJ_TREE) { objects in
+				var diff: OpaquePointer? = nil
+				let diffResult = git_diff_tree_to_tree(&diff,
+																							 self.pointer,
+																							 objects[0],
+																							 objects[1],
+																							 nil)
+				guard diffResult == GIT_OK.rawValue else {
+					return .failure(NSError(gitError: diffResult,
+																	pointOfFailure: "git_diff_tree_to_tree"))
+				}
+
+				let diffObj = Diff(diff!)
+				git_diff_free(diff)
+				return .success(diffObj)
+			}
+		} else if let tree = oldTree {
+			return withGitObject(tree.oid, type: GIT_OBJ_TREE, transform: { tree in
+				var diff: OpaquePointer? = nil
+				let diffResult = git_diff_tree_to_tree(&diff,
+																							 self.pointer,
+																							 tree,
+																							 nil,
+																							 nil)
+				guard diffResult == GIT_OK.rawValue else {
+					return .failure(NSError(gitError: diffResult,
+																	pointOfFailure: "git_diff_tree_to_tree"))
+				}
+
+				let diffObj = Diff(diff!)
+				git_diff_free(diff)
+				return .success(diffObj)
+			})
+		} else if let tree = newTree {
+			return withGitObject(tree.oid, type: GIT_OBJ_TREE, transform: { tree in
+				var diff: OpaquePointer? = nil
+				let diffResult = git_diff_tree_to_tree(&diff,
+																							 self.pointer,
+																							 nil,
+																							 tree,
+																							 nil)
+				guard diffResult == GIT_OK.rawValue else {
+					return .failure(NSError(gitError: diffResult,
+																	pointOfFailure: "git_diff_tree_to_tree"))
+				}
+
+				let diffObj = Diff(diff!)
+				git_diff_free(diff)
+				return .success(diffObj)
+			})
+		}
+
+		return .failure(NSError(gitError: -1, pointOfFailure: "diff(from: to:)"))
 	}
 
 	private func processDiffDeltas(_ diffResult: OpaquePointer) -> Result<[Diff.Delta], NSError> {
@@ -657,14 +748,45 @@ final public class Repository {
 			let gitDiffDelta = Diff.Delta((delta?.pointee)!)
 
 			returnDict.append(gitDiffDelta)
-
-			git_diff_free(OpaquePointer(delta))
 		}
 
 		let result = Result<[Diff.Delta], NSError>.success(returnDict)
 		return result
 	}
 
+	private func safeTreeForCommitId(_ oid: OID) -> Result<Tree, NSError> {
+		return withGitObject(oid, type: GIT_OBJ_COMMIT) { commit in
+			let treeId = git_commit_tree_id(commit)
+			let tree = self.tree(OID(treeId!.pointee))
+			guard tree.error == nil else {
+				return .failure(tree.error!)
+			}
+			return tree
+		}
+	}
+
+	/// Caller responsible to free returned tree with git_object_free
+	private func unsafeTreeForCommitId(_ oid: OID) -> Result<OpaquePointer, NSError> {
+		var commit: OpaquePointer? = nil
+		var oid = oid.oid
+		let commitResult = git_object_lookup(&commit, self.pointer, &oid, GIT_OBJ_COMMIT)
+		guard commitResult == GIT_OK.rawValue else {
+			return .failure(NSError(gitError: commitResult, pointOfFailure: "git_object_lookup"))
+		}
+
+		var tree: OpaquePointer? = nil
+		let treeId = git_commit_tree_id(commit)
+		let treeResult = git_object_lookup(&tree, self.pointer, treeId, GIT_OBJ_TREE)
+
+		git_object_free(commit)
+
+		guard treeResult == GIT_OK.rawValue else {
+			return .failure(NSError(gitError: treeResult, pointOfFailure: "git_object_lookup"))
+		}
+
+		return Result<OpaquePointer, NSError>.success(tree!)
+	}
+
 	// MARK: - Status
 
 	public func status() -> Result<[StatusEntry], NSError> {
@@ -693,7 +815,7 @@ final public class Repository {
 				continue
 			}
 
-			let statusEntry = StatusEntry(from: (s?.pointee)!)
+			let statusEntry = StatusEntry(from: s!.pointee)
 			returnArray.append(statusEntry)
 		}
 

SwiftGit2Tests/RepositorySpec.swift

@@ -737,18 +737,12 @@ class RepositorySpec: QuickSpec {
 
 				let head = repo.HEAD().value!
 				let commit = repo.object(head.oid).value! as! Commit
-				let deltas = repo.diff(for: commit).value!
+				let diff = repo.diff(for: commit).value!
 
-				var newFilePaths: [String] = []
-				for delta in deltas {
-					newFilePaths.append((delta.newFile?.path)!)
-				}
-				var oldFilePaths: [String] = []
-				for delta in deltas {
-					oldFilePaths.append((delta.oldFile?.path)!)
-				}
+				let newFilePaths = diff.deltas.map { $0.newFile!.path }
+				let oldFilePaths = diff.deltas.map { $0.oldFile!.path }
 
-				expect(deltas.count).to(equal(expectedCount))
+				expect(diff.deltas.count).to(equal(expectedCount))
 				expect(newFilePaths).to(equal(expectedNewFilePaths))
 				expect(oldFilePaths).to(equal(expectedOldFilePaths))
 			}
@@ -769,18 +763,18 @@ class RepositorySpec: QuickSpec {
 				                     strategy: CheckoutStrategy.None).error).to(beNil())
 				let head = repo.HEAD().value!
 				let initalCommit = repo.object(head.oid).value! as! Commit
-				let deltas = repo.diff(for: initalCommit).value!
+				let diff = repo.diff(for: initalCommit).value!
 
 				var newFilePaths: [String] = []
-				for delta in deltas {
+				for delta in diff.deltas {
 					newFilePaths.append((delta.newFile?.path)!)
 				}
 				var oldFilePaths: [String] = []
-				for delta in deltas {
+				for delta in diff.deltas {
 					oldFilePaths.append((delta.oldFile?.path)!)
 				}
 
-				expect(deltas.count).to(equal(expectedCount))
+				expect(diff.deltas.count).to(equal(expectedCount))
 				expect(newFilePaths).to(equal(expectedNewFilePaths))
 				expect(oldFilePaths).to(equal(expectedOldFilePaths))
 			}
@@ -837,18 +831,18 @@ class RepositorySpec: QuickSpec {
 				                     strategy: CheckoutStrategy.None).error).to(beNil())
 				let head = repo.HEAD().value!
 				let initalCommit = repo.object(head.oid).value! as! Commit
-				let deltas = repo.diff(for: initalCommit).value!
+				let diff = repo.diff(for: initalCommit).value!
 
 				var newFilePaths: [String] = []
-				for delta in deltas {
+				for delta in diff.deltas {
 					newFilePaths.append((delta.newFile?.path)!)
 				}
 				var oldFilePaths: [String] = []
-				for delta in deltas {
+				for delta in diff.deltas {
 					oldFilePaths.append((delta.oldFile?.path)!)
 				}
 
-				expect(deltas.count).to(equal(expectedCount))
+				expect(diff.deltas.count).to(equal(expectedCount))
 				expect(newFilePaths).to(equal(expectedNewFilePaths))
 				expect(oldFilePaths).to(equal(expectedOldFilePaths))
 			}