From 9442ec215c2db8495702929f8f8ba0e637faee4f Mon Sep 17 00:00:00 2001 From: Thanh Tran Date: Sat, 26 Sep 2026 08:53:00 +0700 Subject: [PATCH 1/8] Add selective apply and revert actions to History --- macgit/App/CommitPatchController.swift | 51 ++ macgit/Models/CommitPatchRequest.swift | 32 ++ macgit/Models/PreparedCommitPatch.swift | 14 + macgit/Services/CommitPatchBuilder.swift | 135 ++++++ macgit/Services/GitDiffModels.swift | 2 +- .../GitStatusService+CommitPatch.swift | 180 ++++++++ macgit/Services/GitStatusService+Diff.swift | 31 +- macgit/Services/GitStatusService.swift | 2 + macgit/Services/GitUndoExecutor.swift | 2 + macgit/Services/GitUndoModels.swift | 1 + macgit/Views/Common/DiffView.swift | 61 ++- macgit/Views/History/CommitFileListView.swift | 56 ++- .../History/CommitPatchReviewSheet.swift | 64 +++ macgit/Views/History/HistoryView.swift | 76 ++- macgitTests/CommitPatchIntegrationTests.swift | 435 ++++++++++++++++++ 15 files changed, 1094 insertions(+), 48 deletions(-) create mode 100644 macgit/App/CommitPatchController.swift create mode 100644 macgit/Models/CommitPatchRequest.swift create mode 100644 macgit/Models/PreparedCommitPatch.swift create mode 100644 macgit/Services/CommitPatchBuilder.swift create mode 100644 macgit/Services/GitStatusService+CommitPatch.swift create mode 100644 macgit/Views/History/CommitPatchReviewSheet.swift create mode 100644 macgitTests/CommitPatchIntegrationTests.swift diff --git a/macgit/App/CommitPatchController.swift b/macgit/App/CommitPatchController.swift new file mode 100644 index 0000000..2c421e5 --- /dev/null +++ b/macgit/App/CommitPatchController.swift @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +import Foundation +import Observation + +@MainActor @Observable +final class CommitPatchController { + var prepared: PreparedCommitPatch? + private(set) var isBusy = false + var errorMessage = "" + var showingError = false + private(set) var reviewError: String? + + func prepare(_ request: CommitPatchRequest, in repositoryURL: URL) { + guard !isBusy, prepared == nil else { return } + reviewError = nil + isBusy = true + Task { + defer { isBusy = false } + do { + prepared = try await GitStatusService.shared.prepareCommitPatch(request, in: repositoryURL) + } catch { + errorMessage = error.localizedDescription + showingError = true + } + } + } + + func apply(undoManager: GitUndoManager?, syncState: SyncState?, run: RepositoryOperationRunner) { + guard let prepared, !isBusy else { return } + reviewError = nil + isBusy = true + run("\(prepared.request.direction.rawValue) selected changes…") { [self] in + defer { isBusy = false } + do { + try await GitStatusService.shared.applyCommitPatch(prepared) + undoManager?.register(GitUndoEntry( + repositoryURL: prepared.repositoryURL, + label: "\(prepared.request.direction.rawValue) selected changes from \(prepared.request.commit.prefix(7))", + undoOperation: .sequence([.requireHead(prepared.targetHead), .checkedWorkingTreePatch(patch: prepared.patch, reverse: true)]), + redoOperation: .sequence([.requireHead(prepared.targetHead), .checkedWorkingTreePatch(patch: prepared.patch, reverse: false)]) + )) + self.prepared = nil + await syncState?.refresh(repositoryURL: prepared.repositoryURL) + NotificationCenter.default.post(name: .repositoryDidChange, object: nil, + userInfo: ["repositoryURL": prepared.repositoryURL]) + } catch { + reviewError = error.localizedDescription + } + } + } +} diff --git a/macgit/Models/CommitPatchRequest.swift b/macgit/Models/CommitPatchRequest.swift new file mode 100644 index 0000000..cbaabc3 --- /dev/null +++ b/macgit/Models/CommitPatchRequest.swift @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +import Foundation + +nonisolated struct CommitPatchRequest: Sendable { + enum Direction: String, Sendable { + case apply = "Apply" + case revert = "Revert" + } + + // Line numbers are relative to the immutable source commit, never the working copy. + struct Line: Hashable, Sendable { + let old: Int? + let new: Int? + + init(_ line: DiffLine) { + old = line.oldLineNumber + new = line.newLineNumber + } + + init(old: Int?, new: Int?) { + self.old = old + self.new = new + } + } + + let commit: String + let files: [CommitFileChange] + let direction: Direction + // nil means complete files; a nonempty set means only these changed lines in one file. + let lines: Set? + let scope: String +} diff --git a/macgit/Models/PreparedCommitPatch.swift b/macgit/Models/PreparedCommitPatch.swift new file mode 100644 index 0000000..aaff8ea --- /dev/null +++ b/macgit/Models/PreparedCommitPatch.swift @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +import Foundation + +nonisolated struct PreparedCommitPatch: Identifiable, Sendable { + let id = UUID() + let request: CommitPatchRequest + let repositoryURL: URL + let parent: String + let targetBranch: String + let targetHead: String + let paths: [String] + let patch: String + let fingerprint: String +} diff --git a/macgit/Services/CommitPatchBuilder.swift b/macgit/Services/CommitPatchBuilder.swift new file mode 100644 index 0000000..631fec4 --- /dev/null +++ b/macgit/Services/CommitPatchBuilder.swift @@ -0,0 +1,135 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +import Foundation + +/// Builds a forward patch from an already direction-oriented Git diff. +/// The existing display parser is deliberately not used: metadata and EOF markers are significant here. +nonisolated enum CommitPatchBuilder { + static func build(raw: String, selectedLines: Set?, reversed: Bool) throws -> String { + let lines = raw.components(separatedBy: "\n") + guard lines.filter({ $0.hasPrefix("diff --git ") }).count == 1 else { + throw failure("Could not isolate the selected file's patch.") + } + guard !raw.utf8.contains(0), !lines.contains(where: { $0.hasPrefix("Binary files ") || $0 == "GIT binary patch" }) else { + throw failure("Binary changes cannot be applied selectively.") + } + let metadata = lines.prefix { !$0.hasPrefix("@@ ") } + guard !metadata.contains(where: { + ($0.hasPrefix("index ") || $0.contains("file mode ") || $0.hasPrefix("old mode ") || $0.hasPrefix("new mode ")) && + ($0.hasSuffix("160000") || $0.hasSuffix("120000")) + }) else { + throw failure("Submodule and symbolic-link changes are not supported.") + } + guard let selectedLines else { return raw } + guard !selectedLines.isEmpty else { throw failure("Select at least one changed line.") } + let wanted = Set(selectedLines.map { reversed ? CommitPatchRequest.Line(old: $0.new, new: $0.old) : $0 }) + var found: Set = [] + let firstHunk = lines.firstIndex { $0.hasPrefix("@@ ") } ?? lines.count + var headers = Array(lines[.. 0 { + guard let source = headers.first(where: { $0.hasPrefix("--- ") }) else { + throw failure("Missing source path.") + } + let path = String(source.dropFirst(4)) + let destination = path.hasPrefix("\"a/") ? "\"b/" + path.dropFirst(3) : "b/" + path.dropFirst(2) + headers = headers.filter { !$0.hasPrefix("deleted file mode ") }.map { + $0 == "+++ /dev/null" ? "+++ \(destination)" : $0 + } + } + return (headers + output).joined(separator: "\n") + "\n" + } + + private static func ranges(_ header: String) throws -> (oldStart: Int, oldCount: Int, newStart: Int, newCount: Int) { + let regex = try NSRegularExpression(pattern: #"^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@"#) + guard let match = regex.firstMatch(in: header, range: NSRange(header.startIndex..., in: header)) else { + throw failure("The selected patch has an invalid hunk header.") + } + func number(_ group: Int, default fallback: Int = 1) -> Int { + guard let range = Range(match.range(at: group), in: header) else { return fallback } + return Int(header[range]) ?? fallback + } + return (number(1), number(2), number(3), number(4)) + } + + private static func failure(_ message: String) -> GitError { .commandFailed(message) } + + private static func preservingEOF(in body: [String]) -> [String] { + var result: [String] = [] + for (index, line) in body.enumerated() { + // Retaining an old unterminated line before a selected insertion needs a newline on + // the result side. Express that as a replacement instead of an invalid context line. + if line.hasPrefix("\\ No newline"), let previous = result.last, previous.hasPrefix(" "), + body.dropFirst(index + 1).contains(where: { $0.hasPrefix("+") || $0.hasPrefix(" ") }) { + result.removeLast() + result += ["-" + previous.dropFirst(), line, "+" + previous.dropFirst()] + } else { result.append(line) } + } + return result + } +} diff --git a/macgit/Services/GitDiffModels.swift b/macgit/Services/GitDiffModels.swift index f5c8243..a104353 100644 --- a/macgit/Services/GitDiffModels.swift +++ b/macgit/Services/GitDiffModels.swift @@ -52,7 +52,7 @@ nonisolated enum DiffParser { var oldLine = 0 var newLine = 0 - let lines = raw.split(separator: "\n", omittingEmptySubsequences: false) + let lines = raw.components(separatedBy: "\n") var inHunk = false for line in lines { diff --git a/macgit/Services/GitStatusService+CommitPatch.swift b/macgit/Services/GitStatusService+CommitPatch.swift new file mode 100644 index 0000000..ecc73b7 --- /dev/null +++ b/macgit/Services/GitStatusService+CommitPatch.swift @@ -0,0 +1,180 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +import Foundation +import CryptoKit + +extension GitStatusService { + func commitPatchUnavailableReasons(commit: String, in repositoryURL: URL) async throws -> [String: String] { + let sha = try await resolveComparisonRef(commit, branchesOnly: false, in: repositoryURL) + var reasons: [String: String] = [:] + let stats = try await runGitRaw(arguments: ["diff-tree", "--root", "--no-commit-id", "--no-renames", "-r", "--numstat", "-z", sha], in: repositoryURL) + for record in stats.split(separator: 0) { + let fields = record.split(separator: 9, maxSplits: 2) + if fields.count == 3, fields[0] == Data("-".utf8) { + reasons[String(decoding: fields[2], as: UTF8.self)] = "Binary changes are not supported." + } + } + let raw = try await runGitRaw(arguments: ["diff-tree", "--root", "--no-commit-id", "--no-renames", "-r", "--raw", "-z", sha], in: repositoryURL) + let fields = raw.split(separator: 0) + var index = 0 + while index + 1 < fields.count { + let metadata = String(decoding: fields[index], as: UTF8.self).split(separator: " ") + let path = String(decoding: fields[index + 1], as: UTF8.self) + if metadata.prefix(2).contains(where: { $0.contains("160000") }) { + reasons[path] = "Submodule changes are not supported." + } else if metadata.prefix(2).contains(where: { $0.contains("120000") }) { + reasons[path] = "Symbolic-link changes are not supported." + } + index += 2 + } + return reasons + } + + func prepareCommitPatch(_ request: CommitPatchRequest, in repositoryURL: URL) async throws -> PreparedCommitPatch { + guard !request.files.isEmpty, request.lines == nil || request.files.count == 1 else { + throw GitError.commandFailed("Select files or changed lines from one file.") + } + try await validateCommitPatchState(in: repositoryURL) + let commit = try await resolveComparisonRef(request.commit, branchesOnly: false, in: repositoryURL) + let ancestry = try await runGit(arguments: ["rev-list", "--parents", "-n", "1", commit], in: repositoryURL) + .split(whereSeparator: \.isWhitespace).map(String.init) + guard ancestry.count <= 2 else { + throw GitError.commandFailed("Selected changes from merge commits are not supported. Choose a non-merge commit.") + } + let parent: String + if ancestry.count == 2 { parent = ancestry[1] } + else { + parent = try await runGit(arguments: ["hash-object", "-t", "tree", "/dev/null"], in: repositoryURL) + .trimmingCharacters(in: .whitespacesAndNewlines) + } + let allFiles = try Self.parseComparisonFiles(try await runGitRaw(arguments: [ + "diff", "--name-status", "-z", "--find-renames", parent, commit, "--" + ], in: repositoryURL)) + let files = try request.files.map { selected in + guard let file = allFiles.first(where: { $0.path == selected.path }) else { + throw GitError.commandFailed("The selected files no longer match the commit.") + } + return file + } + guard Set(files.map(\.path)).count == files.count else { + throw GitError.commandFailed("A file was selected more than once.") + } + let paths = Set(files.flatMap { [$0.oldPath, $0.path].compactMap { $0 } }).sorted() + let fingerprint = try await commitPatchFingerprint(paths: paths, in: repositoryURL) + let targetHead = try await runGit(arguments: ["rev-parse", "HEAD"], in: repositoryURL) + .trimmingCharacters(in: .whitespacesAndNewlines) + var patch = "" + for file in files { + let refs = request.direction == .apply ? [parent, commit] : [commit, parent] + let data = try await runGitRaw(arguments: [ + "--literal-pathspecs", "-c", "core.quotePath=true", "diff", "--no-color", "--no-ext-diff", "--no-textconv", + "--no-relative", "--src-prefix=a/", "--dst-prefix=b/", "--find-renames", "--full-index", "--diff-algorithm=myers", "-U3" + ] + refs + ["--"] + [file.oldPath, file.path].compactMap { $0 }, in: repositoryURL) + guard let raw = String(data: data, encoding: .utf8), data.count <= 5_000_000 else { + throw GitError.commandFailed("This patch is too large or is not UTF-8 text.") + } + patch += try CommitPatchBuilder.build(raw: raw, selectedLines: request.lines, reversed: request.direction == .revert) + } + guard !patch.isEmpty else { throw GitError.commandFailed("There are no selected changes to apply.") } + try await runCommitPatch(patch, checkOnly: true, reverse: false, in: repositoryURL) + guard try await commitPatchFingerprint(paths: paths, in: repositoryURL) == fingerprint else { + throw GitError.commandFailed("The working copy changed while preparing the patch. Try again.") + } + let branch = (try? await runGit(arguments: ["symbolic-ref", "--quiet", "--short", "HEAD"], in: repositoryURL))? + .trimmingCharacters(in: .whitespacesAndNewlines) ?? "Detached HEAD" + let resolved = CommitPatchRequest(commit: commit, files: files, direction: request.direction, lines: request.lines, scope: request.scope) + return PreparedCommitPatch(request: resolved, repositoryURL: repositoryURL, parent: parent, + targetBranch: branch, targetHead: targetHead, paths: paths, patch: patch, fingerprint: fingerprint) + } + + func applyCommitPatch(_ prepared: PreparedCommitPatch) async throws { + try await executeCommitPatch(prepared.patch, reverse: false, in: prepared.repositoryURL, expected: prepared) + } + + /// Also used for undo/redo. A rejected patch never creates .rej files or an in-progress merge. + func applyCheckedWorkingTreePatch(_ patch: String, reverse: Bool, in repositoryURL: URL) async throws { + try await executeCommitPatch(patch, reverse: reverse, in: repositoryURL, expected: nil) + } + + private func executeCommitPatch(_ patch: String, reverse: Bool, in repositoryURL: URL, expected: PreparedCommitPatch?) async throws { + let key = repositoryURL.resolvingSymlinksInPath().standardizedFileURL.path + guard activeCommitPatchRepositories.insert(key).inserted else { + throw GitError.commandFailed("Another selected-patch operation is already running in this working copy.") + } + defer { activeCommitPatchRepositories.remove(key) } + try await validateCommitPatchState(in: repositoryURL) + try await runCommitPatch(patch, checkOnly: true, reverse: reverse, in: repositoryURL) + if let expected, + try await commitPatchFingerprint(paths: expected.paths, in: repositoryURL) != expected.fingerprint { + throw GitError.commandFailed("The branch, index, or selected files changed after review. Close this sheet and review the changes again.") + } + try Task.checkCancellation() + // Cancellation is allowed during preflight, but must not terminate Git halfway through writing files. + try await Task { try await self.runCommitPatch(patch, checkOnly: false, reverse: reverse, in: repositoryURL) }.value + } + + private func runCommitPatch(_ patch: String, checkOnly: Bool, reverse: Bool, in repositoryURL: URL) async throws { + let directory = FileManager.default.temporaryDirectory.appendingPathComponent("commit-patch-\(UUID().uuidString)") + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: false, attributes: [.posixPermissions: 0o700]) + defer { try? FileManager.default.removeItem(at: directory) } + let url = directory.appendingPathComponent("changes.patch") + try Data(patch.utf8).write(to: url) + var arguments = ["apply", "--whitespace=nowarn"] + if checkOnly { arguments.append("--check") } + if reverse { arguments.append("--reverse") } + arguments += ["--", url.path] + do { _ = try await runGit(arguments: arguments, in: repositoryURL) } + catch { + let message = checkOnly + ? "The selected changes could not be applied cleanly. Your working copy has been preserved." + : "Git could not finish applying the selected changes. Review the working copy before trying again." + throw GitError.commandFailed("\(message)\n\n\(error.localizedDescription)") + } + } + + private func validateCommitPatchState(in repositoryURL: URL) async throws { + for name in ["MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "rebase-merge", "rebase-apply", "sequencer", "index.lock"] { + let path = try await runGit(arguments: ["rev-parse", "--path-format=absolute", "--git-path", name], in: repositoryURL) + .trimmingCharacters(in: .whitespacesAndNewlines) + if FileManager.default.fileExists(atPath: path) { + throw GitError.commandFailed("Finish the current Git operation before applying selected changes.") + } + } + let unmerged = try await runGitRaw(arguments: ["ls-files", "--unmerged", "-z"], in: repositoryURL) + guard unmerged.isEmpty else { throw GitError.commandFailed("Resolve existing conflicts before applying selected changes.") } + } + + private func commitPatchFingerprint(paths: [String], in repositoryURL: URL) async throws -> String { + var hash = SHA256() + func append(_ data: Data) { + hash.update(data: Data("\(data.count):".utf8)) + hash.update(data: data) + } + append(try await runGitRaw(arguments: ["rev-parse", "HEAD"], in: repositoryURL)) + append(Data(((try? await runGit(arguments: ["symbolic-ref", "--quiet", "HEAD"], in: repositoryURL)) ?? "detached").utf8)) + append(try await runGitRaw(arguments: ["ls-files", "--stage", "-z"], in: repositoryURL)) + for path in paths { + guard !path.hasPrefix("/"), !path.split(separator: "/").contains(".."), !path.split(separator: "/").contains(".git") else { + throw GitError.commandFailed("The patch contains an unsafe path.") + } + let url = repositoryURL.appendingPathComponent(path) + var componentURL = repositoryURL + for component in path.split(separator: "/") { + componentURL.appendPathComponent(String(component)) + if let attributes = try? FileManager.default.attributesOfItem(atPath: componentURL.path), + attributes[.type] as? FileAttributeType == .typeSymbolicLink { + throw GitError.commandFailed("The selected path contains a symbolic link: \(path)") + } + } + append(Data(path.utf8)) + if FileManager.default.fileExists(atPath: url.path) { + let attributes = try FileManager.default.attributesOfItem(atPath: url.path) + guard attributes[.type] as? FileAttributeType == .typeRegular else { + throw GitError.commandFailed("The selected path is not a regular file: \(path)") + } + append(Data("file:\(attributes[.posixPermissions] ?? 0)".utf8)) + append(try Data(contentsOf: url)) + } else { append(Data("missing".utf8)) } + } + return hash.finalize().map { String(format: "%02x", $0) }.joined() + } +} diff --git a/macgit/Services/GitStatusService+Diff.swift b/macgit/Services/GitStatusService+Diff.swift index c484b43..19576a7 100644 --- a/macgit/Services/GitStatusService+Diff.swift +++ b/macgit/Services/GitStatusService+Diff.swift @@ -111,33 +111,10 @@ extension GitStatusService { } func changedFiles(in commit: String, in repositoryURL: URL) async -> [CommitFileChange] { - let output = (try? await runGit(arguments: ["show", "--name-status", "--first-parent", "--format=", commit], in: repositoryURL)) ?? "" - var changes: [CommitFileChange] = [] - for line in output.split(separator: "\n") { - let trimmed = line.trimmingCharacters(in: .whitespaces) - guard !trimmed.isEmpty else { continue } - let parts = trimmed.split(separator: "\t") - guard parts.count >= 2 else { continue } - let statusCode = String(parts[0]).trimmingCharacters(in: .whitespaces) - let path: String - if (statusCode.hasPrefix("R") || statusCode.hasPrefix("C")) && parts.count >= 3 { - path = String(parts[2]) - } else { - path = String(parts[1]) - } - - let status: CommitFileStatus - switch statusCode.prefix(1) { - case "A": status = .added - case "M": status = .modified - case "D": status = .deleted - case "R": status = .renamed - case "C": status = .copied - default: status = .modified - } - changes.append(CommitFileChange(path: path, status: status)) - } - return changes + do { + let output = try await runGitRaw(arguments: ["show", "--name-status", "-z", "--find-renames", "--first-parent", "--format=", commit], in: repositoryURL) + return try Self.parseComparisonFiles(output) + } catch { return [] } } func changedFiles(from baseRef: String, to targetRef: String, in repositoryURL: URL) async -> [CommitFileChange] { diff --git a/macgit/Services/GitStatusService.swift b/macgit/Services/GitStatusService.swift index 7980f7f..523a48d 100644 --- a/macgit/Services/GitStatusService.swift +++ b/macgit/Services/GitStatusService.swift @@ -222,6 +222,8 @@ actor GitStatusService { private let runner: (any GitCommandRunning)? let runtimeManager: GitRuntimeManager let branchListCache = BranchListCache() + // Prevent two selected-patch operations from interleaving across windows for the same checkout. + var activeCommitPatchRepositories: Set = [] init( runner: (any GitCommandRunning)? = nil, diff --git a/macgit/Services/GitUndoExecutor.swift b/macgit/Services/GitUndoExecutor.swift index 9cfa16a..89fccfb 100644 --- a/macgit/Services/GitUndoExecutor.swift +++ b/macgit/Services/GitUndoExecutor.swift @@ -91,6 +91,8 @@ struct GitUndoExecutor { try await runFileCommand(["reset", "HEAD", "--"], paths: paths, in: repositoryURL) case .applyPatch(let patch, let cached, let reverse): try await patchRunner.applyPatch(patch, in: repositoryURL, cached: cached, reverse: reverse) + case .checkedWorkingTreePatch(let patch, let reverse): + try await GitStatusService.shared.applyCheckedWorkingTreePatch(patch, reverse: reverse, in: repositoryURL) case .resetHead(let target, let mode, let expectedHead): if let expectedHead { let actual = try await runner.runGit(arguments: ["rev-parse", "HEAD"], in: repositoryURL) diff --git a/macgit/Services/GitUndoModels.swift b/macgit/Services/GitUndoModels.swift index 480a010..cc11d5a 100644 --- a/macgit/Services/GitUndoModels.swift +++ b/macgit/Services/GitUndoModels.swift @@ -46,6 +46,7 @@ indirect enum GitUndoOperation: Equatable { case stageFiles(paths: [String]) case unstageFiles(paths: [String]) case applyPatch(patch: String, cached: Bool, reverse: Bool) + case checkedWorkingTreePatch(patch: String, reverse: Bool) case resetHead(target: String, mode: GitUndoResetMode, expectedHead: String?) case commit(message: String, noVerify: Bool, signOff: Bool) case cherryPick(commit: String) diff --git a/macgit/Views/Common/DiffView.swift b/macgit/Views/Common/DiffView.swift index c512e11..68be7ad 100644 --- a/macgit/Views/Common/DiffView.swift +++ b/macgit/Views/Common/DiffView.swift @@ -31,6 +31,8 @@ struct DiffView: View { let onError: (String) -> Void let filePath: String? let gitRef: String? + let onCommitPatch: (([DiffLine], CommitPatchRequest.Direction, String) -> Void)? + let commitPatchDisabledReason: String? init( hunks: [DiffHunk], @@ -40,7 +42,9 @@ struct DiffView: View { onRefresh: @escaping () -> Void = {}, onError: @escaping (String) -> Void = { _ in }, filePath: String? = nil, - gitRef: String? = nil + gitRef: String? = nil, + onCommitPatch: (([DiffLine], CommitPatchRequest.Direction, String) -> Void)? = nil, + commitPatchDisabledReason: String? = nil ) { self.hunks = hunks self.file = file @@ -50,6 +54,8 @@ struct DiffView: View { self.onError = onError self.filePath = filePath self.gitRef = gitRef + self.onCommitPatch = onCommitPatch + self.commitPatchDisabledReason = commitPatchDisabledReason } @State private var selectedLineIDs: Set = [] @@ -94,7 +100,20 @@ struct DiffView: View { selectedLineIDs: $selectedLineIDs, lastSelectedLineID: $lastSelectedLineID, onRefresh: onRefresh, - onError: onError + onError: onError, + onCommitHunk: onCommitPatch.map { action in + { hunk, direction in + action(hunk.lines.filter { $0.type == .added || $0.type == .removed }, direction, "Selected hunk") + } + }, + onCommitLines: onCommitPatch.map { action in + { ids, direction in + action(hunks.flatMap(\.lines).filter { + ids.contains($0.id) && ($0.type == .added || $0.type == .removed) + }, direction, "Selected lines") + } + }, + commitPatchDisabledReason: commitPatchDisabledReason ) .frame(height: block.height) .padding(.bottom, DiffRenderBlock.spacing) @@ -115,6 +134,8 @@ struct DiffView: View { renderedBlockRange = range } .onChange(of: hunks.first?.id) { + selectedLineIDs.removeAll() + lastSelectedLineID = nil renderedBlockRange = 0.. Void let onError: (String) -> Void + var onCommitHunk: ((DiffHunk, CommitPatchRequest.Direction) -> Void)? = nil + var onCommitLines: ((Set, CommitPatchRequest.Direction) -> Void)? = nil + var commitPatchDisabledReason: String? = nil @State private var availableWidth: CGFloat = 0 @State private var horizontalViewport = CGRect(x: 0, y: 0, width: 1_024, height: 0) @@ -229,6 +253,12 @@ struct HunkView: View { Spacer() + if onCommitHunk != nil { + Menu("Changes") { commitPatchMenu(line: nil) } + .menuStyle(.borderlessButton) + .fixedSize() + .help(commitPatchDisabledReason ?? "Apply or revert this hunk") + } if canInteract { if isStaged { Button("Unstage") { @@ -322,6 +352,7 @@ struct HunkView: View { private var hunkContextMenu: some View { Group { + commitPatchMenu(line: nil) if canInteract { if isStaged { Button("Unstage Hunk") { @@ -360,6 +391,7 @@ struct HunkView: View { private func lineContextMenu(for line: DiffLine) -> some View { Group { + commitPatchMenu(line: line) if canInteract { if isStaged { Button("Unstage Hunk") { @@ -401,6 +433,31 @@ struct HunkView: View { } } + @ViewBuilder + private func commitPatchMenu(line: DiffLine?) -> some View { + if let onCommitHunk { + Button("Apply Hunk") { onCommitHunk(hunk, .apply) } + .disabled(commitPatchDisabledReason != nil) + Button("Revert Hunk") { onCommitHunk(hunk, .revert) } + .disabled(commitPatchDisabledReason != nil) + if let onCommitLines, hasSelectedLines || line?.type == .added || line?.type == .removed { + Divider() + Button("Apply Selected Changes") { onCommitLines(commitLineIDs(line), .apply) } + .disabled(commitPatchDisabledReason != nil) + Button("Revert Selected Changes") { onCommitLines(commitLineIDs(line), .revert) } + .disabled(commitPatchDisabledReason != nil) + } + if let commitPatchDisabledReason { Text(commitPatchDisabledReason) } + } + } + + private func commitLineIDs(_ line: DiffLine?) -> Set { + if let line, !selectedLineIDs.contains(line.id), line.type == .added || line.type == .removed { + return [line.id] + } + return selectedLineIDs + } + private func handleLineTap(at index: Int) { let line = hunk.lines[index] guard line.type == .added || line.type == .removed else { return } diff --git a/macgit/Views/History/CommitFileListView.swift b/macgit/Views/History/CommitFileListView.swift index 0b4b168..a262495 100644 --- a/macgit/Views/History/CommitFileListView.swift +++ b/macgit/Views/History/CommitFileListView.swift @@ -26,11 +26,48 @@ struct CommitFileListView: View { let changes: [CommitFileChange] @Binding var selectedFile: CommitFileChange? var onPreview: ((CommitFileChange) -> Void)? = nil + var onPatch: (([CommitFileChange], CommitPatchRequest.Direction) -> Void)? = nil + var patchDisabledReason: (([CommitFileChange]) -> String?)? = nil + @State private var selectedFiles: Set = [] @State private var visibleFileCount = 200 private let pageSize = 200 var body: some View { - List(selection: $selectedFile) { + Group { + if onPatch != nil { + List(selection: $selectedFiles) { fileRows } + .onChange(of: selectedFiles) { old, new in + if let added = changes.first(where: { new.subtracting(old).contains($0) }) { + selectedFile = added + } else if let selectedFile, !new.contains(selectedFile) { + self.selectedFile = changes.first(where: { new.contains($0) }) + } + } + } else { + List(selection: $selectedFile) { fileRows } + } + } + .listStyle(.inset) + .onAppear { synchronizeSelection() } + .onChange(of: changes) { + visibleFileCount = pageSize + selectedFiles = selectedFiles.intersection(Set(changes)) + synchronizeSelection() + revealSelectedFile() + } + .onChange(of: selectedFile) { + synchronizeSelection() + revealSelectedFile() + } + } + + private func synchronizeSelection() { + if let selectedFile, !selectedFiles.contains(selectedFile) { selectedFiles = [selectedFile] } + if selectedFile == nil { selectedFiles = [] } + } + + private var fileRows: some View { + Group { ForEach(changes.prefix(visibleFileCount)) { change in HStack(spacing: 8) { Image(systemName: statusSymbol(for: change.status)) @@ -85,6 +122,15 @@ struct CommitFileListView: View { } .padding(.vertical, 2) .tag(change) + .contextMenu { + if let onPatch { + let files = selectedFiles.contains(change) ? changes.filter { selectedFiles.contains($0) } : [change] + let reason = patchDisabledReason?(files) + Button("Apply Selected Changes") { onPatch(files, .apply) }.disabled(reason != nil) + Button("Revert Selected Changes") { onPatch(files, .revert) }.disabled(reason != nil) + if let reason { Text(reason) } + } + } } if visibleFileCount < changes.count { Text("Loading more files… (\(visibleFileCount) of \(changes.count))") @@ -97,14 +143,6 @@ struct CommitFileListView: View { } } } - .listStyle(.inset) - .onChange(of: changes.first?.id) { - visibleFileCount = pageSize - revealSelectedFile() - } - .onChange(of: selectedFile) { - revealSelectedFile() - } } private func revealSelectedFile() { diff --git a/macgit/Views/History/CommitPatchReviewSheet.swift b/macgit/Views/History/CommitPatchReviewSheet.swift new file mode 100644 index 0000000..7ac2b68 --- /dev/null +++ b/macgit/Views/History/CommitPatchReviewSheet.swift @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +import SwiftUI + +struct CommitPatchReviewSheet: View { + let prepared: PreparedCommitPatch + let isBusy: Bool + let errorMessage: String? + let onCancel: () -> Void + let onApply: () -> Void + + var body: some View { + VStack(alignment: .leading, spacing: 16) { + Text("\(prepared.request.direction.rawValue) Selected Changes") + .font(.title2.bold()) + Text(prepared.repositoryURL.path) + .font(.caption) + .foregroundStyle(.secondary) + .textSelection(.enabled) + Text("Commit \(prepared.request.commit.prefix(12)) → \(prepared.targetBranch)") + .font(.callout.monospaced()) + Text("\(prepared.request.scope) · Changes will remain unstaged. Nothing will be committed.") + .foregroundStyle(.secondary) + if let errorMessage { + Text(errorMessage) + .font(.callout) + .foregroundStyle(.red) + .textSelection(.enabled) + } + ScrollView { + VStack(alignment: .leading, spacing: 6) { + ForEach(prepared.request.files) { file in + if let oldPath = file.oldPath { + Text(prepared.request.direction == .apply ? "\(oldPath) → \(file.path)" : "\(file.path) → \(oldPath)") + Text("The rename is included with the selected content changes.") + .font(.caption).foregroundStyle(.secondary) + } else { Text(file.path) } + } + } + .frame(maxWidth: .infinity, alignment: .leading) + } + .frame(maxHeight: 100) + ScrollView([.horizontal, .vertical]) { + Text(prepared.patch) + .font(.system(.caption, design: .monospaced)) + .textSelection(.enabled) + .frame(maxWidth: .infinity, alignment: .leading) + .padding(10) + } + .background(.quaternary.opacity(0.4), in: RoundedRectangle(cornerRadius: 8)) + HStack { + if isBusy { ProgressView().controlSize(.small) } + Spacer() + Button("Cancel", action: onCancel) + .keyboardShortcut(.cancelAction) + Button(prepared.request.direction.rawValue, action: onApply) + .keyboardShortcut(.defaultAction) + } + .disabled(isBusy) + } + .padding(24) + .frame(width: 720, height: 560) + .interactiveDismissDisabled(isBusy) + } +} diff --git a/macgit/Views/History/HistoryView.swift b/macgit/Views/History/HistoryView.swift index ad5babe..60037ac 100644 --- a/macgit/Views/History/HistoryView.swift +++ b/macgit/Views/History/HistoryView.swift @@ -52,6 +52,10 @@ struct HistoryView: View { @State private var dragCompletionMonitorTask: Task? @State private var selectedCommit: Commit? = nil @State private var showingCommitInfo = false + @State private var commitPatchController = CommitPatchController() + @State private var commitPatchReasons: [String: String] = [:] + @State private var commitPatchEligibilityLoaded = false + @State private var commitPatchEligibilityError: String? @State private var fullFilePreview: CommitFilePreviewRequest? @State private var previewAvailableSize = CGSize(width: 1000, height: 700) @State private var fullCommitMessage: String? @@ -60,6 +64,8 @@ struct HistoryView: View { @State private var fileChanges: [CommitFileChange] = [] @State private var selectedFile: CommitFileChange? = nil @State private var diffHunks: [DiffHunk] = [] + @State private var diffCommitHash: String? + @State private var diffFilePath: String? @State private var commitFilesLoadID = UUID() @State private var diffLoadID = UUID() @AppStorage("history.tableColumns") private var tableColumnCustomization = TableColumnCustomization() @@ -284,6 +290,17 @@ struct HistoryView: View { .replacingSheet(isPresented: $showingRebaseConfirmation) { rebaseConfirmationSheet } + .replacingSheet(item: $commitPatchController.prepared) { prepared in + CommitPatchReviewSheet(prepared: prepared, isBusy: commitPatchController.isBusy, + errorMessage: commitPatchController.reviewError, + onCancel: { commitPatchController.prepared = nil }, + onApply: { + commitPatchController.apply(undoManager: undoManager, syncState: syncState, run: onRunRepositoryOperation) + }) + } + .alert("Selected changes", isPresented: $commitPatchController.showingError) { + Button("OK", role: .cancel) {} + } message: { Text(commitPatchController.errorMessage) } .replacingSheet(item: $squashSheetPresentation) { presentation in SquashCommitsSheet( commits: presentation.commits, @@ -712,13 +729,17 @@ struct HistoryView: View { PersistentHSplit( autosaveName: "HistoryDetailSplit", left: { - CommitFileListView(changes: fileChanges, selectedFile: $selectedFile) { file in - fullFilePreview = CommitFilePreviewRequest( - repositoryURL: repositoryURL, - commitHash: commit.hash, - file: file - ) - } + CommitFileListView(changes: fileChanges, selectedFile: $selectedFile, + onPreview: { file in + fullFilePreview = CommitFilePreviewRequest( + repositoryURL: repositoryURL, commitHash: commit.hash, file: file) + }, + onPatch: { files, direction in + commitPatchController.prepare(CommitPatchRequest(commit: commit.hash, + files: files, direction: direction, lines: nil, + scope: "\(files.count) selected file(s)"), in: repositoryURL) + }, + patchDisabledReason: { files in commitPatchDisabledReason(for: files) }) .frame(minWidth: 220) }, right: { @@ -866,7 +887,16 @@ struct HistoryView: View { onRefresh: {}, onError: { _ in }, filePath: file.path, - gitRef: selectedCommit.map(\.hash) + gitRef: selectedCommit.map(\.hash), + onCommitPatch: { lines, direction, scope in + guard let commit = selectedCommit, + diffCommitHash == commit.hash, diffFilePath == file.path else { return } + commitPatchController.prepare(CommitPatchRequest(commit: commit.hash, + files: [file], direction: direction, + lines: Set(lines.map(CommitPatchRequest.Line.init)), scope: scope), in: repositoryURL) + }, + commitPatchDisabledReason: commitPatchDisabledReason(for: [file]) + ?? (diffCommitHash == selectedCommit?.hash && diffFilePath == file.path ? nil : "Loading commit diff…") ) } } else { @@ -1368,6 +1398,9 @@ struct HistoryView: View { let loadID = UUID() await MainActor.run { commitFilesLoadID = loadID + commitPatchEligibilityLoaded = false + commitPatchEligibilityError = nil + commitPatchReasons = [:] fileChanges = [] selectedFile = nil diffHunks = [] @@ -1390,13 +1423,36 @@ struct HistoryView: View { fileChanges = changes selectedFile = changes.first } + do { + let reasons = try await GitStatusService.shared.commitPatchUnavailableReasons(commit: commit.hash, in: repositoryURL) + guard commitFilesLoadID == loadID, selectedCommit?.hash == commit.hash else { return } + commitPatchReasons = reasons + commitPatchEligibilityLoaded = true + } catch { + guard commitFilesLoadID == loadID else { return } + commitPatchEligibilityError = error.localizedDescription + commitPatchEligibilityLoaded = true + } } - + + private func commitPatchDisabledReason(for files: [CommitFileChange]) -> String? { + if selectedCommit?.isMerge == true { return "Selected changes from merge commits are not supported." } + if commitPatchController.isBusy { return "Preparing or applying selected changes…" } + if !commitPatchEligibilityLoaded { return "Checking selected changes…" } + if let commitPatchEligibilityError { return commitPatchEligibilityError } + for file in files { + if let reason = commitPatchReasons[file.path] ?? file.oldPath.flatMap({ commitPatchReasons[$0] }) { return reason } + } + return nil + } + private func loadDiff(for file: CommitFileChange?, in commit: Commit?) async { let loadID = UUID() await MainActor.run { diffLoadID = loadID diffHunks = [] + diffCommitHash = nil + diffFilePath = nil } guard let file = file, let commit = commit else { @@ -1415,6 +1471,8 @@ struct HistoryView: View { return } diffHunks = hunks + diffCommitHash = commit.hash + diffFilePath = file.path } } diff --git a/macgitTests/CommitPatchIntegrationTests.swift b/macgitTests/CommitPatchIntegrationTests.swift new file mode 100644 index 0000000..25bd156 --- /dev/null +++ b/macgitTests/CommitPatchIntegrationTests.swift @@ -0,0 +1,435 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +import XCTest +@testable import macgit + +@MainActor +final class CommitPatchIntegrationTests: XCTestCase { + private var repositories: [URL] = [] + private let service = GitStatusService.shared + + override func tearDown() { + for url in repositories { try? FileManager.default.removeItem(at: url) } + repositories = [] + super.tearDown() + } + + func testSelectedFileLeavesOtherFilesAndIndexUntouchedAndSupportsUndoRedo() async throws { + let repo = try repository() + try write("one\n", "a.txt", repo); try write("two\n", "b.txt", repo) + let base = try commit(repo) + try write("ONE\n", "a.txt", repo); try write("TWO\n", "b.txt", repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + let indexBefore = try git(["ls-files", "--stage"], repo) + let prepared = try await prepare(source, "a.txt", repo) + try await service.applyCommitPatch(prepared) + XCTAssertEqual(try read("a.txt", repo), "ONE\n") + XCTAssertEqual(try read("b.txt", repo), "two\n") + XCTAssertEqual(try git(["ls-files", "--stage"], repo), indexBefore) + XCTAssertEqual(try git(["rev-parse", "HEAD"], repo), base) + let executor = GitUndoExecutor() + try await executor.execute(.checkedWorkingTreePatch(patch: prepared.patch, reverse: true), in: repo) + XCTAssertEqual(try read("a.txt", repo), "one\n") + try await executor.execute(.checkedWorkingTreePatch(patch: prepared.patch, reverse: false), in: repo) + XCTAssertEqual(try read("a.txt", repo), "ONE\n") + } + + func testSelectedAdditionPreservesUnselectedRemovalInBothDirections() async throws { + let repo = try repository() + try write("before\nold\nafter\n", "a.txt", repo) + let base = try commit(repo) + try write("before\nnew\nafter\n", "a.txt", repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + let selection: Set = [.init(old: nil, new: 2)] + let forward = try await prepare(source, "a.txt", repo, lines: selection) + try await service.applyCommitPatch(forward) + XCTAssertEqual(try read("a.txt", repo), "before\nold\nnew\nafter\n") + try git(["reset", "--hard", source], repo) + let reverse = try await prepare(source, "a.txt", repo, direction: .revert, lines: selection) + try await service.applyCommitPatch(reverse) + XCTAssertEqual(try read("a.txt", repo), "before\nafter\n") + } + + func testSelectedRemovalInBothDirections() async throws { + let repo = try repository() + try write("before\nold\nafter\n", "a.txt", repo) + let base = try commit(repo) + try write("before\nnew\nafter\n", "a.txt", repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + let selection: Set = [.init(old: 2, new: nil)] + let forward = try await prepare(source, "a.txt", repo, lines: selection) + try await service.applyCommitPatch(forward) + XCTAssertEqual(try read("a.txt", repo), "before\nafter\n") + try git(["reset", "--hard", source], repo) + let reverse = try await prepare(source, "a.txt", repo, direction: .revert, lines: selection) + try await service.applyCommitPatch(reverse) + XCTAssertEqual(try read("a.txt", repo), "before\nnew\nold\nafter\n") + } + + func testOneHunkPreservesDirtyChangesElsewhereInSameFileAndStagedFile() async throws { + let repo = try repository() + let original = (1...40).map { "line\($0)" }.joined(separator: "\n") + "\n" + try write(original, "a.txt", repo); try write("base\n", "other.txt", repo) + let base = try commit(repo) + try write(original.replacingOccurrences(of: "line2\n", with: "changed2\n") + .replacingOccurrences(of: "line25\n", with: "changed25\n"), "a.txt", repo) + let source = try commit(repo) + let hunks = await service.diff(for: "a.txt", in: source, in: repo) + XCTAssertEqual(hunks.count, 2) + let lines = Set(try XCTUnwrap(hunks.first).lines.filter { $0.type == .added || $0.type == .removed }.map(CommitPatchRequest.Line.init)) + try git(["checkout", "--detach", base], repo) + try write(original.replacingOccurrences(of: "line38\n", with: "local38\n"), "a.txt", repo) + try git(["add", "a.txt"], repo) + try write(original.replacingOccurrences(of: "line38\n", with: "local38\n") + .replacingOccurrences(of: "line39\n", with: "unstaged39\n"), "a.txt", repo) + try write("staged\n", "other.txt", repo); try git(["add", "other.txt"], repo) + try write("unstaged too\n", "other.txt", repo) + let index = try git(["ls-files", "--stage"], repo) + let prepared = try await prepare(source, "a.txt", repo, lines: lines) + try await service.applyCommitPatch(prepared) + let content = try read("a.txt", repo) + XCTAssertTrue(content.contains("changed2\n")); XCTAssertTrue(content.contains("line25\n")) + XCTAssertTrue(content.contains("local38\n")) + XCTAssertTrue(content.contains("unstaged39\n")) + XCTAssertEqual(try read("other.txt", repo), "unstaged too\n") + XCTAssertEqual(try git(["ls-files", "--stage"], repo), index) + } + + func testPartialAddAndReverseAddPreserveFileWhenLinesRemain() async throws { + let repo = try repository() + try write("base\n", "base.txt", repo) + let base = try commit(repo) + try write("first\nsecond\nthird\n", "new.txt", repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + let lines: Set = [.init(old: nil, new: 2)] + let forward = try await prepare(source, "new.txt", repo, lines: lines) + try await service.applyCommitPatch(forward) + XCTAssertEqual(try read("new.txt", repo), "second\n") + try FileManager.default.removeItem(at: repo.appendingPathComponent("new.txt")) + try git(["reset", "--hard", source], repo) + let reverse = try await prepare(source, "new.txt", repo, direction: .revert, lines: lines) + try await service.applyCommitPatch(reverse) + XCTAssertEqual(try read("new.txt", repo), "first\nthird\n") + } + + func testPartialDeleteAndReverseDelete() async throws { + let repo = try repository() + try write("first\nsecond\nthird\n", "a.txt", repo) + let base = try commit(repo) + try git(["rm", "a.txt"], repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + let lines: Set = [.init(old: 2, new: nil)] + let forward = try await prepare(source, "a.txt", repo, lines: lines) + try await service.applyCommitPatch(forward) + XCTAssertEqual(try read("a.txt", repo), "first\nthird\n") + try git(["reset", "--hard", source], repo) + let reverse = try await prepare(source, "a.txt", repo, direction: .revert, lines: lines) + try await service.applyCommitPatch(reverse) + XCTAssertEqual(try read("a.txt", repo), "second\n") + } + + func testWholeRenameWithEditsForwardAndReverse() async throws { + let repo = try repository() + try write("a\nb\nc\nd\ne\nf\ng\nh\n", "old name.txt", repo) + let base = try commit(repo) + try git(["mv", "old name.txt", "new name.txt"], repo) + try write("a\nB\nc\nd\ne\nf\ng\nh\n", "new name.txt", repo) + let source = try commit(repo) + let files = await service.changedFiles(in: source, in: repo) + XCTAssertEqual(files.first?.oldPath, "old name.txt") + try git(["checkout", "--detach", base], repo) + let forward = try await prepare(source, "new name.txt", repo) + try await service.applyCommitPatch(forward) + XCTAssertFalse(FileManager.default.fileExists(atPath: repo.appendingPathComponent("old name.txt").path)) + XCTAssertTrue(try read("new name.txt", repo).contains("B\n")) + try await service.applyCheckedWorkingTreePatch(forward.patch, reverse: true, in: repo) + XCTAssertTrue(try read("old name.txt", repo).contains("b\n")) + try git(["reset", "--hard", source], repo) + let reverse = try await prepare(source, "new name.txt", repo, direction: .revert) + try await service.applyCommitPatch(reverse) + XCTAssertTrue(try read("old name.txt", repo).contains("b\n")) + } + + func testConflictRejectsEntireMultiFilePatchWithoutMutation() async throws { + let repo = try repository() + try write("old\n", "a.txt", repo); try write("old\n", "b.txt", repo) + let base = try commit(repo) + try write("new\n", "a.txt", repo); try write("new\n", "b.txt", repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + try write("local\n", "b.txt", repo) + let files = await service.changedFiles(in: source, in: repo) + await reject { + _ = try await self.service.prepareCommitPatch(.init(commit: source, files: files, direction: .apply, lines: nil, scope: "Files"), in: repo) + } + XCTAssertEqual(try read("a.txt", repo), "old\n") + XCTAssertEqual(try read("b.txt", repo), "local\n") + XCTAssertFalse(FileManager.default.fileExists(atPath: repo.appendingPathComponent("a.txt.rej").path)) + } + + func testRevalidationRejectsEditsAndBranchMovementAfterReview() async throws { + let repo = try repository() + try write("old\n", "a.txt", repo) + let base = try commit(repo) + try write("new\n", "a.txt", repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + let prepared = try await prepare(source, "a.txt", repo) + try write("local\n", "a.txt", repo) + await reject { try await self.service.applyCommitPatch(prepared) } + XCTAssertEqual(try read("a.txt", repo), "local\n") + try git(["reset", "--hard", base], repo) + try git(["checkout", "-b", "another"], repo) + await reject { try await self.service.applyCommitPatch(prepared) } + XCTAssertEqual(try read("a.txt", repo), "old\n") + } + + func testRootCommitAndQuotedPathWithNoFinalNewline() async throws { + let repo = try repository() + let path = "odd\t\"name.txt" + try write("hello", path, repo) + let source = try commit(repo) + let reverse = try await prepare(source, path, repo, direction: .revert) + try await service.applyCommitPatch(reverse) + XCTAssertFalse(FileManager.default.fileExists(atPath: repo.appendingPathComponent(path).path)) + let forward = try await prepare(source, path, repo) + try await service.applyCommitPatch(forward) + XCTAssertEqual(try read(path, repo), "hello") + } + + func testBinaryAndSubmoduleRejectedAndHaveDisabledReasons() async throws { + let repo = try repository() + try write("base\n", "base.txt", repo) + let base = try commit(repo) + try Data([0, 1, 2, 0]).write(to: repo.appendingPathComponent("binary.bin")) + _ = try commit(repo) + try git(["update-index", "--add", "--cacheinfo", "160000,\(base),module"], repo) + try git(["commit", "-m", "gitlink"], repo) + // Include the binary modification in the same source commit. + try git(["reset", "--soft", base], repo) + try git(["commit", "-m", "binary and gitlink"], repo) + let source = try git(["rev-parse", "HEAD"], repo) + let reasons = try await service.commitPatchUnavailableReasons(commit: source, in: repo) + XCTAssertTrue(reasons["binary.bin"]?.contains("Binary") == true) + XCTAssertTrue(reasons["module"]?.contains("Submodule") == true) + await reject { _ = try await self.prepare(source, "binary.bin", repo, direction: .revert) } + await reject { _ = try await self.prepare(source, "module", repo, direction: .revert) } + } + + func testMergeCommitRejected() async throws { + let repo = try repository() + try write("base\n", "a.txt", repo); _ = try commit(repo) + try git(["checkout", "-b", "side"], repo) + try write("side\n", "side.txt", repo); _ = try commit(repo) + try git(["checkout", "main"], repo) + try write("main\n", "main.txt", repo); _ = try commit(repo) + try git(["merge", "--no-ff", "side", "-m", "merge"], repo) + let source = try git(["rev-parse", "HEAD"], repo) + await reject { + _ = try await self.service.prepareCommitPatch(.init(commit: source, + files: [.init(path: "side.txt", status: .added)], direction: .revert, lines: nil, scope: "File"), in: repo) + } + } + + func testPartialRenameIncludesRenameAndOnlySelectedContentBothDirections() async throws { + let repo = try repository() + let text = "a\nb\nc\nd\ne\nf\ng\nh\n" + try write(text, "old.txt", repo) + let base = try commit(repo) + try git(["mv", "old.txt", "new.txt"], repo) + try write(text.replacingOccurrences(of: "b\n", with: "B\n"), "new.txt", repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + let lines: Set = [.init(old: nil, new: 2)] + let forward = try await prepare(source, "new.txt", repo, lines: lines) + try await service.applyCommitPatch(forward) + XCTAssertEqual(try read("new.txt", repo), text.replacingOccurrences(of: "b\n", with: "b\nB\n")) + XCTAssertFalse(FileManager.default.fileExists(atPath: repo.appendingPathComponent("old.txt").path)) + try await service.applyCheckedWorkingTreePatch(forward.patch, reverse: true, in: repo) + XCTAssertEqual(try read("old.txt", repo), text) + try git(["reset", "--hard", source], repo) + let reverse = try await prepare(source, "new.txt", repo, direction: .revert, lines: lines) + try await service.applyCommitPatch(reverse) + XCTAssertEqual(try read("old.txt", repo), text.replacingOccurrences(of: "b\n", with: "")) + } + + func testMultiplePartialHunksRecalculateOffsetsWithoutIncludingOtherChanges() async throws { + let repo = try repository() + let text = (1...40).map { "line\($0)" }.joined(separator: "\n") + "\n" + try write(text, "a.txt", repo) + let base = try commit(repo) + let changed = text.replacingOccurrences(of: "line2\n", with: "line2\nextra\n") + .replacingOccurrences(of: "line22\n", with: "changed22\n") + try write(changed, "a.txt", repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + let lines: Set = [.init(old: nil, new: 3), .init(old: nil, new: 23)] + let forward = try await prepare(source, "a.txt", repo, lines: lines) + try await service.applyCommitPatch(forward) + XCTAssertEqual(try read("a.txt", repo), text.replacingOccurrences(of: "line2\n", with: "line2\nextra\n") + .replacingOccurrences(of: "line22\n", with: "line22\nchanged22\n")) + try await service.applyCheckedWorkingTreePatch(forward.patch, reverse: true, in: repo) + XCTAssertEqual(try read("a.txt", repo), text) + try git(["reset", "--hard", source], repo) + let reverse = try await prepare(source, "a.txt", repo, direction: .revert, lines: lines) + try await service.applyCommitPatch(reverse) + XCTAssertEqual(try read("a.txt", repo), text.replacingOccurrences(of: "line22\n", with: "")) + } + + func testPartialReplacementOfUnterminatedLine() async throws { + let repo = try repository() + try write("old", "a.txt", repo) + let base = try commit(repo) + try write("new", "a.txt", repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + let forward = try await prepare(source, "a.txt", repo, lines: [.init(old: nil, new: 1)]) + try await service.applyCommitPatch(forward) + XCTAssertEqual(try read("a.txt", repo), "old\nnew") + try await service.applyCheckedWorkingTreePatch(forward.patch, reverse: true, in: repo) + XCTAssertEqual(try read("a.txt", repo), "old") + } + + func testPartialCRLFChangesPreserveLineEndings() async throws { + let repo = try repository() + try git(["config", "core.autocrlf", "false"], repo) + try write("before\r\nold\r\nafter\r\n", "a.txt", repo) + let base = try commit(repo) + try write("before\r\nnew\r\nafter\r\n", "a.txt", repo) + let source = try commit(repo) + let hunks = await service.diff(for: "a.txt", in: source, in: repo) + let added = try XCTUnwrap(hunks.flatMap(\.lines).first { $0.type == .added }) + XCTAssertEqual(added.newLineNumber, 2) + try git(["checkout", "--detach", base], repo) + let forward = try await prepare(source, "a.txt", repo, lines: [.init(added)]) + try await service.applyCommitPatch(forward) + XCTAssertEqual(try read("a.txt", repo), "before\r\nold\r\nnew\r\nafter\r\n") + } + + func testEmptyAndStaleLineSelectionsRejectWithoutApplyingWholeHunk() async throws { + let repo = try repository() + try write("old\n", "a.txt", repo) + let base = try commit(repo) + try write("new\n", "a.txt", repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + await reject { _ = try await self.prepare(source, "a.txt", repo, lines: []) } + await reject { _ = try await self.prepare(source, "a.txt", repo, lines: [.init(old: nil, new: 200)]) } + XCTAssertEqual(try read("a.txt", repo), "old\n") + } + + func testIndexChangeAfterReviewRejectsAndOverlappingUndoPreservesLaterEdit() async throws { + let repo = try repository() + try write("old\n", "a.txt", repo); try write("other\n", "other.txt", repo) + let base = try commit(repo) + try write("new\n", "a.txt", repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + let prepared = try await prepare(source, "a.txt", repo) + try write("staged\n", "other.txt", repo); try git(["add", "other.txt"], repo) + await reject { try await self.service.applyCommitPatch(prepared) } + XCTAssertEqual(try read("a.txt", repo), "old\n") + let reviewed = try await prepare(source, "a.txt", repo) + try await service.applyCommitPatch(reviewed) + try write("later edit\n", "a.txt", repo) + await reject { try await self.service.applyCheckedWorkingTreePatch(reviewed.patch, reverse: true, in: repo) } + XCTAssertEqual(try read("a.txt", repo), "later edit\n") + XCTAssertTrue(try git(["show", ":other.txt"], repo).contains("staged")) + } + + func testPartialContentDoesNotIncludeUnselectedExecutableBit() async throws { + let repo = try repository() + try git(["config", "core.filemode", "true"], repo) + try write("old\n", "script.sh", repo) + try FileManager.default.setAttributes([.posixPermissions: 0o644], ofItemAtPath: repo.appendingPathComponent("script.sh").path) + let base = try commit(repo) + try write("new\n", "script.sh", repo) + try FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: repo.appendingPathComponent("script.sh").path) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + let partial = try await prepare(source, "script.sh", repo, lines: [.init(old: 1, new: nil), .init(old: nil, new: 1)]) + try await service.applyCommitPatch(partial) + let mode = try FileManager.default.attributesOfItem(atPath: repo.appendingPathComponent("script.sh").path)[.posixPermissions] as? NSNumber + XCTAssertEqual((mode?.intValue ?? 0) & 0o111, 0) + XCTAssertEqual(try read("script.sh", repo), "new\n") + try git(["reset", "--hard", base], repo) + let whole = try await prepare(source, "script.sh", repo) + try await service.applyCommitPatch(whole) + XCTAssertTrue(FileManager.default.isExecutableFile(atPath: repo.appendingPathComponent("script.sh").path)) + } + + func testInProgressOperationAndUntrackedCollisionRejectWithoutMutation() async throws { + let repo = try repository() + try write("base\n", "a.txt", repo) + let base = try commit(repo) + try write("new\n", "new.txt", repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + try write("local untracked\n", "new.txt", repo) + await reject { _ = try await self.prepare(source, "new.txt", repo) } + XCTAssertEqual(try read("new.txt", repo), "local untracked\n") + try FileManager.default.removeItem(at: repo.appendingPathComponent("new.txt")) + let state = repo.appendingPathComponent(".git/sequencer") + try FileManager.default.createDirectory(at: state, withIntermediateDirectories: true) + await reject { _ = try await self.prepare(source, "new.txt", repo) } + XCTAssertFalse(FileManager.default.fileExists(atPath: repo.appendingPathComponent("new.txt").path)) + } + + private func prepare(_ commit: String, _ path: String, _ repo: URL, + direction: CommitPatchRequest.Direction = .apply, + lines: Set? = nil) async throws -> PreparedCommitPatch { + let files = await service.changedFiles(in: commit, in: repo) + let file = try XCTUnwrap(files.first { $0.path == path }) + return try await service.prepareCommitPatch(.init(commit: commit, files: [file], direction: direction, + lines: lines, scope: lines == nil ? "File" : "Lines"), in: repo) + } + + private func reject(_ action: () async throws -> Void, file: StaticString = #filePath, line: UInt = #line) async { + do { try await action(); XCTFail("Expected rejection", file: file, line: line) } + catch { /* Rejection must leave the working copy intact; each caller asserts its state. */ } + } + + private func repository() throws -> URL { + let repo = FileManager.default.temporaryDirectory.appendingPathComponent("commit-patch-tests-\(UUID())") + try FileManager.default.createDirectory(at: repo, withIntermediateDirectories: true) + repositories.append(repo) + try git(["init", "-b", "main"], repo) + try git(["config", "user.name", "Tests"], repo) + try git(["config", "user.email", "tests@example.com"], repo) + try git(["config", "commit.gpgsign", "false"], repo) + return repo + } + + private func write(_ text: String, _ path: String, _ repo: URL) throws { + try Data(text.utf8).write(to: repo.appendingPathComponent(path)) + } + + private func read(_ path: String, _ repo: URL) throws -> String { + try String(contentsOf: repo.appendingPathComponent(path), encoding: .utf8) + } + + private func commit(_ repo: URL) throws -> String { + try git(["add", "-A"], repo) + try git(["commit", "--allow-empty", "-m", "fixture"], repo) + return try git(["rev-parse", "HEAD"], repo) + } + + @discardableResult private func git(_ arguments: [String], _ repo: URL) throws -> String { + let process = Process() + process.executableURL = URL(fileURLWithPath: "/usr/bin/git") + process.arguments = arguments + process.currentDirectoryURL = repo + let pipe = Pipe() + process.standardOutput = pipe; process.standardError = pipe + try process.run() + let data = pipe.fileHandleForReading.readDataToEndOfFile() + process.waitUntilExit() + let output = String(decoding: data, as: UTF8.self).trimmingCharacters(in: .whitespacesAndNewlines) + guard process.terminationStatus == 0 else { throw NSError(domain: "GitTest", code: Int(process.terminationStatus), userInfo: [NSLocalizedDescriptionKey: output]) } + return output + } +} From 20107f3c1a531aef0b59ff840701156c72e6cf3e Mon Sep 17 00:00:00 2001 From: Thanh Tran Date: Sat, 26 Sep 2026 08:56:29 +0700 Subject: [PATCH 2/8] Reuse DiffView in selected changes review sheet --- macgit/Views/Common/DiffView.swift | 5 +- .../History/CommitPatchReviewSheet.swift | 77 +++++++++++++++---- 2 files changed, 64 insertions(+), 18 deletions(-) diff --git a/macgit/Views/Common/DiffView.swift b/macgit/Views/Common/DiffView.swift index 68be7ad..1e4f886 100644 --- a/macgit/Views/Common/DiffView.swift +++ b/macgit/Views/Common/DiffView.swift @@ -31,6 +31,7 @@ struct DiffView: View { let onError: (String) -> Void let filePath: String? let gitRef: String? + let prefersTextDiff: Bool let onCommitPatch: (([DiffLine], CommitPatchRequest.Direction, String) -> Void)? let commitPatchDisabledReason: String? @@ -43,6 +44,7 @@ struct DiffView: View { onError: @escaping (String) -> Void = { _ in }, filePath: String? = nil, gitRef: String? = nil, + prefersTextDiff: Bool = false, onCommitPatch: (([DiffLine], CommitPatchRequest.Direction, String) -> Void)? = nil, commitPatchDisabledReason: String? = nil ) { @@ -54,6 +56,7 @@ struct DiffView: View { self.onError = onError self.filePath = filePath self.gitRef = gitRef + self.prefersTextDiff = prefersTextDiff self.onCommitPatch = onCommitPatch self.commitPatchDisabledReason = commitPatchDisabledReason } @@ -77,7 +80,7 @@ struct DiffView: View { } var body: some View { - if isImageFile { + if isImageFile && !prefersTextDiff { imagePreview } else if hunks.isEmpty { EmptyStateView(message: "No diff to display", detail: "Select a file to see changes") diff --git a/macgit/Views/History/CommitPatchReviewSheet.swift b/macgit/Views/History/CommitPatchReviewSheet.swift index 7ac2b68..8a6a14c 100644 --- a/macgit/Views/History/CommitPatchReviewSheet.swift +++ b/macgit/Views/History/CommitPatchReviewSheet.swift @@ -8,6 +8,39 @@ struct CommitPatchReviewSheet: View { let onCancel: () -> Void let onApply: () -> Void + private struct FilePreview: Identifiable { + let file: CommitFileChange + let hunks: [DiffHunk] + let metadata: String + var id: UUID { file.id } + } + + private let previews: [FilePreview] + @State private var selectedPreviewID: UUID? + + init(prepared: PreparedCommitPatch, isBusy: Bool, errorMessage: String?, + onCancel: @escaping () -> Void, onApply: @escaping () -> Void) { + self.prepared = prepared + self.isBusy = isBusy + self.errorMessage = errorMessage + self.onCancel = onCancel + self.onApply = onApply + // Preparation concatenates one complete patch per requested file, in this order. + // Parse each separately so headers from another file never become hunk content. + let patches = prepared.patch.components(separatedBy: "\ndiff --git ") + self.previews = zip(prepared.request.files, patches).map { file, patch in + FilePreview(file: file, hunks: DiffParser.parse(patch), metadata: patch.components(separatedBy: "\n") + .prefix { !$0.hasPrefix("@@ ") } + .filter { $0.hasPrefix("old mode ") || $0.hasPrefix("new mode ") || $0.hasPrefix("new file mode ") || $0.hasPrefix("deleted file mode ") } + .joined(separator: "\n")) + } + self._selectedPreviewID = State(initialValue: prepared.request.files.first?.id) + } + + private var selectedPreview: FilePreview? { + previews.first { $0.id == selectedPreviewID } ?? previews.first + } + var body: some View { VStack(alignment: .leading, spacing: 16) { Text("\(prepared.request.direction.rawValue) Selected Changes") @@ -26,27 +59,37 @@ struct CommitPatchReviewSheet: View { .foregroundStyle(.red) .textSelection(.enabled) } - ScrollView { - VStack(alignment: .leading, spacing: 6) { - ForEach(prepared.request.files) { file in - if let oldPath = file.oldPath { - Text(prepared.request.direction == .apply ? "\(oldPath) → \(file.path)" : "\(file.path) → \(oldPath)") - Text("The rename is included with the selected content changes.") - .font(.caption).foregroundStyle(.secondary) - } else { Text(file.path) } + if previews.count > 1 { + Picker("File", selection: $selectedPreviewID) { + ForEach(previews) { preview in + Text(preview.file.path).tag(Optional(preview.id)) } } - .frame(maxWidth: .infinity, alignment: .leading) } - .frame(maxHeight: 100) - ScrollView([.horizontal, .vertical]) { - Text(prepared.patch) - .font(.system(.caption, design: .monospaced)) - .textSelection(.enabled) - .frame(maxWidth: .infinity, alignment: .leading) - .padding(10) + if let preview = selectedPreview { + if let oldPath = preview.file.oldPath { + Text(prepared.request.direction == .apply ? "\(oldPath) → \(preview.file.path)" : "\(preview.file.path) → \(oldPath)") + Text("The rename is included with the selected content changes.") + .font(.caption).foregroundStyle(.secondary) + } else if previews.count == 1 { + Text(preview.file.path) + } + if !preview.metadata.isEmpty { + Text(preview.metadata) + .font(.caption.monospaced()) + .foregroundStyle(.secondary) + } + Group { + if preview.hunks.isEmpty { + EmptyStateView(message: "File metadata changes", detail: "This file has no changed lines.") + } else { + DiffView(hunks: preview.hunks, filePath: preview.file.path, prefersTextDiff: true) + .id(preview.id) + } + } + .frame(maxWidth: .infinity, maxHeight: .infinity) + .background(.quaternary.opacity(0.4), in: RoundedRectangle(cornerRadius: 8)) } - .background(.quaternary.opacity(0.4), in: RoundedRectangle(cornerRadius: 8)) HStack { if isBusy { ProgressView().controlSize(.small) } Spacer() From da0106c3b9ef631180f90a375cd3206b77dea15b Mon Sep 17 00:00:00 2001 From: Thanh Tran Date: Sat, 26 Sep 2026 09:24:07 +0700 Subject: [PATCH 3/8] Safely merge and resolve selected commit changes before applying --- macgit/App/CommitPatchController.swift | 49 ++++- macgit/Models/CommitPatchReviewFile.swift | 38 ++++ macgit/Models/PreparedCommitPatch.swift | 10 +- .../Services/ConflictResolutionModels.swift | 2 +- .../GitStatusService+CommitPatch.swift | 25 ++- .../GitStatusService+CommitPatchMerge.swift | 155 +++++++++++++++ .../History/CommitPatchConflictSheet.swift | 115 ++++++++++++ .../CommitPatchConflictWindowContext.swift | 23 +++ .../History/CommitPatchReviewSheet.swift | 61 ++++-- macgit/Views/History/HistoryView.swift | 27 ++- macgitTests/CommitPatchIntegrationTests.swift | 177 +++++++++++++++++- 11 files changed, 644 insertions(+), 38 deletions(-) create mode 100644 macgit/Models/CommitPatchReviewFile.swift create mode 100644 macgit/Services/GitStatusService+CommitPatchMerge.swift create mode 100644 macgit/Views/History/CommitPatchConflictSheet.swift create mode 100644 macgit/Views/History/CommitPatchConflictWindowContext.swift diff --git a/macgit/App/CommitPatchController.swift b/macgit/App/CommitPatchController.swift index 2c421e5..239cd17 100644 --- a/macgit/App/CommitPatchController.swift +++ b/macgit/App/CommitPatchController.swift @@ -6,6 +6,9 @@ import Observation final class CommitPatchController { var prepared: PreparedCommitPatch? private(set) var isBusy = false + private(set) var isPreparing = false + @ObservationIgnored private var preparationTask: Task? + @ObservationIgnored private var preparationID = UUID() var errorMessage = "" var showingError = false private(set) var reviewError: String? @@ -14,19 +17,45 @@ final class CommitPatchController { guard !isBusy, prepared == nil else { return } reviewError = nil isBusy = true - Task { - defer { isBusy = false } + isPreparing = true + let id = UUID() + preparationID = id + preparationTask = Task { + defer { + if preparationID == id { + isBusy = false + isPreparing = false + preparationTask = nil + } + } do { - prepared = try await GitStatusService.shared.prepareCommitPatch(request, in: repositoryURL) + let result = try await GitStatusService.shared.prepareCommitPatch(request, in: repositoryURL) + guard preparationID == id, !Task.isCancelled else { return } + prepared = result } catch { + guard preparationID == id, !Task.isCancelled else { return } errorMessage = error.localizedDescription showingError = true } } } + func cancelPreparation() { + guard isPreparing else { return } + preparationID = UUID() + preparationTask?.cancel() + preparationTask = nil + isPreparing = false + isBusy = false + } + func apply(undoManager: GitUndoManager?, syncState: SyncState?, run: RepositoryOperationRunner) { guard let prepared, !isBusy else { return } + guard !prepared.hasConflicts else { return } + guard prepared.hasChanges else { + self.prepared = nil + return + } reviewError = nil isBusy = true run("\(prepared.request.direction.rawValue) selected changes…") { [self] in @@ -48,4 +77,18 @@ final class CommitPatchController { } } } + + func resolve(fileID: UUID, result: String?) async -> Bool { + guard let prepared, !isBusy else { return false } + isBusy = true + reviewError = nil + defer { isBusy = false } + do { + self.prepared = try await GitStatusService.shared.resolveCommitPatch(prepared, fileID: fileID, result: result) + return true + } catch { + reviewError = error.localizedDescription + return false + } + } } diff --git a/macgit/Models/CommitPatchReviewFile.swift b/macgit/Models/CommitPatchReviewFile.swift new file mode 100644 index 0000000..553a4b3 --- /dev/null +++ b/macgit/Models/CommitPatchReviewFile.swift @@ -0,0 +1,38 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +import Foundation + +nonisolated struct CommitPatchReviewFile: Identifiable, Sendable { + enum State: String, Sendable { + case ready = "Ready" + case merged = "Merged with your changes" + case alreadyApplied = "Already present" + case conflict = "Needs resolution" + case skipped = "Skipped" + case resolved = "Resolved" + } + + let file: CommitFileChange + var patch: String + var state: State + var conflict: Conflict? + var id: UUID { file.id } + + struct Conflict: Sendable { + let message: String + let current: String + let selected: String + // nil means a structural conflict that cannot safely use the text editor. + let markedResult: String? + let permissions: Int + } + + var appliesChanges: Bool { + state != .conflict && state != .skipped && state != .alreadyApplied && !patch.isEmpty + } + + static func containsConflictMarkers(_ text: String) -> Bool { + text.components(separatedBy: "\n").contains { + $0.hasPrefix("<<<<<<<") || $0.hasPrefix("=======") || $0.hasPrefix(">>>>>>>") || $0.hasPrefix("|||||||") + } + } +} diff --git a/macgit/Models/PreparedCommitPatch.swift b/macgit/Models/PreparedCommitPatch.swift index aaff8ea..826ef7b 100644 --- a/macgit/Models/PreparedCommitPatch.swift +++ b/macgit/Models/PreparedCommitPatch.swift @@ -9,6 +9,14 @@ nonisolated struct PreparedCommitPatch: Identifiable, Sendable { let targetBranch: String let targetHead: String let paths: [String] - let patch: String + var patch: String let fingerprint: String + var reviewFiles: [CommitPatchReviewFile] = [] + + var hasConflicts: Bool { reviewFiles.contains { $0.state == .conflict } } + var hasChanges: Bool { !patch.isEmpty } + + mutating func rebuildPatch() { + patch = reviewFiles.filter(\.appliesChanges).map(\.patch).joined() + } } diff --git a/macgit/Services/ConflictResolutionModels.swift b/macgit/Services/ConflictResolutionModels.swift index b78b446..dc41133 100644 --- a/macgit/Services/ConflictResolutionModels.swift +++ b/macgit/Services/ConflictResolutionModels.swift @@ -282,7 +282,7 @@ struct ConflictResolutionDocument: Equatable, Sendable { var index = text.startIndex while index < text.endIndex { - if text[index] == "\n" { + if text[index] == "\n" || text[index] == "\r\n" { let nextIndex = text.index(after: index) fragments.append(String(text[lineStart.. String { + func commitPatchFingerprint(paths: [String], in repositoryURL: URL) async throws -> String { var hash = SHA256() func append(_ data: Data) { hash.update(data: Data("\(data.count):".utf8)) diff --git a/macgit/Services/GitStatusService+CommitPatchMerge.swift b/macgit/Services/GitStatusService+CommitPatchMerge.swift new file mode 100644 index 0000000..655f1b6 --- /dev/null +++ b/macgit/Services/GitStatusService+CommitPatchMerge.swift @@ -0,0 +1,155 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +import Foundation + +extension GitStatusService { + func prepareCommitPatchReviewFile(file: CommitFileChange, patch: String, base: String, + in repositoryURL: URL) async throws -> CommitPatchReviewFile { + do { + try await runCommitPatch(patch, checkOnly: true, reverse: false, in: repositoryURL) + return CommitPatchReviewFile(file: file, patch: patch, state: .ready) + } catch { try Task.checkCancellation() } + + // Check each file independently: a batch can contain both new and already-present changes. + do { + try await runCommitPatch(patch, checkOnly: true, reverse: true, in: repositoryURL) + return CommitPatchReviewFile(file: file, patch: "", state: .alreadyApplied) + } catch { try Task.checkCancellation() } + + let url = repositoryURL.appendingPathComponent(file.path) + guard file.status == .modified, file.oldPath == nil, + FileManager.default.fileExists(atPath: url.path), + !patch.components(separatedBy: "\n").contains(where: { $0.hasPrefix("old mode ") || $0.hasPrefix("new mode ") }) else { + return CommitPatchReviewFile(file: file, patch: patch, state: .conflict, conflict: .init( + message: "This change adds, deletes, renames, or changes the permissions of a file whose current state is different. Commit+ will not overwrite it automatically. Skip this file to apply the others, or cancel and review its current state.", + current: "", selected: "", markedResult: nil, permissions: 0)) + } + let currentData = try Data(contentsOf: url) + let baseData = try await showFile(at: file.path, ref: base, in: repositoryURL) + guard let current = String(data: currentData, encoding: .utf8), !currentData.contains(0), + String(data: baseData, encoding: .utf8) != nil, !baseData.contains(0), + currentData.count <= 5_000_000, baseData.count <= 5_000_000 else { + throw GitError.commandFailed("\(file.path) cannot be merged safely because its current or original content is binary, too large, or not UTF-8.") + } + let permissions = (try FileManager.default.attributesOfItem(atPath: url.path)[.posixPermissions] as? NSNumber)?.intValue ?? 0o644 + let scratch = try await makeCommitPatchScratch() + defer { try? FileManager.default.removeItem(at: scratch) } + try writeCommitPatchScratchFile(baseData, path: file.path, permissions: permissions, in: scratch) + let patchURL = scratch.appendingPathComponent(".git/selected.patch") + try Data(patch.utf8).write(to: patchURL) + // Materialize ONLY the selected patch against its true base, never the whole commit. + _ = try await scratchGit(["apply", "--whitespace=nowarn", "--", patchURL.path], in: scratch) + let selectedData = try Data(contentsOf: scratch.appendingPathComponent(file.path)) + guard let selected = String(data: selectedData, encoding: .utf8) else { + throw GitError.commandFailed("Could not decode the selected changes in \(file.path).") + } + if currentData == selectedData { + return CommitPatchReviewFile(file: file, patch: "", state: .alreadyApplied) + } + guard !CommitPatchReviewFile.containsConflictMarkers(current), + !CommitPatchReviewFile.containsConflictMarkers(selected), + !CommitPatchReviewFile.containsConflictMarkers(String(decoding: baseData, as: UTF8.self)) else { + return CommitPatchReviewFile(file: file, patch: patch, state: .conflict, conflict: .init( + message: "This file already contains conflict markers. Skip it or cancel and resolve those markers first.", + current: current, selected: selected, markedResult: nil, permissions: permissions)) + } + let oursURL = scratch.appendingPathComponent(".git/current") + let baseURL = scratch.appendingPathComponent(".git/base") + let selectedURL = scratch.appendingPathComponent(".git/selected") + try currentData.write(to: oursURL) + try baseData.write(to: baseURL) + try selectedData.write(to: selectedURL) + do { + // merge-file writes only this scratch file, even when it returns conflicts. + _ = try await scratchGit(["merge-file", "--no-diff3", "-L", "Your working copy", + "-L", "Original", "-L", "Selected changes", oursURL.path, baseURL.path, selectedURL.path], in: scratch) + } catch { + try Task.checkCancellation() + let marked = try String(contentsOf: oursURL, encoding: .utf8) + let markerLines = marked.components(separatedBy: "\n").map { $0.trimmingCharacters(in: .newlines) } + guard markerLines.contains("<<<<<<< Your working copy"), markerLines.contains(">>>>>>> Selected changes") else { throw error } + return CommitPatchReviewFile(file: file, patch: patch, state: .conflict, conflict: .init( + message: "Your working copy and the selected changes edit the same lines. Resolve the result in a temporary preview; no files will be written until you click Apply.", + current: current, selected: selected, markedResult: marked, permissions: permissions)) + } + let merged = try String(contentsOf: oursURL, encoding: .utf8) + if merged == current { return CommitPatchReviewFile(file: file, patch: "", state: .alreadyApplied) } + let mergedPatch = try await commitPatchResultDiff(path: file.path, current: current, + result: merged, permissions: permissions) + try await runCommitPatch(mergedPatch, checkOnly: true, reverse: false, in: repositoryURL) + return CommitPatchReviewFile(file: file, patch: mergedPatch, state: .merged) + } + + /// A nil result explicitly skips the file. Neither resolving nor skipping mutates the real repository. + func resolveCommitPatch(_ prepared: PreparedCommitPatch, fileID: UUID, result: String?) async throws -> PreparedCommitPatch { + try await validateCommitPatchState(in: prepared.repositoryURL) + guard try await commitPatchFingerprint(paths: prepared.paths, in: prepared.repositoryURL) == prepared.fingerprint else { + throw GitError.commandFailed("Your working copy changed while reviewing. Cancel and select the changes again so nothing is overwritten.") + } + var updated = prepared + guard let index = updated.reviewFiles.firstIndex(where: { $0.id == fileID }), + let conflict = updated.reviewFiles[index].conflict, updated.reviewFiles[index].state == .conflict else { + throw GitError.commandFailed("This file no longer needs resolution.") + } + if let result { + guard conflict.markedResult != nil, !CommitPatchReviewFile.containsConflictMarkers(result), !result.utf8.contains(0), + result.utf8.count <= 5_000_000 else { + throw GitError.commandFailed("Remove all conflict markers and review the result before continuing.") + } + let patch = try await commitPatchResultDiff(path: updated.reviewFiles[index].file.path, + current: conflict.current, result: result, permissions: conflict.permissions) + if !patch.isEmpty { try await runCommitPatch(patch, checkOnly: true, reverse: false, in: prepared.repositoryURL) } + updated.reviewFiles[index].patch = patch + updated.reviewFiles[index].state = .resolved + } else { + updated.reviewFiles[index].patch = "" + updated.reviewFiles[index].state = .skipped + } + updated.reviewFiles[index].conflict = nil + updated.rebuildPatch() + return updated + } + + private func commitPatchResultDiff(path: String, current: String, result: String, permissions: Int) async throws -> String { + if current == result { return "" } + let scratch = try await makeCommitPatchScratch() + defer { try? FileManager.default.removeItem(at: scratch) } + try writeCommitPatchScratchFile(Data(current.utf8), path: path, permissions: permissions, in: scratch) + _ = try await scratchGit(["--literal-pathspecs", "add", "--force", "--", path], in: scratch) + try writeCommitPatchScratchFile(Data(result.utf8), path: path, permissions: permissions, in: scratch) + let data = try await scratchGit(["--literal-pathspecs", "diff", "--no-color", "--no-ext-diff", "--no-textconv", + "--no-relative", "--src-prefix=a/", "--dst-prefix=b/", "--full-index", "--", path], in: scratch) + guard let patch = String(data: data, encoding: .utf8) else { throw GitError.commandFailed("Could not preview the merged result.") } + return patch + } + + private func makeCommitPatchScratch() async throws -> URL { + let url = FileManager.default.temporaryDirectory.appendingPathComponent("commit-patch-merge-\(UUID())") + try FileManager.default.createDirectory(at: url, withIntermediateDirectories: false, attributes: [.posixPermissions: 0o700]) + do { + _ = try await scratchGit(["init", "--quiet", "--template="], in: url) + let info = url.appendingPathComponent(".git/info") + try FileManager.default.createDirectory(at: info, withIntermediateDirectories: true) + // Do not run filters or normalize line endings from repository attributes in a scratch preview. + try Data("* -text -filter -ident -working-tree-encoding diff\n".utf8).write(to: info.appendingPathComponent("attributes")) + return url + } catch { + try? FileManager.default.removeItem(at: url) + throw error + } + } + + private func writeCommitPatchScratchFile(_ data: Data, path: String, permissions: Int, in directory: URL) throws { + let url = directory.appendingPathComponent(path) + try FileManager.default.createDirectory(at: url.deletingLastPathComponent(), withIntermediateDirectories: true) + try data.write(to: url) + try FileManager.default.setAttributes([.posixPermissions: permissions], ofItemAtPath: url.path) + } + + private func scratchGit(_ arguments: [String], in directory: URL) async throws -> Data { + var environment = ProcessInfo.processInfo.environment.filter { !$0.key.hasPrefix("GIT_") } + environment["GIT_CONFIG_NOSYSTEM"] = "1" + environment["GIT_CONFIG_GLOBAL"] = "/dev/null" + return try await runGitRaw(arguments: ["-c", "core.autocrlf=false", "-c", "core.filemode=true"] + arguments, + in: directory, environment: environment, outputByteLimit: 6_000_000) + } +} diff --git a/macgit/Views/History/CommitPatchConflictSheet.swift b/macgit/Views/History/CommitPatchConflictSheet.swift new file mode 100644 index 0000000..53d617e --- /dev/null +++ b/macgit/Views/History/CommitPatchConflictSheet.swift @@ -0,0 +1,115 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +import SwiftUI + +/// Edits an in-memory merge result. The existing code panes/editor never receive a repository mutation callback. +struct CommitPatchConflictSheet: View { + let file: CommitPatchReviewFile + let isBusy: Bool + let errorMessage: String? + let onCancel: () -> Void + let onResolve: (String?) async -> Bool + @State private var result: String + @State private var scrollController = SyncedScrollController() + @State private var undoResetGeneration = 0 + @State private var commandContext = ConflictUndoCommandContext.makeIdentifier() + @State private var undoResults: [String] = [] + @State private var redoResults: [String] = [] + + init(file: CommitPatchReviewFile, isBusy: Bool, errorMessage: String?, onCancel: @escaping () -> Void, + onResolve: @escaping (String?) async -> Bool) { + self.file = file + self.isBusy = isBusy + self.errorMessage = errorMessage + self.onCancel = onCancel + self.onResolve = onResolve + self._result = State(initialValue: file.conflict?.markedResult ?? "") + } + + var body: some View { + VStack(alignment: .leading, spacing: 12) { + Text("Resolve Selected Changes").font(.title2.bold()) + Text(file.file.path).font(.callout.monospaced()).textSelection(.enabled) + if let conflict = file.conflict { + Text(conflict.message).foregroundStyle(.secondary) + if conflict.markedResult != nil { + HStack(spacing: 12) { + codePane("Your working copy", text: conflict.current, color: .blue) + codePane("Selected changes applied to their original version", text: conflict.selected, color: .green) + } + .frame(maxHeight: .infinity) + HStack { + Button("Keep current conflicts") { choose(.current) } + Button("Use selected conflicts") { choose(.incoming) } + Spacer() + } + Text("Choose a side for the conflicting sections or edit the result below. Non-conflicting changes are kept. Remove all conflict markers before continuing.") + .font(.caption).foregroundStyle(.secondary) + ConflictResultEditorView(text: result, onTextChange: { result = $0 }, + fileExtension: SyntaxHighlighter.syntaxIdentifier(forFilePath: file.file.path), + baselineText: conflict.current, isDisabled: isBusy, undoResetGeneration: undoResetGeneration, + scrollController: scrollController) + .frame(maxHeight: .infinity) + } else { Spacer() } + } + if let errorMessage { Text(errorMessage).foregroundStyle(.red).font(.callout) } + HStack { + Text("Only a preview. Your files and staging area are unchanged.") + .font(.caption).foregroundStyle(.secondary) + Spacer() + Button("Cancel", action: onCancel).keyboardShortcut(.cancelAction) + Button("Skip this file") { finish(nil) } + if file.conflict?.markedResult != nil { + Button("Review Result") { finish(result) } + .keyboardShortcut(.defaultAction) + .disabled(CommitPatchReviewFile.containsConflictMarkers(result)) + } + } + } + .padding(20) + .frame(width: 1000, height: 700) + .disabled(isBusy) + .interactiveDismissDisabled(isBusy) + .background(CommitPatchConflictWindowContext(identifier: commandContext)) + .onReceive(NotificationCenter.default.publisher(for: .conflictUndoAction)) { notification in + guard !isBusy, notification.userInfo?["commandContext"] as? String == commandContext.rawValue, + let action = notification.userInfo?["action"] as? GitUndoMenuAction else { return } + switch action { + case .undo: + guard let previous = undoResults.popLast() else { return } + redoResults.append(result) + result = previous + case .redo: + guard let next = redoResults.popLast() else { return } + undoResults.append(result) + result = next + } + undoResetGeneration += 1 + } + } + + private func codePane(_ title: String, text: String, color: Color) -> some View { + VStack(alignment: .leading, spacing: 6) { + Text(title).font(.caption.bold()) + SyncedScrollView(id: title, controller: scrollController, + virtualizedRowCount: text.components(separatedBy: "\n").count) { + ConflictCodeView(text: text, fileExtension: SyntaxHighlighter.syntaxIdentifier(forFilePath: file.file.path), + highlightedLines: [], highlightColor: color) + } + .background(.quaternary.opacity(0.3)) + } + } + + private func choose(_ side: ConflictSectionResolution) { + guard let marked = file.conflict?.markedResult, + var document = try? ConflictResolutionDocument.parse(marked) else { return } + document.selectAllConflicts(side) + undoResults.append(result) + redoResults.removeAll() + result = document.resolvedText + undoResetGeneration += 1 + } + + private func finish(_ text: String?) { + Task { if await onResolve(text) { onCancel() } } + } +} diff --git a/macgit/Views/History/CommitPatchConflictWindowContext.swift b/macgit/Views/History/CommitPatchConflictWindowContext.swift new file mode 100644 index 0000000..725fd42 --- /dev/null +++ b/macgit/Views/History/CommitPatchConflictWindowContext.swift @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +import SwiftUI + +/// Routes Cmd-Z inside the temporary resolver to editor/resolution undo, never repository undo. +struct CommitPatchConflictWindowContext: NSViewRepresentable { + let identifier: NSUserInterfaceItemIdentifier + + func makeNSView(context: Context) -> ContextView { + let view = ContextView() + view.contextIdentifier = identifier + return view + } + + func updateNSView(_ view: ContextView, context: Context) {} + + final class ContextView: NSView { + var contextIdentifier: NSUserInterfaceItemIdentifier? + override func viewDidMoveToWindow() { + super.viewDidMoveToWindow() + if let contextIdentifier { window?.identifier = contextIdentifier } + } + } +} diff --git a/macgit/Views/History/CommitPatchReviewSheet.swift b/macgit/Views/History/CommitPatchReviewSheet.swift index 8a6a14c..8888d86 100644 --- a/macgit/Views/History/CommitPatchReviewSheet.swift +++ b/macgit/Views/History/CommitPatchReviewSheet.swift @@ -7,9 +7,12 @@ struct CommitPatchReviewSheet: View { let errorMessage: String? let onCancel: () -> Void let onApply: () -> Void + let onResolve: (UUID, String?) async -> Bool + @State private var resolvingFile: CommitPatchReviewFile? private struct FilePreview: Identifiable { - let file: CommitFileChange + let review: CommitPatchReviewFile + var file: CommitFileChange { review.file } let hunks: [DiffHunk] let metadata: String var id: UUID { file.id } @@ -19,22 +22,23 @@ struct CommitPatchReviewSheet: View { @State private var selectedPreviewID: UUID? init(prepared: PreparedCommitPatch, isBusy: Bool, errorMessage: String?, - onCancel: @escaping () -> Void, onApply: @escaping () -> Void) { + onCancel: @escaping () -> Void, onApply: @escaping () -> Void, + onResolve: @escaping (UUID, String?) async -> Bool) { self.prepared = prepared self.isBusy = isBusy self.errorMessage = errorMessage self.onCancel = onCancel self.onApply = onApply - // Preparation concatenates one complete patch per requested file, in this order. - // Parse each separately so headers from another file never become hunk content. - let patches = prepared.patch.components(separatedBy: "\ndiff --git ") - self.previews = zip(prepared.request.files, patches).map { file, patch in - FilePreview(file: file, hunks: DiffParser.parse(patch), metadata: patch.components(separatedBy: "\n") + self.onResolve = onResolve + self.previews = prepared.reviewFiles.map { review in + FilePreview(review: review, hunks: DiffParser.parse(review.patch), metadata: review.patch.components(separatedBy: "\n") .prefix { !$0.hasPrefix("@@ ") } .filter { $0.hasPrefix("old mode ") || $0.hasPrefix("new mode ") || $0.hasPrefix("new file mode ") || $0.hasPrefix("deleted file mode ") } .joined(separator: "\n")) } - self._selectedPreviewID = State(initialValue: prepared.request.files.first?.id) + self._selectedPreviewID = State(initialValue: prepared.reviewFiles.first(where: { $0.state == .conflict })?.id + ?? prepared.reviewFiles.first?.id) + } private var selectedPreview: FilePreview? { @@ -62,15 +66,38 @@ struct CommitPatchReviewSheet: View { if previews.count > 1 { Picker("File", selection: $selectedPreviewID) { ForEach(previews) { preview in - Text(preview.file.path).tag(Optional(preview.id)) + Text("\(preview.file.path) — \(preview.review.state.rawValue)").tag(Optional(preview.id)) } } } + if prepared.hasConflicts { + Text("Resolve or skip each file marked ‘Needs resolution’ before applying. Nothing has been changed yet.") + .font(.callout).foregroundStyle(.orange) + } else if !prepared.hasChanges { + Text(prepared.reviewFiles.allSatisfy { $0.state == .alreadyApplied } + ? "These changes are already present in your working copy. Nothing needs to be applied." + : "Nothing remains to apply. Your working copy is unchanged.") + .font(.callout).foregroundStyle(.secondary) + } if let preview = selectedPreview { + HStack { + Text(preview.review.state.rawValue) + .font(.callout.weight(.medium)) + .foregroundStyle(preview.review.state == .conflict ? .orange : .secondary) + Spacer() + if preview.review.state == .conflict { + Button(preview.review.conflict?.markedResult == nil ? "Review options…" : "Resolve…") { + resolvingFile = preview.review + } + } + } + if let oldPath = preview.file.oldPath { Text(prepared.request.direction == .apply ? "\(oldPath) → \(preview.file.path)" : "\(preview.file.path) → \(oldPath)") - Text("The rename is included with the selected content changes.") - .font(.caption).foregroundStyle(.secondary) + if preview.review.appliesChanges || preview.review.state == .conflict { + Text("The rename is included with the selected content changes.") + .font(.caption).foregroundStyle(.secondary) + } } else if previews.count == 1 { Text(preview.file.path) } @@ -80,7 +107,10 @@ struct CommitPatchReviewSheet: View { .foregroundStyle(.secondary) } Group { - if preview.hunks.isEmpty { + if !preview.review.appliesChanges && preview.review.state != .conflict { + EmptyStateView(message: preview.review.state == .resolved ? "No changes needed" : preview.review.state.rawValue, + detail: "This file will not be changed.") + } else if preview.hunks.isEmpty { EmptyStateView(message: "File metadata changes", detail: "This file has no changed lines.") } else { DiffView(hunks: preview.hunks, filePath: preview.file.path, prefersTextDiff: true) @@ -95,13 +125,18 @@ struct CommitPatchReviewSheet: View { Spacer() Button("Cancel", action: onCancel) .keyboardShortcut(.cancelAction) - Button(prepared.request.direction.rawValue, action: onApply) + Button(prepared.hasChanges || prepared.hasConflicts ? prepared.request.direction.rawValue : "Done", action: onApply) .keyboardShortcut(.defaultAction) + .disabled(prepared.hasConflicts) } .disabled(isBusy) } .padding(24) .frame(width: 720, height: 560) .interactiveDismissDisabled(isBusy) + .replacingSheet(item: $resolvingFile) { file in + CommitPatchConflictSheet(file: file, isBusy: isBusy, errorMessage: errorMessage, + onCancel: { resolvingFile = nil }, onResolve: { result in await onResolve(file.id, result) }) + } } } diff --git a/macgit/Views/History/HistoryView.swift b/macgit/Views/History/HistoryView.swift index 60037ac..9542644 100644 --- a/macgit/Views/History/HistoryView.swift +++ b/macgit/Views/History/HistoryView.swift @@ -290,13 +290,17 @@ struct HistoryView: View { .replacingSheet(isPresented: $showingRebaseConfirmation) { rebaseConfirmationSheet } - .replacingSheet(item: $commitPatchController.prepared) { prepared in - CommitPatchReviewSheet(prepared: prepared, isBusy: commitPatchController.isBusy, - errorMessage: commitPatchController.reviewError, - onCancel: { commitPatchController.prepared = nil }, - onApply: { - commitPatchController.apply(undoManager: undoManager, syncState: syncState, run: onRunRepositoryOperation) - }) + .replacingSheet(item: $commitPatchController.prepared) { _ in + // Read the live review after each resolution, not the sheet's initial item snapshot. + if let prepared = commitPatchController.prepared { + CommitPatchReviewSheet(prepared: prepared, isBusy: commitPatchController.isBusy, + errorMessage: commitPatchController.reviewError, + onCancel: { commitPatchController.prepared = nil }, + onApply: { + commitPatchController.apply(undoManager: undoManager, syncState: syncState, run: onRunRepositoryOperation) + }, + onResolve: { id, result in await commitPatchController.resolve(fileID: id, result: result) }) + } } .alert("Selected changes", isPresented: $commitPatchController.showingError) { Button("OK", role: .cancel) {} @@ -725,6 +729,15 @@ struct HistoryView: View { VStack(spacing: 0) { // Commit info header commitInfoHeader(for: commit) + if commitPatchController.isPreparing { + HStack(spacing: 8) { + ProgressView().controlSize(.small) + Text("Checking and merging selected changes…").font(.callout) + Spacer() + Button("Cancel") { commitPatchController.cancelPreparation() } + } + .padding(10) + } PersistentHSplit( autosaveName: "HistoryDetailSplit", diff --git a/macgitTests/CommitPatchIntegrationTests.swift b/macgitTests/CommitPatchIntegrationTests.swift index 25bd156..969a636 100644 --- a/macgitTests/CommitPatchIntegrationTests.swift +++ b/macgitTests/CommitPatchIntegrationTests.swift @@ -163,9 +163,10 @@ final class CommitPatchIntegrationTests: XCTestCase { try git(["checkout", "--detach", base], repo) try write("local\n", "b.txt", repo) let files = await service.changedFiles(in: source, in: repo) - await reject { - _ = try await self.service.prepareCommitPatch(.init(commit: source, files: files, direction: .apply, lines: nil, scope: "Files"), in: repo) - } + let review = try await service.prepareCommitPatch(.init(commit: source, files: files, direction: .apply, lines: nil, scope: "Files"), in: repo) + XCTAssertTrue(review.hasConflicts) + XCTAssertEqual(review.reviewFiles.first { $0.file.path == "a.txt" }?.state, .ready) + await reject { try await self.service.applyCommitPatch(review) } XCTAssertEqual(try read("a.txt", repo), "old\n") XCTAssertEqual(try read("b.txt", repo), "local\n") XCTAssertFalse(FileManager.default.fileExists(atPath: repo.appendingPathComponent("a.txt.rej").path)) @@ -370,7 +371,10 @@ final class CommitPatchIntegrationTests: XCTestCase { let source = try commit(repo) try git(["checkout", "--detach", base], repo) try write("local untracked\n", "new.txt", repo) - await reject { _ = try await self.prepare(source, "new.txt", repo) } + let review = try await prepare(source, "new.txt", repo) + XCTAssertTrue(review.hasConflicts) + XCTAssertNil(review.reviewFiles.first?.conflict?.markedResult) + await reject { try await self.service.applyCommitPatch(review) } XCTAssertEqual(try read("new.txt", repo), "local untracked\n") try FileManager.default.removeItem(at: repo.appendingPathComponent("new.txt")) let state = repo.appendingPathComponent(".git/sequencer") @@ -388,6 +392,171 @@ final class CommitPatchIntegrationTests: XCTestCase { lines: lines, scope: lines == nil ? "File" : "Lines"), in: repo) } + func testAlreadyPresentBatchIsANoopAndMixedBatchAppliesOnlyMissingChanges() async throws { + let repo = try repository() + try write("old a\n", "a.txt", repo); try write("old b\n", "b.txt", repo) + let base = try commit(repo) + try write("new a\n", "a.txt", repo); try write("new b\n", "b.txt", repo) + let source = try commit(repo) + let files = await service.changedFiles(in: source, in: repo) + let request = CommitPatchRequest(commit: source, files: files, direction: .apply, lines: nil, scope: "Files") + let index = try git(["ls-files", "--stage"], repo) + let already = try await service.prepareCommitPatch(request, in: repo) + XCTAssertFalse(already.hasConflicts) + XCTAssertFalse(already.hasChanges) + XCTAssertTrue(already.reviewFiles.allSatisfy { $0.state == .alreadyApplied }) + try await service.applyCommitPatch(already) + XCTAssertEqual(try git(["ls-files", "--stage"], repo), index) + try git(["checkout", "--detach", base], repo) + try write("new a\n", "a.txt", repo) + let mixed = try await service.prepareCommitPatch(request, in: repo) + XCTAssertEqual(mixed.reviewFiles.first { $0.file.path == "a.txt" }?.state, .alreadyApplied) + XCTAssertEqual(mixed.reviewFiles.first { $0.file.path == "b.txt" }?.state, .ready) + try await service.applyCommitPatch(mixed) + XCTAssertEqual(try read("a.txt", repo), "new a\n") + XCTAssertEqual(try read("b.txt", repo), "new b\n") + try await service.applyCheckedWorkingTreePatch(mixed.patch, reverse: true, in: repo) + XCTAssertEqual(try read("a.txt", repo), "new a\n") + XCTAssertEqual(try read("b.txt", repo), "old b\n") + } + + func testThreeWayMergePreservesDirtyContextAndOnlySelectedLines() async throws { + let repo = try repository() + let original = (1...20).map { "line\($0)" }.joined(separator: "\n") + "\n" + try write(original, "a.txt", repo) + let base = try commit(repo) + try write(original.replacingOccurrences(of: "line5\n", with: "selected5\n") + .replacingOccurrences(of: "line15\n", with: "unselected15\n"), "a.txt", repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + let local = original.replacingOccurrences(of: "line2\n", with: "local2\n") + try write(local, "a.txt", repo) + try git(["add", "a.txt"], repo) + let staged = try git(["ls-files", "--stage"], repo) + let review = try await prepare(source, "a.txt", repo, lines: [.init(old: 5, new: nil), .init(old: nil, new: 5)]) + XCTAssertEqual(review.reviewFiles.first?.state, .merged) + XCTAssertEqual(try read("a.txt", repo), local, "Preparation/Cancel must not change files") + XCTAssertEqual(try git(["ls-files", "--stage"], repo), staged) + try await service.applyCommitPatch(review) + XCTAssertEqual(try read("a.txt", repo), local.replacingOccurrences(of: "line5\n", with: "selected5\n")) + XCTAssertEqual(try git(["ls-files", "--stage"], repo), staged) + try await service.applyCheckedWorkingTreePatch(review.patch, reverse: true, in: repo) + XCTAssertEqual(try read("a.txt", repo), local) + } + + func testReverseThreeWayMergePreservesDirtyContext() async throws { + let repo = try repository() + let original = (1...20).map { "line\($0)" }.joined(separator: "\n") + "\n" + try write(original, "a.txt", repo); _ = try commit(repo) + let changed = original.replacingOccurrences(of: "line5\n", with: "selected5\n") + try write(changed, "a.txt", repo) + let source = try commit(repo) + try write(changed.replacingOccurrences(of: "line2\n", with: "local2\n"), "a.txt", repo) + let review = try await prepare(source, "a.txt", repo, direction: .revert) + XCTAssertEqual(review.reviewFiles.first?.state, .merged) + try await service.applyCommitPatch(review) + XCTAssertEqual(try read("a.txt", repo), original.replacingOccurrences(of: "line2\n", with: "local2\n")) + } + + func testConflictResolutionStaysInPreviewUntilFinalApplyAndUndoRestoresLocalEdits() async throws { + let repo = try repository() + let original = (1...20).map { "line\($0)" }.joined(separator: "\n") + "\n" + try write(original, "a.txt", repo) + let base = try commit(repo) + try write(original.replacingOccurrences(of: "line5\n", with: "selected5\n") + .replacingOccurrences(of: "line15\n", with: "selected15\n"), "a.txt", repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + let local = original.replacingOccurrences(of: "line5\n", with: "local5\n") + try write(local, "a.txt", repo) + let index = try git(["ls-files", "--stage"], repo) + let review = try await prepare(source, "a.txt", repo) + let conflictFile = try XCTUnwrap(review.reviewFiles.first) + let marked = try XCTUnwrap(conflictFile.conflict?.markedResult) + XCTAssertTrue(review.hasConflicts) + await reject { try await self.service.applyCommitPatch(review) } + await reject { _ = try await self.service.resolveCommitPatch(review, fileID: conflictFile.id, result: marked) } + var document = try ConflictResolutionDocument.parse(marked) + document.selectAllConflicts(.current) + let resolved = try await service.resolveCommitPatch(review, fileID: conflictFile.id, result: document.resolvedText) + XCTAssertFalse(resolved.hasConflicts) + XCTAssertEqual(try read("a.txt", repo), local) + XCTAssertEqual(try git(["ls-files", "--stage"], repo), index) + try await service.applyCommitPatch(resolved) + XCTAssertEqual(try read("a.txt", repo), local.replacingOccurrences(of: "line15\n", with: "selected15\n")) + XCTAssertEqual(try git(["ls-files", "--stage"], repo), index) + try await service.applyCheckedWorkingTreePatch(resolved.patch, reverse: true, in: repo) + XCTAssertEqual(try read("a.txt", repo), local) + } + + func testSkippingConflictAppliesOtherFilesWithoutTouchingSkippedFile() async throws { + let repo = try repository() + try write("old\n", "a.txt", repo); try write("old\n", "b.txt", repo) + let base = try commit(repo) + try write("new\n", "a.txt", repo); try write("new\n", "b.txt", repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + try write("local\n", "b.txt", repo) + let files = await service.changedFiles(in: source, in: repo) + let review = try await service.prepareCommitPatch(.init(commit: source, files: files, direction: .apply, lines: nil, scope: "Files"), in: repo) + let conflict = try XCTUnwrap(review.reviewFiles.first { $0.file.path == "b.txt" }) + let skipped = try await service.resolveCommitPatch(review, fileID: conflict.id, result: nil) + XCTAssertFalse(skipped.hasConflicts) + try await service.applyCommitPatch(skipped) + XCTAssertEqual(try read("a.txt", repo), "new\n") + XCTAssertEqual(try read("b.txt", repo), "local\n") + } + + func testChangedWorkingCopyRejectsResolutionAndFinalMergedApply() async throws { + let repo = try repository() + try write("old\n", "a.txt", repo) + let base = try commit(repo) + try write("new\n", "a.txt", repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + try write("local\n", "a.txt", repo) + let review = try await prepare(source, "a.txt", repo) + let file = try XCTUnwrap(review.reviewFiles.first) + let resolved = try await service.resolveCommitPatch(review, fileID: file.id, result: "resolved\n") + try write("newer edit\n", "a.txt", repo) + await reject { _ = try await self.service.resolveCommitPatch(review, fileID: file.id, result: "resolved\n") } + await reject { try await self.service.applyCommitPatch(resolved) } + XCTAssertEqual(try read("a.txt", repo), "newer edit\n") + } + + func testAlreadyPresentPartialChangeWithDifferentContextIsRecognizedByMerge() async throws { + let repo = try repository() + let original = (1...20).map { "line\($0)" }.joined(separator: "\n") + "\n" + try write(original, "a.txt", repo); _ = try commit(repo) + let selected = original.replacingOccurrences(of: "line5\n", with: "selected5\n") + try write(selected, "a.txt", repo) + let source = try commit(repo) + let local = selected.replacingOccurrences(of: "line2\n", with: "local2\n") + try write(local, "a.txt", repo) + let review = try await prepare(source, "a.txt", repo, lines: [.init(old: 5, new: nil), .init(old: nil, new: 5)]) + XCTAssertEqual(review.reviewFiles.first?.state, .alreadyApplied) + XCTAssertFalse(review.hasChanges) + XCTAssertEqual(try read("a.txt", repo), local) + } + + func testCRLFConflictUsesExistingResolutionDocumentWithoutLosingLines() async throws { + let repo = try repository() + try git(["config", "core.autocrlf", "false"], repo) + try write("before\r\nold\r\nafter\r\n", "a.txt", repo) + let base = try commit(repo) + try write("before\r\nnew\r\nafter\r\n", "a.txt", repo) + let source = try commit(repo) + try git(["checkout", "--detach", base], repo) + try write("before\r\nlocal\r\nafter\r\n", "a.txt", repo) + let review = try await prepare(source, "a.txt", repo) + let file = try XCTUnwrap(review.reviewFiles.first) + var document = try ConflictResolutionDocument.parse(try XCTUnwrap(file.conflict?.markedResult)) + document.selectAllConflicts(.incoming) + let resolved = try await service.resolveCommitPatch(review, fileID: file.id, result: document.resolvedText) + try await service.applyCommitPatch(resolved) + XCTAssertEqual(try read("a.txt", repo), "before\r\nnew\r\nafter\r\n") + } + private func reject(_ action: () async throws -> Void, file: StaticString = #filePath, line: UInt = #line) async { do { try await action(); XCTFail("Expected rejection", file: file, line: line) } catch { /* Rejection must leave the working copy intact; each caller asserts its state. */ } From a431a03b8180b73d93b3dcc50b0447878bcc97d2 Mon Sep 17 00:00:00 2001 From: Thanh Tran Date: Sat, 26 Sep 2026 09:28:26 +0700 Subject: [PATCH 4/8] Keep selected changes preview available for already-present and skipped files --- macgit/Services/GitStatusService+CommitPatchMerge.swift | 8 ++++---- macgit/Views/History/CommitPatchReviewSheet.swift | 9 ++++++--- macgitTests/CommitPatchIntegrationTests.swift | 4 ++++ 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/macgit/Services/GitStatusService+CommitPatchMerge.swift b/macgit/Services/GitStatusService+CommitPatchMerge.swift index 655f1b6..415a2b4 100644 --- a/macgit/Services/GitStatusService+CommitPatchMerge.swift +++ b/macgit/Services/GitStatusService+CommitPatchMerge.swift @@ -12,7 +12,7 @@ extension GitStatusService { // Check each file independently: a batch can contain both new and already-present changes. do { try await runCommitPatch(patch, checkOnly: true, reverse: true, in: repositoryURL) - return CommitPatchReviewFile(file: file, patch: "", state: .alreadyApplied) + return CommitPatchReviewFile(file: file, patch: patch, state: .alreadyApplied) } catch { try Task.checkCancellation() } let url = repositoryURL.appendingPathComponent(file.path) @@ -43,7 +43,7 @@ extension GitStatusService { throw GitError.commandFailed("Could not decode the selected changes in \(file.path).") } if currentData == selectedData { - return CommitPatchReviewFile(file: file, patch: "", state: .alreadyApplied) + return CommitPatchReviewFile(file: file, patch: patch, state: .alreadyApplied) } guard !CommitPatchReviewFile.containsConflictMarkers(current), !CommitPatchReviewFile.containsConflictMarkers(selected), @@ -72,7 +72,7 @@ extension GitStatusService { current: current, selected: selected, markedResult: marked, permissions: permissions)) } let merged = try String(contentsOf: oursURL, encoding: .utf8) - if merged == current { return CommitPatchReviewFile(file: file, patch: "", state: .alreadyApplied) } + if merged == current { return CommitPatchReviewFile(file: file, patch: patch, state: .alreadyApplied) } let mergedPatch = try await commitPatchResultDiff(path: file.path, current: current, result: merged, permissions: permissions) try await runCommitPatch(mergedPatch, checkOnly: true, reverse: false, in: repositoryURL) @@ -101,7 +101,7 @@ extension GitStatusService { updated.reviewFiles[index].patch = patch updated.reviewFiles[index].state = .resolved } else { - updated.reviewFiles[index].patch = "" + // Keep the selected patch available for preview; skipped files are excluded by rebuildPatch(). updated.reviewFiles[index].state = .skipped } updated.reviewFiles[index].conflict = nil diff --git a/macgit/Views/History/CommitPatchReviewSheet.swift b/macgit/Views/History/CommitPatchReviewSheet.swift index 8888d86..0277c19 100644 --- a/macgit/Views/History/CommitPatchReviewSheet.swift +++ b/macgit/Views/History/CommitPatchReviewSheet.swift @@ -106,10 +106,13 @@ struct CommitPatchReviewSheet: View { .font(.caption.monospaced()) .foregroundStyle(.secondary) } + if preview.review.state == .alreadyApplied || preview.review.state == .skipped { + Text("Preview of the selected changes. This file will not be changed.") + .font(.caption).foregroundStyle(.secondary) + } Group { - if !preview.review.appliesChanges && preview.review.state != .conflict { - EmptyStateView(message: preview.review.state == .resolved ? "No changes needed" : preview.review.state.rawValue, - detail: "This file will not be changed.") + if preview.review.state == .resolved && preview.review.patch.isEmpty { + EmptyStateView(message: "No changes needed", detail: "This file will not be changed.") } else if preview.hunks.isEmpty { EmptyStateView(message: "File metadata changes", detail: "This file has no changed lines.") } else { diff --git a/macgitTests/CommitPatchIntegrationTests.swift b/macgitTests/CommitPatchIntegrationTests.swift index 969a636..7eb453a 100644 --- a/macgitTests/CommitPatchIntegrationTests.swift +++ b/macgitTests/CommitPatchIntegrationTests.swift @@ -405,6 +405,8 @@ final class CommitPatchIntegrationTests: XCTestCase { XCTAssertFalse(already.hasConflicts) XCTAssertFalse(already.hasChanges) XCTAssertTrue(already.reviewFiles.allSatisfy { $0.state == .alreadyApplied }) + XCTAssertTrue(already.reviewFiles.allSatisfy { !DiffParser.parse($0.patch).isEmpty }, "Already-present changes remain previewable") + XCTAssertTrue(already.patch.isEmpty, "Preview patches must not be applied again") try await service.applyCommitPatch(already) XCTAssertEqual(try git(["ls-files", "--stage"], repo), index) try git(["checkout", "--detach", base], repo) @@ -502,6 +504,8 @@ final class CommitPatchIntegrationTests: XCTestCase { let conflict = try XCTUnwrap(review.reviewFiles.first { $0.file.path == "b.txt" }) let skipped = try await service.resolveCommitPatch(review, fileID: conflict.id, result: nil) XCTAssertFalse(skipped.hasConflicts) + XCTAssertFalse(try XCTUnwrap(skipped.reviewFiles.first { $0.id == conflict.id }).patch.isEmpty, + "Skipped changes remain previewable") try await service.applyCommitPatch(skipped) XCTAssertEqual(try read("a.txt", repo), "new\n") XCTAssertEqual(try read("b.txt", repo), "local\n") From 660137441e8c745f36f2c33c93f70f94be20bc83 Mon Sep 17 00:00:00 2001 From: Thanh Tran Date: Sat, 26 Sep 2026 14:51:09 +0700 Subject: [PATCH 5/8] feat: add description explain why cannot apply --- .../CommitPatchConflictWindowController.swift | 67 +++++++++++++++++++ macgit/App/CommitPatchController.swift | 49 +++++++++++++- macgit/Models/CommitPatchReviewFile.swift | 21 ++++++ .../GitStatusService+CommitPatchMerge.swift | 10 ++- macgit/Services/GitStatusService+Remote.swift | 1 + macgit/Services/SyncState.swift | 27 ++++++-- .../History/CommitPatchConflictSheet.swift | 49 +++++++++++--- .../History/CommitPatchReviewSheet.swift | 21 +++--- macgit/Views/History/HistoryView.swift | 4 +- .../MainWindowView+CheckoutActions.swift | 2 +- macgit/Views/MainWindow/MainWindowView.swift | 3 + .../MainWindow/Sidebar/SidebarBranchRow.swift | 25 +++---- .../MainWindow/Sidebar/SidebarRemoteRow.swift | 4 +- ...ce.swift => SidebarBranchDragSource.swift} | 34 ++++++---- .../SidebarView+BranchActions.swift | 36 ++-------- macgit/Views/MainWindow/SidebarView.swift | 25 ++++--- macgitTests/CommitPatchIntegrationTests.swift | 9 ++- macgitTests/GitSubmoduleLifecycleTests.swift | 1 + 18 files changed, 285 insertions(+), 103 deletions(-) create mode 100644 macgit/App/CommitPatchConflictWindowController.swift rename macgit/Views/MainWindow/{SidebarRemoteBranchDragSource.swift => SidebarBranchDragSource.swift} (84%) diff --git a/macgit/App/CommitPatchConflictWindowController.swift b/macgit/App/CommitPatchConflictWindowController.swift new file mode 100644 index 0000000..7c835e5 --- /dev/null +++ b/macgit/App/CommitPatchConflictWindowController.swift @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +import AppKit +import SwiftUI + +@MainActor +final class CommitPatchConflictWindowController: NSWindowController, NSWindowDelegate { + private weak var controller: CommitPatchController? + + init() { super.init(window: nil) } + + required init?(coder: NSCoder) { + fatalError("init(coder:) has not been implemented") + } + + func show(file: CommitPatchReviewFile, controller: CommitPatchController) { + self.controller = controller + let visibleFrame = (NSApp.keyWindow?.screen ?? NSScreen.main)?.visibleFrame + ?? NSRect(x: 0, y: 0, width: 1200, height: 800) + let size = NSSize(width: min(1100, visibleFrame.width - 40), + height: min(760, visibleFrame.height - 80)) + let window = NSWindow(contentRect: NSRect(origin: .zero, size: size), + styleMask: [.titled, .closable, .miniaturizable, .resizable], backing: .buffered, defer: false) + window.title = "\(file.reviewTitle) — \(file.file.path)" + window.isReleasedWhenClosed = false + window.tabbingMode = .disallowed + window.contentMinSize = NSSize(width: min(700, size.width), height: min(500, size.height)) + window.delegate = self + let hostingView = NSHostingView(rootView: GeometryReader { geometry in + CommitPatchConflictWindowContent(file: file, controller: controller) + .frame(width: geometry.size.width, height: geometry.size.height) + }) + // The window owns the viewport, including for long lines and large diffs. + hostingView.sizingOptions = [] + window.contentView = hostingView + window.setContentSize(size) + window.setFrameOrigin(NSPoint(x: visibleFrame.midX - window.frame.width / 2, + y: visibleFrame.midY - window.frame.height / 2)) + self.window = window + showWindow(nil) + window.makeKeyAndOrderFront(nil) + } + + func windowShouldClose(_ sender: NSWindow) -> Bool { + controller?.isBusy != true + } + + func windowWillClose(_ notification: Notification) { + // Release the hosted observation/callbacks before notifying the owner. + window?.contentView = nil + window = nil + let owner = controller + controller = nil + owner?.closeConflict() + } +} + +private struct CommitPatchConflictWindowContent: View { + let file: CommitPatchReviewFile + let controller: CommitPatchController + + var body: some View { + CommitPatchConflictSheet(file: file, isBusy: controller.isBusy, errorMessage: controller.reviewError, + onCancel: { controller.closeConflict() }, + onSkip: { controller.skip(fileID: file.id) }, + onResolve: { result in controller.resolveAndClose(fileID: file.id, result: result) }) + } +} diff --git a/macgit/App/CommitPatchController.swift b/macgit/App/CommitPatchController.swift index 239cd17..887ea9b 100644 --- a/macgit/App/CommitPatchController.swift +++ b/macgit/App/CommitPatchController.swift @@ -1,10 +1,36 @@ // SPDX-License-Identifier: AGPL-3.0-or-later +import AppKit import Foundation import Observation @MainActor @Observable final class CommitPatchController { - var prepared: PreparedCommitPatch? + var prepared: PreparedCommitPatch? { + didSet { if prepared == nil { closeConflict() } } + } + private(set) var isResolving = false + @ObservationIgnored private var conflictWindow: CommitPatchConflictWindowController? + + func openConflict(_ file: CommitPatchReviewFile) { + guard !isBusy, prepared?.reviewFiles.contains(where: { $0.id == file.id && $0.state == .conflict }) == true else { return } + if let conflictWindow { + conflictWindow.showWindow(nil) + conflictWindow.window?.makeKeyAndOrderFront(nil) + return + } + let window = CommitPatchConflictWindowController() + conflictWindow = window + isResolving = true + window.show(file: file, controller: self) + } + + func closeConflict() { + let window = conflictWindow + conflictWindow = nil + isResolving = false + window?.close() + } + private(set) var isBusy = false private(set) var isPreparing = false @ObservationIgnored private var preparationTask: Task? @@ -78,6 +104,27 @@ final class CommitPatchController { } } + /// Skipping only changes the preview; Apply revalidates the repository before writing. + func skip(fileID: UUID) { + guard !isBusy, var updated = prepared, + let index = updated.reviewFiles.firstIndex(where: { $0.id == fileID && $0.state == .conflict }) else { return } + updated.reviewFiles[index].state = .skipped + updated.reviewFiles[index].conflict = nil + updated.rebuildPatch() + reviewError = nil + prepared = updated + closeConflict() + } + + func resolveAndClose(fileID: UUID, result: String) { + guard !isBusy else { return } + Task { [self] in + if await resolve(fileID: fileID, result: result) { + closeConflict() + } + } + } + func resolve(fileID: UUID, result: String?) async -> Bool { guard let prepared, !isBusy else { return false } isBusy = true diff --git a/macgit/Models/CommitPatchReviewFile.swift b/macgit/Models/CommitPatchReviewFile.swift index 553a4b3..4f50e0e 100644 --- a/macgit/Models/CommitPatchReviewFile.swift +++ b/macgit/Models/CommitPatchReviewFile.swift @@ -18,6 +18,14 @@ nonisolated struct CommitPatchReviewFile: Identifiable, Sendable { var id: UUID { file.id } struct Conflict: Sendable { + enum Kind: Sendable, Equatable { + case missingFile + case unsupportedChange + case existingMarkers + case overlappingEdits + } + + let kind: Kind let message: String let current: String let selected: String @@ -26,6 +34,19 @@ nonisolated struct CommitPatchReviewFile: Identifiable, Sendable { let permissions: Int } + var reviewTitle: String { + conflict?.markedResult == nil ? "Review Selected Changes" : "Resolve Text Conflict" + } + + var displayState: String { + guard state == .conflict, let conflict else { return state.rawValue } + switch conflict.kind { + case .missingFile: return "File missing" + case .unsupportedChange: return "Cannot apply safely" + case .existingMarkers, .overlappingEdits: return "Needs resolution" + } + } + var appliesChanges: Bool { state != .conflict && state != .skipped && state != .alreadyApplied && !patch.isEmpty } diff --git a/macgit/Services/GitStatusService+CommitPatchMerge.swift b/macgit/Services/GitStatusService+CommitPatchMerge.swift index 415a2b4..e0c5cfb 100644 --- a/macgit/Services/GitStatusService+CommitPatchMerge.swift +++ b/macgit/Services/GitStatusService+CommitPatchMerge.swift @@ -16,11 +16,15 @@ extension GitStatusService { } catch { try Task.checkCancellation() } let url = repositoryURL.appendingPathComponent(file.path) + let fileExists = FileManager.default.fileExists(atPath: url.path) guard file.status == .modified, file.oldPath == nil, - FileManager.default.fileExists(atPath: url.path), + fileExists, !patch.components(separatedBy: "\n").contains(where: { $0.hasPrefix("old mode ") || $0.hasPrefix("new mode ") }) else { return CommitPatchReviewFile(file: file, patch: patch, state: .conflict, conflict: .init( - message: "This change adds, deletes, renames, or changes the permissions of a file whose current state is different. Commit+ will not overwrite it automatically. Skip this file to apply the others, or cancel and review its current state.", + kind: !fileExists && file.status == .modified && file.oldPath == nil ? .missingFile : .unsupportedChange, + message: !fileExists && file.status == .modified && file.oldPath == nil + ? "This file is missing from your working copy, so these edits cannot be merged safely. Review the selected changes below, then skip this file or cancel and restore the file before trying again." + : "The file's current state does not match this addition, deletion, rename, or permission change. Commit+ will not overwrite it automatically. Review the selected changes below, then skip this file or cancel and review its current state.", current: "", selected: "", markedResult: nil, permissions: 0)) } let currentData = try Data(contentsOf: url) @@ -49,6 +53,7 @@ extension GitStatusService { !CommitPatchReviewFile.containsConflictMarkers(selected), !CommitPatchReviewFile.containsConflictMarkers(String(decoding: baseData, as: UTF8.self)) else { return CommitPatchReviewFile(file: file, patch: patch, state: .conflict, conflict: .init( + kind: .existingMarkers, message: "This file already contains conflict markers. Skip it or cancel and resolve those markers first.", current: current, selected: selected, markedResult: nil, permissions: permissions)) } @@ -68,6 +73,7 @@ extension GitStatusService { let markerLines = marked.components(separatedBy: "\n").map { $0.trimmingCharacters(in: .newlines) } guard markerLines.contains("<<<<<<< Your working copy"), markerLines.contains(">>>>>>> Selected changes") else { throw error } return CommitPatchReviewFile(file: file, patch: patch, state: .conflict, conflict: .init( + kind: .overlappingEdits, message: "Your working copy and the selected changes edit the same lines. Resolve the result in a temporary preview; no files will be written until you click Apply.", current: current, selected: selected, markedResult: marked, permissions: permissions)) } diff --git a/macgit/Services/GitStatusService+Remote.swift b/macgit/Services/GitStatusService+Remote.swift index 8e16609..01d3047 100644 --- a/macgit/Services/GitStatusService+Remote.swift +++ b/macgit/Services/GitStatusService+Remote.swift @@ -359,6 +359,7 @@ extension GitStatusService { arguments.append("\(trimmedRemote)/\(trimmedBranch)") } _ = try await runGit(arguments: arguments, in: repositoryURL) + await invalidateBranchListCache(in: repositoryURL) return trimmedLocalBranch } diff --git a/macgit/Services/SyncState.swift b/macgit/Services/SyncState.swift index 1fe3f9a..cb5c1e8 100644 --- a/macgit/Services/SyncState.swift +++ b/macgit/Services/SyncState.swift @@ -35,15 +35,22 @@ extension Notification.Name { private actor SyncRefreshCoordinator { private var localRefreshInFlight = false private var lastLocalRefreshDate: Date? + private var queuedForcedRefreshes: [URL] = [] private var automaticFetchInFlight = false private var lastAutomaticFetchDate = Date.distantPast func beginLocalRefresh( force: Bool, minimumInterval: TimeInterval, - now: Date + now: Date, + repositoryURL: URL ) -> Bool { - guard !localRefreshInFlight else { return false } + guard !localRefreshInFlight else { + if force, !queuedForcedRefreshes.contains(repositoryURL) { + queuedForcedRefreshes.append(repositoryURL) + } + return false + } if !force, let lastLocalRefreshDate, now.timeIntervalSince(lastLocalRefreshDate) < minimumInterval { @@ -54,9 +61,11 @@ private actor SyncRefreshCoordinator { return true } - func finishLocalRefresh(at date: Date) { + func finishLocalRefresh(at date: Date) -> [URL] { localRefreshInFlight = false lastLocalRefreshDate = date + defer { queuedForcedRefreshes.removeAll() } + return queuedForcedRefreshes } func beginAutomaticFetch( @@ -130,7 +139,8 @@ class SyncState: ObservableObject { guard await refreshCoordinator.beginLocalRefresh( force: force, minimumInterval: Self.localRefreshCoalescingInterval, - now: .now + now: .now, + repositoryURL: repositoryURL ) else { return } @@ -173,7 +183,14 @@ class SyncState: ObservableObject { ) } - await refreshCoordinator.finishLocalRefresh(at: .now) + let queuedRefreshes = await refreshCoordinator.finishLocalRefresh(at: .now) + if !queuedRefreshes.isEmpty { + Task { + for queuedRepositoryURL in queuedRefreshes { + await refresh(repositoryURL: queuedRepositoryURL) + } + } + } } func startBackgroundSync( diff --git a/macgit/Views/History/CommitPatchConflictSheet.swift b/macgit/Views/History/CommitPatchConflictSheet.swift index 53d617e..6e7259b 100644 --- a/macgit/Views/History/CommitPatchConflictSheet.swift +++ b/macgit/Views/History/CommitPatchConflictSheet.swift @@ -7,7 +7,8 @@ struct CommitPatchConflictSheet: View { let isBusy: Bool let errorMessage: String? let onCancel: () -> Void - let onResolve: (String?) async -> Bool + let onSkip: () -> Void + let onResolve: (String) -> Void @State private var result: String @State private var scrollController = SyncedScrollController() @State private var undoResetGeneration = 0 @@ -16,18 +17,19 @@ struct CommitPatchConflictSheet: View { @State private var redoResults: [String] = [] init(file: CommitPatchReviewFile, isBusy: Bool, errorMessage: String?, onCancel: @escaping () -> Void, - onResolve: @escaping (String?) async -> Bool) { + onSkip: @escaping () -> Void, onResolve: @escaping (String) -> Void) { self.file = file self.isBusy = isBusy self.errorMessage = errorMessage self.onCancel = onCancel + self.onSkip = onSkip self.onResolve = onResolve self._result = State(initialValue: file.conflict?.markedResult ?? "") } var body: some View { VStack(alignment: .leading, spacing: 12) { - Text("Resolve Selected Changes").font(.title2.bold()) + Text(file.reviewTitle).font(.title2.bold()) Text(file.file.path).font(.callout.monospaced()).textSelection(.enabled) if let conflict = file.conflict { Text(conflict.message).foregroundStyle(.secondary) @@ -49,7 +51,37 @@ struct CommitPatchConflictSheet: View { baselineText: conflict.current, isDisabled: isBusy, undoResetGeneration: undoResetGeneration, scrollController: scrollController) .frame(maxHeight: .infinity) - } else { Spacer() } + } else { + if conflict.kind == .missingFile { + HStack(alignment: .top, spacing: 12) { + Image(systemName: "doc.badge.questionmark") + .font(.title2) + .foregroundStyle(.secondary) + VStack(alignment: .leading, spacing: 6) { + Text("File isn’t on this branch") + .font(.headline) + Text("The commit changes an existing file, but this branch has no copy of it. The green lines below are only the changes from the commit; they aren’t a merge conflict. Apply Selected Changes can add changes to an existing file, but it can’t recreate the missing file from those lines alone.") + .foregroundStyle(.secondary) + .fixedSize(horizontal: false, vertical: true) + .frame(maxWidth: .infinity, alignment: .leading) + } + } + .frame(maxWidth: .infinity, alignment: .leading) + .padding(14) + .background(.quaternary.opacity(0.35), in: RoundedRectangle(cornerRadius: 8)) + } else { + Text("Changes from the source commit · This is not a conflict diff") + .font(.caption).foregroundStyle(.secondary) + } + if DiffParser.parse(file.patch).isEmpty { + ScrollView { + Text(file.patch).font(.callout.monospaced()).textSelection(.enabled) + .frame(maxWidth: .infinity, alignment: .leading) + } + } else { + DiffView(hunks: DiffParser.parse(file.patch), filePath: file.file.path, prefersTextDiff: true) + } + } } if let errorMessage { Text(errorMessage).foregroundStyle(.red).font(.callout) } HStack { @@ -57,7 +89,7 @@ struct CommitPatchConflictSheet: View { .font(.caption).foregroundStyle(.secondary) Spacer() Button("Cancel", action: onCancel).keyboardShortcut(.cancelAction) - Button("Skip this file") { finish(nil) } + Button("Skip this file", action: onSkip) if file.conflict?.markedResult != nil { Button("Review Result") { finish(result) } .keyboardShortcut(.defaultAction) @@ -66,9 +98,8 @@ struct CommitPatchConflictSheet: View { } } .padding(20) - .frame(width: 1000, height: 700) + .frame(maxWidth: .infinity, maxHeight: .infinity) .disabled(isBusy) - .interactiveDismissDisabled(isBusy) .background(CommitPatchConflictWindowContext(identifier: commandContext)) .onReceive(NotificationCenter.default.publisher(for: .conflictUndoAction)) { notification in guard !isBusy, notification.userInfo?["commandContext"] as? String == commandContext.rawValue, @@ -109,7 +140,7 @@ struct CommitPatchConflictSheet: View { undoResetGeneration += 1 } - private func finish(_ text: String?) { - Task { if await onResolve(text) { onCancel() } } + private func finish(_ text: String) { + onResolve(text) } } diff --git a/macgit/Views/History/CommitPatchReviewSheet.swift b/macgit/Views/History/CommitPatchReviewSheet.swift index 0277c19..7b31244 100644 --- a/macgit/Views/History/CommitPatchReviewSheet.swift +++ b/macgit/Views/History/CommitPatchReviewSheet.swift @@ -7,8 +7,7 @@ struct CommitPatchReviewSheet: View { let errorMessage: String? let onCancel: () -> Void let onApply: () -> Void - let onResolve: (UUID, String?) async -> Bool - @State private var resolvingFile: CommitPatchReviewFile? + let onOpenConflict: (CommitPatchReviewFile) -> Void private struct FilePreview: Identifiable { let review: CommitPatchReviewFile @@ -23,13 +22,13 @@ struct CommitPatchReviewSheet: View { init(prepared: PreparedCommitPatch, isBusy: Bool, errorMessage: String?, onCancel: @escaping () -> Void, onApply: @escaping () -> Void, - onResolve: @escaping (UUID, String?) async -> Bool) { + onOpenConflict: @escaping (CommitPatchReviewFile) -> Void) { self.prepared = prepared self.isBusy = isBusy self.errorMessage = errorMessage self.onCancel = onCancel self.onApply = onApply - self.onResolve = onResolve + self.onOpenConflict = onOpenConflict self.previews = prepared.reviewFiles.map { review in FilePreview(review: review, hunks: DiffParser.parse(review.patch), metadata: review.patch.components(separatedBy: "\n") .prefix { !$0.hasPrefix("@@ ") } @@ -66,12 +65,12 @@ struct CommitPatchReviewSheet: View { if previews.count > 1 { Picker("File", selection: $selectedPreviewID) { ForEach(previews) { preview in - Text("\(preview.file.path) — \(preview.review.state.rawValue)").tag(Optional(preview.id)) + Text("\(preview.file.path) — \(preview.review.displayState)").tag(Optional(preview.id)) } } } if prepared.hasConflicts { - Text("Resolve or skip each file marked ‘Needs resolution’ before applying. Nothing has been changed yet.") + Text("Some selected changes need attention. Resolve overlapping edits, or review files that are missing or cannot be changed safely. Nothing has been changed yet.") .font(.callout).foregroundStyle(.orange) } else if !prepared.hasChanges { Text(prepared.reviewFiles.allSatisfy { $0.state == .alreadyApplied } @@ -81,13 +80,13 @@ struct CommitPatchReviewSheet: View { } if let preview = selectedPreview { HStack { - Text(preview.review.state.rawValue) + Text(preview.review.displayState) .font(.callout.weight(.medium)) .foregroundStyle(preview.review.state == .conflict ? .orange : .secondary) Spacer() if preview.review.state == .conflict { - Button(preview.review.conflict?.markedResult == nil ? "Review options…" : "Resolve…") { - resolvingFile = preview.review + Button(preview.review.conflict?.markedResult == nil ? "Why can’t I apply this?" : "Resolve…") { + onOpenConflict(preview.review) } } } @@ -137,9 +136,5 @@ struct CommitPatchReviewSheet: View { .padding(24) .frame(width: 720, height: 560) .interactiveDismissDisabled(isBusy) - .replacingSheet(item: $resolvingFile) { file in - CommitPatchConflictSheet(file: file, isBusy: isBusy, errorMessage: errorMessage, - onCancel: { resolvingFile = nil }, onResolve: { result in await onResolve(file.id, result) }) - } } } diff --git a/macgit/Views/History/HistoryView.swift b/macgit/Views/History/HistoryView.swift index 9542644..7125465 100644 --- a/macgit/Views/History/HistoryView.swift +++ b/macgit/Views/History/HistoryView.swift @@ -299,7 +299,9 @@ struct HistoryView: View { onApply: { commitPatchController.apply(undoManager: undoManager, syncState: syncState, run: onRunRepositoryOperation) }, - onResolve: { id, result in await commitPatchController.resolve(fileID: id, result: result) }) + onOpenConflict: { commitPatchController.openConflict($0) }) + .disabled(commitPatchController.isResolving) + .onDisappear { commitPatchController.closeConflict() } } } .alert("Selected changes", isPresented: $commitPatchController.showingError) { diff --git a/macgit/Views/MainWindow/MainWindowView+CheckoutActions.swift b/macgit/Views/MainWindow/MainWindowView+CheckoutActions.swift index 33aab52..333c14f 100644 --- a/macgit/Views/MainWindow/MainWindowView+CheckoutActions.swift +++ b/macgit/Views/MainWindow/MainWindowView+CheckoutActions.swift @@ -88,7 +88,7 @@ extension MainWindowView { NotificationCenter.default.post( name: .repositoryDidChange, object: nil, - userInfo: ["repositoryURL": repositoryURL] + userInfo: ["repositoryURL": repositoryURL, "checkedOutBranch": checkedOutBranch] ) } catch { syncState.showError(error.localizedDescription) diff --git a/macgit/Views/MainWindow/MainWindowView.swift b/macgit/Views/MainWindow/MainWindowView.swift index 1a7d31c..e2c7242 100644 --- a/macgit/Views/MainWindow/MainWindowView.swift +++ b/macgit/Views/MainWindow/MainWindowView.swift @@ -816,6 +816,9 @@ struct MainWindowView: View { showingCheckoutConfirmation = true } }, + onRequestRemoteBranchCheckout: { target in + pendingRemoteBranchCheckout = target + }, onRequestFetchBranch: { branch in Task { let remote = await trackedRemote(for: branch) diff --git a/macgit/Views/MainWindow/Sidebar/SidebarBranchRow.swift b/macgit/Views/MainWindow/Sidebar/SidebarBranchRow.swift index 9ab1285..c847f62 100644 --- a/macgit/Views/MainWindow/Sidebar/SidebarBranchRow.swift +++ b/macgit/Views/MainWindow/Sidebar/SidebarBranchRow.swift @@ -62,16 +62,6 @@ struct SidebarBranchRow: View { private var branchLeafRow: some View { let rowView = content .tag(SidebarSelection.branch(row.fullPath)) - .onTapGesture { - actions.select(.branch(row.fullPath)) - } - .simultaneousGesture( - TapGesture(count: 2).onEnded { - if !isCurrentBranch { - actions.checkout(row.fullPath) - } - } - ) .contextMenu { SidebarBranchContextMenu( branch: row.fullPath, @@ -83,12 +73,6 @@ struct SidebarBranchRow: View { actions: actions ) } - .contentShape(.dragPreview, RoundedRectangle(cornerRadius: 8)) - .onDrag { - actions.makeItemProvider(row.fullPath) - } preview: { - BranchDragPreview(branchName: row.fullPath) - } if isCurrentBranch { rowView @@ -129,6 +113,15 @@ struct SidebarBranchRow: View { } } else { rowView + .overlay { + SidebarBranchDragSource( + onTap: { actions.select(.branch(row.fullPath)) }, + onDoubleTap: { actions.checkout(row.fullPath) }, + dragPayload: { makeBranchPayload(row.fullPath) }, + dragTitle: row.fullPath, + onDragEnded: finishBranchDrag + ) + } } } diff --git a/macgit/Views/MainWindow/Sidebar/SidebarRemoteRow.swift b/macgit/Views/MainWindow/Sidebar/SidebarRemoteRow.swift index c5fc22f..8f87b33 100644 --- a/macgit/Views/MainWindow/Sidebar/SidebarRemoteRow.swift +++ b/macgit/Views/MainWindow/Sidebar/SidebarRemoteRow.swift @@ -58,7 +58,7 @@ struct SidebarRemoteRow: View { } else { rowView .overlay { - SidebarRemoteBranchDragSource( + SidebarBranchDragSource( onTap: { actions.select(.remoteBranch(row.fullPath)) }, @@ -70,7 +70,7 @@ struct SidebarRemoteRow: View { actions.makePayload(row.fullPath) }, dragTitle: row.fullPath, - onDragEnded: { + onDragEnded: { _ in actions.finishDrag(row.fullPath) } ) diff --git a/macgit/Views/MainWindow/SidebarRemoteBranchDragSource.swift b/macgit/Views/MainWindow/SidebarBranchDragSource.swift similarity index 84% rename from macgit/Views/MainWindow/SidebarRemoteBranchDragSource.swift rename to macgit/Views/MainWindow/SidebarBranchDragSource.swift index fd82acd..496bd6c 100644 --- a/macgit/Views/MainWindow/SidebarRemoteBranchDragSource.swift +++ b/macgit/Views/MainWindow/SidebarBranchDragSource.swift @@ -1,5 +1,5 @@ // -// SidebarRemoteBranchDragSource.swift +// SidebarBranchDragSource.swift // macgit // // Created by Thanh Tran on 26/5/26. @@ -26,12 +26,12 @@ import AppKit import SwiftUI -struct SidebarRemoteBranchDragSource: NSViewRepresentable { +struct SidebarBranchDragSource: NSViewRepresentable { let onTap: () -> Void let onDoubleTap: () -> Void let dragPayload: () -> GitDragPayload let dragTitle: String - let onDragEnded: () -> Void + let onDragEnded: (GitDragPayload) -> Void func makeNSView(context: Context) -> DragSourceView { DragSourceView( @@ -56,17 +56,17 @@ struct SidebarRemoteBranchDragSource: NSViewRepresentable { var onDoubleTap: () -> Void var dragPayload: () -> GitDragPayload var dragTitle: String - var onDragEnded: () -> Void + var onDragEnded: (GitDragPayload) -> Void private var dragStartEvent: NSEvent? - private var isDragging = false + private var activeDragPayload: GitDragPayload? init( onTap: @escaping () -> Void, onDoubleTap: @escaping () -> Void, dragPayload: @escaping () -> GitDragPayload, dragTitle: String, - onDragEnded: @escaping () -> Void + onDragEnded: @escaping (GitDragPayload) -> Void ) { self.onTap = onTap self.onDoubleTap = onDoubleTap @@ -83,25 +83,31 @@ struct SidebarRemoteBranchDragSource: NSViewRepresentable { override func mouseDown(with event: NSEvent) { dragStartEvent = event + // AppKit owns double-click timing; selection never waits for a tap recognizer. + onTap() if event.clickCount == 2 { + dragStartEvent = nil onDoubleTap() - } else { - onTap() } } override func mouseDragged(with event: NSEvent) { - guard !isDragging, dragStartEvent != nil else { + guard activeDragPayload == nil, let dragStartEvent else { + return + } + let start = dragStartEvent.locationInWindow + let location = event.locationInWindow + guard hypot(location.x - start.x, location.y - start.y) >= 4 else { return } let payload = dragPayload() guard let item = SidebarBranchDropTarget.DropTargetView.pasteboardItem(for: payload) else { - onDragEnded() + onDragEnded(payload) return } - isDragging = true + activeDragPayload = payload let draggingItem = NSDraggingItem(pasteboardWriter: item) let image = dragImage(title: dragTitle) draggingItem.setDraggingFrame( @@ -133,8 +139,10 @@ struct SidebarRemoteBranchDragSource: NSViewRepresentable { operation: NSDragOperation ) { dragStartEvent = nil - isDragging = false - onDragEnded() + if let activeDragPayload { + onDragEnded(activeDragPayload) + } + activeDragPayload = nil } private func dragImage(title: String) -> NSImage { diff --git a/macgit/Views/MainWindow/SidebarView+BranchActions.swift b/macgit/Views/MainWindow/SidebarView+BranchActions.swift index 4afa479..b26d8f3 100644 --- a/macgit/Views/MainWindow/SidebarView+BranchActions.swift +++ b/macgit/Views/MainWindow/SidebarView+BranchActions.swift @@ -37,38 +37,16 @@ extension SidebarView { } } - func checkoutRemoteBranch(_ fullPath: String) async { + func requestRemoteBranchCheckout(_ fullPath: String) { guard let remoteBranch = remoteBranchParts(from: fullPath) else { - await MainActor.run { - errorMessage = "Could not parse remote branch '\(fullPath)'." - showingError = true - } + errorMessage = "Could not parse remote branch '\(fullPath)'." + showingError = true return } - - do { - let localBranch = try await GitStatusService.shared.checkoutRemoteBranch( - remote: remoteBranch.remote, - branch: remoteBranch.branch, - in: repositoryURL - ) - expandBranchesSection() - await loadBranches(force: true) - await loadRemotes() - await MainActor.run { - selection = .branch(localBranch) - } - NotificationCenter.default.post( - name: .repositoryDidChange, - object: nil, - userInfo: ["repositoryURL": repositoryURL] - ) - } catch { - await MainActor.run { - errorMessage = error.localizedDescription - showingError = true - } - } + guard remoteBranch.branch != "HEAD" else { return } + onRequestRemoteBranchCheckout( + RemoteBranchCheckoutTarget(remote: remoteBranch.remote, branch: remoteBranch.branch) + ) } func deleteRemoteBranch(_ target: RemoteBranchDeleteTarget) async { diff --git a/macgit/Views/MainWindow/SidebarView.swift b/macgit/Views/MainWindow/SidebarView.swift index 596aec9..247012e 100644 --- a/macgit/Views/MainWindow/SidebarView.swift +++ b/macgit/Views/MainWindow/SidebarView.swift @@ -42,6 +42,7 @@ struct SidebarView: View { let isBranchSyncing: (String) -> Bool let canUpdateCurrentBranch: Bool let onRequestCheckout: (String, Bool) -> Void + let onRequestRemoteBranchCheckout: (RemoteBranchCheckoutTarget) -> Void let onRequestFetchBranch: (String) -> Void let onRequestPullRemoteBranch: (String, String) -> Void let onRequestPullTracked: (String) -> Void @@ -202,6 +203,7 @@ struct SidebarView: View { isBranchSyncing: @escaping (String) -> Bool = { _ in false }, canUpdateCurrentBranch: Bool = true, onRequestCheckout: @escaping (String, Bool) -> Void, + onRequestRemoteBranchCheckout: @escaping (RemoteBranchCheckoutTarget) -> Void, onRequestFetchBranch: @escaping (String) -> Void, onRequestPullRemoteBranch: @escaping (String, String) -> Void = { _, _ in }, onRequestPullTracked: @escaping (String) -> Void = { _ in }, @@ -273,6 +275,7 @@ struct SidebarView: View { self.isBranchSyncing = isBranchSyncing self.canUpdateCurrentBranch = canUpdateCurrentBranch self.onRequestCheckout = onRequestCheckout + self.onRequestRemoteBranchCheckout = onRequestRemoteBranchCheckout self.onRequestFetchBranch = onRequestFetchBranch self.onRequestPullRemoteBranch = onRequestPullRemoteBranch self.onRequestPullTracked = onRequestPullTracked @@ -441,16 +444,8 @@ struct SidebarView: View { toggleSection: { toggleSection(.remotes) }, toggleFolder: toggleRemoteFolder, select: { selection = $0 }, - checkoutFromRow: { fullPath in - Task { - await checkoutRemoteBranch(fullPath) - } - }, - checkoutFromContextMenu: { fullPath in - onRunRepositoryOperation("Checking out \(fullPath)...") { - await checkoutRemoteBranch(fullPath) - } - }, + checkoutFromRow: requestRemoteBranchCheckout, + checkoutFromContextMenu: requestRemoteBranchCheckout, pullIntoCurrent: onRequestPullRemoteBranch, confirmDelete: { remoteBranchDeleteTarget = $0 }, createPullRequest: onRequestCreatePullRequestForRemote, @@ -677,8 +672,17 @@ struct SidebarView: View { } .onReceive(NotificationCenter.default.publisher(for: .repositoryDidChange)) { notification in if let url = notification.userInfo?["repositoryURL"] as? URL, url == repositoryURL { + let checkedOutBranch = notification.userInfo?["checkedOutBranch"] as? String + if checkedOutBranch != nil { + expandBranchesSection() + } Task { await loadAllSections(force: true) + if let checkedOutBranch { + expandedFolders.formUnion( + SidebarTreeBuilder.expandedFolderPaths(revealing: checkedOutBranch) + ) + } } } } @@ -870,6 +874,7 @@ struct SidebarView: View { selection: .constant(nil), isBranchSyncing: { _ in false }, onRequestCheckout: { _, _ in }, + onRequestRemoteBranchCheckout: { _ in }, onRequestFetchBranch: { _ in }, onRequestPullTracked: { _ in }, onRequestPushToTracked: { _ in }, diff --git a/macgitTests/CommitPatchIntegrationTests.swift b/macgitTests/CommitPatchIntegrationTests.swift index 7eb453a..baa2e27 100644 --- a/macgitTests/CommitPatchIntegrationTests.swift +++ b/macgitTests/CommitPatchIntegrationTests.swift @@ -502,7 +502,14 @@ final class CommitPatchIntegrationTests: XCTestCase { let files = await service.changedFiles(in: source, in: repo) let review = try await service.prepareCommitPatch(.init(commit: source, files: files, direction: .apply, lines: nil, scope: "Files"), in: repo) let conflict = try XCTUnwrap(review.reviewFiles.first { $0.file.path == "b.txt" }) - let skipped = try await service.resolveCommitPatch(review, fileID: conflict.id, result: nil) + let skipped = try await MainActor.run { + let controller = CommitPatchController() + controller.prepared = review + controller.skip(fileID: conflict.id) + return try XCTUnwrap(controller.prepared) + } + XCTAssertEqual(try read("a.txt", repo), "old\n", "Skip must not apply other files yet") + XCTAssertEqual(try read("b.txt", repo), "local\n", "Skip must not change the working copy") XCTAssertFalse(skipped.hasConflicts) XCTAssertFalse(try XCTUnwrap(skipped.reviewFiles.first { $0.id == conflict.id }).patch.isEmpty, "Skipped changes remain previewable") diff --git a/macgitTests/GitSubmoduleLifecycleTests.swift b/macgitTests/GitSubmoduleLifecycleTests.swift index 2c5c4e0..b6da8e8 100644 --- a/macgitTests/GitSubmoduleLifecycleTests.swift +++ b/macgitTests/GitSubmoduleLifecycleTests.swift @@ -205,6 +205,7 @@ final class GitSubmoduleLifecycleTests: XCTestCase { repositoryURL: setup.parent, selection: .constant(nil), onRequestCheckout: { _, _ in }, + onRequestRemoteBranchCheckout: { _ in }, onRequestFetchBranch: { _ in }, onRequestRemoveSubmodule: { path, force in try await GitStatusService.shared.removeSubmodule(path: path, force: force, in: setup.parent) From 2c06ac31d39b5d3586775d85b92b411c59edd0c6 Mon Sep 17 00:00:00 2001 From: Thanh Tran Date: Sat, 26 Sep 2026 15:03:48 +0700 Subject: [PATCH 6/8] chore: Replace SPDX license headers with full AGPL notices Add explanatory provider error message when GitHub returns a Validation Failed response during pull request creation. --- .../CommitPatchConflictWindowController.swift | 20 ++++++++- macgit/App/CommitPatchController.swift | 20 ++++++++- macgit/Models/CommitPatchRequest.swift | 20 ++++++++- macgit/Models/CommitPatchReviewFile.swift | 20 ++++++++- macgit/Models/PreparedCommitPatch.swift | 20 ++++++++- macgit/Services/CommitPatchBuilder.swift | 20 ++++++++- .../Services/GitHubPullRequestService.swift | 5 +++ .../GitStatusService+CommitPatch.swift | 20 ++++++++- .../GitStatusService+CommitPatchMerge.swift | 42 ++++++++++++++----- macgit/Views/Common/DiffView.swift | 14 ++++--- .../History/CommitPatchConflictSheet.swift | 20 ++++++++- .../CommitPatchConflictWindowContext.swift | 20 ++++++++- .../History/CommitPatchReviewSheet.swift | 20 ++++++++- macgitTests/CommitPatchIntegrationTests.swift | 20 ++++++++- .../GitHubPullRequestServiceTests.swift | 17 ++++++++ 15 files changed, 272 insertions(+), 26 deletions(-) diff --git a/macgit/App/CommitPatchConflictWindowController.swift b/macgit/App/CommitPatchConflictWindowController.swift index 7c835e5..d1f4b15 100644 --- a/macgit/App/CommitPatchConflictWindowController.swift +++ b/macgit/App/CommitPatchConflictWindowController.swift @@ -1,4 +1,22 @@ -// SPDX-License-Identifier: AGPL-3.0-or-later +// +// CommitPatchConflictWindowController.swift +// macgit +// +// Copyright (C) 2026 Thanh Tran +// +// This program is free software; you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . +// import AppKit import SwiftUI diff --git a/macgit/App/CommitPatchController.swift b/macgit/App/CommitPatchController.swift index 887ea9b..1750f9d 100644 --- a/macgit/App/CommitPatchController.swift +++ b/macgit/App/CommitPatchController.swift @@ -1,4 +1,22 @@ -// SPDX-License-Identifier: AGPL-3.0-or-later +// +// CommitPatchController.swift +// macgit +// +// Copyright (C) 2026 Thanh Tran +// +// This program is free software; you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . +// import AppKit import Foundation import Observation diff --git a/macgit/Models/CommitPatchRequest.swift b/macgit/Models/CommitPatchRequest.swift index cbaabc3..b3114d3 100644 --- a/macgit/Models/CommitPatchRequest.swift +++ b/macgit/Models/CommitPatchRequest.swift @@ -1,4 +1,22 @@ -// SPDX-License-Identifier: AGPL-3.0-or-later +// +// CommitPatchRequest.swift +// macgit +// +// Copyright (C) 2026 Thanh Tran +// +// This program is free software; you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . +// import Foundation nonisolated struct CommitPatchRequest: Sendable { diff --git a/macgit/Models/CommitPatchReviewFile.swift b/macgit/Models/CommitPatchReviewFile.swift index 4f50e0e..206bf26 100644 --- a/macgit/Models/CommitPatchReviewFile.swift +++ b/macgit/Models/CommitPatchReviewFile.swift @@ -1,4 +1,22 @@ -// SPDX-License-Identifier: AGPL-3.0-or-later +// +// CommitPatchReviewFile.swift +// macgit +// +// Copyright (C) 2026 Thanh Tran +// +// This program is free software; you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . +// import Foundation nonisolated struct CommitPatchReviewFile: Identifiable, Sendable { diff --git a/macgit/Models/PreparedCommitPatch.swift b/macgit/Models/PreparedCommitPatch.swift index 826ef7b..36941f0 100644 --- a/macgit/Models/PreparedCommitPatch.swift +++ b/macgit/Models/PreparedCommitPatch.swift @@ -1,4 +1,22 @@ -// SPDX-License-Identifier: AGPL-3.0-or-later +// +// PreparedCommitPatch.swift +// macgit +// +// Copyright (C) 2026 Thanh Tran +// +// This program is free software; you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . +// import Foundation nonisolated struct PreparedCommitPatch: Identifiable, Sendable { diff --git a/macgit/Services/CommitPatchBuilder.swift b/macgit/Services/CommitPatchBuilder.swift index 631fec4..f1b57bf 100644 --- a/macgit/Services/CommitPatchBuilder.swift +++ b/macgit/Services/CommitPatchBuilder.swift @@ -1,4 +1,22 @@ -// SPDX-License-Identifier: AGPL-3.0-or-later +// +// CommitPatchBuilder.swift +// macgit +// +// Copyright (C) 2026 Thanh Tran +// +// This program is free software; you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . +// import Foundation /// Builds a forward patch from an already direction-oriented Git diff. diff --git a/macgit/Services/GitHubPullRequestService.swift b/macgit/Services/GitHubPullRequestService.swift index 9e5c86a..70e1ee6 100644 --- a/macgit/Services/GitHubPullRequestService.swift +++ b/macgit/Services/GitHubPullRequestService.swift @@ -294,6 +294,11 @@ struct GitHubPullRequestService: PullRequestProviding { } return PullRequestCreationResult(summary: created.summary, warnings: warnings) } catch let error as PullRequestProviderError { + if case .providerMessage("Validation Failed") = error { + throw PullRequestProviderError.providerMessage( + "GitHub couldn't create this pull request. A pull request from this branch to the selected target may already exist. Check the repository's pull requests, or choose a different target branch." + ) + } throw error } catch { throw PullRequestProviderError.providerMessage("GitHub returned an invalid pull request response.") diff --git a/macgit/Services/GitStatusService+CommitPatch.swift b/macgit/Services/GitStatusService+CommitPatch.swift index ba21d43..aff4fac 100644 --- a/macgit/Services/GitStatusService+CommitPatch.swift +++ b/macgit/Services/GitStatusService+CommitPatch.swift @@ -1,4 +1,22 @@ -// SPDX-License-Identifier: AGPL-3.0-or-later +// +// GitStatusService+CommitPatch.swift +// macgit +// +// Copyright (C) 2026 Thanh Tran +// +// This program is free software; you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . +// import Foundation import CryptoKit diff --git a/macgit/Services/GitStatusService+CommitPatchMerge.swift b/macgit/Services/GitStatusService+CommitPatchMerge.swift index e0c5cfb..ea3777f 100644 --- a/macgit/Services/GitStatusService+CommitPatchMerge.swift +++ b/macgit/Services/GitStatusService+CommitPatchMerge.swift @@ -1,4 +1,22 @@ -// SPDX-License-Identifier: AGPL-3.0-or-later +// +// GitStatusService+CommitPatchMerge.swift +// macgit +// +// Copyright (C) 2026 Thanh Tran +// +// This program is free software; you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . +// import Foundation extension GitStatusService { @@ -9,17 +27,21 @@ extension GitStatusService { return CommitPatchReviewFile(file: file, patch: patch, state: .ready) } catch { try Task.checkCancellation() } - // Check each file independently: a batch can contain both new and already-present changes. - do { - try await runCommitPatch(patch, checkOnly: true, reverse: true, in: repositoryURL) - return CommitPatchReviewFile(file: file, patch: patch, state: .alreadyApplied) - } catch { try Task.checkCancellation() } - let url = repositoryURL.appendingPathComponent(file.path) let fileExists = FileManager.default.fileExists(atPath: url.path) - guard file.status == .modified, file.oldPath == nil, - fileExists, - !patch.components(separatedBy: "\n").contains(where: { $0.hasPrefix("old mode ") || $0.hasPrefix("new mode ") }) else { + let canThreeWayMerge = file.status == .modified && file.oldPath == nil && fileExists && + !patch.components(separatedBy: "\n").contains(where: { + $0.hasPrefix("old mode ") || $0.hasPrefix("new mode ") + }) + if !canThreeWayMerge { + // Structural changes cannot use the scratch three-way merge path. + // Check them in reverse so already-present additions/deletions stay no-ops. + do { + try await runCommitPatch(patch, checkOnly: true, reverse: true, in: repositoryURL) + return CommitPatchReviewFile(file: file, patch: patch, state: .alreadyApplied) + } catch { try Task.checkCancellation() } + } + guard canThreeWayMerge else { return CommitPatchReviewFile(file: file, patch: patch, state: .conflict, conflict: .init( kind: !fileExists && file.status == .modified && file.oldPath == nil ? .missingFile : .unsupportedChange, message: !fileExists && file.status == .modified && file.oldPath == nil diff --git a/macgit/Views/Common/DiffView.swift b/macgit/Views/Common/DiffView.swift index 1e4f886..cbf9bc8 100644 --- a/macgit/Views/Common/DiffView.swift +++ b/macgit/Views/Common/DiffView.swift @@ -22,6 +22,10 @@ // import SwiftUI +private func isChangedDiffLine(_ line: DiffLine) -> Bool { + (line.oldLineNumber == nil) != (line.newLineNumber == nil) +} + struct DiffView: View { let hunks: [DiffHunk] let file: StatusFile? @@ -106,13 +110,13 @@ struct DiffView: View { onError: onError, onCommitHunk: onCommitPatch.map { action in { hunk, direction in - action(hunk.lines.filter { $0.type == .added || $0.type == .removed }, direction, "Selected hunk") + action(hunk.lines.filter(isChangedDiffLine), direction, "Selected hunk") } }, onCommitLines: onCommitPatch.map { action in { ids, direction in action(hunks.flatMap(\.lines).filter { - ids.contains($0.id) && ($0.type == .added || $0.type == .removed) + ids.contains($0.id) && isChangedDiffLine($0) }, direction, "Selected lines") } }, @@ -455,7 +459,7 @@ struct HunkView: View { } private func commitLineIDs(_ line: DiffLine?) -> Set { - if let line, !selectedLineIDs.contains(line.id), line.type == .added || line.type == .removed { + if let line, !selectedLineIDs.contains(line.id), isChangedDiffLine(line) { return [line.id] } return selectedLineIDs @@ -463,7 +467,7 @@ struct HunkView: View { private func handleLineTap(at index: Int) { let line = hunk.lines[index] - guard line.type == .added || line.type == .removed else { return } + guard isChangedDiffLine(line) else { return } let flags = NSEvent.modifierFlags let isShift = flags.contains(.shift) @@ -478,7 +482,7 @@ struct HunkView: View { } let start = min(lastIndex, index) let end = max(lastIndex, index) - let rangeIDs = Set(hunk.lines[start...end].filter { $0.type == .added || $0.type == .removed }.map(\.id)) + let rangeIDs = Set(hunk.lines[start...end].filter(isChangedDiffLine).map(\.id)) if isCommand { selectedLineIDs.formSymmetricDifference(rangeIDs) } else { diff --git a/macgit/Views/History/CommitPatchConflictSheet.swift b/macgit/Views/History/CommitPatchConflictSheet.swift index 6e7259b..8e2ed29 100644 --- a/macgit/Views/History/CommitPatchConflictSheet.swift +++ b/macgit/Views/History/CommitPatchConflictSheet.swift @@ -1,4 +1,22 @@ -// SPDX-License-Identifier: AGPL-3.0-or-later +// +// CommitPatchConflictSheet.swift +// macgit +// +// Copyright (C) 2026 Thanh Tran +// +// This program is free software; you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . +// import SwiftUI /// Edits an in-memory merge result. The existing code panes/editor never receive a repository mutation callback. diff --git a/macgit/Views/History/CommitPatchConflictWindowContext.swift b/macgit/Views/History/CommitPatchConflictWindowContext.swift index 725fd42..a11da85 100644 --- a/macgit/Views/History/CommitPatchConflictWindowContext.swift +++ b/macgit/Views/History/CommitPatchConflictWindowContext.swift @@ -1,4 +1,22 @@ -// SPDX-License-Identifier: AGPL-3.0-or-later +// +// CommitPatchConflictWindowContext.swift +// macgit +// +// Copyright (C) 2026 Thanh Tran +// +// This program is free software; you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . +// import SwiftUI /// Routes Cmd-Z inside the temporary resolver to editor/resolution undo, never repository undo. diff --git a/macgit/Views/History/CommitPatchReviewSheet.swift b/macgit/Views/History/CommitPatchReviewSheet.swift index 7b31244..fbecd2f 100644 --- a/macgit/Views/History/CommitPatchReviewSheet.swift +++ b/macgit/Views/History/CommitPatchReviewSheet.swift @@ -1,4 +1,22 @@ -// SPDX-License-Identifier: AGPL-3.0-or-later +// +// CommitPatchReviewSheet.swift +// macgit +// +// Copyright (C) 2026 Thanh Tran +// +// This program is free software; you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . +// import SwiftUI struct CommitPatchReviewSheet: View { diff --git a/macgitTests/CommitPatchIntegrationTests.swift b/macgitTests/CommitPatchIntegrationTests.swift index baa2e27..4e2fc9a 100644 --- a/macgitTests/CommitPatchIntegrationTests.swift +++ b/macgitTests/CommitPatchIntegrationTests.swift @@ -1,4 +1,22 @@ -// SPDX-License-Identifier: AGPL-3.0-or-later +// +// CommitPatchIntegrationTests.swift +// macgit +// +// Copyright (C) 2026 Thanh Tran +// +// This program is free software; you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . +// import XCTest @testable import macgit diff --git a/macgitTests/GitHubPullRequestServiceTests.swift b/macgitTests/GitHubPullRequestServiceTests.swift index 553ff87..12f70e5 100644 --- a/macgitTests/GitHubPullRequestServiceTests.swift +++ b/macgitTests/GitHubPullRequestServiceTests.swift @@ -508,6 +508,23 @@ final class GitHubPullRequestServiceTests: XCTestCase { } } + func testCreatePullRequestValidationFailedExplainsPossibleExistingPullRequest() async throws { + let client = StubPullRequestHTTPClient(responses: [ + .json(statusCode: 422, body: #"{"message":"Validation Failed"}"#) + ]) + let service = GitHubPullRequestService(httpClient: client) + + do { + _ = try await service.createPullRequest(makeDraft(), token: makeToken()) + XCTFail("Expected createPullRequest to throw") + } catch { + XCTAssertEqual( + error as? PullRequestProviderError, + .providerMessage("GitHub couldn't create this pull request. A pull request from this branch to the selected target may already exist. Check the repository's pull requests, or choose a different target branch.") + ) + } + } + func testMergePullRequestUsesProviderMergeEndpoint() async throws { let client = StubPullRequestHTTPClient(responses: [ .json(statusCode: 200, body: #"{"merged":true,"message":"Pull Request successfully merged"}"#) From d795127224cc985207bf9c6cb5694eb64a7892f4 Mon Sep 17 00:00:00 2001 From: Thanh Tran Date: Sat, 26 Sep 2026 15:11:36 +0700 Subject: [PATCH 7/8] fix: code review --- .../MainWindow/Sidebar/SidebarBranchRow.swift | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/macgit/Views/MainWindow/Sidebar/SidebarBranchRow.swift b/macgit/Views/MainWindow/Sidebar/SidebarBranchRow.swift index c847f62..b6bf88b 100644 --- a/macgit/Views/MainWindow/Sidebar/SidebarBranchRow.swift +++ b/macgit/Views/MainWindow/Sidebar/SidebarBranchRow.swift @@ -62,6 +62,8 @@ struct SidebarBranchRow: View { private var branchLeafRow: some View { let rowView = content .tag(SidebarSelection.branch(row.fullPath)) + + let rowWithContextMenu = rowView .contextMenu { SidebarBranchContextMenu( branch: row.fullPath, @@ -75,7 +77,7 @@ struct SidebarBranchRow: View { } if isCurrentBranch { - rowView + rowWithContextMenu .overlay { SidebarBranchDropTarget( passthroughTrailingWidth: Self.currentBranchTrailingControlsWidth, @@ -122,6 +124,17 @@ struct SidebarBranchRow: View { onDragEnded: finishBranchDrag ) } + .contextMenu { + SidebarBranchContextMenu( + branch: row.fullPath, + currentBranch: currentBranch, + syncStatus: branchSyncStatus[row.fullPath], + upstream: upstreamByBranch[row.fullPath], + remoteNames: remoteNames, + branchesByRemote: branchesByRemote, + actions: actions + ) + } } } From 541b592b9dc10ba1cbf0b56d48204eccbb116312 Mon Sep 17 00:00:00 2001 From: Thanh Tran Date: Sat, 26 Sep 2026 15:27:15 +0700 Subject: [PATCH 8/8] fix: narrow duplicate pull request guidance --- .../Services/GitHubPullRequestService.swift | 25 +++++++++++++++---- .../GitHubPullRequestServiceTests.swift | 20 ++++++++++++--- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/macgit/Services/GitHubPullRequestService.swift b/macgit/Services/GitHubPullRequestService.swift index 70e1ee6..bbf74dd 100644 --- a/macgit/Services/GitHubPullRequestService.swift +++ b/macgit/Services/GitHubPullRequestService.swift @@ -243,6 +243,13 @@ struct GitHubPullRequestService: PullRequestProviding { let (data, response) = try await httpClient.data( for: try makeJSONRequest(url: url, token: token, method: "POST", body: payload) ) + if response.statusCode == 422, + let errorResponse = try? decoder.decode(GitHubErrorResponse.self, from: data), + errorResponse.isDuplicatePullRequest { + throw PullRequestProviderError.providerMessage( + "GitHub couldn't create this pull request. A pull request from this branch to the selected target already exists. Check the repository's pull requests, or choose a different target branch." + ) + } try validateWrite(response: response, data: data) let created = try decoder.decode(GitHubPullRequestResponse.self, from: data) var warnings: [String] = [] @@ -294,11 +301,6 @@ struct GitHubPullRequestService: PullRequestProviding { } return PullRequestCreationResult(summary: created.summary, warnings: warnings) } catch let error as PullRequestProviderError { - if case .providerMessage("Validation Failed") = error { - throw PullRequestProviderError.providerMessage( - "GitHub couldn't create this pull request. A pull request from this branch to the selected target may already exist. Check the repository's pull requests, or choose a different target branch." - ) - } throw error } catch { throw PullRequestProviderError.providerMessage("GitHub returned an invalid pull request response.") @@ -853,6 +855,19 @@ private struct GitHubReviewCommentResponse: Decodable { private struct GitHubErrorResponse: Decodable { var message: String + var errors: [ValidationError]? + + var isDuplicatePullRequest: Bool { + errors?.contains { + $0.resource == "PullRequest" && + $0.message?.localizedCaseInsensitiveContains("a pull request already exists") == true + } == true + } + + struct ValidationError: Decodable { + var resource: String? + var message: String? + } } private struct GitHubCombinedStatusResponse: Decodable { diff --git a/macgitTests/GitHubPullRequestServiceTests.swift b/macgitTests/GitHubPullRequestServiceTests.swift index 12f70e5..4074d0a 100644 --- a/macgitTests/GitHubPullRequestServiceTests.swift +++ b/macgitTests/GitHubPullRequestServiceTests.swift @@ -508,9 +508,9 @@ final class GitHubPullRequestServiceTests: XCTestCase { } } - func testCreatePullRequestValidationFailedExplainsPossibleExistingPullRequest() async throws { + func testCreatePullRequestDuplicateValidationExplainsPossibleExistingPullRequest() async throws { let client = StubPullRequestHTTPClient(responses: [ - .json(statusCode: 422, body: #"{"message":"Validation Failed"}"#) + .json(statusCode: 422, body: #"{"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"A pull request already exists for feature/pr-actions:main"}]}"#) ]) let service = GitHubPullRequestService(httpClient: client) @@ -520,11 +520,25 @@ final class GitHubPullRequestServiceTests: XCTestCase { } catch { XCTAssertEqual( error as? PullRequestProviderError, - .providerMessage("GitHub couldn't create this pull request. A pull request from this branch to the selected target may already exist. Check the repository's pull requests, or choose a different target branch.") + .providerMessage("GitHub couldn't create this pull request. A pull request from this branch to the selected target already exists. Check the repository's pull requests, or choose a different target branch.") ) } } + func testCreatePullRequestOtherValidationFailurePreservesGeneralError() async throws { + let client = StubPullRequestHTTPClient(responses: [ + .json(statusCode: 422, body: #"{"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"No commits between main and feature/pr-actions"}]}"#) + ]) + let service = GitHubPullRequestService(httpClient: client) + + do { + _ = try await service.createPullRequest(makeDraft(), token: makeToken()) + XCTFail("Expected createPullRequest to throw") + } catch { + XCTAssertEqual(error as? PullRequestProviderError, .providerMessage("Validation Failed")) + } + } + func testMergePullRequestUsesProviderMergeEndpoint() async throws { let client = StubPullRequestHTTPClient(responses: [ .json(statusCode: 200, body: #"{"merged":true,"message":"Pull Request successfully merged"}"#)