Improve Signature Model with Public Initializer and Better API#8
Conversation
…signature method - Introduced a new initializer for the Signature struct to allow setting name, email, date, and timezone. - Added a raw property to convert Signature to a git_signature. - Updated the default signature method to accept a Repository object instead of an OpaquePointer.
There was a problem hiding this comment.
Pull request overview
This PR enhances the Signature model by adding a public initializer to enable custom signature creation and improving the default(in:) API to accept a Repository parameter instead of OpaquePointer, making the API more user-friendly.
- Added public initializer with default parameters for
dateandtimezone - Introduced internal
rawcomputed property for convertingSignaturetogit_signature - Enhanced
default(in:)method with public overload acceptingRepositorytype - Added
.signatureoperation type toSwiftGitXError.Operation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static func `default`(in repository: Repository) throws(SwiftGitXError) -> Signature { | ||
| try `default`(in: repository.pointer) | ||
| } |
There was a problem hiding this comment.
The new public API for default(in:) that accepts a Repository parameter lacks test coverage. Consider adding a test that verifies the method correctly retrieves the default signature from repository configuration.
| ) | ||
| return (pointer, status) | ||
| } | ||
|
|
There was a problem hiding this comment.
Critical memory leak: The signaturePointer allocated by git_signature_new is never freed with git_signature_free. This will leak memory every time the raw property is accessed.
The pointer should be freed after accessing pointee:
var raw: git_signature {
get throws(SwiftGitXError) {
let signaturePointer = try git(operation: .signature) {
var pointer: UnsafeMutablePointer<git_signature>?
let status = git_signature_new(
&pointer,
name,
email,
git_time_t(date.timeIntervalSince1970),
Int32(timezone.secondsFromGMT() / 60)
)
return (pointer, status)
}
defer { git_signature_free(signaturePointer) }
return signaturePointer.pointee
}
}This pattern is consistently used elsewhere in the codebase (see ObjectFactory.makeSignaturePointer usage in StashCollection.swift:114 and TagCollection.swift:89).
| defer { git_signature_free(signaturePointer) } |
| } | ||
| } | ||
|
|
||
| extension SwiftGitXError.Operation { |
There was a problem hiding this comment.
The new operation type .signature should have documentation explaining when it's used. Following the pattern in the codebase, operation extensions typically define all related operations together. Consider adding a documentation comment:
extension SwiftGitXError.Operation {
/// Operation for signature-related errors (creation, validation, etc.).
public static let signature = Self(rawValue: "signature")
}| extension SwiftGitXError.Operation { | |
| extension SwiftGitXError.Operation { | |
| /// Operation for signature-related errors (creation, validation, etc.). |
| /// - date: The date of the action. Defaults to the current date. | ||
| /// - timezone: The timezone of the author. Defaults to the system timezone. | ||
| /// | ||
| /// If `date` and `timezone` are not provided, the current date and system timezone are used. |
There was a problem hiding this comment.
This sentence is redundant with the parameter documentation above. The parameter descriptions with "Defaults to..." already convey this information. Consider removing this line to avoid repetition.
| /// If `date` and `timezone` are not provided, the current date and system timezone are used. |
| public init(name: String, email: String, date: Date = Date(), timezone: TimeZone = .current) { | ||
| self.name = name | ||
| self.email = email | ||
| self.date = date | ||
| self.timezone = timezone | ||
| } |
There was a problem hiding this comment.
The new public initializer lacks test coverage. Consider adding tests to verify:
- Signature creation with all parameters
- Signature creation with default date and timezone
- Signature equality and hashing
- That the signature can be used in operations like commits and tags
Other collections in the codebase (BranchCollection, TagCollection, etc.) have comprehensive test coverage in the Tests/SwiftGitXTests/CollectionTests/ directory.
This PR enhances the Signature model by adding a public initializer and improving the overall API design, making it easier for consumers to create custom signatures for commits, tags, and other Git operations.
Usage