From 0397b29d2427dad70af8e5b51560a74fd277d78f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jake=20Van=20Alstyne=20=F0=9F=8E=A9?= Date: Mon, 8 Jan 2018 20:38:32 -0700 Subject: [PATCH] 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 | 38 +++-- SwiftGit2/Repository.swift | 224 +++++++++++++++++++++------- SwiftGit2Tests/RepositorySpec.swift | 30 ++-- 3 files changed, 212 insertions(+), 80 deletions(-) diff --git a/SwiftGit2/Diffs.swift b/SwiftGit2/Diffs.swift index ff1f69f..7caf8d8 100644 --- a/SwiftGit2/Diffs.swift +++ b/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..(_ oids: [OID], type: git_otype, transform: ([OpaquePointer]) -> Result) -> Result { + 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 { + 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 = self.diff(from: parent.oid, to: commit.oid) guard diff.error == nil else { - return Result<[Diff.Delta], NSError>.failure(diff.error!) + return Result.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 { - 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 { + 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.success(diff!) } - private func treePointer(from commitOid: OID) -> Result { - 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 { + 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.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.failure(result.error!) + } + newTree = result.value! } - let result = Result.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 { + 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 { + 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.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) } diff --git a/SwiftGit2Tests/RepositorySpec.swift b/SwiftGit2Tests/RepositorySpec.swift index f02a87e..d156029 100644 --- a/SwiftGit2Tests/RepositorySpec.swift +++ b/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)) }