From cfb20a8a757841baed6ac60f17e43ff437ee3a15 Mon Sep 17 00:00:00 2001 From: scgopi Date: Sun, 30 Aug 2026 13:09:25 -0700 Subject: [PATCH] Remove worktrees with initialized submodules by passing --force MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit git refuses to remove a worktree whose submodules are initialized, even when the tree is clean — the error users saw: ``working trees containing submodules cannot be moved or removed``. Hygiene never passed --force on a clean row, so those worktrees were stuck. The facts now carry hasSubmodules (initialized checkouts only — uninitialized gitlinks were never the problem), read as a non-`-` line of `git submodule status`, gated on .gitmodules locally so repositories without submodules pay zero extra git calls; remotely it is one more round trip. Forcing for submodules is bookkeeping, not discard: a pristine checkout is restorable from upstream, so it does not raise the confirmation. Uncommitted work inside a submodule keeps its protection — the superproject's status already reports it dirty — and the three removal routes (sweeper, resolve moment, card Reclaim) all pass the flag; the card action carries the offer's fact because the offer is cleared in the same action. --- .../Sources/Domain/WorktreeHygiene.swift | 7 ++++ graphcode/Sources/Clients/GitClient.swift | 17 +++++++- .../Sources/Clients/RemoteGitClient.swift | 10 ++++- .../Features/App/AppFeature+Worktrees.swift | 35 +++++++++++----- .../Overview/GraphOverviewCards.swift | 9 +++- .../Features/Project/ProjectCanvasCards.swift | 7 +++- .../Features/Project/ProjectFeature.swift | 11 +++-- graphcode/Tests/WorktreeHygieneTests.swift | 19 ++++++++- graphcode/Tests/WorktreeRemovalTests.swift | 42 ++++++++++++++++++- 9 files changed, 135 insertions(+), 22 deletions(-) diff --git a/GraphcodeKit/Sources/Domain/WorktreeHygiene.swift b/GraphcodeKit/Sources/Domain/WorktreeHygiene.swift index 93111929..b06411ac 100644 --- a/GraphcodeKit/Sources/Domain/WorktreeHygiene.swift +++ b/GraphcodeKit/Sources/Domain/WorktreeHygiene.swift @@ -24,6 +24,11 @@ public struct WorktreeGitFacts: Codable, Equatable, Sendable { public var prunable: Bool /// `git worktree lock` — git refuses removal even with `--force`. public var locked: Bool + /// Initialized submodule checkouts inside the worktree. Git refuses to remove + /// such a worktree even when the tree is clean, so removal needs `--force` — + /// bookkeeping, not discard: a pristine submodule checkout is restorable from + /// upstream, which is why this does not by itself demand the confirmation. + public var hasSubmodules: Bool public var sizeBytes: Int64? public var landed: Bool { commitsNotLanded == 0 || squashLanded } @@ -37,6 +42,7 @@ public struct WorktreeGitFacts: Codable, Equatable, Sendable { pushed: Bool, prunable: Bool = false, locked: Bool = false, + hasSubmodules: Bool = false, sizeBytes: Int64? = nil ) { self.defaultBranch = defaultBranch @@ -46,6 +52,7 @@ public struct WorktreeGitFacts: Codable, Equatable, Sendable { self.pushed = pushed self.prunable = prunable self.locked = locked + self.hasSubmodules = hasSubmodules self.sizeBytes = sizeBytes } } diff --git a/graphcode/Sources/Clients/GitClient.swift b/graphcode/Sources/Clients/GitClient.swift index 86dbcda2..d527ca42 100644 --- a/graphcode/Sources/Clients/GitClient.swift +++ b/graphcode/Sources/Clients/GitClient.swift @@ -467,11 +467,26 @@ private func gitFacts( "git", ["-C", block.path, "rev-list", "--count", "@{upstream}..HEAD"]) let pushed = ahead.flatMap { Int($0.trimmingCharacters(in: .whitespacesAndNewlines)) } == 0 + let hasSubmodules = await worktreeHasInitializedSubmodules(block.path) return WorktreeGitFacts( defaultBranch: defaultBranch, commitsNotLanded: commitsNotLanded, squashLanded: landing.squashLanded, dirtyFileCount: dirtyFileCount, pushed: pushed, prunable: false, locked: block.locked, - sizeBytes: nil) + hasSubmodules: hasSubmodules, sizeBytes: nil) +} + +/// Whether any of the worktree's submodules is initialized — the state git's +/// "working trees containing submodules cannot be moved or removed" refusal is +/// about. `git submodule status` marks uninitialized entries with `-`; any other +/// line means a checkout is populated. The `.gitmodules` check keeps repositories +/// without submodules at zero extra git calls, and an unreadable status answers +/// no: git then refuses the removal itself and the sheet carries its words. +private func worktreeHasInitializedSubmodules(_ worktreePath: String) async -> Bool { + guard FileManager.default.fileExists( + atPath: (worktreePath as NSString).appendingPathComponent(".gitmodules")) + else { return false } + let status = try? await run("git", ["-C", worktreePath, "submodule", "status"]) + return status?.split(separator: "\n").contains { !$0.hasPrefix("-") } ?? false } extension Optional { diff --git a/graphcode/Sources/Clients/RemoteGitClient.swift b/graphcode/Sources/Clients/RemoteGitClient.swift index 2762ea6a..a34d195e 100644 --- a/graphcode/Sources/Clients/RemoteGitClient.swift +++ b/graphcode/Sources/Clients/RemoteGitClient.swift @@ -304,11 +304,19 @@ private func remoteGitFacts( let ahead = try? await runSSH( location, "git -C \(quoted(block.path)) rev-list --count @{upstream}..HEAD") let pushed = ahead.flatMap { Int($0.trimmingCharacters(in: .whitespacesAndNewlines)) } == 0 + // One more round trip per worktree, and only git can say it — the remote host's + // file system is not reachable from here. `submodule status` marks uninitialized + // entries with `-`; any other line is a populated checkout git would refuse to + // remove without `--force`. + let submoduleStatus = try? await runSSH( + location, "git -C \(quoted(block.path)) submodule status") + let hasSubmodules = + submoduleStatus?.split(separator: "\n").contains { !$0.hasPrefix("-") } ?? false return WorktreeGitFacts( defaultBranch: defaultBranch, commitsNotLanded: commitsNotLanded, squashLanded: landing.squashLanded, dirtyFileCount: dirtyFileCount, pushed: pushed, prunable: false, locked: block.locked, - sizeBytes: nil) + hasSubmodules: hasSubmodules, sizeBytes: nil) } private func appendRemovedBranchRecord(branch: String, tip: String, path: String, host: String) { diff --git a/graphcode/Sources/Features/App/AppFeature+Worktrees.swift b/graphcode/Sources/Features/App/AppFeature+Worktrees.swift index e160df0c..6fc092a5 100644 --- a/graphcode/Sources/Features/App/AppFeature+Worktrees.swift +++ b/graphcode/Sources/Features/App/AppFeature+Worktrees.swift @@ -16,7 +16,9 @@ struct WorktreeFolderStats: Equatable, Sendable { struct WorktreeRemovalCandidate: Equatable, Sendable { var ref: WorktreeRef var prunable: Bool - /// Only ever true for a row whose discard the human confirmed in the sweeper. + /// True when git needs `--force` to go through: a discard the human confirmed, + /// or initialized submodules, which git refuses even on a clean tree and whose + /// force loses nothing the human wrote. var force: Bool } @@ -165,9 +167,12 @@ struct AppWorktreesReducer: Reducer { // Forcing is decided on what git says *now*; consent is what the human gave // to the sheet. A worktree that has grown uncommitted files since the list // was built would fail an unforced removal without a word, so it is refused - // out loud instead. - let needsForce = !current.facts.prunable && !current.facts.clean - guard !needsForce || assessment.removalDiscardsFiles else { + // out loud instead. Initialized submodules need `--force` too, but that is + // bookkeeping, not discard — nothing the human wrote is lost, so it never + // demands the confirmation. + let discardsFiles = !current.facts.prunable && !current.facts.clean + let needsForce = discardsFiles || current.facts.hasSubmodules + guard !discardsFiles || assessment.removalDiscardsFiles else { blocked.append( "\(current.ref.displayName): uncommitted files appeared since this list " + "was built — reopen Worktrees and confirm the discard") @@ -278,7 +283,11 @@ struct AppWorktreesReducer: Reducer { // The card's Reclaim: the project scope already cleared its offer on this same // action; the removal routes through the same verified gate as everything else — // an offer can be minutes old, and another loop may have bound the worktree since. - case .projects(.element(id: let path, action: .reclaimWorktreeTapped(let nodeID))): + // The offer's submodule fact travels with the action because the offer itself + // does not survive it: git refuses submodule worktrees even clean, and the + // removal needs the force flag the offer's facts carried. + case .projects( + .element(id: let path, action: .reclaimWorktreeTapped(let nodeID, let hasSubmodules))): guard let node = state.projects[id: path]?.graph.nodes[id: nodeID], let ref = node.worktreeBinding else { return .none } @@ -286,7 +295,10 @@ struct AppWorktreesReducer: Reducer { .worktrees( .performRemovals( projectPath: path, - candidates: [WorktreeRemovalCandidate(ref: ref, prunable: false, force: false)], + candidates: [ + WorktreeRemovalCandidate( + ref: ref, prunable: false, force: hasSubmodules) + ], blocked: []))) default: @@ -449,16 +461,19 @@ extension AppWorktreesReducer { guard assessment.tier == .safeToRemove else { return } switch policy.onResolveLanded { case .remove: - // Never forced (the safe tier is clean by definition), and routed through the - // verified gate: the inspection above took real time, and a loop can have - // claimed the worktree meanwhile. + // Never forced for discard (the safe tier is clean by definition), and routed + // through the verified gate: the inspection above took real time, and a loop + // can have claimed the worktree meanwhile. Initialized submodules still take + // `--force` — git refuses their worktrees even clean, and nothing the human + // wrote is lost by it. await send( .worktrees( .performRemovals( projectPath: path, candidates: [ WorktreeRemovalCandidate( - ref: inspection.ref, prunable: inspection.facts.prunable, force: false) + ref: inspection.ref, prunable: inspection.facts.prunable, + force: !inspection.facts.prunable && inspection.facts.hasSubmodules) ], blocked: []))) case .ask: diff --git a/graphcode/Sources/Features/Overview/GraphOverviewCards.swift b/graphcode/Sources/Features/Overview/GraphOverviewCards.swift index 9f4b557a..84e7d872 100644 --- a/graphcode/Sources/Features/Overview/GraphOverviewCards.swift +++ b/graphcode/Sources/Features/Overview/GraphOverviewCards.swift @@ -181,7 +181,14 @@ extension GraphOverviewView { reclaimOffer: store.projects[id: loop.projectPath]?.worktreeReclaimOffers[node.id], onReclaim: { store.send( - .projects(.element(id: loop.projectPath, action: .reclaimWorktreeTapped(node.id)))) + .projects( + .element( + id: loop.projectPath, + action: .reclaimWorktreeTapped( + id: node.id, + hasSubmodules: + store.projects[id: loop.projectPath]?.worktreeReclaimOffers[node.id]? + .facts.hasSubmodules == true)))) }, onKeep: { store.send( diff --git a/graphcode/Sources/Features/Project/ProjectCanvasCards.swift b/graphcode/Sources/Features/Project/ProjectCanvasCards.swift index e5a1ea5d..9fb25de2 100644 --- a/graphcode/Sources/Features/Project/ProjectCanvasCards.swift +++ b/graphcode/Sources/Features/Project/ProjectCanvasCards.swift @@ -38,7 +38,12 @@ extension ProjectCanvasView { onWireUp: { dragSourceID = node.id }, onMarkAsEntry: { store.send(.markAsEntryTapped(node.id)) }, reclaimOffer: store.worktreeReclaimOffers[node.id], - onReclaim: { store.send(.reclaimWorktreeTapped(node.id)) }, + onReclaim: { + store.send( + .reclaimWorktreeTapped( + id: node.id, + hasSubmodules: store.worktreeReclaimOffers[node.id]?.facts.hasSubmodules == true)) + }, onKeep: { store.send(.keepWorktreeTapped(node.id)) } ) .contentShape(Rectangle()) diff --git a/graphcode/Sources/Features/Project/ProjectFeature.swift b/graphcode/Sources/Features/Project/ProjectFeature.swift index b7edcfba..960f18f2 100644 --- a/graphcode/Sources/Features/Project/ProjectFeature.swift +++ b/graphcode/Sources/Features/Project/ProjectFeature.swift @@ -239,8 +239,11 @@ struct ProjectFeature { /// Reclaim/Keep while the human still remembers what the worktree was. case worktreeReclaimOffered(nodeID: UUID, assessment: WorktreeAssessment) /// Clearing the offer is this scope's job; the removal itself needs `GitClient` - /// and happens in `AppWorktreesReducer`, which intercepts the same action. - case reclaimWorktreeTapped(UUID) + /// and happens in `AppWorktreesReducer`, which intercepts the same action. The + /// offer's submodule fact rides along because the offer does not survive it — + /// git refuses submodule worktrees even clean, so the removal needs its force + /// flag. + case reclaimWorktreeTapped(id: UUID, hasSubmodules: Bool) case keepWorktreeTapped(UUID) /// The canvas background's folder menu. Pure signals like `.nodeTapped`: the sheets /// they open are hosted by `AppView`, so `AppWorktreesReducer` intercepts both. @@ -358,8 +361,8 @@ struct ProjectFeature { state.worktreeReclaimOffers[nodeID] = assessment return .none - case .reclaimWorktreeTapped(let nodeID), .keepWorktreeTapped(let nodeID): - state.worktreeReclaimOffers[nodeID] = nil + case .reclaimWorktreeTapped(let id, _), .keepWorktreeTapped(let id): + state.worktreeReclaimOffers[id] = nil return .none case .worktreeSweepTapped, .projectSettingsTapped: diff --git a/graphcode/Tests/WorktreeHygieneTests.swift b/graphcode/Tests/WorktreeHygieneTests.swift index f2c230c2..2a24dcf9 100644 --- a/graphcode/Tests/WorktreeHygieneTests.swift +++ b/graphcode/Tests/WorktreeHygieneTests.swift @@ -16,12 +16,12 @@ struct WorktreeHygieneTests { private func facts( notLanded: Int = 0, squashLanded: Bool = false, dirty: Int = 0, pushed: Bool = true, - prunable: Bool = false, locked: Bool = false, size: Int64? = 100 + prunable: Bool = false, locked: Bool = false, submodules: Bool = false, size: Int64? = 100 ) -> WorktreeGitFacts { WorktreeGitFacts( defaultBranch: "main", commitsNotLanded: notLanded, squashLanded: squashLanded, dirtyFileCount: dirty, pushed: pushed, prunable: prunable, locked: locked, - sizeBytes: size) + hasSubmodules: submodules, sizeBytes: size) } private func node( @@ -160,6 +160,21 @@ struct WorktreeHygieneTests { #expect(locked.summary.contains("locked")) } + @Test + func submodulesChangeTheRemovalMechanicsNotTheTier() { + // Initialized submodules make git refuse a plain removal, so they travel as a + // fact for removal to force with. They are not a safety signal: a pristine + // checkout is restorable from upstream, the worktree stays safe and selectable, + // and — the part that must hold — removing one never reads as discarding files, + // so no confirmation stands in the way. + let withSubmodules = WorktreeAssessment( + ref: ref(), facts: facts(submodules: true), binding: .none) + + #expect(withSubmodules.tier == .safeToRemove) + #expect(withSubmodules.isRemovable) + #expect(!withSubmodules.removalDiscardsFiles) + } + @Test func theLookSummaryNamesWhatWouldBeLost() { let unmergedAndDirty = WorktreeAssessment( diff --git a/graphcode/Tests/WorktreeRemovalTests.swift b/graphcode/Tests/WorktreeRemovalTests.swift index c5985b34..058d285a 100644 --- a/graphcode/Tests/WorktreeRemovalTests.swift +++ b/graphcode/Tests/WorktreeRemovalTests.swift @@ -11,7 +11,8 @@ import Testing @Suite struct WorktreeRemovalTests { private func inspection( - branch: String, dirty: Int = 0, size: Int64? = 100, locked: Bool = false + branch: String, dirty: Int = 0, size: Int64? = 100, locked: Bool = false, + submodules: Bool = false ) -> WorktreeInspection { WorktreeInspection( ref: WorktreeRef( @@ -19,7 +20,7 @@ struct WorktreeRemovalTests { branch: branch), facts: WorktreeGitFacts( defaultBranch: "main", commitsNotLanded: 0, dirtyFileCount: dirty, pushed: true, - locked: locked, sizeBytes: size)) + locked: locked, hasSubmodules: submodules, sizeBytes: size)) } /// The state a sheet mid-use would hold, for the app-level removal tests. @@ -218,4 +219,41 @@ struct WorktreeRemovalTests { #expect(store.state.worktreeSweep == nil) await store.finish() } + + @Test + @MainActor + func aCleanSubmoduleWorktreeIsForcedWithoutAConfirmation() async { + // git refuses to remove worktrees with initialized submodules even when the + // tree is clean — the refusal that used to surface as "working trees containing + // submodules cannot be moved or removed". The force is bookkeeping, not + // discard: a pristine submodule checkout is restorable from upstream, so no + // confirmation stands between the click and the removal. + let sub = inspection(branch: "landed", submodules: true) + let forced = LockIsolated<[String: Bool]>([:]) + var initial = AppFeature.State(projects: [ + ProjectFeature.State( + graph: LoopGraph(scope: .project(ProjectRef(path: "/repo", name: "repo")))) + ]) + initial.worktreeSweep = openSweep([sub], selecting: [sub.ref.worktreePath]) + let store = TestStore(initialState: initial) { + AppFeature() + } withDependencies: { + $0.gitClient.inspectWorktrees = { _ in [sub] } + $0.gitClient.worktreeSizeBytes = { _ in nil } + $0.gitClient.removeWorktreeAndBranch = { ref, _, force in + forced.withValue { $0[ref.branch] = force } + } + } + store.exhaustivity = .off + + // Straight through — no `isConfirmingRemoval` on the way. + await store.send(.worktrees(.sweep(.removeTapped))) { + $0.worktreeSweep?.isRemoving = true + } + await store.receive(\.worktrees.removalsFinished) + + #expect(forced.value == ["landed": true]) + #expect(store.state.worktreeSweep == nil) + await store.finish() + } }