diff --git a/src/api/api.d.ts b/src/api/api.d.ts index 89d1cfd021..816f86a86f 100644 --- a/src/api/api.d.ts +++ b/src/api/api.d.ts @@ -85,6 +85,7 @@ export interface RepositoryState { readonly mergeChanges: Change[]; readonly indexChanges: Change[]; readonly workingTreeChanges: Change[]; + readonly untrackedChanges?: Change[]; readonly onDidChange: Event; } @@ -211,6 +212,8 @@ export interface Repository { fetch(remote?: string, ref?: string, depth?: number): Promise; pull(unshallow?: boolean): Promise; push(remoteName?: string, branchName?: string, setUpstream?: boolean): Promise; + pushRefWithLease?(remote: string, branch: string, newSha: string, expectedSha: string): Promise; + getRemoteRefs?(remote: string, opts?: { heads?: boolean; tags?: boolean }): Promise; blame(path: string): Promise; log(options?: LogOptions): Promise; @@ -219,6 +222,10 @@ export interface Repository { add(paths: string[]): Promise; merge(ref: string): Promise; mergeAbort(): Promise; + rebase?(upstream: string, options?: { onto?: string; rebaseMerges?: boolean }): Promise; + rebaseAbort?(): Promise; + updateRef?(ref: string, newSha: string, oldSha: string): Promise; + resetKeep?(ref: string): Promise; createWorktree?(options?: { path?: string; commitish?: string; branch?: string }): Promise; deleteWorktree?(path: string, options?: { force?: boolean }): Promise; @@ -253,6 +260,7 @@ export interface IGit { registerPostCommitCommandsProvider?(provider: PostCommitCommandsProvider): Disposable; getRepositoryWorkspace?(uri: Uri): Promise; + openWorktreeRepository?(uri: Uri): Promise; clone?(uri: Uri, options?: CloneOptions): Promise; } diff --git a/src/commands.ts b/src/commands.ts index 259f408fc7..d93e662188 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -193,7 +193,7 @@ export function registerCommands( ) { const logId = 'RegisterCommands'; - PullRequestOverviewPanel.registerGlobalCommands(context, telemetry); + PullRequestOverviewPanel.registerGlobalCommands(context, telemetry, reposManager); context.subscriptions.push( vscode.commands.registerCommand( diff --git a/src/gitProviders/builtinGit.ts b/src/gitProviders/builtinGit.ts index bb6df80374..a4367fdb49 100644 --- a/src/gitProviders/builtinGit.ts +++ b/src/gitProviders/builtinGit.ts @@ -50,6 +50,10 @@ export class BuiltinGitProvider extends Disposable implements IGit { return this._gitAPI.getRepositoryWorkspace(uri); } + openWorktreeRepository(uri: vscode.Uri): Promise { + return this._gitAPI.openRepository(uri); + } + clone(uri: vscode.Uri, options?: CloneOptions): Promise { return this._gitAPI.clone(uri, options); } diff --git a/src/github/folderRepositoryManager.ts b/src/github/folderRepositoryManager.ts index a447e2608c..43d9e715b9 100644 --- a/src/github/folderRepositoryManager.ts +++ b/src/github/folderRepositoryManager.ts @@ -395,6 +395,14 @@ export class FolderRepositoryManager extends Disposable { return this._repository; } + async openWorktreeRepository(uri: vscode.Uri): Promise { + const provider = this._git.getGitProvider(this._repository.rootUri); + if (!provider?.openWorktreeRepository) { + throw new Error(vscode.l10n.t('The Git provider cannot open a temporary worktree.')); + } + return provider.openWorktreeRepository(uri); + } + set repository(repository: Repository) { this._repository = repository; } diff --git a/src/github/graphql.ts b/src/github/graphql.ts index 36aaa10682..ad09d16b7f 100644 --- a/src/github/graphql.ts +++ b/src/github/graphql.ts @@ -832,9 +832,13 @@ export interface PullRequestStackResponse { url: string; state: GithubItemStateEnum; isDraft: boolean; + baseRefName: string; + baseRepository: { owner: { login: string } } | null; headRefName: string; + headRepository: { owner: { login: string } } | null; mergeable: 'MERGEABLE' | 'CONFLICTING' | 'UNKNOWN'; mergeStateStatus: 'BEHIND' | 'BLOCKED' | 'CLEAN' | 'DIRTY' | 'HAS_HOOKS' | 'UNKNOWN' | 'UNSTABLE'; + mergeQueueEntry?: { state: string } | null; }; }[]; pageInfo: { hasNextPage: boolean; endCursor: string | null }; diff --git a/src/github/interface.ts b/src/github/interface.ts index ac04839e69..19d4f06f6a 100644 --- a/src/github/interface.ts +++ b/src/github/interface.ts @@ -27,6 +27,7 @@ export interface PullRequestStack { position: number; size: number; base: string; + needsUpdate?: boolean; pullRequests: { position: number; number: number; @@ -35,6 +36,7 @@ export interface PullRequestStack { head: string; state: GithubItemStateEnum; isDraft: boolean; + isQueued?: boolean; mergeable: PullRequestMergeability; }[]; } @@ -47,6 +49,24 @@ export enum PullRequestMergeability { Behind, } +export function getUpdatableStackEntries(stack: PullRequestStack): PullRequestStack['pullRequests'] | undefined { + if (stack.pullRequests.length !== stack.size) { + return; + } + const firstClosed = stack.pullRequests.findIndex(entry => entry.state === GithubItemStateEnum.Closed); + const entries = firstClosed < 0 ? stack.pullRequests : stack.pullRequests.slice(0, firstClosed); + if (!entries.length || entries.some(entry => entry.state !== GithubItemStateEnum.Open || entry.isQueued + || entry.mergeable === PullRequestMergeability.Conflict || entry.mergeable === PullRequestMergeability.Unknown) + || stack.pullRequests.slice(entries.length).some(entry => entry.state !== GithubItemStateEnum.Closed)) { + return; + } + return entries; +} + +export function isStackUpdatable(stack: PullRequestStack): boolean { + return !!stack.needsUpdate && !!getUpdatableStackEntries(stack); +} + export function isStackMergeable(stack: PullRequestStack, number: number): boolean { const current = stack.pullRequests.find(entry => entry.number === number); if (!current || current.position !== stack.position || current.state !== GithubItemStateEnum.Open diff --git a/src/github/pullRequestModel.ts b/src/github/pullRequestModel.ts index 94d7cf8378..75004e21bc 100644 --- a/src/github/pullRequestModel.ts +++ b/src/github/pullRequestModel.ts @@ -1716,6 +1716,7 @@ export class PullRequestModel extends IssueModel implements IPullRe const { query, remote, schema } = await this.githubRepository.ensure(); let stack: PullRequestStack | undefined; let after: string | null = null; + const comparisons = new Map(); while (true) { let data: PullRequestStackResponse; @@ -1757,22 +1758,50 @@ export class PullRequestModel extends IssueModel implements IPullRe position: pullRequest.stackEntry.position, size: pullRequest.stack.size, base: pullRequest.stack.baseRefName, + needsUpdate: false, pullRequests: [], }; - stack.pullRequests.push(...pullRequest.stack.entries.nodes.map(entry => ({ - position: entry.position, - number: entry.pullRequest.number, - title: entry.pullRequest.title, - url: entry.pullRequest.url, - head: entry.pullRequest.headRefName, - state: entry.pullRequest.state, - isDraft: entry.pullRequest.isDraft, - mergeable: parseMergeability(entry.pullRequest.mergeable, entry.pullRequest.mergeStateStatus), - }))); + stack.pullRequests.push(...pullRequest.stack.entries.nodes.map(entry => { + const pr = entry.pullRequest; + if (pr.state === GithubItemStateEnum.Open) { + if (!pr.baseRepository || !pr.headRepository || !pr.baseRefName || !pr.headRefName) { + throw new Error(`Missing branch information for pull request #${pr.number} in this stack.`); + } + comparisons.set(pr.number, { + base: `${pr.baseRepository.owner.login}:${pr.baseRefName}`, + head: `${pr.headRepository.owner.login}:${pr.headRefName}`, + }); + } + return { + position: entry.position, + number: pr.number, + title: pr.title, + url: pr.url, + head: pr.headRefName, + state: pr.state, + isDraft: pr.isDraft, + isQueued: !!pr.mergeQueueEntry, + mergeable: parseMergeability(pr.mergeable, pr.mergeStateStatus), + }; + })); const pageInfo = pullRequest.stack.entries.pageInfo; if (!pageInfo.hasNextPage) { stack.pullRequests.sort((a, b) => a.position - b.position); + const outdated = await Promise.all(stack.pullRequests.filter(entry => + entry.state === GithubItemStateEnum.Open && entry.mergeable !== PullRequestMergeability.Conflict, + ).map(async entry => { + const refs = comparisons.get(entry.number)!; + const comparison = await this.githubRepository.compareCommits(refs.base, refs.head); + if (comparison?.behind_by === undefined) { + throw new Error(`Unable to check whether pull request #${entry.number} is behind its stack base.`); + } + if (comparison.behind_by > 0) { + entry.mergeable = PullRequestMergeability.Behind; + } + return comparison.behind_by > 0; + })); + stack.needsUpdate = outdated.some(Boolean); return stack; } if (!pageInfo.endCursor || pageInfo.endCursor === after) { diff --git a/src/github/pullRequestOverview.ts b/src/github/pullRequestOverview.ts index e2d9d5b3b1..4bd49f00e9 100644 --- a/src/github/pullRequestOverview.ts +++ b/src/github/pullRequestOverview.ts @@ -7,12 +7,14 @@ import * as crypto from 'crypto'; import * as vscode from 'vscode'; import { OpenCommitChangesArgs, OpenLocalFileArgs } from '../../common/views'; +import { Remote as GitRemote, Repository } from '../api/api'; import { openItemOnGitHub } from '../commands'; import { addAttestationCommit, isAttestationCommitsEnabled } from './attestationCommit'; import { getCopilotApi } from './copilotApi'; import { SessionIdForPr } from './copilotRemoteAgent'; import { FolderRepositoryManager } from './folderRepositoryManager'; import { + getUpdatableStackEntries, GithubItemStateEnum, IAccount, isITeam, @@ -28,6 +30,8 @@ import { IssueOverviewPanel, panelKey } from './issueOverview'; import { isCopilotOnMyBehalf, PullRequestModel } from './pullRequestModel'; import { PullRequestReviewCommon, ReviewContext } from './pullRequestReviewCommon'; import { branchPicks, pickEmail, reviewersQuickPick } from './quickPicks'; +import type { RepositoriesManager } from './repositoriesManager'; +import { supportsStackGitOperations, updateStackBranches } from './updateStackBranches'; import { getIssueOrURLExpression, parseIssueExpressionOutput, parseReviewers, processDiffLinks, processPermalinks } from './utils'; import { CancelCodingAgentReply, ChangeBaseReply, ChangeReviewersReply, DeleteReviewResult, MergeArguments, MergeResult, PullRequest, ReadyForReviewAndMergeContext, ReadyForReviewContext, ReviewCommentContext, ReviewType, SubmitReviewArgs, UnresolvedIdentity, UnstackAllResult } from './views'; import { debounce } from '../common/async'; @@ -35,8 +39,10 @@ import { COPILOT_ACCOUNTS, IComment } from '../common/comment'; import { COPILOT_REVIEWER, COPILOT_REVIEWER_ACCOUNT, COPILOT_SWE_AGENT, copilotEventToStatus, CopilotPRStatus, mostRecentCopilotEvent } from '../common/copilot'; import { commands, contexts } from '../common/executeCommands'; import { openWithDefaultExternalOpener } from '../common/externalUri'; +import { findLocalRepoRemoteFromGitHubRef } from '../common/githubRef'; import { disposeAll } from '../common/lifecycle'; import Logger from '../common/logger'; +import { parseRemote } from '../common/remote'; import { CHECKOUT_DEFAULT_BRANCH, CHECKOUT_PULL_REQUEST_BASE_BRANCH, DEFAULT_MERGE_METHOD, DELETE_BRANCH_AFTER_MERGE, EXPERIMENTAL_STACKS, POST_DONE, PR_SETTINGS_NAMESPACE } from '../common/settingKeys'; import { areStacksEnabled, assertStacksEnabled } from '../common/settingsUtils'; import { ITelemetry } from '../common/telemetry'; @@ -55,6 +61,8 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel = new Map(); + private static _repositoriesManager: RepositoriesManager | undefined; + private static readonly _updatingStacks = new Set(); /** * Event emitter for when a PR overview becomes active @@ -73,8 +81,69 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel | undefined; private _updateSequence = 0; private _previewSequence = 0; + private _updatingStack = false; + private _canUpdateStackAccess = false; + private _updatableStackPullRequestNumbers: Set | undefined; private _resolveCommentThreadQueue: Promise = Promise.resolve(); + private getStackManagers(): FolderRepositoryManager[] { + return [...new Set([this._folderRepositoryManager, ...(PullRequestOverviewPanel._repositoriesManager?.folderManagers ?? [])])]; + } + + private getStackRepository(item: PullRequestModel): { manager: FolderRepositoryManager; repository: Repository; remote: GitRemote } | undefined { + if (vscode.env.uiKind !== vscode.UIKind.Desktop || !item.head) { + return; + } + const required = ['rebase', 'rebaseAbort', 'pushRefWithLease', 'getRemoteRefs', 'updateRef', + 'resetKeep', 'createWorktree', 'deleteWorktree'] as const; + const candidates = this.getStackManagers().flatMap(manager => { + const repository = manager.repository; + if (repository.rootUri.scheme !== 'file' || required.some(method => typeof repository[method] !== 'function')) { + return []; + } + const remote = findLocalRepoRemoteFromGitHubRef(repository, item.head!); + const push = remote && parseRemote(remote.name, remote.pushUrl ?? remote.fetchUrl ?? ''); + return remote && !remote.isReadOnly && push + && push.owner.toLowerCase() === item.remote.owner.toLowerCase() + && push.repositoryName.toLowerCase() === item.remote.repositoryName.toLowerCase() + && push.matchesServerUri(item.githubRepository.hub.serverUri) + ? [{ manager, repository, remote }] : []; + }); + const selected = candidates.find(candidate => this.sameActiveRepository(candidate.manager, item)) + ?? candidates.find(candidate => candidate.manager === this._folderRepositoryManager) + ?? candidates[0]; + return selected; + } + + private sameActiveRepository(manager: FolderRepositoryManager, item: PullRequestModel): boolean { + const active = manager.activePullRequest; + return !!active && active.remote.owner.toLowerCase() === item.remote.owner.toLowerCase() + && active.remote.repositoryName.toLowerCase() === item.remote.repositoryName.toLowerCase() + && active.githubRepository.remote.normalizedHost === item.githubRepository.remote.normalizedHost; + } + + private postCheckoutStatus(): void { + if (this._item) { + const checkedOutPullRequestNumber = this.getCheckedOutPullRequestNumber(this._item); + this._postMessage({ + command: 'pr.update-checkout-status', + isCurrentlyCheckedOut: checkedOutPullRequestNumber === this._item.number, + canUpdateStack: this.canUpdateStack(this._item), + }); + } + } + + private canUpdateStack(item: PullRequestModel): boolean { + const checkedOutPullRequestNumber = this.getCheckedOutPullRequestNumber(item); + return areStacksEnabled() && this._canUpdateStackAccess + && checkedOutPullRequestNumber !== undefined && !!this._updatableStackPullRequestNumbers?.has(checkedOutPullRequestNumber) + && !!this.getStackRepository(item); + } + + private getCheckedOutPullRequestNumber(item: PullRequestModel): number | undefined { + return this.getStackManagers().find(manager => this.sameActiveRepository(manager, item))?.activePullRequest?.number; + } + public static override async createOrShow( telemetry: ITelemetry, extensionUri: vscode.Uri, @@ -195,8 +264,17 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel { + for (const panel of this._panels.values()) { + if (panel._folderRepositoryManager !== manager) { + panel.postCheckoutStatus(); + } + } + }), + { dispose: () => { this._repositoriesManager = undefined; } }, vscode.commands.registerCommand('pr.readyForReviewDescription', async (ctx: ReadyForReviewContext) => { const panel = PullRequestOverviewPanel.findPanel(ctx.owner, ctx.repo, ctx.number); if (panel) { @@ -267,15 +345,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel { - if (this._folderRepositoryManager && this._item) { - const isCurrentlyCheckedOut = this._item.equals(this._folderRepositoryManager.activePullRequest); - this._postMessage({ - command: 'pr.update-checkout-status', - isCurrentlyCheckedOut, - }); - } - })); + this._prListeners.push(this._folderRepositoryManager.onDidChangeActivePullRequest(() => this.postCheckoutStatus())); if (this._item) { this._prListeners.push(this._item.onDidChange(e => { @@ -457,12 +527,13 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel(POST_DONE, CHECKOUT_DEFAULT_BRANCH); const doneCheckoutBranch = postDoneAction.startsWith(CHECKOUT_PULL_REQUEST_BASE_BRANCH) @@ -495,6 +567,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel = { ...baseContext, + canUpdateStack: this.canUpdateStack(pullRequest), canRequestCopilotReview: false, isCurrentlyCheckedOut: isCurrentlyCheckedOut, isRemoteBaseDeleted: pullRequest.isRemoteBaseDeleted, @@ -677,12 +750,16 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel entry.number)) : undefined; onLoaded(); await this._postMessage({ command: 'pr.update', pullrequest: { stack: linkedStack, stackLoaded: true, + canUpdateStack: this.canUpdateStack(pullRequestModel), ...(stack ? { mergeQueueMethod: stackQueueMethod } : {}), } satisfies Partial, }); @@ -690,7 +767,8 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel }); + this._updatableStackPullRequestNumbers = undefined; + void this._postMessage({ command: 'pr.update', pullrequest: { stackLoadError: true, canUpdateStack: false } satisfies Partial }); } } } @@ -786,6 +864,8 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel): Promise { + let ownsUpdate = false; + let lockKey: string | undefined; + let stackNumbers: number[] = []; + try { + if (!areStacksEnabled()) { + throw new Error(vscode.l10n.t('Pull request stack features are disabled.')); + } + if (this._updatingStack) { + throw new Error(vscode.l10n.t('The pull request stack is already being updated.')); + } + this._updatingStack = true; + ownsUpdate = true; + const access = await this._folderRepositoryManager.getPullRequestRepositoryAccessAndMergeMethods(this._item); + if (!access.hasWritePermission) { + throw new Error(vscode.l10n.t('You do not have permission to update this stack.')); + } + const stack = await this._item.getStack(); + const openEntries = stack && getUpdatableStackEntries(stack); + if (!stack || !openEntries) { + throw new Error(vscode.l10n.t('Update stack requires an open, conflict-free chain of pull requests.')); + } + const activeNumber = this.getCheckedOutPullRequestNumber(this._item); + if (activeNumber === undefined || !openEntries.some(entry => entry.number === activeNumber)) { + throw new Error(vscode.l10n.t('Check out a pull request in this stack before updating it.')); + } + stackNumbers = stack.pullRequests.map(entry => entry.number); + const key = `${this._identity.owner.toLowerCase()}/${this._identity.repo.toLowerCase()}#${stack.pullRequests[0].number}`; + if (PullRequestOverviewPanel._updatingStacks.has(key)) { + throw new Error(vscode.l10n.t('The pull request stack is already being updated.')); + } + lockKey = key; + PullRequestOverviewPanel._updatingStacks.add(lockKey); + const action = vscode.l10n.t('Update stack'); + const answer = await vscode.window.showWarningMessage( + vscode.l10n.t('Rebase and push {0} pull requests in this stack?', openEntries.length), + { + modal: true, detail: stack.size > openEntries.length + ? vscode.l10n.t('Closed pull requests at the top of the stack will be skipped. Open branches will be rebased and rewritten on GitHub.') + : vscode.l10n.t('This rewrites the stack branches on GitHub. The extension checks for conflicts before pushing and stops if a branch changes on GitHub.') + }, + action, + ); + if (answer !== action) { + await this._replyMessage(message, { updatedPullRequests: [] }); + return; + } + const updatedPullRequests = await vscode.window.withProgress( + { location: vscode.ProgressLocation.Notification, title: vscode.l10n.t('Updating pull request stack') }, + async progress => { + progress.report({ message: vscode.l10n.t('Checking stack branches') }); + const current = await this._item.getStack(); + const currentOpenEntries = current && getUpdatableStackEntries(current); + if (!current || !currentOpenEntries || currentOpenEntries.length !== openEntries.length + || this.getCheckedOutPullRequestNumber(this._item) !== activeNumber + || current.size !== stack.size || current.pullRequests.some((entry, index) => + entry.number !== stack.pullRequests[index].number || entry.head !== stack.pullRequests[index].head)) { + throw new Error(vscode.l10n.t('The pull request stack changed. Refresh and try again.')); + } + const target = this.getStackRepository(this._item); + if (!target) { + throw new Error(vscode.l10n.t('A writable Git remote for this stack is not available locally.')); + } + const { manager, repository, remote } = target; + if (!supportsStackGitOperations(repository)) { + throw new Error(vscode.l10n.t('A newer built-in Git extension is required to update this stack.')); + } + const branches = await Promise.all(currentOpenEntries.map(async (entry, index) => { + const pr = await this._item.githubRepository.getPullRequest(entry.number, 'updateStack'); + const base = index === 0 ? current.base : current.pullRequests[index - 1].head; + if (!pr || pr.state !== GithubItemStateEnum.Open || !pr.head || !pr.base + || pr.mergeQueueEntry || pr.head.ref !== entry.head || pr.base.ref !== base + || pr.head.owner.toLowerCase() !== this._item.remote.owner.toLowerCase() + || pr.base.owner.toLowerCase() !== this._item.remote.owner.toLowerCase() + || pr.head.repositoryCloneUrl.repositoryName.toLowerCase() !== this._item.remote.repositoryName.toLowerCase() + || pr.base.repositoryCloneUrl.repositoryName.toLowerCase() !== this._item.remote.repositoryName.toLowerCase()) { + throw new Error(vscode.l10n.t('Pull request #{0} is no longer part of a supported, open branch chain.', entry.number)); + } + return { number: entry.number, base, head: entry.head, sha: pr.head.sha }; + })); + if (!areStacksEnabled()) { + throw new Error(vscode.l10n.t('Pull request stack features are disabled.')); + } + return updateStackBranches(repository, + path => manager.openWorktreeRepository(vscode.Uri.file(path)), + remote.name, branches, activeNumber, step => progress.report({ message: step })); + }, + ); + await this._replyMessage(message, { updatedPullRequests }); + await PullRequestOverviewPanel.refreshStackPanels(this._identity.owner, this._identity.repo, + stackNumbers); + void vscode.window.showInformationMessage(updatedPullRequests.length + ? vscode.l10n.t('Pull request stack updated.') + : vscode.l10n.t('Pull request stack is already up to date.')); + } catch (error) { + Logger.error(`Failed to update pull request stack: ${formatError(error)}`, PullRequestOverviewPanel.ID); + void vscode.window.showErrorMessage(vscode.l10n.t('Unable to update pull request stack: {0}', formatError(error))); + await this._throwError(message, formatError(error)); + if (ownsUpdate && stackNumbers.length) { + try { + await PullRequestOverviewPanel.refreshStackPanels(this._identity.owner, this._identity.repo, stackNumbers); + } catch (refreshError) { + Logger.error(`Failed to refresh pull request stack after update error: ${formatError(refreshError)}`, PullRequestOverviewPanel.ID); + } + } + } finally { + if (lockKey) { + PullRequestOverviewPanel._updatingStacks.delete(lockKey); + } + if (ownsUpdate) { + this._updatingStack = false; + } + } + } + private async unstackAll(message: IRequestMessage): Promise { + let acquiredLockKey: string | undefined; try { assertStacksEnabled(); const access = await this._folderRepositoryManager.getPullRequestRepositoryAccessAndMergeMethods(this._item); @@ -1142,6 +1338,10 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel pr.number); + const lockKey = `${this._identity.owner.toLowerCase()}/${this._identity.repo.toLowerCase()}#${stack.pullRequests[0].number}`; + if (PullRequestOverviewPanel._updatingStacks.has(lockKey)) { + throw new Error(vscode.l10n.t('The pull request stack is already being updated.')); + } const action = vscode.l10n.t('Unstack all'); const answer = await vscode.window.showWarningMessage( vscode.l10n.t('Unstack all eligible pull requests?'), @@ -1155,6 +1355,11 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel(); readonly onDidChangeFolderRepositories = this._onDidChangeFolderRepositories.event; + private _onDidChangeActivePullRequest = new vscode.EventEmitter(); + readonly onDidChangeActivePullRequest = this._onDidChangeActivePullRequest.event; private _onDidLoadAnyRepositories = new vscode.EventEmitter(); readonly onDidLoadAnyRepositories = this._onDidLoadAnyRepositories.event; @@ -113,7 +115,10 @@ export class RepositoriesManager extends Disposable { this._onDidLoadAnyRepositories.fire(); }), folderManager.onDidChangeRepositories(() => this._onDidLoadAnyRepositories.fire()), - folderManager.onDidChangeActivePullRequest(() => this.updateActiveReviewCount()), + folderManager.onDidChangeActivePullRequest(() => { + this.updateActiveReviewCount(); + this._onDidChangeActivePullRequest.fire(folderManager); + }), folderManager.onDidDispose(() => this.removeRepo(folderManager.repository)), folderManager.onDidChangeAnyPullRequests(e => this._onDidChangeAnyPullRequests.fire(e)), folderManager.onDidAddPullRequest(e => this._onDidAddPullRequest.fire(e)), diff --git a/src/github/updateStackBranches.ts b/src/github/updateStackBranches.ts new file mode 100644 index 0000000000..c0c7b48040 --- /dev/null +++ b/src/github/updateStackBranches.ts @@ -0,0 +1,246 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { tmpdir } from 'os'; +import * as path from 'path'; +import { Repository } from '../api/api'; +import { generateUuid } from '../common/uuid'; + +export interface StackBranch { + number: number; + head: string; + base: string; + sha: string; +} + +export type StackGitRepository = Repository & Required>; + +export function supportsStackGitOperations(repository: Repository): repository is StackGitRepository { + return typeof repository.createWorktree === 'function' + && typeof repository.deleteWorktree === 'function' + && typeof repository.rebase === 'function' + && typeof repository.rebaseAbort === 'function' + && typeof repository.resetKeep === 'function' + && typeof repository.updateRef === 'function' + && typeof repository.pushRefWithLease === 'function' + && typeof repository.getRemoteRefs === 'function'; +} + +async function localBranches(repository: Repository): Promise> { + const branches = await repository.getBranches({ remote: false }); + const result = new Map(); + for (const branch of branches) { + if (branch.name && branch.commit) { + result.set(branch.name, branch.commit); + } + } + return result; +} + +async function ensureClean(repository: Repository): Promise { + await repository.status(); + const { indexChanges, workingTreeChanges, mergeChanges } = repository.state; + if (indexChanges.length || workingTreeChanges.length || mergeChanges.length || repository.state.untrackedChanges?.length) { + throw new Error('Commit or stash all working tree changes before updating the stack.'); + } +} + +function checkedOutElsewhere(repository: Repository, name: string, activeName: string): boolean { + return name !== activeName && (repository.state.worktrees?.some(worktree => + !worktree.detached && worktree.ref === `refs/heads/${name}`) ?? false); +} + +async function verifyBases(repository: StackGitRepository, remote: string, branches: readonly StackBranch[], + original: Map, skipped: number): Promise { + const remoteRefs = await repository.getRemoteRefs(remote, { heads: true }); + for (const ref of [branches[0].base, ...branches.slice(0, skipped).map(branch => branch.head)]) { + if (remoteRefs.find(branch => branch.name === ref)?.commit !== original.get(ref)) { + throw new Error(`Base branch ${ref} changed on GitHub. Refresh and try again.`); + } + } +} + +export async function updateStackBranches( + repository: StackGitRepository, + openWorktree: (worktreePath: string) => Promise, + remote: string, + branches: readonly StackBranch[], + activeNumber: number, + report: (message: string) => void, +): Promise { + if (!branches.length || new Set(branches.map(branch => branch.number)).size !== branches.length + || new Set([branches[0].base, ...branches.map(branch => branch.head)]).size !== branches.length + 1 + || !branches.some(branch => branch.number === activeNumber) + || branches.some((branch, index) => !branch.sha || index > 0 && branch.base !== branches[index - 1].head)) { + throw new Error('Pull request stack branches do not form a chain.'); + } + + report('Fetching stack branches'); + const refs = [branches[0].base, ...branches.map(branch => branch.head)]; + for (const ref of refs) { + await repository.fetch(remote, `+refs/heads/${ref}:refs/remotes/${remote}/${ref}`); + } + const original = new Map(); + for (const ref of refs) { + const branch = await repository.getBranch(`refs/remotes/${remote}/${ref}`); + if (!branch.commit) { + throw new Error(`Could not resolve remote stack branch ${ref}.`); + } + original.set(ref, branch.commit); + } + for (const branch of branches) { + if (original.get(branch.head) !== branch.sha) { + throw new Error(`Pull request #${branch.number} changed on GitHub. Refresh and try again.`); + } + } + + const activeName = repository.state.HEAD?.name; + const active = branches.find(branch => branch.number === activeNumber)!; + if (!activeName || (await repository.getBranch(activeName)).commit !== active.sha) { + throw new Error(`Checked-out branch has local commits that differ from pull request #${activeNumber}.`); + } + await ensureClean(repository); + const heads = await localBranches(repository); + for (const branch of branches) { + const sha = heads.get(branch.head); + if (sha && sha !== branch.sha) { + throw new Error(`Local branch ${branch.head} differs from pull request #${branch.number}. Preserve those commits before updating the stack.`); + } + if (checkedOutElsewhere(repository, branch.head, activeName)) { + throw new Error(`Branch ${branch.head} is checked out in another worktree. Close that worktree before updating the stack.`); + } + } + const existingNames = [...heads.keys()].map(name => name.toLowerCase()); + for (const branch of branches) { + const name = branch.head.toLowerCase(); + if (!heads.has(branch.head) && existingNames.some(existing => + existing === name || existing.startsWith(`${name}/`) || name.startsWith(`${existing}/`))) { + throw new Error(`Local branch ${branch.head} cannot be created because its name conflicts with another branch.`); + } + } + + const forks: string[] = []; + let startIndex = branches.length; + for (const [index, branch] of branches.entries()) { + const base = original.get(branch.base)!; + const fork = await repository.getMergeBase(base, branch.sha); + if (!fork) { + throw new Error(`Could not find a common ancestor for pull request #${branch.number}.`); + } + forks.push(fork); + if (startIndex === branches.length && fork !== base) { + startIndex = index; + } + } + const branchesToRebase = branches.slice(startIndex); + if (!branchesToRebase.length) { + await verifyBases(repository, remote, branches, original, startIndex); + return []; + } + + let worktreePath: string | undefined; + const updated: number[] = []; + let failure: Error | undefined; + try { + worktreePath = await repository.createWorktree({ + path: path.join(tmpdir(), `vscode-pr-stack-${generateUuid()}`), + commitish: original.get(branchesToRebase[0].head)!, + }); + const opened = await openWorktree(worktreePath); + if (!opened || !supportsStackGitOperations(opened)) { + throw new Error('The Git extension could not open the temporary stack worktree.'); + } + const rebased = new Map(); + for (const [index, branch] of branchesToRebase.entries()) { + const oldHead = original.get(branch.head)!; + const oldBase = original.get(branch.base)!; + const newBase = rebased.get(branch.base) ?? oldBase; + report(`Rebasing #${branch.number} (${branch.head}) onto ${branch.base}`); + await opened.checkout(oldHead); + try { + await opened.rebase(forks[startIndex + index], { onto: newBase, rebaseMerges: true }); + } catch (error) { + await opened.rebaseAbort(); + throw new Error(`Rebasing #${branch.number} (${branch.head}) caused a conflict or failed: ${error}`); + } + rebased.set(branch.head, (await opened.getCommit('HEAD')).hash); + } + + await verifyBases(repository, remote, branches, original, startIndex); + await ensureClean(repository); + if (repository.state.HEAD?.name !== activeName || (await repository.getBranch(activeName)).commit !== active.sha) { + throw new Error('The checked-out branch changed during the update. No branches were pushed.'); + } + const beforePush = await localBranches(repository); + for (const branch of branchesToRebase) { + if (beforePush.get(branch.head) !== heads.get(branch.head)) { + throw new Error(`Local branch ${branch.head} changed during the update. No branches were pushed.`); + } + } + for (const branch of branchesToRebase) { + await repository.status(); + if (repository.state.HEAD?.name !== activeName) { + throw new Error(`Checked-out branch changed during the update; already updated: ${updated.length ? updated.map(number => `#${number}`).join(', ') : 'none'}.`); + } + const newHead = rebased.get(branch.head)!; + if (newHead !== branch.sha) { + report(`Pushing #${branch.number} (${branch.head})`); + try { + await repository.pushRefWithLease(remote, branch.head, newHead, branch.sha); + } catch (error) { + throw new Error(`Could not push #${branch.number} (${branch.head}); already updated: ${updated.length ? updated.map(number => `#${number}`).join(', ') : 'none'}. ${error}`); + } + updated.push(branch.number); + } + report(`Updating local branch ${branch.head}`); + try { + await repository.status(); + if (repository.state.HEAD?.name !== activeName) { + throw new Error('The checked-out branch changed during the push.'); + } + if (branch.number === activeNumber) { + if ((await repository.getBranch(activeName)).commit !== active.sha) { + throw new Error(`Checked-out branch ${activeName} changed while updating the stack.`); + } + await ensureClean(repository); + if (activeName !== branch.head) { + if (checkedOutElsewhere(repository, branch.head, activeName)) { + throw new Error(`Branch ${branch.head} was checked out in another worktree during the update.`); + } + await repository.updateRef(`refs/heads/${branch.head}`, newHead, heads.get(branch.head) ?? '0'.repeat(40)); + } + if (newHead !== active.sha) { + await repository.resetKeep(newHead); + } + } else { + if (checkedOutElsewhere(repository, branch.head, activeName)) { + throw new Error(`Branch ${branch.head} was checked out in another worktree during the update.`); + } + await repository.updateRef(`refs/heads/${branch.head}`, newHead, heads.get(branch.head) ?? '0'.repeat(40)); + } + if (!heads.has(branch.head)) { + await repository.setBranchUpstream(branch.head, `refs/remotes/${remote}/${branch.head}`); + } + } catch (error) { + throw new Error(`Pull request #${branch.number} was pushed${newHead === branch.sha ? ' previously' : ''} but local branch ${branch.head} could not be updated. Reconcile the local branch before retrying: ${error}`); + } + } + } catch (error) { + failure = error instanceof Error ? error : new Error(String(error)); + } finally { + if (worktreePath) { + try { + await repository.deleteWorktree(worktreePath, { force: true }); + } catch (error) { + failure = new Error(`${failure ? `${failure.message} ` : ''}Could not remove temporary stack worktree ${worktreePath}: ${error}`); + } + } + } + if (failure) { + throw failure; + } + return updated; +} diff --git a/src/github/views.ts b/src/github/views.ts index 67c7cb195d..9795b2ac75 100644 --- a/src/github/views.ts +++ b/src/github/views.ts @@ -81,6 +81,7 @@ export type PullRequestPreview = Pick(); + const worktree = new MockRepository(); + const openWorktreeRepository = sinon.stub().resolves(worktree); + const unrelated = new MockRepository(); + unrelated.rootUri = Uri.file('/unrelated'); + const unrelatedOpen = sinon.stub().resolves(unrelated); + git.registerGitProvider({ + repositories: [unrelated], + onDidOpenRepository: events.event, + onDidCloseRepository: events.event, + openWorktreeRepository: unrelatedOpen, + }); + const provider: IGit = { + repositories: [repository], + onDidOpenRepository: events.event, + onDidCloseRepository: events.event, + openWorktreeRepository, + }; + git.registerGitProvider(provider); + const uri = Uri.file('/tmp/pr-stack-worktree'); + + assert.strictEqual(await manager.openWorktreeRepository(uri), worktree); + assert(openWorktreeRepository.calledOnceWithExactly(uri)); + assert(unrelatedOpen.notCalled); + events.dispose(); + }); + + it('reports a provider that cannot open temporary worktrees', async function () { + const events = new EventEmitter(); + git.registerGitProvider({ + repositories: [repository], + onDidOpenRepository: events.event, + onDidCloseRepository: events.event, + }); + await assert.rejects(manager.openWorktreeRepository(Uri.file('/tmp/pr-stack-worktree')), + /The Git provider cannot open a temporary worktree/); + events.dispose(); + }); + }); + describe('updateRepositories', function () { it('skips a repository after a 404 without affecting healthy repositories', async function () { const inaccessibleUrl = 'https://github.com/owner/missing'; diff --git a/src/test/github/pullRequestModel.test.ts b/src/test/github/pullRequestModel.test.ts index 98c12c3de5..c190e030da 100644 --- a/src/test/github/pullRequestModel.test.ts +++ b/src/test/github/pullRequestModel.test.ts @@ -11,11 +11,11 @@ import { GitChangeType, SlimFileChange } from '../../common/file'; import { CredentialStore } from '../../github/credentials'; import { FolderRepositoryManager } from '../../github/folderRepositoryManager'; import { PullRequestModel } from '../../github/pullRequestModel'; -import { GithubItemStateEnum, PullRequestMergeability, PullRequestStack } from '../../github/interface'; +import { GithubItemStateEnum, isStackMergeable, isStackUpdatable, PullRequestMergeability, PullRequestStack } from '../../github/interface'; import { Protocol } from '../../common/protocol'; import { GitHubRemote, Remote } from '../../common/remote'; import { convertRESTPullRequestToRawPullRequest } from '../../github/utils'; -import { SinonSandbox, createSandbox } from 'sinon'; +import { SinonSandbox, SinonStub, createSandbox } from 'sinon'; import { PullRequestBuilder } from '../builders/rest/pullRequestBuilder'; import { PullRequestBuilder as GraphQLPullRequestBuilder } from '../builders/graphql/pullRequestBuilder'; import { MockTelemetry } from '../mocks/mockTelemetry'; @@ -105,14 +105,21 @@ describe('PullRequestModel', function () { }); describe('getStack', function () { - function createModel() { - const pr = new PullRequestBuilder().number(794).build(); + let compare: SinonStub; + + beforeEach(function () { + compare = sinon.stub(repo, 'compareCommits').resolves({ behind_by: 0 } as Awaited>); + }); + + function createModel(number = 794) { + const pr = new PullRequestBuilder().number(number).build(); return new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo)); } function createEnterpriseModel() { const enterpriseRemote = new GitHubRemote('enterprise', 'https://enterprise.example.com/github/test', new Protocol('https://enterprise.example.com/github/test'), GitHubServerType.Enterprise); const enterpriseRepo = new MockGitHubRepository(enterpriseRemote, credentials, telemetry, sinon); + sinon.stub(enterpriseRepo, 'compareCommits').resolves({ behind_by: 0 } as Awaited>); const pr = new PullRequestBuilder().number(794).build(); const model = new PullRequestModel(credentials, telemetry, enterpriseRepo, enterpriseRemote, convertRESTPullRequestToRawPullRequest(pr, enterpriseRepo)); return { enterpriseRepo, model }; @@ -123,14 +130,18 @@ describe('PullRequestModel', function () { number: number; mergeable?: 'MERGEABLE' | 'CONFLICTING' | 'UNKNOWN'; mergeStateStatus?: 'CLEAN' | 'BLOCKED' | 'BEHIND' | 'DIRTY' | 'UNKNOWN'; - }[], endCursor: string | null, position = 2) { + queued?: boolean; + state?: GithubItemStateEnum; + baseOwner?: string; + headOwner?: string; + }[], endCursor: string | null, position = 2, size = 3) { return { data: { repository: { pullRequest: { stackEntry: { position }, stack: { - size: 3, + size, baseRefName: 'main', entries: { nodes: entries.map(entry => ({ @@ -139,11 +150,15 @@ describe('PullRequestModel', function () { number: entry.number, title: `Change ${entry.number}`, url: `https://github.com/github/test/pull/${entry.number}`, - state: GithubItemStateEnum.Open, + state: entry.state ?? GithubItemStateEnum.Open, isDraft: false, + baseRefName: entry.position === 1 ? 'main' : `D${entry.position - 1}`, + baseRepository: entry.state && entry.state !== GithubItemStateEnum.Open ? null : { owner: { login: entry.baseOwner ?? 'github' } }, headRefName: `D${entry.position}`, + headRepository: entry.state && entry.state !== GithubItemStateEnum.Open ? null : { owner: { login: entry.headOwner ?? 'github' } }, mergeable: entry.mergeable ?? 'MERGEABLE', mergeStateStatus: entry.mergeStateStatus ?? 'CLEAN', + mergeQueueEntry: entry.queued ? { state: 'QUEUED' } : null, }, })), pageInfo: { hasNextPage: endCursor !== null, endCursor }, @@ -173,6 +188,8 @@ describe('PullRequestModel', function () { assert.strictEqual(stack?.position, 2); assert.strictEqual(stack?.size, 3); assert.strictEqual(stack?.base, 'main'); + assert.strictEqual(stack?.needsUpdate, false); + assert.strictEqual(isStackUpdatable(stack!), false); assert.deepStrictEqual(stack?.pullRequests.map(entry => entry.number), [793, 794, 795]); assert.strictEqual(stack?.pullRequests[0].url, 'https://github.com/github/test/pull/793'); assert.deepStrictEqual(stack?.pullRequests.map(entry => entry.mergeable), [ @@ -199,6 +216,154 @@ describe('PullRequestModel', function () { PullRequestMergeability.NotMergeable, PullRequestMergeability.Behind, ]); + assert.strictEqual(compare.callCount, 2); + }); + + it('marks a clean PR behind its stack base as waiting and blocks stack merge', async function () { + const model = createModel(); + compare.callsFake(async (_base: string, head: string) => + ({ behind_by: head === 'github:D2' ? 1 : 0 }) as Awaited>); + repo.queryProvider.expectGraphQLQuery({ + query: queries.PullRequestStack, + variables: { owner: 'github', name: 'test', number: 794, after: null }, + }, stackPage([ + { position: 1, number: 793 }, + { position: 2, number: 794 }, + { position: 3, number: 795 }, + ], null)); + + const stack = await model.getStack(); + assert.deepStrictEqual({ + readiness: stack?.pullRequests.map(entry => entry.mergeable), + canMerge: isStackMergeable(stack!, 794), + canUpdate: isStackUpdatable(stack!), + needsUpdate: stack?.needsUpdate, + comparedSecond: compare.calledWithExactly('github:D1', 'github:D2'), + }, { + readiness: [PullRequestMergeability.Mergeable, PullRequestMergeability.Behind, PullRequestMergeability.Mergeable], + canMerge: false, + canUpdate: true, + needsUpdate: true, + comparedSecond: true, + }); + }); + + it('offers an update when only the bottom branch is behind the stack base', async function () { + const model = createModel(); + compare.callsFake(async (_base: string, head: string) => + ({ behind_by: head === 'github:D1' ? 1 : 0 }) as Awaited>); + repo.queryProvider.expectGraphQLQuery({ + query: queries.PullRequestStack, + variables: { owner: 'github', name: 'test', number: 794, after: null }, + }, stackPage([ + { position: 1, number: 793 }, + { position: 2, number: 794 }, + { position: 3, number: 795 }, + ], null)); + + const stack = await model.getStack(); + assert.strictEqual(stack?.needsUpdate, true); + assert.strictEqual(isStackUpdatable(stack!), true); + assert(compare.calledWithExactly('github:main', 'github:D1')); + }); + + it('offers an update when a changed middle PR has not been propagated to the top', async function () { + const model = createModel(795); + compare.callsFake(async (_base: string, head: string) => + ({ behind_by: head === 'github:D3' ? 1 : 0, ahead_by: head === 'github:D2' ? 2 : 1 }) as Awaited>); + repo.queryProvider.expectGraphQLQuery({ + query: queries.PullRequestStack, + variables: { owner: 'github', name: 'test', number: 795, after: null }, + }, stackPage([ + { position: 1, number: 793 }, + { position: 2, number: 794 }, + { position: 3, number: 795 }, + ], null, 3)); + + const stack = await model.getStack(); + assert.deepStrictEqual(stack?.pullRequests.map(entry => entry.mergeable), [ + PullRequestMergeability.Mergeable, + PullRequestMergeability.Mergeable, + PullRequestMergeability.Behind, + ]); + assert.strictEqual(stack?.needsUpdate, true); + assert.strictEqual(isStackUpdatable(stack!), true); + assert(compare.calledWithExactly('github:main', 'github:D1')); + assert(compare.calledWithExactly('github:D1', 'github:D2')); + assert(compare.calledWithExactly('github:D2', 'github:D3')); + assert.strictEqual(compare.callCount, 3); + }); + + it('blocks merging an up-to-date PR above a behind PR with a closed top', async function () { + const model = createModel(795); + compare.callsFake(async (_base: string, head: string) => + ({ behind_by: head === 'github:D2' ? 1 : 0 }) as Awaited>); + repo.queryProvider.expectGraphQLQuery({ + query: queries.PullRequestStack, + variables: { owner: 'github', name: 'test', number: 795, after: null }, + }, stackPage([ + { position: 1, number: 793 }, + { position: 2, number: 794 }, + { position: 3, number: 795 }, + { position: 4, number: 798, state: GithubItemStateEnum.Closed }, + ], null, 3, 4)); + + const stack = await model.getStack(); + assert.deepStrictEqual(stack?.pullRequests.map(entry => entry.mergeable), [ + PullRequestMergeability.Mergeable, + PullRequestMergeability.Behind, + PullRequestMergeability.Mergeable, + PullRequestMergeability.Mergeable, + ]); + assert.strictEqual(isStackMergeable(stack!, 795), false); + assert.strictEqual(isStackUpdatable(stack!), true); + assert(compare.calledWithExactly('github:D1', 'github:D2')); + assert.strictEqual(compare.callCount, 3); + }); + + it('does not report a stack merge-ready when the base comparison fails', async function () { + const model = createModel(); + compare.resolves(undefined); + repo.queryProvider.expectGraphQLQuery({ + query: queries.PullRequestStack, + variables: { owner: 'github', name: 'test', number: 794, after: null }, + }, stackPage([{ position: 2, number: 794 }], null)); + + await assert.rejects(model.getStack(), /Unable to check whether pull request #794 is behind its stack base/); + }); + + it('compares fork branches by owner and skips closed entries without branch information', async function () { + const model = createModel(); + repo.queryProvider.expectGraphQLQuery({ + query: queries.PullRequestStack, + variables: { owner: 'github', name: 'test', number: 794, after: null }, + }, stackPage([ + { position: 1, number: 793, state: GithubItemStateEnum.Merged }, + { position: 2, number: 794, baseOwner: 'base-owner', headOwner: 'fork-owner' }, + { position: 3, number: 795, state: GithubItemStateEnum.Closed }, + ], null)); + + const stack = await model.getStack(); + assert.deepStrictEqual(stack?.pullRequests.map(entry => entry.state), [ + GithubItemStateEnum.Merged, GithubItemStateEnum.Open, GithubItemStateEnum.Closed, + ]); + assert(compare.calledOnceWithExactly('base-owner:D1', 'fork-owner:D2')); + }); + + it('does not offer a stack update when a pull request is queued', async function () { + const model = createModel(); + repo.queryProvider.expectGraphQLQuery({ + query: queries.PullRequestStack, + variables: { owner: 'github', name: 'test', number: 794, after: null }, + }, stackPage([ + { position: 1, number: 793 }, + { position: 2, number: 794, queued: true }, + { position: 3, number: 795 }, + ], null)); + + const stack = await model.getStack(); + assert.deepStrictEqual(stack?.pullRequests.map(entry => entry.isQueued), [false, true, false]); + assert.strictEqual(isStackUpdatable(stack!), false); }); describe('mergeStack', function () { diff --git a/src/test/github/pullRequestOverview.test.ts b/src/test/github/pullRequestOverview.test.ts index f047d9fb4a..24f602e658 100644 --- a/src/test/github/pullRequestOverview.test.ts +++ b/src/test/github/pullRequestOverview.test.ts @@ -19,6 +19,7 @@ import { convertRESTPullRequestToRawPullRequest } from '../../github/utils'; import { PullRequestBuilder } from '../builders/rest/pullRequestBuilder'; import { MockExtensionContext } from '../mocks/mockExtensionContext'; import { MockGitHubRepository } from '../mocks/mockGitHubRepository'; +import { Repository } from '../../api/api'; import { GitApiImpl } from '../../api/api1'; import { CredentialStore } from '../../github/credentials'; import { GitHubServerType } from '../../common/authentication'; @@ -27,6 +28,7 @@ import { CheckState, GithubItemStateEnum, IAccount, PullRequestMergeability, Pul import { CreatePullRequestHelper } from '../../view/createPullRequestHelper'; import { RepositoriesManager } from '../../github/repositoriesManager'; import { MockThemeWatcher } from '../mocks/mockThemeWatcher'; +import { GitHubRef } from '../../common/githubRef'; import { mockStackSetting } from '../mocks/mockStackSetting'; import { TimelineEvent } from '../../common/timelineEvent'; import { PullRequestReviewCommon, ReviewContext } from '../../github/pullRequestReviewCommon'; @@ -45,10 +47,13 @@ describe('PullRequestOverview', function () { let telemetry: MockTelemetry; let credentialStore: CredentialStore; let mockThemeWatcher: MockThemeWatcher; + let repositoriesManager: RepositoriesManager; let setStacksEnabled: (enabled: boolean) => void; + let panelServices: ReturnType | undefined; beforeEach(async function () { sinon = createSandbox(); + panelServices = undefined; MockCommandRegistry.install(sinon); setStacksEnabled = mockStackSetting(sinon); context = new MockExtensionContext(); @@ -58,7 +63,7 @@ describe('PullRequestOverview', function () { credentialStore = new CredentialStore(telemetry, context); mockThemeWatcher = new MockThemeWatcher(); const createPrHelper = new CreatePullRequestHelper(); - const repositoriesManager = new RepositoriesManager(credentialStore, telemetry); + repositoriesManager = new RepositoriesManager(credentialStore, telemetry); pullRequestManager = new FolderRepositoryManager(0, context, repository, telemetry, new GitApiImpl(repositoriesManager), credentialStore, createPrHelper, mockThemeWatcher); const url = 'https://github.com/aaa/bbb'; @@ -73,8 +78,17 @@ describe('PullRequestOverview', function () { } pullRequestManager.dispose(); + repositoriesManager.dispose(); + repo.dispose(); + credentialStore.dispose(); context.dispose(); - sinon.restore(); + try { + if (panelServices) { + sinon.assert.notCalled(panelServices.query); + } + } finally { + sinon.restore(); + } }); describe('createOrShow', function () { @@ -724,26 +738,12 @@ describe('PullRequestOverview', function () { describe('mergePullRequest', function () { it('prompts to delete the local branch when GitHub deletes branches after merge', async function () { repo.buildMetadata(repository => repository.delete_branch_on_merge!(true)); - repo.addGraphQLPullRequest(builder => { - builder.pullRequest(response => { - response.repository(r => { - r.pullRequest(pr => pr.number(1000)); - }); - }); - }); - - const prItem = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build(), repo); - const prModel = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem); - const identity = { owner: prModel.remote.owner, repo: prModel.remote.repositoryName, number: prModel.number }; - await PullRequestOverviewPanel.createOrShow(telemetry, EXTENSION_URI, pullRequestManager, identity, prModel); - - const panel = PullRequestOverviewPanel.findPanel(identity.owner, identity.repo, identity.number)!; + const { panel, model: prModel, branch } = await createPanel(); sinon.stub(prModel, 'merge').resolves({ merged: true, message: '', timeline: [] }); - sinon.stub(pullRequestManager, 'getBranchNameForPullRequest').resolves({ + branch.resolves({ branch: 'new-feature', createdForPullRequest: false, }); - sinon.stub(pullRequestManager, 'getPullRequestRepositoryDefaultBranch').resolves('main'); const showWarningMessage = sinon.stub(vscode.window, 'showWarningMessage').resolves(undefined); const replyMessage = sinon.stub(panel as any, '_replyMessage'); @@ -833,32 +833,109 @@ describe('PullRequestOverview', function () { }); }); - async function createPanel() { - const prItem = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build(), repo); + function stubPanelServices() { + const models = new Map(); + const access = sinon.stub(pullRequestManager, 'getPullRequestRepositoryAccessAndMergeMethods').resolves({ + hasWritePermission: true, + mergeMethodsAvailability: { merge: true, squash: true, rebase: true }, + viewerCanAutoMerge: false, + }); + sinon.stub(pullRequestManager, 'getPullRequestRepositoryDefaultBranch').resolves('main'); + sinon.stub(pullRequestManager, 'getCurrentUser').callsFake(async () => { + const model = models.values().next().value; + assert(model); + return model.author; + }); + sinon.stub(pullRequestManager, 'getAssignableUsers').resolves({}); + const branch = sinon.stub(pullRequestManager, 'getBranchNameForPullRequest').resolves(undefined); + sinon.stub(pullRequestManager, 'mergeQueueMethodForBranch').resolves(undefined); + sinon.stub(pullRequestManager, 'isHeadUpToDateWithBase').resolves(true); + sinon.stub(pullRequestManager, 'getPreferredEmail').resolves(undefined); + sinon.stub(pullRequestManager, 'checkBranchUpToDate').resolves(); + sinon.stub(pullRequestManager, 'resolvePullRequest').callsFake(async (_owner, _repo, number) => models.get(number)); + const externalUri = sinon.stub(vscode.env, 'asExternalUri').callsFake(async uri => uri); + const showError = sinon.stub(vscode.window, 'showErrorMessage').resolves(undefined); + const query = sinon.spy(repo, 'query'); + return { models, access, branch, externalUri, showError, query }; + } + + async function createPanel(number = 1000) { + const prItem = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(number).build(), repo); const model = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem); + const services = panelServices ??= stubPanelServices(); + services.models.set(number, model); + sinon.stub(model, 'canEdit').resolves(true); + sinon.stub(model, 'getReviewRequests').resolves([]); + sinon.stub(model, 'getTimelineEvents').resolves([]); + sinon.stub(model, 'validateDraftMode').resolves(false); + sinon.stub(model, 'getStatusChecks').resolves([{ state: CheckState.Success, statuses: [] }, null]); + sinon.stub(model, 'getMergeability').resolves({ mergeability: PullRequestMergeability.Mergeable }); + sinon.stub(model, 'getCoAuthors').resolves([]); + sinon.stub(model, 'getLastUpdateTime').resolves(new Date(0)); + const stackQuery = sinon.stub(model, 'getStack').resolves(undefined); + const received = new vscode.EventEmitter<{ command: string }>(); + const webviewPanel = vscode.window.createWebviewPanel(PullRequestOverviewPanel.viewType, `#${number}`, vscode.ViewColumn.One, {}); + context.subscriptions.push(received, webviewPanel); + sinon.stub(webviewPanel.webview, 'onDidReceiveMessage').callsFake(received.event); + const pendingUpdates = new Set(['status', 'events', 'canAssignCopilot']); + let finishInitialization!: () => void; + const initialized = new Promise(resolve => { finishInitialization = resolve; }); + const webviewPostMessage = sinon.stub(webviewPanel.webview, 'postMessage').callsFake(async (message: { + res?: { command?: string; pullrequest?: Partial }; + }) => { + const response = message.res; + if (response?.command === 'pr.initialize' && response.pullrequest?.stackLoaded === false) { + pendingUpdates.add('stackLoaded'); + } else if (response?.command === 'pr.update' && response.pullrequest) { + for (const key of pendingUpdates) { + if (key in response.pullrequest) { + pendingUpdates.delete(key); + } + } + if (pendingUpdates.size === 0) { + finishInitialization(); + } + } + return true; + }); const identity = { owner: remote.owner, repo: remote.repositoryName, number: model.number }; - await PullRequestOverviewPanel.createOrShow(telemetry, EXTENSION_URI, pullRequestManager, identity, model); + const opening = PullRequestOverviewPanel.createOrShow(telemetry, EXTENSION_URI, pullRequestManager, + identity, model, false, true, webviewPanel); + received.fire({ command: 'ready' }); + await opening; + await initialized; + assert(services.query.notCalled); const panel = PullRequestOverviewPanel.findPanel(identity.owner, identity.repo, identity.number)!; - const stackQuery = sinon.stub(model, 'getStack').resolves({ + stackQuery.resetHistory(); + stackQuery.resolves({ position: 2, size: 2, base: 'main', pullRequests: [ { position: 1, number: 999, title: 'First', url: '', head: 'D1', state: GithubItemStateEnum.Merged, isDraft: false, mergeable: PullRequestMergeability.Unknown }, { position: 2, number: 1000, title: 'Second', url: '', head: 'D2', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, ], }); - const access = sinon.stub(pullRequestManager, 'getPullRequestRepositoryAccessAndMergeMethods').resolves({ - hasWritePermission: true, - mergeMethodsAvailability: { merge: true, squash: true, rebase: true }, - viewerCanAutoMerge: false, - }); - return { panel, model, access, stackQuery }; + return { ...services, panel, model, stackQuery, webviewPostMessage }; } + describe('stack panel fixture', function () { + it('posts replies without depending on the renderer sending a ready message', async function () { + const { panel, webviewPostMessage } = await createPanel(); + webviewPostMessage.resetHistory(); + const message = { req: 'fixture', command: 'pr.update-stack' }; + const response = { updatedPullRequests: [] }; + + await (panel as any)._replyMessage(message, response); + + assert(webviewPostMessage.calledOnce); + assert.deepStrictEqual(webviewPostMessage.firstCall.args[0], { seq: message.req, res: response }); + }); + }); + describe('loadStack', function () { it('marks the stack loaded before posting linked stack details', async function () { - const externalUri = sinon.stub(vscode.env, 'asExternalUri').callsFake(async uri => uri.with({ scheme: 'test-external' })); - const { panel, model, stackQuery } = await createPanel(); - sinon.stub(pullRequestManager, 'mergeQueueMethodForBranch').resolves(undefined); + const { panel, model, stackQuery, externalUri } = await createPanel(); + externalUri.callsFake(async uri => uri.with({ scheme: 'test-external' })); + externalUri.resetHistory(); const onLoaded = sinon.spy(); const postMessage = sinon.stub(panel as any, '_postMessage').callsFake(async (message: { pullrequest?: { stackLoaded?: boolean } }) => { if (message.pullrequest?.stackLoaded) { @@ -879,6 +956,7 @@ describe('PullRequestOverview', function () { { owner: remote.owner, repo: remote.repositoryName, pullRequestNumber: 999 }, { owner: remote.owner, repo: remote.repositoryName, pullRequestNumber: 1000 }, ]); + assert.strictEqual(update.args[0].pullrequest.canUpdateStack, false); }); it('ignores results from a stale overview update', async function () { @@ -893,7 +971,442 @@ describe('PullRequestOverview', function () { }); }); + describe('updateStack', function () { + function provideStackGit(repository: Repository): void { + Object.assign(repository, { + rebase: async () => undefined, + rebaseAbort: async () => undefined, + pushRefWithLease: async () => undefined, + getRemoteRefs: async () => [], + updateRef: async () => undefined, + resetKeep: async () => undefined, + createWorktree: async () => '', + deleteWorktree: async () => undefined, + }); + } + + async function openStackPanel() { + const result = await createPanel(); + pullRequestManager.activePullRequest = result.model; + provideStackGit(pullRequestManager.repository); + result.stackQuery.resolves({ + position: 2, size: 2, base: 'main', needsUpdate: true, + pullRequests: [ + { position: 1, number: 999, title: 'First', url: '', head: 'D1', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Behind }, + { position: 2, number: 1000, title: 'Second', url: '', head: 'D2', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + ], + }); + return result; + } + + it('offers Update stack for unpropagated middle changes with a checked-out stack PR in another folder', async function () { + const { panel, model, stackQuery } = await openStackPanel(); + pullRequestManager.activePullRequest = undefined; + const url = `https://github.com/${remote.owner}/${remote.repositoryName}.git`; + model.head = new GitHubRef('D2', `${remote.owner}:D2`, 'a'.repeat(40), + url, remote.owner, remote.repositoryName, false); + const other = new FolderRepositoryManager(1, context, new MockRepository(), telemetry, + new GitApiImpl(repositoriesManager), credentialStore, new CreatePullRequestHelper(), mockThemeWatcher); + try { + provideStackGit(other.repository); + await other.repository.addRemote('origin', url); + repositoriesManager.insertFolderManager(other); + PullRequestOverviewPanel.registerGlobalCommands(context, telemetry, repositoriesManager); + const postMessage = sinon.stub(panel as any, '_postMessage').resolves(); + (panel as any)._canUpdateStackAccess = true; + const checkedOut = new PullRequestModel(credentialStore, telemetry, repo, remote, + convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(999).build(), repo)); + + other.activePullRequest = checkedOut; + + assert.strictEqual((panel as any).getCheckedOutPullRequestNumber(model), 999); + assert.strictEqual((panel as any).getStackRepository(model)?.repository, other.repository); + assert(postMessage.calledWithMatch({ + command: 'pr.update-checkout-status', + canUpdateStack: false, + })); + stackQuery.resolves({ + position: 3, size: 3, base: 'main', needsUpdate: true, + pullRequests: [ + { position: 1, number: 998, title: 'First', url: '', head: 'D1', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + { position: 2, number: 999, title: 'Middle', url: '', head: 'D2', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + { position: 3, number: 1000, title: 'Top', url: '', head: 'D3', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Behind }, + ], + }); + await (panel as any).loadStack(model, (panel as any)._updateSequence, () => undefined); + assert(postMessage.calledWithMatch({ + command: 'pr.update', + pullrequest: { stackLoaded: true, canUpdateStack: true }, + })); + + other.activePullRequest = undefined; + assert(postMessage.lastCall.calledWithMatch({ + command: 'pr.update-checkout-status', + canUpdateStack: false, + })); + other.activePullRequest = checkedOut; + assert(postMessage.lastCall.calledWithMatch({ + command: 'pr.update-checkout-status', + canUpdateStack: true, + })); + + const unrelated = new PullRequestModel(credentialStore, telemetry, repo, remote, + convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(5000).build(), repo)); + other.activePullRequest = unrelated; + assert(postMessage.lastCall.calledWithMatch({ + command: 'pr.update-checkout-status', + canUpdateStack: false, + })); + other.activePullRequest = checkedOut; + assert(postMessage.lastCall.calledWithMatch({ + command: 'pr.update-checkout-status', + canUpdateStack: true, + })); + + (panel as any)._canUpdateStackAccess = false; + other.activePullRequest = undefined; + other.activePullRequest = checkedOut; + assert(postMessage.lastCall.calledWithMatch({ + command: 'pr.update-checkout-status', + canUpdateStack: false, + })); + (panel as any)._canUpdateStackAccess = true; + } finally { + other.dispose(); + } + }); + + it('hides Update stack when the stack no longer needs updating', async function () { + const { panel, model, stackQuery } = await openStackPanel(); + const url = `https://github.com/${remote.owner}/${remote.repositoryName}.git`; + model.head = new GitHubRef('D2', `${remote.owner}:D2`, 'a'.repeat(40), url, remote.owner, remote.repositoryName, false); + await pullRequestManager.repository.addRemote('origin', url); + (panel as any)._canUpdateStackAccess = true; + const postMessage = sinon.stub(panel as any, '_postMessage').resolves(); + stackQuery.resolves({ + position: 2, size: 2, base: 'main', needsUpdate: false, + pullRequests: [ + { position: 1, number: 999, title: 'First', url: '', head: 'D1', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + { position: 2, number: 1000, title: 'Second', url: '', head: 'D2', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + ], + }); + + await (panel as any).loadStack(model, (panel as any)._updateSequence, () => undefined); + + assert(postMessage.lastCall.calledWithMatch({ + command: 'pr.update', + pullrequest: { stackLoaded: true, canUpdateStack: false }, + })); + pullRequestManager.activePullRequest = undefined; + pullRequestManager.activePullRequest = model; + assert(postMessage.lastCall.calledWithMatch({ + command: 'pr.update-checkout-status', + canUpdateStack: false, + })); + }); + + it('hides Update stack when a closed PR interrupts the open chain', async function () { + const { panel, model, stackQuery } = await openStackPanel(); + const url = `https://github.com/${remote.owner}/${remote.repositoryName}.git`; + model.head = new GitHubRef('D2', `${remote.owner}:D2`, 'a'.repeat(40), url, remote.owner, remote.repositoryName, false); + await pullRequestManager.repository.addRemote('origin', url); + (panel as any)._canUpdateStackAccess = true; + const postMessage = sinon.stub(panel as any, '_postMessage').resolves(); + stackQuery.resolves({ + position: 2, size: 3, base: 'main', needsUpdate: true, + pullRequests: [ + { position: 1, number: 998, title: 'First', url: '', head: 'D1', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + { position: 2, number: 999, title: 'Closed', url: '', head: 'D2', state: GithubItemStateEnum.Closed, isDraft: false, mergeable: PullRequestMergeability.Unknown }, + { position: 3, number: 1000, title: 'Third', url: '', head: 'D3', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + ], + }); + + await (panel as any).loadStack(model, (panel as any)._updateSequence, () => undefined); + + assert(postMessage.lastCall.calledWithMatch({ + command: 'pr.update', + pullrequest: { stackLoaded: true, canUpdateStack: false }, + })); + }); + + it('requires a checked-out PR in the stack even for direct webview requests', async function () { + const { panel } = await openStackPanel(); + pullRequestManager.activePullRequest = undefined; + const warning = sinon.stub(vscode.window, 'showWarningMessage'); + const throwError = sinon.stub(panel as any, '_throwError').resolves(); + + await (panel as any).updateStack({ req: 'unchecked', command: 'pr.update-stack' }); + + assert(warning.notCalled); + assert.match(throwError.firstCall.args[1], /Check out a pull request in this stack/); + }); + + it('updates open panel checkout state when another stack PR becomes active', async function () { + const { panel } = await openStackPanel(); + const sibling = new PullRequestModel(credentialStore, telemetry, repo, remote, + convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(999).build(), repo)); + const postMessage = sinon.stub(panel as any, '_postMessage').resolves(); + + pullRequestManager.activePullRequest = sibling; + + assert(postMessage.calledWithMatch({ + command: 'pr.update-checkout-status', + isCurrentlyCheckedOut: false, + canUpdateStack: false, + })); + }); + + it('uses only a writable remote that pushes to the PR repository', async function () { + const { panel, model } = await openStackPanel(); + const url = `https://github.com/${remote.owner}/${remote.repositoryName}.git`; + model.head = new GitHubRef('D2', `${remote.owner}:D2`, 'a'.repeat(40), + url, remote.owner, remote.repositoryName, false); + await pullRequestManager.repository.addRemote('origin', url); + assert.strictEqual((panel as any).getStackRepository(model)?.remote.name, 'origin'); + + (pullRequestManager.repository.state.remotes[0] as { pushUrl: string }).pushUrl = 'https://github.com/someone-else/repository.git'; + assert.strictEqual((panel as any).getStackRepository(model), undefined); + }); + + it('hides Update stack when the built-in Git API lacks safe push operations', async function () { + const { panel, model } = await openStackPanel(); + const url = `https://github.com/${remote.owner}/${remote.repositoryName}.git`; + model.head = new GitHubRef('D2', `${remote.owner}:D2`, 'a'.repeat(40), + url, remote.owner, remote.repositoryName, false); + await pullRequestManager.repository.addRemote('origin', url); + Reflect.deleteProperty(pullRequestManager.repository, 'pushRefWithLease'); + + assert.strictEqual((panel as any).getStackRepository(model), undefined); + }); + + it('rejects a stack with conflicts before confirming or performing Git operations', async function () { + const { panel, stackQuery } = await openStackPanel(); + stackQuery.resolves({ + position: 1, size: 1, base: 'main', + pullRequests: [{ position: 1, number: 1000, title: 'First', url: '', head: 'D1', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Conflict }], + }); + const warning = sinon.stub(vscode.window, 'showWarningMessage'); + const progress = sinon.stub(vscode.window, 'withProgress'); + const throwError = sinon.stub(panel as any, '_throwError').resolves(); + + await (panel as any).updateStack({ req: '1', command: 'pr.update-stack' }); + + assert(warning.notCalled); + assert(progress.notCalled); + assert.match(throwError.firstCall.args[1], /conflict-free chain/); + }); + + it('confirms only open PRs when trailing stack members are closed', async function () { + const { panel, stackQuery } = await openStackPanel(); + stackQuery.resolves({ + position: 2, size: 4, base: 'master', + pullRequests: [ + { position: 1, number: 999, title: 'First', url: '', head: 'D1', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + { position: 2, number: 1000, title: 'Second', url: '', head: 'D2', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Behind }, + { position: 3, number: 1001, title: 'Third', url: '', head: 'D3', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + { position: 4, number: 1002, title: 'Fourth', url: '', head: 'D4', state: GithubItemStateEnum.Closed, isDraft: true, mergeable: PullRequestMergeability.Unknown }, + ], + }); + const confirm = sinon.stub(vscode.window, 'showWarningMessage').resolves(undefined); + const progress = sinon.stub(vscode.window, 'withProgress'); + const reply = sinon.stub(panel as any, '_replyMessage').resolves(); + + await (panel as any).updateStack({ req: 'closed-top', command: 'pr.update-stack' }); + + assert.match(confirm.firstCall.args[0], /3 pull requests/); + assert.match((confirm.firstCall.args[1] as vscode.MessageOptions).detail!, /Closed pull requests at the top/); + assert(progress.notCalled); + assert(reply.calledOnce); + }); + + it('rejects a closed PR in the middle of an otherwise open stack', async function () { + const { panel, stackQuery } = await openStackPanel(); + stackQuery.resolves({ + position: 2, size: 3, base: 'master', + pullRequests: [ + { position: 1, number: 999, title: 'First', url: '', head: 'D1', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + { position: 2, number: 1000, title: 'Second', url: '', head: 'D2', state: GithubItemStateEnum.Closed, isDraft: false, mergeable: PullRequestMergeability.Unknown }, + { position: 3, number: 1001, title: 'Third', url: '', head: 'D3', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + ], + }); + const confirm = sinon.stub(vscode.window, 'showWarningMessage'); + const throwError = sinon.stub(panel as any, '_throwError').resolves(); + + await (panel as any).updateStack({ req: 'closed-middle', command: 'pr.update-stack' }); + + assert(confirm.notCalled); + assert.match(throwError.firstCall.args[1], /open, conflict-free chain/); + }); + + it('rejects a direct update request when stacks are disabled', async function () { + setStacksEnabled(false); + const { panel, stackQuery } = await openStackPanel(); + const throwError = sinon.stub(panel as any, '_throwError').resolves(); + + await (panel as any).updateStack({ req: 'disabled', command: 'pr.update-stack' }); + + assert(stackQuery.notCalled); + assert.match(throwError.firstCall.args[1], /stack features are disabled/); + }); + + it('does not update branches when the user cancels the force-push confirmation', async function () { + const { panel } = await openStackPanel(); + sinon.stub(vscode.window, 'showWarningMessage').resolves(undefined); + const progress = sinon.stub(vscode.window, 'withProgress'); + const reply = sinon.stub(panel as any, '_replyMessage').resolves(); + const message = { req: '2', command: 'pr.update-stack' }; + + await (panel as any).updateStack(message); + + assert(progress.notCalled); + sinon.assert.calledWithExactly(reply, message, { updatedPullRequests: [] }); + }); + + it('rejects overlapping updates without unlocking the first update', async function () { + const { panel } = await openStackPanel(); + let finishConfirmation!: () => void; + const confirmation = new Promise(resolve => { finishConfirmation = resolve; }); + let startConfirmation!: () => void; + const confirmationStarted = new Promise(resolve => { startConfirmation = resolve; }); + const warning = sinon.stub(vscode.window, 'showWarningMessage').callsFake(async () => { + startConfirmation(); + await confirmation; + return undefined; + }); + const throwError = sinon.stub(panel as any, '_throwError').resolves(); + const reply = sinon.stub(panel as any, '_replyMessage').resolves(); + + const first = (panel as any).updateStack({ req: 'first', command: 'pr.update-stack' }); + await confirmationStarted; + await (panel as any).updateStack({ req: 'second', command: 'pr.update-stack' }); + assert.match(throwError.firstCall.args[1], /already being updated/); + await (panel as any).updateStack({ req: 'third', command: 'pr.update-stack' }); + assert.match(throwError.secondCall.args[1], /already being updated/); + assert(warning.calledOnce); + finishConfirmation(); + await first; + assert(reply.calledOnce); + }); + + it('shows a progress notification and reports a missing writable remote', async function () { + const { panel } = await openStackPanel(); + const cancellation = new vscode.CancellationTokenSource(); + sinon.stub(vscode.window, 'showWarningMessage').resolves('Update stack' as never); + const progress = sinon.stub(vscode.window, 'withProgress').callsFake((_options, task) => + task({ report: () => undefined }, cancellation.token)); + const throwError = sinon.stub(panel as any, '_throwError').resolves(); + const refresh = sinon.stub(panel, 'refreshPanel').resolves(); + + await (panel as any).updateStack({ req: '3', command: 'pr.update-stack' }); + + assert(progress.calledOnce); + assert.strictEqual(progress.firstCall.args[0].location, vscode.ProgressLocation.Notification); + assert(throwError.calledOnce); + assert.match(throwError.firstCall.args[1], /writable Git remote/); + assert(refresh.calledOnce); + sinon.assert.callOrder(progress, throwError, refresh); + cancellation.dispose(); + }); + }); + describe('unstackAll', function () { + it('rejects unstacking while another panel updates the stack', async function () { + const { panel } = await createPanel(); + const lock = (PullRequestOverviewPanel as any)._updatingStacks as Set; + lock.add(`${remote.owner}/${remote.repositoryName}#999`); + const unstack = sinon.stub(repo, 'unstackAll'); + const throwError = sinon.stub(panel as any, '_throwError').resolves(); + try { + await (panel as any).unstackAll({ req: 'locked', command: 'pr.unstack-all' }); + assert(unstack.notCalled); + assert.match(throwError.firstCall.args[1], /already being updated/); + assert(lock.has(`${remote.owner}/${remote.repositoryName}#999`)); + } finally { + lock.delete(`${remote.owner}/${remote.repositoryName}#999`); + } + }); + + it('excludes updates and other unstack requests while unstacking another panel', async function () { + const { panel } = await createPanel(); + const { panel: other, model, stackQuery } = await createPanel(999); + stackQuery.resolves({ + position: 1, size: 2, base: 'main', needsUpdate: true, + pullRequests: [ + { position: 1, number: 999, title: 'First', url: '', head: 'D1', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Behind }, + { position: 2, number: 1000, title: 'Second', url: '', head: 'D2', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + ], + }); + pullRequestManager.activePullRequest = model; + const locks = (PullRequestOverviewPanel as any)._updatingStacks as Set; + const key = `${remote.owner}/${remote.repositoryName}#999`; + const confirm = sinon.stub(vscode.window, 'showWarningMessage').resolves('Unstack all' as never); + sinon.stub(vscode.window, 'showInformationMessage').resolves(undefined); + sinon.stub(panel, 'refreshPanel').resolves(); + sinon.stub(other, 'refreshPanel').resolves(); + const errors = sinon.stub(other as any, '_throwError').resolves(); + let finish!: (remaining: number[]) => void; + let started!: () => void; + const unstackStarted = new Promise(resolve => { started = resolve; }); + const unstack = sinon.stub(repo, 'unstackAll').callsFake(() => { + assert(locks.has(key)); + started(); + return new Promise(resolve => { finish = resolve; }); + }); + const pending = (panel as any).unstackAll({ req: 'first', command: 'pr.unstack-all' }); + try { + await unstackStarted; + await (other as any).updateStack({ req: 'update', command: 'pr.update-stack' }); + assert.match(errors.lastCall.args[1], /already being updated/); + assert(locks.has(key)); + await (other as any).unstackAll({ req: 'unstack', command: 'pr.unstack-all' }); + assert.match(errors.lastCall.args[1], /already being updated/); + assert(locks.has(key)); + assert(unstack.calledOnce); + assert(confirm.calledOnce); + } finally { + finish([]); + await pending; + } + assert.strictEqual(locks.has(key), false); + }); + + it('rechecks the lock after confirmation without releasing another action lock', async function () { + const { panel } = await createPanel(); + const locks = (PullRequestOverviewPanel as any)._updatingStacks as Set; + const key = `${remote.owner}/${remote.repositoryName}#999`; + const unstack = sinon.stub(repo, 'unstackAll'); + const errors = sinon.stub(panel as any, '_throwError').resolves(); + sinon.stub(vscode.window, 'showWarningMessage').callsFake(async (_message, _options, action) => { + locks.add(key); + return action; + }); + try { + await (panel as any).unstackAll({ req: 'confirmation-race', command: 'pr.unstack-all' }); + assert(unstack.notCalled); + assert.match(errors.firstCall.args[1], /already being updated/); + assert(locks.has(key)); + } finally { + locks.delete(key); + } + }); + + it('releases its stack lock when the unstack operation fails', async function () { + const { panel } = await createPanel(); + const locks = (PullRequestOverviewPanel as any)._updatingStacks as Set; + const key = `${remote.owner}/${remote.repositoryName}#999`; + sinon.stub(vscode.window, 'showWarningMessage').resolves('Unstack all' as never); + sinon.stub(repo, 'unstackAll').callsFake(async () => { + assert(locks.has(key)); + throw new Error('Stack is locked on GitHub'); + }); + const errors = sinon.stub(panel as any, '_throwError').resolves(); + + await (panel as any).unstackAll({ req: 'failed', command: 'pr.unstack-all' }); + + assert.match(errors.firstCall.args[1], /Stack is locked on GitHub/); + assert.strictEqual(locks.has(key), false); + }); + it('confirms unstacking all eligible PRs and reports remaining locked PRs', async function () { const { panel } = await createPanel(); @@ -950,9 +1463,8 @@ describe('PullRequestOverview', function () { it('does not unstack when the feature is disabled', async function () { setStacksEnabled(false); - const { panel } = await createPanel(); + const { panel, showError } = await createPanel(); const unstack = sinon.stub(repo, 'unstackAll'); - const showError = sinon.stub(vscode.window, 'showErrorMessage').resolves(undefined); const throwError = sinon.stub(panel as any, '_throwError').resolves(); const message = { req: 'disabled', command: 'pr.unstack-all', args: undefined }; @@ -965,11 +1477,7 @@ describe('PullRequestOverview', function () { it('refreshes other visible PR panels in the unstacked stack', async function () { const { panel } = await createPanel(); - const siblingModel = new PullRequestModel(credentialStore, telemetry, repo, remote, - convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(999).build(), repo)); - await PullRequestOverviewPanel.createOrShow(telemetry, EXTENSION_URI, pullRequestManager, - { owner: remote.owner, repo: remote.repositoryName, number: 999 }, siblingModel); - const sibling = PullRequestOverviewPanel.findPanel(remote.owner, remote.repositoryName, 999)!; + const { panel: sibling } = await createPanel(999); const refreshSibling = sinon.stub(sibling, 'refreshPanel').resolves(); sinon.stub(panel, 'refreshPanel').resolves(); sinon.stub(panel as any, '_replyMessage').resolves(); @@ -984,11 +1492,7 @@ describe('PullRequestOverview', function () { it('refreshes the stack entry in other open panels when a PR changes draft state', async function () { const { panel, model } = await createPanel(); - const siblingModel = new PullRequestModel(credentialStore, telemetry, repo, remote, - convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(999).build(), repo)); - await PullRequestOverviewPanel.createOrShow(telemetry, EXTENSION_URI, pullRequestManager, - { owner: remote.owner, repo: remote.repositoryName, number: 999 }, siblingModel); - const sibling = PullRequestOverviewPanel.findPanel(remote.owner, remote.repositoryName, 999)!; + const { panel: sibling } = await createPanel(999); const refreshCurrent = sinon.stub(panel, 'refreshPanel').resolves(); let finishRefresh: () => void; const refreshedSibling = new Promise(resolve => { finishRefresh = resolve; }); @@ -1021,6 +1525,7 @@ describe('PullRequestOverview', function () { assert(unstack.notCalled); sinon.assert.calledWithExactly(reply, message, { cancelled: true }); + assert.strictEqual((PullRequestOverviewPanel as any)._updatingStacks.has(`${remote.owner}/${remote.repositoryName}#999`), false); }); it('rejects unstacking without write permission', async function () { @@ -1032,7 +1537,6 @@ describe('PullRequestOverview', function () { }); const unstack = sinon.stub(repo, 'unstackAll'); const reply = sinon.stub(panel as any, '_throwError').resolves(); - sinon.stub(vscode.window, 'showErrorMessage').resolves(undefined); const message = { req: '3', command: 'pr.unstack-all', args: undefined }; await (panel as any).unstackAll(message); @@ -1050,7 +1554,6 @@ describe('PullRequestOverview', function () { const warning = sinon.stub(vscode.window, 'showWarningMessage').resolves(undefined); const unstack = sinon.stub(repo, 'unstackAll'); const reply = sinon.stub(panel as any, '_throwError').resolves(); - sinon.stub(vscode.window, 'showErrorMessage').resolves(undefined); await (panel as any).unstackAll({ req: '4', command: 'pr.unstack-all', args: undefined }); @@ -1062,11 +1565,7 @@ describe('PullRequestOverview', function () { describe('deleteBranch', function () { it('replies with the deletion state after deletion completes', async function () { - const prItem = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build(), repo); - const prModel = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem); - const identity = { owner: prModel.remote.owner, repo: prModel.remote.repositoryName, number: prModel.number }; - await PullRequestOverviewPanel.createOrShow(telemetry, EXTENSION_URI, pullRequestManager, identity, prModel); - const panel = PullRequestOverviewPanel.findPanel(identity.owner, identity.repo, identity.number)!; + const { panel } = await createPanel(); const response = { command: 'pr.deleteBranch', branchTypes: ['local'] }; sinon.stub(PullRequestReviewCommon, 'deleteBranch').resolves({ isReply: false, message: response }); const replyMessage = sinon.stub(panel as any, '_replyMessage').resolves(); diff --git a/src/test/github/updateStackBranches.test.ts b/src/test/github/updateStackBranches.test.ts new file mode 100644 index 0000000000..0a7ac99921 --- /dev/null +++ b/src/test/github/updateStackBranches.test.ts @@ -0,0 +1,260 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { strict as assert } from 'assert'; +import { Uri } from 'vscode'; +import { Branch, BranchQuery, Commit, FetchOptions, Ref, Repository, RepositoryState } from '../../api/api'; +import { RefType, Status } from '../../api/api1'; +import { StackBranch, StackGitRepository, updateStackBranches } from '../../github/updateStackBranches'; +import { MockRepository } from '../mocks/mockRepository'; + +class StackRepository extends MockRepository implements StackGitRepository { + readonly local = new Map([['D1', 'd1-old'], ['D2', 'd2-old']]); + readonly remote = new Map([['main', 'main-new'], ['D1', 'd1-old'], ['D2', 'd2-old']]); + readonly calls: string[] = []; + readonly upstream = new Map(); + failRebase = false; + failPush?: string; + switchCheckoutOnPush?: string; + private head = 'd2-old'; + private readonly isWorktree: boolean; + + constructor(isWorktree = false) { + super(); + this.isWorktree = isWorktree; + this.state = { + HEAD: { type: RefType.Head, name: isWorktree ? undefined : 'D2', commit: this.head }, + remotes: [], submodules: [], worktrees: [], + rebaseCommit: undefined, mergeChanges: [], indexChanges: [], workingTreeChanges: [], + onDidChange: () => ({ dispose() { } }), + } satisfies RepositoryState; + } + + override async fetch(_remote?: string | FetchOptions, ref?: string): Promise { + this.calls.push(`fetch ${ref}`); + } + + override async getBranch(name: string): Promise { + const remote = name.startsWith('refs/remotes/origin/'); + const branch = remote ? name.substring('refs/remotes/origin/'.length) : name; + const commit = (remote ? this.remote : this.local).get(branch); + if (!commit) { + throw new Error(`Missing branch ${name}`); + } + return { type: RefType.Head, name: branch, commit }; + } + + override async getBranches(_query: BranchQuery): Promise { + return [...this.local].map(([name, commit]) => ({ type: RefType.Head, name, commit })); + } + + override async getMergeBase(_base: string, head: string): Promise { + return head === 'd1-old' || head === 'd1-new' ? 'main-old' : 'd1-old'; + } + + override async status(): Promise { } + + override async checkout(ref: string): Promise { + this.head = ref; + this.state = { ...this.state, HEAD: { type: RefType.Head, commit: ref } }; + this.calls.push(`checkout ${ref}`); + } + + override async getCommit(_ref: string): Promise { + return { hash: this.head, message: '', parents: [] }; + } + + override async createBranch(name: string, _checkout: boolean, ref: string): Promise { + this.local.set(name, ref); + this.calls.push(`create ${name} ${ref}`); + } + + override async setBranchUpstream(name: string, upstream: string): Promise { + this.upstream.set(name, upstream); + } + + async getRemoteRefs(): Promise { + return [...this.remote].map(([name, commit]) => ({ type: RefType.Head, name, commit })); + } + + async createWorktree(): Promise { + this.calls.push('create worktree'); + return 'temporary-worktree'; + } + + async deleteWorktree(): Promise { + this.calls.push('delete worktree'); + } + + async rebase(upstream: string, options: { onto?: string; rebaseMerges?: boolean }): Promise { + this.calls.push(`rebase ${upstream} onto ${options.onto} preserving merges`); + if (this.failRebase) { + throw new Error('Conflict'); + } + if (options.onto !== upstream) { + this.head = this.head === 'd1-old' ? 'd1-new' : 'd2-new'; + } + } + + async rebaseAbort(): Promise { + this.calls.push('abort rebase'); + } + + async pushRefWithLease(_remote: string, branch: string, newSha: string, expectedSha: string): Promise { + this.calls.push(`push ${branch} ${newSha} if ${expectedSha}`); + if (branch === this.failPush) { + throw new Error('Stale remote branch'); + } + assert.equal(this.remote.get(branch), expectedSha); + this.remote.set(branch, newSha); + if (branch === this.switchCheckoutOnPush) { + this.state = { ...this.state, HEAD: { type: RefType.Head, name: 'main', commit: 'main-new' } }; + } + } + + async updateRef(ref: string, newSha: string, expectedSha: string): Promise { + const branch = ref.replace(/^refs\/heads\//, ''); + assert.equal(this.local.get(branch) ?? '0'.repeat(40), expectedSha); + this.local.set(branch, newSha); + this.calls.push(`local ${branch} ${newSha}`); + } + + async resetKeep(ref: string): Promise { + this.local.set(this.state.HEAD!.name!, ref); + this.state = { ...this.state, HEAD: { ...this.state.HEAD!, commit: ref } }; + this.calls.push(`reset-keep ${ref}`); + } +} + +describe('Update pull request stack branches', function () { + let repository: StackRepository; + let temporary: StackRepository; + const branches: StackBranch[] = [ + { number: 1, base: 'main', head: 'D1', sha: 'd1-old' }, + { number: 2, base: 'D1', head: 'D2', sha: 'd2-old' }, + ]; + const open = async (_path: string): Promise => temporary; + const update = () => updateStackBranches(repository, open, 'origin', branches, 2, () => undefined); + + beforeEach(function () { + repository = new StackRepository(); + temporary = new StackRepository(true); + }); + + it('rebases bottom-to-top and updates both the remote and local branches', async function () { + const result = await update(); + assert.deepEqual(result, [1, 2]); + assert.deepEqual({ + local: [...repository.local], remote: [...repository.remote], + operations: repository.calls.filter(call => call.startsWith('push') || call.startsWith('reset-keep')), + rebases: temporary.calls.filter(call => call.startsWith('rebase')), + }, { + local: [['D1', 'd1-new'], ['D2', 'd2-new']], + remote: [['main', 'main-new'], ['D1', 'd1-new'], ['D2', 'd2-new']], + operations: ['push D1 d1-new if d1-old', 'push D2 d2-new if d2-old', 'reset-keep d2-new'], + rebases: ['rebase main-old onto main-new preserving merges', 'rebase d1-old onto d1-new preserving merges'], + }); + }); + + it('creates a missing local branch after its remote has been pushed', async function () { + repository.local.delete('D1'); + await update(); + assert.equal(repository.local.get('D1'), 'd1-new'); + assert.equal(repository.upstream.get('D1'), 'refs/remotes/origin/D1'); + }); + + it('does not push unchanged branches when the stack is already current', async function () { + repository.remote.set('main', 'main-old'); + const updated = await update(); + assert.deepEqual(updated, []); + assert.equal(repository.calls.some(call => call.startsWith('push')), false); + assert.deepEqual([...repository.local], [['D1', 'd1-old'], ['D2', 'd2-old']]); + }); + + it('starts at the first outdated PR and does not rewrite its current parent', async function () { + repository.remote.set('main', 'main-old'); + repository.remote.set('D1', 'd1-new'); + repository.local.set('D1', 'd1-new'); + const changedParent: StackBranch[] = [{ ...branches[0], sha: 'd1-new' }, branches[1]]; + + const updated = await updateStackBranches(repository, open, 'origin', changedParent, 2, () => undefined); + + assert.deepEqual(updated, [2]); + assert.deepEqual({ + rebases: temporary.calls.filter(call => call.startsWith('rebase')), + pushes: repository.calls.filter(call => call.startsWith('push')), + local: [...repository.local], + }, { + rebases: ['rebase d1-old onto d1-new preserving merges'], + pushes: ['push D2 d2-new if d2-old'], + local: [['D1', 'd1-new'], ['D2', 'd2-new']], + }); + }); + + it('refuses divergent local commits before pushing', async function () { + repository.local.set('D1', 'unpublished'); + await assert.rejects(update(), /Local branch D1 differs/); + assert.deepEqual([...repository.remote], [['main', 'main-new'], ['D1', 'd1-old'], ['D2', 'd2-old']]); + }); + + it('refuses dirty work before pushing', async function () { + repository.state.workingTreeChanges.push({ + uri: Uri.file('/root/file'), originalUri: Uri.file('/root/file'), + renameUri: undefined, status: Status.MODIFIED, + }); + await assert.rejects(update(), /Commit or stash/); + assert.equal(repository.calls.some(call => call.startsWith('push')), false); + }); + + it('aborts a conflicting preflight without pushing', async function () { + temporary.failRebase = true; + await assert.rejects(update(), /caused a conflict/); + assert.deepEqual(temporary.calls.filter(call => call.includes('rebase')), [ + 'rebase main-old onto main-new preserving merges', 'abort rebase', + ]); + assert(repository.calls.includes('delete worktree')); + assert.equal(repository.calls.some(call => call.startsWith('push')), false); + }); + + it('cleans up when the Git extension cannot open the temporary worktree', async function () { + await assert.rejects( + updateStackBranches(repository, async () => null, 'origin', branches, 2, () => undefined), + /could not open the temporary stack worktree/, + ); + assert(repository.calls.includes('delete worktree')); + assert.equal(repository.calls.some(call => call.startsWith('push')), false); + }); + + it('refuses to rewrite a branch checked out in a different worktree', async function () { + repository.state.worktrees?.push({ name: 'other', path: '/another', ref: 'refs/heads/D1', main: false, detached: false }); + await assert.rejects(update(), /another worktree/); + assert.equal(repository.calls.some(call => call.startsWith('push')), false); + }); + + it('reports partial success while keeping pushed and local branches synchronized', async function () { + repository.failPush = 'D2'; + await assert.rejects(update(), /already updated: #1/); + assert.deepEqual({ + remote: [...repository.remote], local: [...repository.local], + }, { + remote: [['main', 'main-new'], ['D1', 'd1-new'], ['D2', 'd2-old']], + local: [['D1', 'd1-new'], ['D2', 'd2-old']], + }); + }); + + it('never resets a different checkout when the branch changes during a push', async function () { + repository.switchCheckoutOnPush = 'D2'; + await assert.rejects(update(), /#2 was pushed.*local branch D2 could not be updated/); + assert.deepEqual({ + head: repository.state.HEAD, + local: [...repository.local], + remote: [...repository.remote], + }, { + head: { type: RefType.Head, name: 'main', commit: 'main-new' }, + local: [['D1', 'd1-new'], ['D2', 'd2-old']], + remote: [['main', 'main-new'], ['D1', 'd1-new'], ['D2', 'd2-new']], + }); + }); +}); diff --git a/webviews/common/context.tsx b/webviews/common/context.tsx index 1ec3651548..e352990012 100644 --- a/webviews/common/context.tsx +++ b/webviews/common/context.tsx @@ -11,7 +11,7 @@ import { CloseResult, DescriptionResult, OpenCommitChangesArgs, OpenLocalFileArg import { IComment } from '../../src/common/comment'; import { EventType, ReviewEvent, SessionLinkInfo, TimelineEvent } from '../../src/common/timelineEvent'; import { IProjectItem, MergeMethod, PullRequestCheckStatus, ReadyForReview } from '../../src/github/interface'; -import { CancelCodingAgentReply, ChangeAssigneesReply, ChangeBaseReply, ConvertToDraftReply, DeleteReviewResult, FileUploadCompletedMessage, MergeArguments, MergeResult, ProjectItemsReply, PullRequest, PullRequestPreview, ReadyForReviewReply, StackMergeResult, SubmitReviewArgs, SubmitReviewReply, UnstackAllResult, UploadFilesReply } from '../../src/github/views'; +import { CancelCodingAgentReply, ChangeAssigneesReply, ChangeBaseReply, ConvertToDraftReply, DeleteReviewResult, FileUploadCompletedMessage, MergeArguments, MergeResult, ProjectItemsReply, PullRequest, PullRequestPreview, ReadyForReviewReply, StackMergeResult, SubmitReviewArgs, SubmitReviewReply, UnstackAllResult, UpdateStackResult, UploadFilesReply } from '../../src/github/views'; /** * Encode a {@linkcode Uint8Array} as a base64 string. Uses fixed-size chunks to @@ -107,6 +107,9 @@ export class PRContext { public unstackAll = (): Promise => this.postMessage({ command: 'pr.unstack-all' }); + public updateStack = (): Promise => + this.postMessage({ command: 'pr.update-stack' }); + public openOnGitHub = () => this.postMessage({ command: 'pr.openOnGitHub', args: this.preview ? { url: this.preview.url } : undefined, @@ -584,7 +587,10 @@ export class PRContext { case 'update-state': return this.updatePR({ state: message.state }); case 'pr.update-checkout-status': - return this.updatePR({ isCurrentlyCheckedOut: message.isCurrentlyCheckedOut }); + return this.updatePR({ + isCurrentlyCheckedOut: message.isCurrentlyCheckedOut, + canUpdateStack: message.canUpdateStack, + }); case 'pr.deleteBranch': const stateChange: { isLocalHeadDeleted?: boolean, isRemoteHeadDeleted?: boolean } = {}; message.branchTypes && message.branchTypes.map((branchType: string) => { diff --git a/webviews/components/merge.tsx b/webviews/components/merge.tsx index b13faf6ca2..089726572b 100644 --- a/webviews/components/merge.tsx +++ b/webviews/components/merge.tsx @@ -186,6 +186,7 @@ export const MergeStatusAndActions = ({ pr, isSimple }: { pr: PullRequest; isSim } const { mergeable: _mergeable } = pr; + const canUpdateWithMergeCommit = !pr.stack && pr.stackLoaded !== false && !pr.stackLoadError; const [mergeable, setMergeability] = useState(_mergeable); if ((_mergeable !== mergeable) && (_mergeable !== PullRequestMergeability.Unknown)) { @@ -204,8 +205,10 @@ export const MergeStatusAndActions = ({ pr, isSimple }: { pr: PullRequest; isSim return (
- - + +
); @@ -213,7 +216,7 @@ export const MergeStatusAndActions = ({ pr, isSimple }: { pr: PullRequest; isSim export default StatusChecksSection; -export const MergeStatus = ({ mergeable, isSimple, canUpdateBranch }: { mergeable: PullRequestMergeability; isSimple: boolean; canUpdateBranch: boolean }) => { +export const MergeStatus = ({ mergeable, isSimple, canUpdateBranch, canUpdateWithMergeCommit }: { mergeable: PullRequestMergeability; isSimple: boolean; canUpdateBranch: boolean; canUpdateWithMergeCommit: boolean }) => { const { updateBranch } = useContext(PullRequestContext); const [busy, setBusy] = useState(false); @@ -238,7 +241,7 @@ export const MergeStatus = ({ mergeable, isSimple, canUpdateBranch }: { mergeabl } else if (mergeable === PullRequestMergeability.Behind) { icon = closeIcon; summary = 'This branch is out-of-date with the base branch.'; - action = 'Update with merge commit'; + action = canUpdateWithMergeCommit ? 'Update with merge commit' : null; } if (isSimple) { @@ -266,7 +269,7 @@ export const MergeStatus = ({ mergeable, isSimple, canUpdateBranch }: { mergeabl ); }; -export const OfferToUpdate = ({ mergeable, isSimple, isCurrentlyCheckedOut, canUpdateBranch }: { mergeable: PullRequestMergeability; isSimple: boolean; isCurrentlyCheckedOut: boolean, canUpdateBranch: boolean }) => { +export const OfferToUpdate = ({ mergeable, isSimple, isCurrentlyCheckedOut, canUpdateBranch }: { mergeable: PullRequestMergeability; isSimple: boolean; isCurrentlyCheckedOut: boolean; canUpdateBranch: boolean }) => { const { updateBranch } = useContext(PullRequestContext); const [isBusy, setBusy] = useState(false); const update = () => { diff --git a/webviews/components/pullRequestStack.tsx b/webviews/components/pullRequestStack.tsx index d5fba43a44..1f193d8416 100644 --- a/webviews/components/pullRequestStack.tsx +++ b/webviews/components/pullRequestStack.tsx @@ -9,7 +9,7 @@ import { GithubItemStateEnum, PullRequestMergeability, PullRequestStack as Stack import { PullRequest } from '../../src/github/views'; import PullRequestContext from '../common/context'; -function getReadiness(entry: Stack['pullRequests'][number], currentPosition: number): { icon: JSX.Element; label: string; kind: string } { +function getReadiness(entry: Stack['pullRequests'][number], stack: Stack): { icon: JSX.Element; label: string; kind: string } { if (entry.state === GithubItemStateEnum.Merged) { return { icon: gitMergeIcon, label: 'Already merged', kind: 'merged' }; } @@ -21,7 +21,11 @@ function getReadiness(entry: Stack['pullRequests'][number], currentPosition: num } switch (entry.mergeable) { case PullRequestMergeability.Mergeable: - return { icon: entry.position > currentPosition ? circleFilledIcon : passIcon, label: 'Ready to merge', kind: 'ready' }; + if (stack.pullRequests.some(below => below.position < entry.position && below.state === GithubItemStateEnum.Open + && below.mergeable === PullRequestMergeability.Behind)) { + return { icon: circleFilledIcon, label: 'A pull request below is behind its base', kind: 'waiting' }; + } + return { icon: entry.position > stack.position ? circleFilledIcon : passIcon, label: 'Ready to merge', kind: 'ready' }; case PullRequestMergeability.Conflict: return { icon: circleFilledIcon, label: 'Merge conflicts', kind: 'waiting' }; case PullRequestMergeability.NotMergeable: @@ -41,30 +45,47 @@ export const StackBadge = ({ stack }: { stack?: Stack }) => stack ? ( ) : null; export const StackSection = ({ pr }: { pr: PullRequest }) => { - const { unstackAll } = React.useContext(PullRequestContext); + const { unstackAll, updateStack } = React.useContext(PullRequestContext); const [busy, setBusy] = React.useState(false); const [error, setError] = React.useState(); + const [updating, setUpdating] = React.useState(false); + const [updateError, setUpdateError] = React.useState(); const { stack } = pr; if (!stack) { return null; } const openBelow = stack.pullRequests.filter(entry => entry.position < stack.position && entry.state === GithubItemStateEnum.Open).length; const canUnstack = pr.hasWritePermission && stack.pullRequests.some(entry => entry.state !== GithubItemStateEnum.Merged); + const canUpdate = !!pr.canUpdateStack && pr.stackLoaded !== false && !pr.stackLoadError; const unstack = async () => { try { setBusy(true); setError(undefined); + setUpdateError(undefined); await unstackAll(); } catch (unstackError) { - setError(unstackError instanceof Error ? unstackError.message || unstackError.name : String(unstackError)); + setError(`Unable to unstack pull requests: ${unstackError instanceof Error ? unstackError.message || unstackError.name : String(unstackError)}`); } finally { setBusy(false); } }; + const update = async () => { + try { + setUpdating(true); + setUpdateError(undefined); + setError(undefined); + await updateStack(); + } catch (updateFailure) { + setUpdateError(`Unable to update the stack: ${updateFailure instanceof Error ? updateFailure.message || updateFailure.name : String(updateFailure)}`); + } finally { + setUpdating(false); + } + }; + return ( -
+
{layersIcon} @@ -77,17 +98,21 @@ export const StackSection = ({ pr }: { pr: PullRequest }) => { {chevronDownIcon} - {canUnstack ?
- : null} + {canUnstack ? + : null}
: null} - {error ?
Unable to unstack pull requests: {error}
: null} + {error || updateError ?
{error ?? updateError}
: null}
    {[...stack.pullRequests].reverse().map(entry => { const current = entry.number === pr.number; - const readiness = getReadiness(entry, stack.position); + const readiness = getReadiness(entry, stack); return
  1. {readiness.icon} diff --git a/webviews/editorWebview/index.css b/webviews/editorWebview/index.css index 90b10eccd0..577d74313c 100644 --- a/webviews/editorWebview/index.css +++ b/webviews/editorWebview/index.css @@ -729,7 +729,7 @@ body button .icon { } .stack-section[open].has-actions .stack-heading-text { - padding-right: 128px; + padding-right: 230px; } .stack-section:not([open]) .stack-chevron { @@ -870,13 +870,20 @@ body button .icon { position: absolute; top: 16px; right: 48px; + display: flex; + gap: 8px; + margin-left: auto; } .stack-section:not([open]) .stack-actions { display: none; } -@media (max-width: 480px) { +@media (max-width: 700px) { + .stack-section[open].has-actions summary { + flex-wrap: wrap; + } + .stack-section[open].has-actions .stack-heading-text { padding-right: 0; } @@ -889,7 +896,7 @@ body button .icon { } } -.stack-unstack-error { +.stack-action-error { padding: 8px 16px 0; color: var(--vscode-errorForeground); } diff --git a/webviews/editorWebview/test/builder/pullRequest.ts b/webviews/editorWebview/test/builder/pullRequest.ts index b764bb8656..7504b9a3ef 100644 --- a/webviews/editorWebview/test/builder/pullRequest.ts +++ b/webviews/editorWebview/test/builder/pullRequest.ts @@ -30,6 +30,7 @@ export const PullRequestBuilder = createBuilderClass()({ isLocalHeadDeleted: { default: false }, head: { default: 'my-fork:my-branch' }, stack: { default: undefined }, + canUpdateStack: { default: false }, stackLoaded: { default: true }, stackLoadError: { default: false }, stackMergeStatus: { default: undefined }, diff --git a/webviews/editorWebview/test/overview.test.tsx b/webviews/editorWebview/test/overview.test.tsx index b8d4243c8d..e8b0663af3 100644 --- a/webviews/editorWebview/test/overview.test.tsx +++ b/webviews/editorWebview/test/overview.test.tsx @@ -126,6 +126,39 @@ describe('Overview', function () { assert(openOnGitHub.notCalled); }); + it('shows a behind base as waiting and withholds stack merging for it and PRs above it', function () { + for (const [number, position] of [[794, 2], [795, 3]]) { + const pr = new PullRequestBuilder().number(number).stack({ + position, + size: 4, + base: 'main', + pullRequests: [ + { position: 1, number: 793, title: 'First', head: 'D1', url: 'https://example.com/793', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + { position: 2, number: 794, title: 'Second', head: 'D2', url: 'https://example.com/794', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Behind }, + { position: 3, number: 795, title: 'Third', head: 'D3', url: 'https://example.com/795', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + { position: 4, number: 798, title: 'Closed', head: 'D4', url: 'https://example.com/798', state: GithubItemStateEnum.Closed, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + ], + }).build(); + const out = render( + + + , + ); + + assert.deepStrictEqual([...out.container.querySelectorAll('.stack-entry-readiness')].map(readiness => [ + readiness.getAttribute('aria-label'), readiness.classList[1], + ]), [ + ['Closed pull request cannot be merged', 'blocked'], + ['A pull request below is behind its base', 'waiting'], + ['Branch is behind its base', 'waiting'], + ['Ready to merge', 'ready'], + ]); + assert.strictEqual(out.container.querySelector('.stack-merge'), null); + assert.strictEqual(out.queryByText('Merge Pull Request'), null); + out.unmount(); + } + }); + it('shows stack state icons appropriate to each pull request and its position', function () { const states = [ { state: GithubItemStateEnum.Merged, isDraft: false, mergeable: PullRequestMergeability.Unknown }, @@ -243,11 +276,114 @@ describe('Overview', function () { , ); fireEvent.click(out.getByText('Unstack all')); - const alert = await waitForElement(() => out.container.querySelector('.stack-unstack-error[role="alert"]')); + const alert = await waitForElement(() => out.container.querySelector('.stack-action-error[role="alert"]')); assert.strictEqual(alert?.textContent, 'Unable to unstack pull requests: Stack is locked'); assert.strictEqual((out.getByText('Unstack all') as HTMLButtonElement).disabled, false); }); + it('offers Update stack for an open, conflict-free stack without collapsing the heading', async function () { + const stack = { + position: 2, size: 2, base: 'main', + pullRequests: [ + { position: 1, number: 1233, title: 'First', head: 'D1', url: 'https://example.com/1233', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Behind }, + { position: 2, number: 1234, title: 'Second', head: 'D2', url: 'https://example.com/1234', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.NotMergeable }, + ], + }; + const pr = new PullRequestBuilder().stack(stack).canUpdateStack(true).build(); + const context = new PRContext(pr); + const updateStack = sinon.stub(context, 'updateStack').resolves({ updatedPullRequests: [1233, 1234] }); + const out = render( + + + , + ); + const button = out.getByText('Update stack'); + const section = button.closest('#pull-request-stack'); + assert.strictEqual(button.closest('summary'), null); + assert.strictEqual(section?.children[1], button.parentElement); + assert.strictEqual(fireEvent.click(button), true); + assert(updateStack.calledOnce); + assert(section?.hasAttribute('open')); + }); + + it('does not offer Update stack when the host rejects eligibility or stack details are unavailable', function () { + const entry = { position: 1, number: 1234, title: 'First', head: 'D1', url: 'https://example.com/1234', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }; + const stack = { position: 1, size: 1, base: 'main', pullRequests: [entry] }; + const pr = new PullRequestBuilder().stack(stack).canUpdateStack(true).build(); + const context = new PRContext(pr); + const out = render(); + assert(out.getByText('Update stack')); + for (const change of [ + { canUpdateStack: false }, + { stackLoaded: false }, + { stackLoadError: true }, + { stack: undefined }, + ]) { + out.rerender(); + assert.strictEqual(out.queryByText('Update stack'), null); + } + }); + + it('shows a recoverable inline error when updating the stack fails', async function () { + const pr = new PullRequestBuilder().canUpdateStack(true).stack({ + position: 1, size: 1, base: 'main', + pullRequests: [{ position: 1, number: 1234, title: 'First', head: 'D1', url: 'https://example.com/1234', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }], + }).build(); + const context = new PRContext(pr); + sinon.stub(context, 'updateStack').rejects(new Error('Branch changed on GitHub')); + const out = render(); + fireEvent.click(out.getByText('Update stack')); + const alert = await waitForElement(() => out.container.querySelector('.stack-action-error[role="alert"]')); + assert.strictEqual(alert?.textContent, 'Unable to update the stack: Branch changed on GitHub'); + assert.strictEqual((out.getByText('Update stack') as HTMLButtonElement).disabled, false); + }); + + ['update', 'unstack'].forEach(firstAction => { + it(`clears the previous ${firstAction} error when the other stack action starts`, async function () { + const pr = new PullRequestBuilder().canUpdateStack(true).stack({ + position: 1, size: 1, base: 'main', + pullRequests: [{ + position: 1, number: 1234, title: 'First', head: 'D1', url: 'https://example.com/1234', + state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Behind, + }], + }).build(); + const context = new PRContext(pr); + const failure = new Error('Previous action failed'); + const update = sinon.stub(context, 'updateStack'); + const unstack = sinon.stub(context, 'unstackAll'); + let finish!: () => void; + const pending = new Promise(resolve => { finish = resolve; }); + if (firstAction === 'update') { + update.rejects(failure); + unstack.callsFake(async () => { + await pending; + return { cancelled: false, remainingPullRequests: [] }; + }); + } else { + unstack.rejects(failure); + update.callsFake(async () => { + await pending; + return { updatedPullRequests: [1234] }; + }); + } + const out = render( + + + , + ); + fireEvent.click(out.getByText(firstAction === 'update' ? 'Update stack' : 'Unstack all')); + const alert = await waitForElement(() => out.container.querySelector('.stack-action-error[role="alert"]')); + assert(alert?.textContent?.includes('Previous action failed')); + + fireEvent.click(out.getByText(firstAction === 'update' ? 'Unstack all' : 'Update stack')); + assert.strictEqual(out.container.querySelector('.stack-action-error'), null); + assert(out.getByText(firstAction === 'update' ? 'Unstacking...' : 'Updating...')); + finish(); + await waitForElement(() => out.getByText(firstAction === 'update' ? 'Unstack all' : 'Update stack')); + assert.strictEqual(out.container.querySelector('.stack-action-error'), null); + }); + }); + it('shows a closed stack without suggesting it can be merged', function () { const pr = new PullRequestBuilder().state(GithubItemStateEnum.Closed).stack({ position: 1, @@ -312,6 +448,132 @@ describe('Overview', function () { assert.strictEqual(out.container.querySelector('.stack-merge'), null); }); + it('never offers an update with a merge commit for stacked PRs', function () { + const stack = { + position: 2, size: 2, base: 'main', + pullRequests: [ + { position: 1, number: 793, title: 'First', head: 'D1', url: 'https://example.com/793', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + { position: 2, number: 794, title: 'Second', head: 'D2', url: 'https://example.com/794', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Behind }, + ], + }; + for (const mergeable of [PullRequestMergeability.Behind, PullRequestMergeability.Mergeable, PullRequestMergeability.NotMergeable]) { + const pr = new PullRequestBuilder().number(794).canUpdateBranch(true).mergeable(mergeable).stack(stack).build(); + const out = render(); + assert(out.container.querySelector('#pull-request-stack')); + if (mergeable === PullRequestMergeability.Behind) { + assert(out.getByText('This branch is out-of-date with the base branch.')); + } + assert.strictEqual(out.queryByText(/Update with merge commit/i), null); + out.unmount(); + } + }); + + it('offers Update stack only in the stack header when a PR is behind', function () { + const stack = { + position: 2, size: 2, base: 'main', + pullRequests: [ + { position: 1, number: 793, title: 'First', head: 'D1', url: 'https://example.com/793', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + { position: 2, number: 794, title: 'Second', head: 'D2', url: 'https://example.com/794', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Behind }, + ], + }; + const pr = new PullRequestBuilder().number(794).canUpdateBranch(true).canUpdateStack(true) + .mergeable(PullRequestMergeability.Behind).stack(stack).build(); + const context = new PRContext(pr); + const updateStack = sinon.stub(context, 'updateStack').resolves({ updatedPullRequests: [794] }); + const updateBranch = sinon.stub(context, 'updateBranch'); + const out = render(); + + assert(out.getByText('This branch is out-of-date with the base branch.')); + const button = out.getByText('Update stack'); + assert.strictEqual(button.closest('summary'), null); + assert(button.closest('.stack-section')); + assert.strictEqual(out.queryAllByText('Update stack').length, 1); + assert.strictEqual(out.queryByText(/Update with merge commit/i), null); + fireEvent.click(button); + assert(updateStack.calledOnce); + assert(updateBranch.notCalled); + }); + + it('offers Update stack for an open PR when only the top of the stack is closed', function () { + const stack = { + position: 2, size: 4, base: 'master', + pullRequests: [ + { position: 1, number: 793, title: 'First', head: 'D1', url: 'https://example.com/793', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + { position: 2, number: 794, title: 'Second', head: 'D2', url: 'https://example.com/794', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Behind }, + { position: 3, number: 795, title: 'Third', head: 'D3', url: 'https://example.com/795', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + { position: 4, number: 798, title: 'Closed', head: 'D4', url: 'https://example.com/798', state: GithubItemStateEnum.Closed, isDraft: true, mergeable: PullRequestMergeability.Unknown }, + ], + }; + const pr = new PullRequestBuilder().number(794).canUpdateBranch(true).canUpdateStack(true) + .mergeable(PullRequestMergeability.Behind).stack(stack).build(); + const context = new PRContext(pr); + const updateStack = sinon.stub(context, 'updateStack').resolves({ updatedPullRequests: [794, 795] }); + const out = render(); + + assert.strictEqual(out.getByText('Update stack').closest('summary'), null); + assert.strictEqual(out.queryAllByText('Update stack').length, 1); + assert.strictEqual(out.queryByText(/Update with merge commit/i), null); + fireEvent.click(out.getByText('Update stack')); + assert(updateStack.calledOnce); + }); + + it('does not offer Update stack across a closed PR in the middle of a chain', function () { + const pr = new PullRequestBuilder().number(793).canUpdateStack(false).stack({ + position: 1, size: 3, base: 'master', + pullRequests: [ + { position: 1, number: 793, title: 'First', head: 'D1', url: 'https://example.com/793', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + { position: 2, number: 794, title: 'Closed', head: 'D2', url: 'https://example.com/794', state: GithubItemStateEnum.Closed, isDraft: false, mergeable: PullRequestMergeability.Unknown }, + { position: 3, number: 795, title: 'Third', head: 'D3', url: 'https://example.com/795', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + ], + }).build(); + const out = render(); + assert.strictEqual(out.queryByText('Update stack'), null); + }); + + it('keeps Update stack in the header for other updateable stacked branches', function () { + const pr = new PullRequestBuilder().canUpdateBranch(true).canUpdateStack(true) + .stack({ + position: 1, size: 1, base: 'main', + pullRequests: [{ position: 1, number: 1234, title: 'First', head: 'D1', url: 'https://example.com/1234', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.NotMergeable }], + }).build(); + const context = new PRContext(pr); + const updateStack = sinon.stub(context, 'updateStack').resolves({ updatedPullRequests: [1234] }); + const updateBranch = sinon.stub(context, 'updateBranch'); + const out = render(); + + assert.strictEqual(out.getByText('Update stack').closest('summary'), null); + assert.strictEqual(out.queryAllByText('Update stack').length, 1); + assert.strictEqual(out.queryByText(/Update with merge commit/i), null); + fireEvent.click(out.getByText('Update stack')); + assert(updateStack.calledOnce); + assert(updateBranch.notCalled); + }); + + it('retains merge-commit updates for unstacked PRs and conflict resolution for stacked PRs', function () { + for (const mergeable of [PullRequestMergeability.Behind, PullRequestMergeability.Mergeable]) { + const pr = new PullRequestBuilder().canUpdateBranch(true).mergeable(mergeable).build(); + const out = render(); + assert(out.getByText(/Update with merge commit/i)); + out.unmount(); + } + const pr = new PullRequestBuilder().canUpdateBranch(true).mergeable(PullRequestMergeability.Conflict).stack({ + position: 1, size: 1, base: 'main', + pullRequests: [{ position: 1, number: 1234, title: 'First', head: 'D1', url: 'https://example.com/1234', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Conflict }], + }).build(); + const out = render(); + assert(out.getByText('Resolve conflicts')); + assert.strictEqual(out.queryByText(/Update with merge commit/i), null); + }); + + it('does not offer merge-commit updates before stack membership is known or when loading fails', function () { + for (const change of [{ stackLoaded: false }, { stackLoadError: true }]) { + const pr = { ...new PullRequestBuilder().canUpdateBranch(true).mergeable(PullRequestMergeability.Behind).build(), ...change }; + const out = render(); + assert.strictEqual(out.queryByText(/Update with merge commit/i), null); + out.unmount(); + } + }); + it('hides stack merge when the current or an open downstack PR is not mergeable', function () { const readyStack = { position: 2,