diff --git a/Sources/Bloom/State/AppModel+Projects.swift b/Sources/Bloom/State/AppModel+Projects.swift index f8369408b..e9d14850a 100644 --- a/Sources/Bloom/State/AppModel+Projects.swift +++ b/Sources/Bloom/State/AppModel+Projects.swift @@ -34,6 +34,8 @@ extension AppModel { case .alreadyRepository(let root): guard let repo = await addKnownRepository(at: root) else { return nil } if focusing { await focus(repo) } + // Not awaited: a glab probe to a host behind a VPN that is down takes seconds. + Task { await askForgeIfBoth(repo) } return repo case .refuse(let refusal): @@ -113,6 +115,30 @@ extension AppModel { )) } + /// Asks which forge `repo` uses when its remotes are on both, and returns once it is answered. + func askForgeIfBoth(_ repo: Repo) async { + guard forgeQuestion == nil, await ForgeResolver.offersBoth(repo.path), forgeQuestion == nil + else { return } + await withCheckedContinuation { answered in + forgeQuestionAnswered = answered + forgeQuestion = repo + } + } + + /// The answer to `forgeQuestion`, kept for every workspace of the project. `nil` is Not now. + func answerForgeQuestion(_ forge: Forge?, for repo: Repo? = nil) { + if let forge, let repo { + do { + try ForgeResolver.remember(forge, for: repo.path) + } catch { + alert = BloomAlert(title: "Could not save the forge", message: error.readableMessage) + } + } + let answered = forgeQuestionAnswered + forgeQuestionAnswered = nil + answered?.resume() + } + /// Called by `ProjectSetupSheet` once the folder really is a repository. func finishProjectSetup(_ path: String?) async { ProjectSetup.shared.dismiss() diff --git a/Sources/Bloom/State/AppModel.swift b/Sources/Bloom/State/AppModel.swift index 773a92125..c7b6a482b 100644 --- a/Sources/Bloom/State/AppModel.swift +++ b/Sources/Bloom/State/AppModel.swift @@ -282,6 +282,14 @@ final class AppModel { /// Non-nil after an archive script failed, while the owner decides between keeping the /// workspace and archiving it without the script. RootView presents it. var pendingScriptFailure: ScriptFailureRequest? + /// A project just added with remotes on both GitHub and GitLab. RootView asks which one it + /// uses. See `ForgeResolver.offersBoth`. + var forgeQuestion: Repo? { + // A dialog dismissed without its buttons must still let `askForgeIfBoth` return. + didSet { if forgeQuestion == nil { Task { self.answerForgeQuestion(nil) } } } + } + /// Resumed once `forgeQuestion` is answered, so the create window can wait for it. + @ObservationIgnored var forgeQuestionAnswered: CheckedContinuation? /// The transcript half of a search, one row per workspace. Held here rather than in `HomeView` /// for the same reason `homeFilter` is: the pane is destroyed and rebuilt every time the /// selection leaves Home, and a result list that had to be fetched again on the way back would diff --git a/Sources/Bloom/Views/Chrome/Window/WorkspaceHoverCardView.swift b/Sources/Bloom/Views/Chrome/Window/WorkspaceHoverCardView.swift index bfedee43c..9205ec27a 100644 --- a/Sources/Bloom/Views/Chrome/Window/WorkspaceHoverCardView.swift +++ b/Sources/Bloom/Views/Chrome/Window/WorkspaceHoverCardView.swift @@ -176,7 +176,7 @@ struct WorkspaceHoverCardView: View { // `verbatim`, for the reason `PullRequestBadge` writes down: an interpolated // `Int` inside a `LocalizedStringKey` is formatted for the locale, and a // machine set to Dutch drew "#2.631" for a pull request GitHub calls 2631. - Text(verbatim: "#\(pullRequest.number)") + Text(verbatim: pullRequest.forge.reference(pullRequest.number)) .font(Typo.caption) .monospacedDigit() .foregroundStyle(Palette.textSecondary) diff --git a/Sources/Bloom/Views/Inspector/PullRequestSummary.swift b/Sources/Bloom/Views/Inspector/PullRequestSummary.swift index 1801d1626..b54f7ae5e 100644 --- a/Sources/Bloom/Views/Inspector/PullRequestSummary.swift +++ b/Sources/Bloom/Views/Inspector/PullRequestSummary.swift @@ -401,7 +401,7 @@ struct PullRequestSummary: View { } private var archiveHelp: String { - return "Remove this workspace's worktree. #\(pullRequest.number) is merged, so " + return "Remove this workspace's worktree. \(pullRequest.forge.reference(pullRequest.number)) is merged, so " + "this asks first only when something here exists nowhere else." } @@ -434,7 +434,7 @@ struct PullRequestSummary: View { branchActions.reason ?? "Ask this workspace's agent to \(pushLabel.lowercased()) branch " + "\(pullRequest.branch.isEmpty ? "this branch" : pullRequest.branch), so " - + "#\(pullRequest.number) is what is on this disk." + + "\(pullRequest.forge.reference(pullRequest.number)) is what is on this disk." ) } @@ -502,7 +502,7 @@ struct PullRequestSummary: View { .help( branchActions.reason ?? "Ask this workspace's agent to bring \(baseBranch) into this worktree and " - + "resolve the conflicts here. Nothing is pushed and #\(pullRequest.number)" + + "resolve the conflicts here. Nothing is pushed and \(pullRequest.forge.reference(pullRequest.number))" + " is not merged." ) } @@ -582,7 +582,7 @@ struct PullRequestSummary: View { /// request is this" without spending any of the strip's width on an answer the reader already /// has from the workspace name. private var helpText: String { - var text = "#\(pullRequest.number) \(pullRequest.title)" + var text = "\(pullRequest.forge.reference(pullRequest.number)) \(pullRequest.title)" if let detail = status.detail { text += "\n\(detail)" } if let reason = status.blockedReason { text += "\n\(reason)" } return text diff --git a/Sources/Bloom/Views/RootView.swift b/Sources/Bloom/Views/RootView.swift index ed32ecc35..25d412016 100644 --- a/Sources/Bloom/Views/RootView.swift +++ b/Sources/Bloom/Views/RootView.swift @@ -152,6 +152,20 @@ struct RootView: View { // `ArchiveRequest` in the core, where it can be tested. Text(request.message) } + // Asked when a project with remotes on both forges is added and no forge is declared. + // Not now writes nothing: detection stays in charge, which picks GitHub, and asks again. + .confirmationDialog( + "Which forge does this project use?", + isPresented: $app.forgeQuestion.isPresent(), + titleVisibility: .visible, + presenting: app.forgeQuestion + ) { repo in + Button(Forge.gitLab.name) { app.answerForgeQuestion(.gitLab, for: repo) } + Button(Forge.gitHub.name) { app.answerForgeQuestion(.gitHub, for: repo) } + Button("Not now", role: .cancel) { app.answerForgeQuestion(nil) } + } message: { repo in + Text("\(repo.name) has remotes on GitHub and GitLab. Every workspace of the project will use the one you choose, and you can change it in the project settings. Until you choose, Bloom uses GitHub.") + } // The question asked before a session that is still working is closed. On the window for // the reason the archive confirmation above is: it is raised from the tab strip's close // button and from Cmd+W in the menu bar, and there is no one control both of those could diff --git a/Sources/Bloom/Views/Sidebar/StartProjectView.swift b/Sources/Bloom/Views/Sidebar/StartProjectView.swift index ad824a23a..1cd7521e1 100644 --- a/Sources/Bloom/Views/Sidebar/StartProjectView.swift +++ b/Sources/Bloom/Views/Sidebar/StartProjectView.swift @@ -702,6 +702,8 @@ struct StartProjectView: View { // asking its root view to present it. openWindow(id: BloomApp.mainWindowID) guard let repo else { return } + // Answered before New Workspace goes up: the main window shows one sheet at a time. + await app.askForgeIfBoth(repo) Task { @MainActor in await Task.yield() NotificationCenter.default.post(name: .bloomNewWorkspace, object: repo) diff --git a/Sources/BloomCore/Bridge/WorkspaceMergeTool.swift b/Sources/BloomCore/Bridge/WorkspaceMergeTool.swift index 03905d191..9342c5427 100644 --- a/Sources/BloomCore/Bridge/WorkspaceMergeTool.swift +++ b/Sources/BloomCore/Bridge/WorkspaceMergeTool.swift @@ -306,14 +306,16 @@ public struct WorkspaceMergeTool: BridgeToolHandling { } private func note(workspace: Workspace, pullRequest: PullRequest, chat: String) -> String { - """ + let forge = pullRequest.forge + return """ Nothing is merged. A turn has begun in '\(workspace.name)', in the chat '\(chat)': its \ - agent has been asked to merge #\(pullRequest.number) and it runs `gh pr merge` itself, \ + agent has been asked to merge \(forge.reference(pullRequest.number)) and it runs \ + `\(forge.command("merge"))` itself, \ where the owner can watch it and answer anything it asks. Bloom does not wait for that \ turn and there is no way to wait for it from here, so do not sit idle. The merge may still \ - not happen: GitHub is allowed to refuse, and the agent is told to stop and report a \ + not happen: \(forge.name) is allowed to refuse, and the agent is told to stop and report a \ refusal rather than force it. To find out what became of it, call workspace_list with \ - include_github and read the pull request's state. + include_github and read the \(forge.request)'s state. """ } diff --git a/Sources/BloomCore/GitHub/ForgeRouting.swift b/Sources/BloomCore/GitHub/ForgeRouting.swift index 9eb9c9f5a..6b55876ec 100644 --- a/Sources/BloomCore/GitHub/ForgeRouting.swift +++ b/Sources/BloomCore/GitHub/ForgeRouting.swift @@ -29,9 +29,17 @@ public enum ForgeRouting { static func isGitLabCom(_ host: String) -> Bool { host == "gitlab.com" || host.hasSuffix(".gitlab.com") } + + /// The hosts beside a GitHub remote that could make the repository GitLab's as well. Empty + /// without a GitHub remote, because then `decide` answers without anybody being asked. + static func hostsBesideGitHub(_ hosts: [String]) -> Set { + guard hosts.contains(where: isGitHub) else { return [] } + return Set(hosts.filter { !isGitHub($0) }) + } } -/// `ForgeRouting` applied to a directory, remembered until git's config or a settings file changes. +/// `ForgeRouting` applied to a directory, remembered until git's config or a settings file changes, +/// or for as long as a glab refusal lasts when that refusal is what made it GitHub. public enum ForgeResolver { private static let resolutions = ForgeResolutions() @@ -53,28 +61,65 @@ public enum ForgeResolver { let fingerprint = fingerprint(of: [configPath].compactMap { $0 } + SettingsLoader.candidatePaths(repo: checkout)) if let known = await resolutions.forge(for: directory, fingerprint: fingerprint) { return known } - let forge = await resolve(directory: directory, checkout: checkout, configPath: configPath) - await resolutions.store(forge, for: directory, fingerprint: fingerprint) + let (forge, lasting) = await resolve(directory: directory, checkout: checkout, configPath: configPath) + // GitHub because glab said no is kept no longer than that no: a VPN that was down comes back. + let expiry = lasting ? nil : ContinuousClock.now + GlabSignIn.refusalLifetime + await resolutions.store(forge, for: directory, fingerprint: fingerprint, expiry: expiry) return forge } - private static func resolve(directory: String, checkout: String, configPath: String?) async -> Forge { - if let declared = SettingsLoader.load(repo: checkout).forge { return declared } + private static func resolve( + directory: String, checkout: String, configPath: String? + ) async -> (forge: Forge, lasting: Bool) { + if let declared = SettingsLoader.load(repo: checkout).forge { return (declared, true) } // Settled from the config file's text where possible, so a GitHub user pays no process. let written = configPath.flatMap { try? String(contentsOfFile: $0, encoding: .utf8) } ?? "" let hosts = remoteURLs(in: written).compactMap { GitHub.repositoryHost($0) } - if hosts.contains(where: ForgeRouting.isGitHub) { return .gitHub } - if !hosts.contains(where: ForgeRouting.isGitLabCom), GlabSignIn.isInstalled() == false { return .gitHub } + if hosts.contains(where: ForgeRouting.isGitHub) { return (.gitHub, true) } + if !hosts.contains(where: ForgeRouting.isGitLabCom), GlabSignIn.isInstalled() == false { return (.gitHub, true) } - let config = (try? await Git.repositoryConfiguration(in: directory)) ?? [:] let context = try? await Git.repositoryContext(in: directory) - let remotes = config.filter { $0.key.hasPrefix("remote.") && $0.key.hasSuffix(".url") }.map(\.value) + let remotes = await configuredRemoteURLs(in: directory) switch ForgeRouting.decide(declared: nil, baseRemoteURL: context?.baseRemoteURL, remoteURLs: remotes) { - case .forge(let decided): return decided - case .askGlab(let host): return await GlabSignIn.shared.isSignedIn(to: host) ? .gitLab : .gitHub + case .forge(let decided): return (decided, true) + case .askGlab(let host): + let signedIn = await GlabSignIn.shared.isSignedIn(to: host) + return (signedIn ? .gitLab : .gitHub, signedIn) } } + /// Whether a repository has remotes on both forges and no `git.forge` settling it. `decide` + /// would quietly pick GitHub there, which is wrong for a GitLab project mirrored to GitHub, + /// so the owner is asked instead and the answer goes through `remember`. Never without glab: + /// GitLab is no answer there, and the settings screen hides the choice for the same reason. + public static func offersBoth(_ directory: String) async -> Bool { + guard GlabSignIn.isInstalled() else { return false } + let checkout = mainCheckout(Git.repositoryPaths(in: directory)) ?? directory + guard SettingsLoader.load(repo: checkout).forge == nil else { return false } + let hosts = await configuredRemoteURLs(in: directory).compactMap { GitHub.repositoryHost($0) } + let others = ForgeRouting.hostsBesideGitHub(hosts) + if others.contains(where: ForgeRouting.isGitLabCom) { return true } + for host in others where await GlabSignIn.shared.isSignedIn(to: host) { return true } + return false + } + + /// Writes the owner's answer to `.bloom/settings.local.toml` in the project's own checkout, + /// which `forge(for:)` reads for every workspace. Not the shared file: remotes are this + /// machine's git config, so a teammate with only the GitHub remote must not inherit it. + public static func remember(_ forge: Forge, for directory: String) throws { + let checkout = mainCheckout(Git.repositoryPaths(in: directory)) ?? directory + let path = (checkout as NSString).appendingPathComponent(".bloom/settings.local.toml") + var document = SettingsDocument(contentsOf: path) + SettingsWriter.apply(.forge(forge), to: &document) + SettingsWriter.prepareFolder(for: path, repo: checkout) + try document.write(to: path) + } + + private static func configuredRemoteURLs(in directory: String) async -> [String] { + let config = (try? await Git.repositoryConfiguration(in: directory)) ?? [:] + return config.filter { $0.key.hasPrefix("remote.") && $0.key.hasSuffix(".url") }.map(\.value) + } + /// The `url = ` values of a git config file, without git. Includes and `insteadOf` are not /// followed; anything they hide reaches the full resolution instead. static func remoteURLs(in config: String) -> [String] { @@ -97,15 +142,16 @@ public enum ForgeResolver { } private actor ForgeResolutions { - private var entries: [String: (fingerprint: [Date?], forge: Forge)] = [:] + private var entries: [String: (fingerprint: [Date?], forge: Forge, expiry: ContinuousClock.Instant?)] = [:] func forge(for directory: String, fingerprint: [Date?]) -> Forge? { - guard let entry = entries[directory], entry.fingerprint == fingerprint else { return nil } + guard let entry = entries[directory], entry.fingerprint == fingerprint, + entry.expiry.map({ ContinuousClock.now < $0 }) ?? true else { return nil } return entry.forge } - func store(_ forge: Forge, for directory: String, fingerprint: [Date?]) { - entries[directory] = (fingerprint, forge) + func store(_ forge: Forge, for directory: String, fingerprint: [Date?], expiry: ContinuousClock.Instant?) { + entries[directory] = (fingerprint, forge, expiry) } } @@ -114,7 +160,7 @@ private actor ForgeResolutions { actor GlabSignIn { static let shared = GlabSignIn() private static let timeout = Duration.seconds(10) - private static let refusalLifetime = Duration.seconds(300) + static let refusalLifetime = Duration.seconds(300) private var answers: [String: (signedIn: Bool, at: ContinuousClock.Instant)] = [:] private var probes: [String: Task] = [:] diff --git a/Sources/BloomCore/Presentation/WorkspaceHoverCard.swift b/Sources/BloomCore/Presentation/WorkspaceHoverCard.swift index 0ca5b1dbe..416926ec6 100644 --- a/Sources/BloomCore/Presentation/WorkspaceHoverCard.swift +++ b/Sources/BloomCore/Presentation/WorkspaceHoverCard.swift @@ -33,11 +33,13 @@ public struct WorkspaceHoverCard: Sendable, Hashable { public var number: Int public var title: String public var url: String + public var forge: Forge - public init(number: Int, title: String, url: String) { + public init(number: Int, title: String, url: String, forge: Forge = .gitHub) { self.number = number self.title = title self.url = url + self.forge = forge } } @@ -100,7 +102,7 @@ public struct WorkspaceHoverCard: Sendable, Hashable { /// `PullRequestOwnership` drops it upstream, which is where that rule lives. static func reference(to pullRequest: PullRequest?) -> PullRequestRef? { pullRequest.map { - PullRequestRef(number: $0.number, title: $0.title, url: $0.url) + PullRequestRef(number: $0.number, title: $0.title, url: $0.url, forge: $0.forge) } } } diff --git a/Tests/BloomCoreTests/ForgeRoutingTests.swift b/Tests/BloomCoreTests/ForgeRoutingTests.swift index bbc5f9327..4605211d3 100644 --- a/Tests/BloomCoreTests/ForgeRoutingTests.swift +++ b/Tests/BloomCoreTests/ForgeRoutingTests.swift @@ -58,6 +58,35 @@ struct ForgeRoutingTests { #expect(await ForgeResolver.forge(for: repo.path) == .gitLab) } + @Test("only hosts beside a GitHub remote can make a repository ambiguous", arguments: [ + (hosts: ["github.com", "gitlab.com"], others: Set(["gitlab.com"])), + (hosts: ["github-work", "git.example.fr"], others: Set(["git.example.fr"])), + (hosts: ["gitlab.com", "git.example.fr"], others: Set()), + (hosts: ["github.com"], others: Set()), + ]) + func hostsBesideGitHub(hosts: [String], others: Set) { + #expect(ForgeRouting.hostsBesideGitHub(hosts) == others) + } + + @Test("a repository on both forges asks once, and the answer stays on this machine", .tags(.git)) + func choice() async throws { + let repo = try await TempRepo() + defer { repo.cleanUp() } + try await Shell.check("git", ["remote", "add", "origin", "git@gitlab.com:group/app.git"], cwd: repo.path) + try await Shell.check("git", ["remote", "add", "mirror", "git@github.com:acme/app.git"], cwd: repo.path) + let glabInstalled: @Sendable ([String], String?) async throws -> ShellResult = { _, _ in ShellResult(status: 1, stdout: "", stderr: "") } + let asked = await GitLab.$commandOverride.withValue(glabInstalled) { await ForgeResolver.offersBoth(repo.path) } + #expect(asked) + #expect(await ForgeResolver.forge(for: repo.path) == .gitHub) + + try ForgeResolver.remember(.gitLab, for: repo.path) + #expect(await ForgeResolver.forge(for: repo.path) == .gitLab) + let askedAgain = await GitLab.$commandOverride.withValue(glabInstalled) { await ForgeResolver.offersBoth(repo.path) } + #expect(askedAgain == false) + #expect(FileManager.default.fileExists(atPath: "\(repo.path)/.bloom/settings.local.toml")) + #expect(FileManager.default.fileExists(atPath: "\(repo.path)/.bloom/settings.toml") == false) + } + @Test("remote addresses are read from git's config file without git") func remoteURLs() { let config = "[remote \"origin\"]\n\turl = git@github.com:acme/app.git\n\tpushurl = x\n[remote \"up\"]\n\turl=\"https://gitlab.com/g/a\"\n" diff --git a/Tests/BloomCoreTests/WorkspaceMergeToolTests.swift b/Tests/BloomCoreTests/WorkspaceMergeToolTests.swift index df30e0f8b..62d49d85d 100644 --- a/Tests/BloomCoreTests/WorkspaceMergeToolTests.swift +++ b/Tests/BloomCoreTests/WorkspaceMergeToolTests.swift @@ -565,6 +565,24 @@ struct WorkspaceMergeToolTests { #expect(!json["state"]!.stringValue!.contains("merge")) } + @Test("on GitLab the answer names the merge request, glab and GitLab") + func theAnswerOnGitLab() async throws { + let store = try makeTestStore("merge-answer-gitlab") + let target = try await workspace(in: store) + var mergeRequest = Self.open() + mergeRequest.forge = .gitLab + + let result = await tool(reading: .found(mergeRequest, local: nil)).call( + request(["workspace": .string(target.id.rawValue)]), as: .owner, store: store + ) + + let note = try #require(try answer(result)["note"]?.stringValue) + #expect(note.contains("asked to merge !42 and it runs `glab mr merge` itself")) + #expect(note.contains("GitLab is allowed to refuse")) + #expect(note.contains("read the merge request's state")) + #expect(!note.contains("gh ")) + } + @Test("squash is what it proposes, and the other two are carried through unaltered") func methods() async throws { let store = try makeTestStore("merge-methods") diff --git a/docs/GITLAB.md b/docs/GITLAB.md index bd7adba11..297e44f1b 100644 --- a/docs/GITLAB.md +++ b/docs/GITLAB.md @@ -21,7 +21,15 @@ It never changes the verdict or its sentence. | Another host, `glab` installed and `glab auth status --hostname ` succeeds | GitLab | | Anything else, including no remote | GitHub | -The answer is cached per directory until git's config or a settings file is written. Where the +A project added with a GitHub remote and a GitLab one (gitlab.com, or a host `glab` is signed in +to) and no `git.forge` is asked which forge it uses. The answer is written as `git.forge` to +`.bloom/settings.local.toml` in the project's checkout, which every workspace reads: remotes are +this machine's git config, so the answer is not committed for the team. "Not now" writes nothing: the +table above stays in charge, so GitHub, and the question comes back the next time the folder is +added. + +The answer is cached per directory until git's config or a settings file is written, or for five +minutes when it is GitHub only because `glab` said no. Where the config file's own text settles it, which is every GitHub remote and every machine without `glab`, no process runs at all. The `glab` probe runs at most once per host at a time, a yes is kept for the launch and a no for five minutes.