Draft: Migrate Git Implementation to SwiftGitX #1990
Conversation
| // .gitClient | ||
| // .getCommitHistory( | ||
| // maxCount: 40, | ||
| // fileLocalPath: fileURL, | ||
| // showMergeCommits: Settings.shared.preferences.sourceControl.git.showMergeCommitsPerFileLog | ||
| // ) |
There was a problem hiding this comment.
Should this be commented out or deleted?
There was a problem hiding this comment.
I didn't delete it because I didn't understand what showMergeCommits does and prefers to keep it for now to don't forget.
| @@ -188,7 +188,10 @@ extension SourceControlManager { | |||
|
|
|||
| /// Commit files selected by user | |||
| func commit(message: String, details: String? = nil) async throws { | |||
| try await gitClient.commit(message: message, details: details) | |||
| guard let repository else { return } | |||
There was a problem hiding this comment.
I find guarding on repository in these places confusing. I understand that we need to be able to unwrap this optional object. But I don't think the call sites of this function should ever be shown unless a valid repository exists.
This feels like a great use of a lock and key pattern. Here is a good conference talk that goes over it
There was a problem hiding this comment.
You are right, I just put it there for now because I am not sure the optimal solution for optional repository property.
| @@ -197,13 +200,15 @@ extension SourceControlManager { | |||
| /// Adds the given URLs to the staged changes. | |||
| /// - Parameter files: The files to stage. | |||
| func add(_ files: [URL]) async throws { | |||
| try await gitClient.add(files) | |||
| guard let repository else { return } | |||
| } | ||
|
|
||
| /// Removes the given URLs from the staged changes. | ||
| /// - Parameter files: The URLs to un-stage. | ||
| func reset(_ files: [URL]) async throws { | ||
| try await gitClient.reset(files) | ||
| guard let repository else { return } |
| gitClient = GitClient(directoryURL: workspaceURL, shellClient: currentWorld.shellClient) | ||
| repository = try? Repository.open(at: workspaceURL) |
There was a problem hiding this comment.
Should we be catching these these errors anywhere or should we always assume that if this fails it's because the repository does not exist?
There was a problem hiding this comment.
We could handle this by using a throws initializer to catch other possible errors, but doing so would introduce more code changes. For now, I’ll add a TODO to revisit this.
|
|
||
| struct CommitListItemView: View { | ||
|
|
||
| var commit: GitCommit | ||
| var commit: Commit |
There was a problem hiding this comment.
Lots of changes here which is fine because of the difference in structure between the two structs. But I'm curious if the other models are also different in SwiftGitX?
Makes me wonder if makes sense to actually just map SwiftGitX to the models that current exist?
There was a problem hiding this comment.
I will use SwiftGitX structures as much as possible since they are more structured and better suited for a maintainable implementation. I think we should eventually remove the GitClient implementation from the project.
While mapping SwiftGitX to the existing models is an option, it might introduce unnecessary complexity. SwiftGitX already provides well-defined structures, and adapting them directly could be a cleaner approach in the long run.
Description
This pull request represents an incremental migration of the current Git implementation to SwiftGitX. While the code is still a work in progress and may have some rough edges, I aim to provide a foundational understanding of how SwiftGitX can be integrated. I'll continue to refine and clean up the code in subsequent updates.
Related Issues
Checklist