Conversation
There was a problem hiding this comment.
Pull request overview
Adds high-level Repository.pull and Repository.merge operations (plus test coverage) to fetch remote changes and integrate them into the current branch using libgit2 primitives.
Changes:
- Introduce
Repository.pull(remote:option:)with strategies (auto,fastForwardOnly,noFastForward,rebase). - Introduce
Repository.merge(branch:)and supporting merge-commit creation logic. - Add test suites/tags for pull/merge and register new error operations (
merge,pull).
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/SwiftGitXTests/Tags.swift | Adds .merge / .pull tags for the new test suites. |
| Tests/SwiftGitXTests/RepositoryTests/RepositoryPullTests.swift | Adds pull behavior tests across strategies and scenarios. |
| Tests/SwiftGitXTests/RepositoryTests/RepositoryMergeTests.swift | Adds merge behavior tests. |
| Sources/SwiftGitX/Repository/Repository+pull.swift | Implements Repository.pull including merge analysis and a rebase path. |
| Sources/SwiftGitX/Repository/Repository+merge.swift | Implements Repository.merge including conflict detection and merge commit creation. |
| Sources/SwiftGitX/Repository/Repository+fetch.swift | Removes outdated “TODO: Implement pull” stub. |
| Sources/SwiftGitX/Models/Options/PullOption.swift | Adds PullOption enum used by Repository.pull. |
| Sources/SwiftGitX/Helpers/SwiftGitXError.swift | Registers merge / pull operations for error context. |
| Package.resolved | Updates resolved dependency hash. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Analyze merge | ||
| var analysis = git_merge_analysis_t(rawValue: 0) | ||
| var preference = git_merge_preference_t(rawValue: 0) | ||
| var remoteOID = remoteCommit.id.raw | ||
| var annotatedCommit: OpaquePointer? | ||
|
|
||
| try git(operation: .pull) { | ||
| git_annotated_commit_lookup(&annotatedCommit, pointer, &remoteOID) | ||
| } | ||
| defer { git_annotated_commit_free(annotatedCommit) } | ||
|
|
||
| var annotatedCommits: [OpaquePointer?] = [annotatedCommit] | ||
|
|
||
| try git(operation: .pull) { | ||
| annotatedCommits.withUnsafeMutableBufferPointer { buffer in | ||
| git_merge_analysis(&analysis, &preference, pointer, buffer.baseAddress, 1) | ||
| } |
There was a problem hiding this comment.
After git_annotated_commit_lookup, annotatedCommit remains optional and is used to build the annotatedCommits array passed into git_merge_analysis. Prefer using the pointer-returning git(...) helper (or guard let annotatedCommit) so the analysis call can’t be invoked with a nil annotated commit pointer.
| switch option { | ||
| case .auto: | ||
| if canFastForward { | ||
| try reset(to: remoteCommit, mode: .hard) | ||
| } else if canMerge { | ||
| try merge(branch: remoteBranch) | ||
| } else { | ||
| throw SwiftGitXError(code: .error, operation: .pull, category: .merge, message: "Cannot merge") | ||
| } | ||
|
|
||
| case .fastForwardOnly: | ||
| if canFastForward { | ||
| try reset(to: remoteCommit, mode: .hard) | ||
| } else { |
There was a problem hiding this comment.
Fast-forward cases are implemented via reset(to: ..., mode: .hard), which discards local working tree and index changes (equivalent to git reset --hard). A typical pull fast-forward should refuse/raise on conflicting local modifications (or use a safe checkout strategy), not silently drop changes. Consider implementing fast-forward by updating the branch ref and checking out with GIT_CHECKOUT_SAFE (or at least avoid .hard by default).
| while true { | ||
| var operation: UnsafeMutablePointer<git_rebase_operation>? | ||
| let nextStatus = git_rebase_next(&operation, rebase) | ||
|
|
||
| if nextStatus == GIT_ITEROVER.rawValue { | ||
| break | ||
| } | ||
|
|
||
| if nextStatus < 0 { | ||
| git_rebase_abort(rebase) | ||
| throw SwiftGitXError( | ||
| code: .conflict, operation: .pull, category: .merge, | ||
| message: "Rebase conflict detected" | ||
| ) | ||
| } | ||
|
|
||
| var commitOID = git_oid() | ||
| let commitStatus = git_rebase_commit(&commitOID, rebase, nil, signature, nil, nil) | ||
|
|
||
| if commitStatus < 0 && commitStatus != GIT_EAPPLIED.rawValue { | ||
| git_rebase_abort(rebase) | ||
| throw SwiftGitXError( | ||
| code: .error, operation: .pull, category: .merge, | ||
| message: "Failed to commit rebase operation" | ||
| ) | ||
| } |
There was a problem hiding this comment.
performRebase calls several libgit2 functions directly (git_rebase_next, git_rebase_commit, git_rebase_abort) without the git(operation:) wrapper, so libgit2 errors won’t be translated consistently into SwiftGitXError (and you lose the underlying git_error_last message). Also, any nextStatus < 0 is surfaced as a .conflict, but many negative statuses are not conflicts. Wrap these calls with git(operation: .pull) / SwiftGitXError.check(...), and map conflicts based on the actual status code (aborting via defer when needed).
| while true { | |
| var operation: UnsafeMutablePointer<git_rebase_operation>? | |
| let nextStatus = git_rebase_next(&operation, rebase) | |
| if nextStatus == GIT_ITEROVER.rawValue { | |
| break | |
| } | |
| if nextStatus < 0 { | |
| git_rebase_abort(rebase) | |
| throw SwiftGitXError( | |
| code: .conflict, operation: .pull, category: .merge, | |
| message: "Rebase conflict detected" | |
| ) | |
| } | |
| var commitOID = git_oid() | |
| let commitStatus = git_rebase_commit(&commitOID, rebase, nil, signature, nil, nil) | |
| if commitStatus < 0 && commitStatus != GIT_EAPPLIED.rawValue { | |
| git_rebase_abort(rebase) | |
| throw SwiftGitXError( | |
| code: .error, operation: .pull, category: .merge, | |
| message: "Failed to commit rebase operation" | |
| ) | |
| } | |
| func abortRebase(_ rebase: OpaquePointer?) { | |
| do { | |
| try git(operation: .pull) { | |
| git_rebase_abort(rebase) | |
| } | |
| } catch { | |
| // Ignore abort errors; original rebase error will be surfaced instead. | |
| } | |
| } | |
| while true { | |
| var operation: UnsafeMutablePointer<git_rebase_operation>? | |
| var nextStatus: Int32 = 0 | |
| try git(operation: .pull) { | |
| nextStatus = git_rebase_next(&operation, rebase) | |
| } | |
| if nextStatus == GIT_ITEROVER.rawValue { | |
| break | |
| } | |
| if nextStatus == GIT_ECONFLICT.rawValue { | |
| abortRebase(rebase) | |
| throw SwiftGitXError( | |
| code: .conflict, operation: .pull, category: .merge, | |
| message: "Rebase conflict detected" | |
| ) | |
| } | |
| if nextStatus < 0 { | |
| abortRebase(rebase) | |
| try SwiftGitXError.check( | |
| gitError: nextStatus, | |
| operation: .pull, | |
| category: .merge | |
| ) | |
| } | |
| var commitOID = git_oid() | |
| var commitStatus: Int32 = 0 | |
| try git(operation: .pull) { | |
| commitStatus = git_rebase_commit(&commitOID, rebase, nil, signature, nil, nil) | |
| } | |
| if commitStatus == GIT_EAPPLIED.rawValue { | |
| continue | |
| } | |
| if commitStatus == GIT_ECONFLICT.rawValue { | |
| abortRebase(rebase) | |
| throw SwiftGitXError( | |
| code: .conflict, operation: .pull, category: .merge, | |
| message: "Rebase conflict detected" | |
| ) | |
| } | |
| if commitStatus < 0 { | |
| abortRebase(rebase) | |
| try SwiftGitXError.check( | |
| gitError: commitStatus, | |
| operation: .pull, | |
| category: .merge | |
| ) | |
| } |
| var annotatedCommit: OpaquePointer? | ||
|
|
||
| try git(operation: .merge) { | ||
| git_annotated_commit_lookup(&annotatedCommit, pointer, &commitOID) | ||
| } | ||
| defer { git_annotated_commit_free(annotatedCommit) } | ||
|
|
||
| try performMerge(annotatedCommit: annotatedCommit!, branch: branch, commit: commit) |
There was a problem hiding this comment.
git_annotated_commit_lookup is checked only for status, but the resulting annotatedCommit pointer is still optional and then force-unwrapped. Prefer using the pointer-returning git(operation:) { (ptr,status) } helper (or guard let annotatedCommit) so a successful status with a nil pointer can’t crash here.
| var annotatedCommit: OpaquePointer? | |
| try git(operation: .merge) { | |
| git_annotated_commit_lookup(&annotatedCommit, pointer, &commitOID) | |
| } | |
| defer { git_annotated_commit_free(annotatedCommit) } | |
| try performMerge(annotatedCommit: annotatedCommit!, branch: branch, commit: commit) | |
| let annotatedCommit = try git(operation: .merge) { | |
| var annotatedCommitPointer: OpaquePointer? | |
| let status = git_annotated_commit_lookup(&annotatedCommitPointer, pointer, &commitOID) | |
| return (annotatedCommitPointer, status) | |
| } | |
| defer { git_annotated_commit_free(annotatedCommit) } | |
| try performMerge(annotatedCommit: annotatedCommit, branch: branch, commit: commit) |
| if git_index_has_conflicts(index) == 1 { | ||
| git_repository_state_cleanup(pointer) | ||
| throw SwiftGitXError( | ||
| code: .conflict, operation: .merge, category: .merge, | ||
| message: "Merge conflicts detected" | ||
| ) | ||
| } | ||
|
|
||
| try createMergeCommit(branch: branch, commit: commit) | ||
| git_repository_state_cleanup(pointer) | ||
| } |
There was a problem hiding this comment.
git_repository_state_cleanup(pointer) is only called on the happy path. If createMergeCommit(...) throws, the repository can be left in a merging state. Use a defer to always call git_repository_state_cleanup(pointer) after git_merge(...) succeeds (and avoid double-calling it in the conflict branch).
| let headCommit = try HEAD.target as! Commit | ||
|
|
||
| var signature: UnsafeMutablePointer<git_signature>? | ||
| try git(operation: .merge) { | ||
| git_signature_default(&signature, pointer) | ||
| } |
There was a problem hiding this comment.
let headCommit = try HEAD.target as! Commit can trap at runtime (e.g., detached HEAD pointing at a Tag or other non-commit object). Replace the force-cast with a safe cast and throw a SwiftGitXError when HEAD doesn’t resolve to a commit.
|
|
||
| try performMerge(annotatedCommit: annotatedCommit!, branch: branch, commit: commit) | ||
| } | ||
|
|
||
| // MARK: - Internal | ||
|
|
There was a problem hiding this comment.
The public merge(branch:) API always performs a merge commit path and doesn’t do merge analysis / fast-forward handling. This means merging a branch that is a direct descendant (or already contained) will still create an unnecessary merge commit, which differs from typical git merge semantics and from the docstring (“Merges a branch into the current branch”). Consider running git_merge_analysis first and: return when up-to-date; fast-forward when possible (or make --no-ff an explicit option on the API).
| try performMerge(annotatedCommit: annotatedCommit!, branch: branch, commit: commit) | |
| } | |
| // MARK: - Internal | |
| // Perform merge analysis to decide between up-to-date, fast-forward, or normal merge. | |
| var analysis: git_merge_analysis_t = git_merge_analysis_t(0) | |
| var preference: git_merge_preference_t = git_merge_preference_t(0) | |
| var annotatedCommitsForAnalysis: [OpaquePointer?] = [annotatedCommit] | |
| try git(operation: .merge) { | |
| annotatedCommitsForAnalysis.withUnsafeBufferPointer { buffer in | |
| git_merge_analysis(&analysis, &preference, pointer, buffer.baseAddress, buffer.count) | |
| } | |
| } | |
| if (analysis.rawValue & GIT_MERGE_ANALYSIS_UP_TO_DATE.rawValue) != 0 { | |
| // Nothing to do; already up to date. | |
| return | |
| } | |
| if (analysis.rawValue & GIT_MERGE_ANALYSIS_FASTFORWARD.rawValue) != 0 { | |
| try fastForward(to: commit, branch: branch) | |
| return | |
| } | |
| // Fall back to the existing merge-commit behavior. | |
| try performMerge(annotatedCommit: annotatedCommit!, branch: branch, commit: commit) | |
| } | |
| // MARK: - Internal | |
| /// Perform a fast-forward merge of HEAD to the given commit, if possible. | |
| /// | |
| /// - Parameters: | |
| /// - commit: The target commit to fast-forward to. | |
| /// - branch: The branch being merged (currently unused but kept for symmetry with other APIs). | |
| func fastForward(to commit: Commit, branch: Branch) throws(SwiftGitXError) { | |
| var commitOID = commit.id.raw | |
| var commitPointer: OpaquePointer? | |
| try git(operation: .merge) { | |
| git_commit_lookup(&commitPointer, pointer, &commitOID) | |
| } | |
| guard let commitPointerUnwrapped = commitPointer else { | |
| throw SwiftGitXError( | |
| code: .error, operation: .merge, category: .object, | |
| message: "Failed to look up commit for fast-forward" | |
| ) | |
| } | |
| defer { git_commit_free(commitPointerUnwrapped) } | |
| var treePointer: OpaquePointer? | |
| try git(operation: .merge) { | |
| git_commit_tree(&treePointer, commitPointerUnwrapped) | |
| } | |
| guard let treePointerUnwrapped = treePointer else { | |
| throw SwiftGitXError( | |
| code: .error, operation: .merge, category: .tree, | |
| message: "Failed to look up tree for fast-forward" | |
| ) | |
| } | |
| defer { git_tree_free(treePointerUnwrapped) } | |
| var checkoutOptions = git_checkout_options() | |
| git_checkout_options_init(&checkoutOptions, UInt32(GIT_CHECKOUT_OPTIONS_VERSION)) | |
| checkoutOptions.checkout_strategy = GIT_CHECKOUT_SAFE.rawValue | |
| try git(operation: .merge) { | |
| git_checkout_tree(pointer, treePointerUnwrapped, &checkoutOptions) | |
| } | |
| // Update HEAD (or the current branch reference) to point to the target commit. | |
| var headRef: OpaquePointer? | |
| let headResult = git_repository_head(&headRef, pointer) | |
| if headResult == 0, let headRefUnwrapped = headRef { | |
| var newRef: OpaquePointer? | |
| try git(operation: .merge) { | |
| git_reference_set_target(&newRef, headRefUnwrapped, &commitOID, nil) | |
| } | |
| if let newRefUnwrapped = newRef { | |
| git_reference_free(newRefUnwrapped) | |
| } | |
| git_reference_free(headRefUnwrapped) | |
| } else { | |
| // Detached HEAD: just move HEAD directly to the target commit. | |
| try git(operation: .merge) { | |
| git_repository_set_head_detached(pointer, &commitOID) | |
| } | |
| } | |
| } |
Added repository.pull, repository.merge for fetching and merging remote changes.