From 657c71d61b392fa46fc9855bbd898309517f8030 Mon Sep 17 00:00:00 2001 From: Keith Daulton Date: Thu, 6 Apr 2023 18:05:34 -0400 Subject: [PATCH] Fixes Focus View actions - fixes disabled states on the pull request actions - updates create worktree workflow - tracking existance of local branch and worktree Co-authored-by: Eric Amodio --- src/commands/ghpr/openOrCreateWorktree.ts | 37 +++++++----- src/commands/git/worktree.ts | 48 ++++++++++----- src/commands/quickCommand.steps.ts | 18 +++++- src/git/models/branch.ts | 44 ++++++++++++-- src/git/models/worktree.ts | 30 ++++++++++ src/plus/webviews/focus/focusWebview.ts | 68 ++++++++++++++++------ src/plus/webviews/focus/protocol.ts | 4 +- .../apps/plus/focus/components/pull-request-row.ts | 18 ++++-- src/webviews/apps/plus/focus/focus.ts | 26 +++++---- 9 files changed, 219 insertions(+), 74 deletions(-) diff --git a/src/commands/ghpr/openOrCreateWorktree.ts b/src/commands/ghpr/openOrCreateWorktree.ts index 12b0aee..80c4069 100644 --- a/src/commands/ghpr/openOrCreateWorktree.ts +++ b/src/commands/ghpr/openOrCreateWorktree.ts @@ -4,8 +4,11 @@ import { Commands } from '../../constants'; import type { Container } from '../../container'; import { add as addRemote } from '../../git/actions/remote'; import { create as createWorktree, open as openWorktree } from '../../git/actions/worktree'; -import { createReference } from '../../git/models/reference'; +import { getLocalBranchByNameOrUpstream } from '../../git/models/branch'; +import type { GitBranchReference } from '../../git/models/reference'; +import { createReference, getReferenceFromBranch } from '../../git/models/reference'; import type { GitRemote } from '../../git/models/remote'; +import { getWorktreeForBranch } from '../../git/models/worktree'; import { parseGitRemoteUrl } from '../../git/parsers/remoteParser'; import { command } from '../../system/command'; import { Logger } from '../../system/logger'; @@ -109,28 +112,34 @@ export class OpenOrCreateWorktreeCommand extends Command { const remoteBranchName = `${remote.name}/${ref}`; const localBranchName = `pr/${rootUri.toString() === remoteUri.toString() ? ref : remoteBranchName}`; + const qualifiedRemoteBranchName = `remotes/${remoteBranchName}`; - const worktrees = await repo.getWorktrees(); - const worktree = worktrees.find(w => w.branch === localBranchName); + const worktree = await getWorktreeForBranch(repo, localBranchName, remoteBranchName); if (worktree != null) { void openWorktree(worktree); - return; } + let branchRef: GitBranchReference; + let createBranch: string | undefined; + + const localBranch = await getLocalBranchByNameOrUpstream(repo, localBranchName, remoteBranchName); + if (localBranch != null) { + branchRef = getReferenceFromBranch(localBranch); + // TODO@eamodio check if we are behind and if so ask the user to fast-forward + } else { + branchRef = createReference(qualifiedRemoteBranchName, repo.path, { + refType: 'branch', + name: qualifiedRemoteBranchName, + remote: true, + }); + createBranch = localBranchName; + } + await waitUntilNextTick(); try { - await createWorktree( - repo, - undefined, - createReference(remoteBranchName, repo.path, { - refType: 'branch', - name: remoteBranchName, - remote: true, - }), - { createBranch: localBranchName }, - ); + await createWorktree(repo, undefined, branchRef, { createBranch: createBranch }); // Ensure that the worktree was created const worktree = await this.container.git.getWorktree(repo.path, w => w.branch === localBranchName); diff --git a/src/commands/git/worktree.ts b/src/commands/git/worktree.ts index 880b518..54337cb 100644 --- a/src/commands/git/worktree.ts +++ b/src/commands/git/worktree.ts @@ -71,7 +71,7 @@ interface CreateState { repo: string | Repository; uri: Uri; reference?: GitReference; - createBranch: string; + createBranch?: string; flags: CreateFlags[]; reveal?: boolean; @@ -375,23 +375,39 @@ export class WorktreeGitCommand extends QuickCommand { } } - if (state.flags.includes('-b') && state.createBranch == null) { - const result = yield* inputBranchNameStep(state, context, { - placeholder: 'Please provide a name for the new branch', - titleContext: ` from ${getReferenceLabel(state.reference, { - capitalize: true, - icon: false, - label: state.reference.refType !== 'branch', - })}`, - value: state.createBranch ?? getNameWithoutRemote(state.reference), - }); - if (result === StepResultBreak) { - // Clear the flags, since we can backup after the confirm step below (which is non-standard) - state.flags = []; - continue; + if (state.flags.includes('-b')) { + let createBranchOverride: string | undefined; + if (state.createBranch != null) { + let valid = await this.container.git.validateBranchOrTagName(state.repo.path, state.createBranch); + if (valid) { + const alreadyExists = await state.repo.getBranch(state.createBranch); + valid = alreadyExists == null; + } + + if (!valid) { + createBranchOverride = state.createBranch; + state.createBranch = undefined; + } } - state.createBranch = result; + if (state.createBranch == null) { + const result = yield* inputBranchNameStep(state, context, { + placeholder: 'Please provide a name for the new branch', + titleContext: ` from ${getReferenceLabel(state.reference, { + capitalize: true, + icon: false, + label: state.reference.refType !== 'branch', + })}`, + value: createBranchOverride ?? state.createBranch ?? getNameWithoutRemote(state.reference), + }); + if (result === StepResultBreak) { + // Clear the flags, since we can backup after the confirm step below (which is non-standard) + state.flags = []; + continue; + } + + state.createBranch = result; + } } const uri = state.flags.includes('--direct') diff --git a/src/commands/quickCommand.steps.ts b/src/commands/quickCommand.steps.ts index 52445cf..31415d7 100644 --- a/src/commands/quickCommand.steps.ts +++ b/src/commands/quickCommand.steps.ts @@ -523,7 +523,16 @@ export async function* inputBranchNameStep< if ('repo' in state) { const valid = await Container.instance.git.validateBranchOrTagName(state.repo.path, value); - return [valid, valid ? undefined : `'${value}' isn't a valid branch name`]; + if (!valid) { + return [false, `'${value}' isn't a valid branch name`]; + } + + const alreadyExists = await state.repo.getBranch(value); + if (alreadyExists) { + return [false, `A branch named '${value}' already exists`]; + } + + return [true, undefined]; } let valid = true; @@ -533,6 +542,11 @@ export async function* inputBranchNameStep< if (!valid) { return [false, `'${value}' isn't a valid branch name`]; } + + const alreadyExists = await repo.getBranch(value); + if (alreadyExists) { + return [false, `A branch named '${value}' already exists`]; + } } return [true, undefined]; @@ -572,7 +586,7 @@ export async function* inputRemoteNameStep< if ('repo' in state) { const alreadyExists = (await state.repo.getRemotes({ filter: r => r.name === value })).length !== 0; if (alreadyExists) { - return [false, `Remote named '${value}' already exists`]; + return [false, `A remote named '${value}' already exists`]; } } diff --git a/src/git/models/branch.ts b/src/git/models/branch.ts index 06ea881..d61c532 100644 --- a/src/git/models/branch.ts +++ b/src/git/models/branch.ts @@ -13,6 +13,7 @@ import type { PullRequest, PullRequestState } from './pullRequest'; import type { GitBranchReference, GitReference } from './reference'; import { shortenRevision } from './reference'; import type { GitRemote } from './remote'; +import type { Repository } from './repository'; import { getUpstreamStatus } from './status'; const whitespaceRegex = /\s/; @@ -133,12 +134,12 @@ export class GitBranch implements GitBranchReference { @memoize() getNameWithoutRemote(): string { - return this.remote ? this.name.substring(this.name.indexOf('/') + 1) : this.name; + return this.remote ? this.name.substring(getRemoteNameSlashIndex(this.name) + 1) : this.name; } @memoize() getTrackingWithoutRemote(): string | undefined { - return this.upstream?.name.substring(this.upstream.name.indexOf('/') + 1); + return this.upstream?.name.substring(getRemoteNameSlashIndex(this.upstream.name) + 1); } @memoize() @@ -219,12 +220,16 @@ export function formatDetachedHeadName(sha: string): string { return `(${shortenRevision(sha)}...)`; } +function getRemoteNameSlashIndex(name: string): number { + return name.startsWith('remotes/') ? name.indexOf('/', 8) : name.indexOf('/'); +} + export function getBranchNameWithoutRemote(name: string): string { - return name.substring(name.indexOf('/') + 1); + return name.substring(getRemoteNameSlashIndex(name) + 1); } export function getRemoteNameFromBranchName(name: string): string { - return name.substring(0, name.indexOf('/')); + return name.substring(0, getRemoteNameSlashIndex(name)); } export function isBranch(branch: any): branch is GitBranch { @@ -242,7 +247,7 @@ export function isOfBranchRefType(branch: GitReference | undefined) { } export function splitBranchNameAndRemote(name: string): [name: string, remote: string | undefined] { - const index = name.indexOf('/'); + const index = getRemoteNameSlashIndex(name); if (index === -1) return [name, undefined]; return [name.substring(index + 1), name.substring(0, index)]; @@ -297,3 +302,32 @@ export function sortBranches(branches: GitBranch[], options?: BranchSortOptions) ); } } + +export async function getLocalBranchByNameOrUpstream( + repo: Repository, + branchName: string, + upstreamNames?: string | string[], +): Promise { + if (upstreamNames != null && !Array.isArray(upstreamNames)) { + upstreamNames = [upstreamNames]; + } + + let branches; + do { + branches = await repo.getBranches(branches != null ? { paging: branches.paging } : undefined); + for (const branch of branches.values) { + if ( + branch.name === branchName || + (upstreamNames != null && + branch.upstream?.name != null && + (upstreamNames.includes(branch.upstream?.name) || + (branch.upstream.name.startsWith('remotes/') && + upstreamNames.includes(branch.upstream.name.substring(8))))) + ) { + return branch; + } + } + } while (branches.paging?.more); + + return undefined; +} diff --git a/src/git/models/worktree.ts b/src/git/models/worktree.ts index 0c51832..4c38a1a 100644 --- a/src/git/models/worktree.ts +++ b/src/git/models/worktree.ts @@ -5,6 +5,7 @@ import { memoize } from '../../system/decorators/memoize'; import { normalizePath, relative } from '../../system/path'; import type { GitBranch } from './branch'; import { shortenRevision } from './reference'; +import type { Repository } from './repository'; import type { GitStatus } from './status'; export class GitWorktree { @@ -79,3 +80,32 @@ export class GitWorktree { return relativePath.length === 0 ? folder.name : relativePath; } } + +export async function getWorktreeForBranch( + repo: Repository, + branchName: string, + upstreamNames?: string | string[], +): Promise { + if (upstreamNames != null && !Array.isArray(upstreamNames)) { + upstreamNames = [upstreamNames]; + } + + const worktrees = await repo.getWorktrees(); + for (const worktree of worktrees) { + if (worktree.branch === branchName) return worktree; + + if (upstreamNames == null || worktree.branch == null) continue; + + const branch = await repo.getBranch(worktree.branch); + if ( + branch?.upstream?.name != null && + (upstreamNames.includes(branch.upstream.name) || + (branch.upstream.name.startsWith('remotes/') && + upstreamNames.includes(branch.upstream.name.substring(8)))) + ) { + return worktree; + } + } + + return undefined; +} diff --git a/src/plus/webviews/focus/focusWebview.ts b/src/plus/webviews/focus/focusWebview.ts index c81b4e0..131764a 100644 --- a/src/plus/webviews/focus/focusWebview.ts +++ b/src/plus/webviews/focus/focusWebview.ts @@ -6,6 +6,8 @@ import { setContext } from '../../../context'; import { PlusFeatures } from '../../../features'; import { add as addRemote } from '../../../git/actions/remote'; import * as RepoActions from '../../../git/actions/repository'; +import type { GitBranch } from '../../../git/models/branch'; +import { getLocalBranchByNameOrUpstream } from '../../../git/models/branch'; import type { SearchedIssue } from '../../../git/models/issue'; import { serializeIssue } from '../../../git/models/issue'; import type { PullRequestShape, SearchedPullRequest } from '../../../git/models/pullRequest'; @@ -18,7 +20,7 @@ import { createReference } from '../../../git/models/reference'; import type { GitRemote } from '../../../git/models/remote'; import type { Repository, RepositoryChangeEvent } from '../../../git/models/repository'; import { RepositoryChange, RepositoryChangeComparisonMode } from '../../../git/models/repository'; -import type { GitWorktree } from '../../../git/models/worktree'; +import { getWorktreeForBranch } from '../../../git/models/worktree'; import { parseGitRemoteUrl } from '../../../git/parsers/remoteParser'; import type { RichRemoteProvider } from '../../../git/remotes/richRemoteProvider'; import type { Subscription } from '../../../subscription'; @@ -45,7 +47,10 @@ interface RepoWithRichRemote { interface SearchedPullRequestWithRemote extends SearchedPullRequest { repoAndRemote: RepoWithRichRemote; + branch?: GitBranch; + hasLocalBranch?: boolean; isCurrentBranch?: boolean; + hasWorktree?: boolean; isCurrentWorktree?: boolean; } @@ -170,7 +175,16 @@ export class FocusWebviewProvider implements WebviewProvider { private async onSwitchBranch({ pullRequest }: SwitchToBranchParams) { const searchedPullRequestWithRemote = this.findSearchedPullRequest(pullRequest); - if (searchedPullRequestWithRemote == null) return Promise.resolve(); + if (searchedPullRequestWithRemote == null || searchedPullRequestWithRemote.isCurrentBranch) { + return Promise.resolve(); + } + + if (searchedPullRequestWithRemote.branch != null) { + return RepoActions.switchTo( + searchedPullRequestWithRemote.branch.repoPath, + searchedPullRequestWithRemote.branch, + ); + } const remoteBranch = await this.getRemoteBranch(searchedPullRequestWithRemote); if (remoteBranch == null) return Promise.resolve(); @@ -179,9 +193,13 @@ export class FocusWebviewProvider implements WebviewProvider { } private async onOpenWorktree({ pullRequest }: OpenWorktreeParams) { + const searchedPullRequestWithRemote = this.findSearchedPullRequest(pullRequest); + if (searchedPullRequestWithRemote?.repoAndRemote == null) { + return; + } + const baseUri = Uri.parse(pullRequest.refs!.base.url); - const repoAndRemote = this.findSearchedPullRequest(pullRequest)?.repoAndRemote; - const localInfo = repoAndRemote!.repo.folder; + const localInfo = searchedPullRequestWithRemote.repoAndRemote.repo.folder; return executeCommand(Commands.OpenOrCreateWorktreeForGHPR, { base: { repositoryCloneUrl: { @@ -266,7 +284,9 @@ export class FocusWebviewProvider implements WebviewProvider { pullRequest: serializePullRequest(pr.pullRequest), reasons: pr.reasons, isCurrentBranch: pr.isCurrentBranch ?? false, - iscurrentWorktree: pr.isCurrentWorktree ?? false, + isCurrentWorktree: pr.isCurrentWorktree ?? false, + hasWorktree: pr.hasWorktree ?? false, + hasLocalBranch: pr.hasLocalBranch ?? false, })); const issues = await this.getMyIssues(connectedRepos); @@ -333,20 +353,13 @@ export class FocusWebviewProvider implements WebviewProvider { } } - private async getWorktreeForPullRequest( - searchedPullRequest: SearchedPullRequestWithRemote, - ): Promise { - const worktree = await this.container.git.getWorktree( - searchedPullRequest.repoAndRemote.remote.repoPath, - w => w.branch === searchedPullRequest.pullRequest.refs!.head.branch, - ); - return worktree; - } - private async getMyPullRequests(richRepos: RepoWithRichRemote[]): Promise { const allPrs: SearchedPullRequestWithRemote[] = []; for (const richRepo of richRepos) { - const { remote } = richRepo; + const remotes = await richRepo.repo.getRemotes(); + const remoteNames = remotes.map(r => r.name); + + const remote = richRepo.remote; const prs = await this.container.git.getMyPullRequests(remote); if (prs == null) { continue; @@ -362,10 +375,27 @@ export class FocusWebviewProvider implements WebviewProvider { isCurrentWorktree: false, isCurrentBranch: false, }; - const worktree = await this.getWorktreeForPullRequest(entry); + + const upstreams = remoteNames.map(r => `${r}/${entry.pullRequest.refs!.head.branch}`); + + const worktree = await getWorktreeForBranch( + entry.repoAndRemote.repo, + entry.pullRequest.refs!.head.branch, + upstreams, + ); + entry.hasWorktree = worktree != null; entry.isCurrentWorktree = worktree?.opened === true; - const branch = await richRepo.repo.getBranch(); - entry.isCurrentBranch = branch?.name === entry.pullRequest.refs!.head.branch; + + const branch = await getLocalBranchByNameOrUpstream( + richRepo.repo, + entry.pullRequest.refs!.head.branch, + upstreams, + ); + if (branch) { + entry.branch = branch; + entry.hasLocalBranch = true; + entry.isCurrentBranch = branch.current; + } allPrs.push(entry); } diff --git a/src/plus/webviews/focus/protocol.ts b/src/plus/webviews/focus/protocol.ts index 57c46b6..9a44590 100644 --- a/src/plus/webviews/focus/protocol.ts +++ b/src/plus/webviews/focus/protocol.ts @@ -23,7 +23,9 @@ export interface IssueResult extends SearchResultBase { export interface PullRequestResult extends SearchResultBase { pullRequest: PullRequestShape; isCurrentBranch: boolean; - iscurrentWorktree: boolean; + isCurrentWorktree: boolean; + hasWorktree: boolean; + hasLocalBranch: boolean; } export interface RepoWithRichProvider { diff --git a/src/webviews/apps/plus/focus/components/pull-request-row.ts b/src/webviews/apps/plus/focus/components/pull-request-row.ts index aac3c88..2be4dd4 100644 --- a/src/webviews/apps/plus/focus/components/pull-request-row.ts +++ b/src/webviews/apps/plus/focus/components/pull-request-row.ts @@ -102,14 +102,14 @@ const template = html` { } else { noneEl.setAttribute('hidden', 'true'); loadingEl.setAttribute('hidden', 'true'); - this.state.pullRequests.forEach(({ pullRequest, reasons, isCurrentBranch, iscurrentWorktree }) => { - if (this._prFilter == null || this._prFilter === '' || reasons.includes(this._prFilter)) { - const rowEl = document.createElement('pull-request-row') as PullRequestRow; - rowEl.pullRequest = pullRequest; - rowEl.reasons = reasons; - rowEl.isCurrentBranch = isCurrentBranch; - rowEl.iscurrentWorktree = iscurrentWorktree; - - tableEl.append(rowEl); - } - }); + this.state.pullRequests.forEach( + ({ pullRequest, reasons, isCurrentBranch, isCurrentWorktree, hasWorktree, hasLocalBranch }) => { + if (this._prFilter == null || this._prFilter === '' || reasons.includes(this._prFilter)) { + const rowEl = document.createElement('pull-request-row') as PullRequestRow; + rowEl.pullRequest = pullRequest; + rowEl.reasons = reasons; + rowEl.isCurrentBranch = isCurrentBranch; + rowEl.isCurrentWorktree = isCurrentWorktree; + rowEl.hasWorktree = hasWorktree; + rowEl.hasLocalBranch = hasLocalBranch; + + tableEl.append(rowEl); + } + }, + ); } }