-
Notifications
You must be signed in to change notification settings - Fork 168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Git commit #128
Git commit #128
Conversation
Would you mind rebasing or merging in master? 🙏 |
SwiftGit2/Objects.swift
Outdated
|
||
/// Return an unsafe pointer to the `git_signature` struct. | ||
/// Caller is responsible for freeing it with `git_signature_free`. | ||
var unsafeSignature: Result<UnsafeMutablePointer<git_signature>, NSError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It think we should probably make this a makeX
function. That'd be more in line with the Swift API guidelines. makeUnsafeSignature()
. Or maybe even makeGitSignature()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makeUnsafeSignature()
sounds more consistent with our current unsafe
-prefix stuff
…ick up the new index
@mdiep done |
let lookupResult = git_commit_lookup(&parent, self.pointer, &oid) | ||
guard lookupResult == GIT_OK.rawValue else { | ||
let err = NSError(gitError: lookupResult, pointOfFailure: "git_commit_lookup") | ||
return .failure(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would leak parentGitCommits
. We need a defer
above that frees them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't a defer
free the commit as soon as it goes out of scope i.e. the for loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm I think I found a way
// should be clean now | ||
let newStatus = repo.status() | ||
expect(newStatus.value?.count).to(equal(0)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test. 👍
Looks good apart from this. |
Thanks for taking the time to review! |
Okay this one works but is definitely not as clean as I wanted. And there are way too many levels of nesting. Thoughts?