From 6de04e3e5e9557a431b714d3e4785dd757b0b230 Mon Sep 17 00:00:00 2001 From: Eric Amodio Date: Mon, 25 Sep 2023 23:00:31 -0400 Subject: [PATCH] Improves getting the associated pr with a branch Removes `getPullRequestForBranch` from GitProviderService as it doesn't belong there --- .../openAssociatedPullRequestOnRemote.ts | 54 +++++-------- src/commands/openPullRequestOnRemote.ts | 8 +- src/env/node/git/localGitProvider.ts | 4 +- src/git/gitProviderService.ts | 77 ++++--------------- src/git/models/branch.ts | 63 +++++---------- src/git/parsers/branchParser.ts | 4 +- src/plus/github/githubGitProvider.ts | 27 +++++-- src/views/nodes/repositoryNode.ts | 1 + src/views/nodes/viewNode.ts | 2 +- 9 files changed, 91 insertions(+), 149 deletions(-) diff --git a/src/commands/openAssociatedPullRequestOnRemote.ts b/src/commands/openAssociatedPullRequestOnRemote.ts index c839fefba69d4..07cde8fc8013b 100644 --- a/src/commands/openAssociatedPullRequestOnRemote.ts +++ b/src/commands/openAssociatedPullRequestOnRemote.ts @@ -19,6 +19,7 @@ export class OpenAssociatedPullRequestOnRemoteCommand extends ActiveEditorComman const gitUri = uri != null ? await GitUri.fromUri(uri) : undefined; + let args: OpenPullRequestOnRemoteCommandArgs; if (editor != null && gitUri != null) { const blameline = editor.selection.active.line; if (blameline < 0) return; @@ -27,46 +28,31 @@ export class OpenAssociatedPullRequestOnRemoteCommand extends ActiveEditorComman const blame = await this.container.git.getBlameForLine(gitUri, blameline); if (blame == null) return; - await executeCommand(Commands.OpenPullRequestOnRemote, { - clipboard: false, - ref: blame.commit.sha, - repoPath: blame.commit.repoPath, - }); + args = { clipboard: false, ref: blame.commit.sha, repoPath: blame.commit.repoPath }; } catch (ex) { Logger.error(ex, 'OpenAssociatedPullRequestOnRemoteCommand', `getBlameForLine(${blameline})`); + return; } + } else { + try { + const repo = await getRepositoryOrShowPicker('Open Associated Pull Request', undefined, undefined, { + filter: async r => (await this.container.git.getBestRemoteWithRichProvider(r.uri)) != null, + }); + if (repo == null) return; - return; - } - - try { - const repo = await getRepositoryOrShowPicker('Open Pull Request Associated', undefined, undefined, { - filter: async r => (await this.container.git.getBestRemoteWithRichProvider(r.uri))?.provider != null, - }); - if (repo == null) return; - - const remote = await this.container.git.getBestRemoteWithRichProvider(repo.uri); - if (remote?.provider == null) return; - - const branch = await repo.getBranch(); - if (branch == null) return; - - let pr = await this.container.git.getPullRequestForBranch(branch.ref, remote.provider); - if (pr == null) { - const commit = await repo.getCommit('HEAD'); - if (commit == null) return; + const branch = await repo?.getBranch(); + const pr = await branch?.getAssociatedPullRequest(); - pr = await commit.getAssociatedPullRequest(remote); - if (pr == null) return; + args = + pr != null + ? { clipboard: false, pr: { url: pr.url } } + : { clipboard: false, ref: branch?.name ?? 'HEAD', repoPath: repo.path }; + } catch (ex) { + Logger.error(ex, 'OpenAssociatedPullRequestOnRemoteCommand', 'No editor opened'); + return; } - - await executeCommand(Commands.OpenPullRequestOnRemote, { - pr: { - url: pr.url, - }, - }); - } catch (ex) { - Logger.error(ex, 'OpenAssociatedPullRequestOnRemoteCommand', 'No editor opened'); } + + await executeCommand(Commands.OpenPullRequestOnRemote, args); } } diff --git a/src/commands/openPullRequestOnRemote.ts b/src/commands/openPullRequestOnRemote.ts index 293d32f1d74f8..497c9b8d407c5 100644 --- a/src/commands/openPullRequestOnRemote.ts +++ b/src/commands/openPullRequestOnRemote.ts @@ -1,6 +1,7 @@ -import { env, Uri } from 'vscode'; +import { env, Uri, window } from 'vscode'; import { Commands } from '../constants'; import type { Container } from '../container'; +import { shortenRevision } from '../git/models/reference'; import { command } from '../system/command'; import { PullRequestNode } from '../views/nodes/pullRequestNode'; import type { CommandContext } from './base'; @@ -39,7 +40,10 @@ export class OpenPullRequestOnRemoteCommand extends Command { if (!remote?.hasRichIntegration()) return; const pr = await remote.provider.getPullRequestForCommit(args.ref); - if (pr == null) return; + if (pr == null) { + void window.showInformationMessage(`No pull request associated with '${shortenRevision(args.ref)}'`); + return; + } args = { ...args }; args.pr = pr; diff --git a/src/env/node/git/localGitProvider.ts b/src/env/node/git/localGitProvider.ts index 861f85a352244..368b33c495165 100644 --- a/src/env/node/git/localGitProvider.ts +++ b/src/env/node/git/localGitProvider.ts @@ -1753,6 +1753,7 @@ export class LocalGitProvider implements GitProvider, Disposable { ]); branch = new GitBranch( + this.container, repoPath, rebaseStatus?.incoming.name ?? name, false, @@ -1801,6 +1802,7 @@ export class LocalGitProvider implements GitProvider, Disposable { ]); current = new GitBranch( + this.container, repoPath!, rebaseStatus?.incoming.name ?? name, false, @@ -1818,7 +1820,7 @@ export class LocalGitProvider implements GitProvider, Disposable { return current != null ? { values: [current] } : emptyPagedResult; } - return { values: GitBranchParser.parse(data, repoPath!) }; + return { values: GitBranchParser.parse(this.container, data, repoPath!) }; } catch (ex) { this._branchesCache.delete(repoPath!); diff --git a/src/git/gitProviderService.ts b/src/git/gitProviderService.ts index ace45f4ea7b73..7eab26614572d 100644 --- a/src/git/gitProviderService.ts +++ b/src/git/gitProviderService.ts @@ -63,7 +63,7 @@ import type { GitGraph } from './models/graph'; import type { SearchedIssue } from './models/issue'; import type { GitLog } from './models/log'; import type { GitMergeStatus } from './models/merge'; -import type { PullRequest, PullRequestState, SearchedPullRequest } from './models/pullRequest'; +import type { SearchedPullRequest } from './models/pullRequest'; import type { GitRebaseStatus } from './models/rebase'; import type { GitBranchReference, GitReference } from './models/reference'; import { createRevisionRange, isSha, isUncommitted, isUncommittedParent } from './models/reference'; @@ -1925,59 +1925,6 @@ export class GitProviderService implements Disposable { return provider.getPreviousComparisonUrisForLine(path, uri, editorLine, ref, skip); } - async getPullRequestForBranch( - branch: string, - remote: GitRemote, - options?: { avatarSize?: number; include?: PullRequestState[]; limit?: number; timeout?: number }, - ): Promise; - async getPullRequestForBranch( - branch: string, - provider: RichRemoteProvider, - options?: { avatarSize?: number; include?: PullRequestState[]; limit?: number; timeout?: number }, - ): Promise; - @gate((branch, remoteOrProvider, options) => { - const provider = GitRemote.is(remoteOrProvider) ? remoteOrProvider.provider : remoteOrProvider; - return `${branch}${ - provider != null ? `|${provider.id}:${provider.domain}/${provider.path}` : '' - }|${JSON.stringify(options)}`; - }) - @debug({ args: { 1: remoteOrProvider => remoteOrProvider.name } }) - async getPullRequestForBranch( - branch: string, - remoteOrProvider: GitRemote | RichRemoteProvider, - options?: { avatarSize?: number; include?: PullRequestState[]; limit?: number; timeout?: number }, - ): Promise { - let provider; - if (GitRemote.is(remoteOrProvider)) { - ({ provider } = remoteOrProvider); - if (!provider?.hasRichIntegration()) return undefined; - } else { - provider = remoteOrProvider; - } - - let timeout; - if (options != null) { - ({ timeout, ...options } = options); - } - - let promiseOrPR = provider.getPullRequestForBranch(branch, options); - if (promiseOrPR == null || !isPromise(promiseOrPR)) { - return promiseOrPR; - } - - if (timeout != null && timeout > 0) { - promiseOrPR = cancellable(promiseOrPR, timeout); - } - - try { - return await promiseOrPR; - } catch (ex) { - if (ex instanceof PromiseCancelledError) throw ex; - - return undefined; - } - } - @debug({ args: { 0: remoteOrProvider => remoteOrProvider.name } }) async getMyPullRequests( remoteOrProvider: GitRemote | RichRemoteProvider, @@ -2069,7 +2016,7 @@ export class GitProviderService implements Disposable { async getBestRemoteWithProvider( repoPath: string | Uri, cancellation?: CancellationToken, - ): Promise | undefined> { + ): Promise | undefined> { const remotes = await this.getBestRemotesWithProviders(repoPath, cancellation); return remotes[0]; } @@ -2078,7 +2025,7 @@ export class GitProviderService implements Disposable { async getBestRemotesWithProviders( repoPath: string | Uri, cancellation?: CancellationToken, - ): Promise[]> { + ): Promise[]> { if (repoPath == null) return []; if (typeof repoPath === 'string') { repoPath = this.getAbsoluteUri(repoPath); @@ -2097,7 +2044,7 @@ export class GitProviderService implements Disposable { const defaultRemote = remotes.find(r => r.default)?.name; const currentBranchRemote = (await this.getBranch(remotes[0].repoPath))?.getRemoteName(); - const weighted: [number, GitRemote][] = []; + const weighted: [number, GitRemote][] = []; let originalFound = false; @@ -2195,11 +2142,19 @@ export class GitProviderService implements Disposable { repoPath: string | Uri, options?: { sort?: boolean }, cancellation?: CancellationToken, - ): Promise[]> { + ): Promise[]> { const remotes = await this.getRemotes(repoPath, options, cancellation); - return remotes.filter( - (r: GitRemote): r is GitRemote => r.provider != null, - ); + return remotes.filter((r: GitRemote): r is GitRemote => r.provider != null); + } + + @log() + async getRemotesWithRichProviders( + repoPath: string | Uri, + options?: { sort?: boolean }, + cancellation?: CancellationToken, + ): Promise[]> { + const remotes = await this.getRemotes(repoPath, options, cancellation); + return remotes.filter((r: GitRemote): r is GitRemote => r.hasRichIntegration()); } getBestRepository(): Repository | undefined; diff --git a/src/git/models/branch.ts b/src/git/models/branch.ts index 94ae98c06fabe..3e582d4bb2f3a 100644 --- a/src/git/models/branch.ts +++ b/src/git/models/branch.ts @@ -1,14 +1,11 @@ import { BranchSorting, DateStyle } from '../../config'; -import { Container } from '../../container'; +import type { Container } from '../../container'; import { configuration } from '../../system/configuration'; import { formatDate, fromNow } from '../../system/date'; import { debug } from '../../system/decorators/log'; import { memoize } from '../../system/decorators/memoize'; import { getLoggableName } from '../../system/logger'; -import { cancellable } from '../../system/promise'; import { sortCompare } from '../../system/string'; -import type { RemoteProvider } from '../remotes/remoteProvider'; -import type { RichRemoteProvider } from '../remotes/richRemoteProvider'; import type { PullRequest, PullRequestState } from './pullRequest'; import type { GitBranchReference, GitReference } from './reference'; import { getBranchTrackingWithoutRemote, shortenRevision } from './reference'; @@ -53,6 +50,7 @@ export class GitBranch implements GitBranchReference { readonly state: GitTrackingState; constructor( + private readonly container: Container, public readonly repoPath: string, public readonly name: string, public readonly remote: boolean, @@ -84,8 +82,8 @@ export class GitBranch implements GitBranchReference { } get formattedDate(): string { - return Container.instance.BranchDateFormatting.dateStyle === DateStyle.Absolute - ? this.formatDate(Container.instance.BranchDateFormatting.dateFormat) + return this.container.BranchDateFormatting.dateStyle === DateStyle.Absolute + ? this.formatDate(this.container.BranchDateFormatting.dateFormat) : this.formatDateFromNow(); } @@ -102,27 +100,18 @@ export class GitBranch implements GitBranchReference { return this.date != null ? fromNow(this.date) : ''; } - private _pullRequest: Promise | undefined; - @debug() async getAssociatedPullRequest(options?: { avatarSize?: number; include?: PullRequestState[]; - limit?: number; - timeout?: number; }): Promise { - if (this._pullRequest == null) { - async function getCore(this: GitBranch): Promise { - const remote = await this.getRemoteWithProvider(); - if (remote == null) return undefined; - - const branch = this.getTrackingWithoutRemote() ?? this.getNameWithoutRemote(); - return Container.instance.git.getPullRequestForBranch(branch, remote, options); - } - this._pullRequest = getCore.call(this); - } - - return cancellable(this._pullRequest, options?.timeout); + const remote = await this.getRemote(); + return remote?.hasRichIntegration() + ? remote.provider.getPullRequestForBranch( + this.getTrackingWithoutRemote() ?? this.getNameWithoutRemote(), + options, + ) + : undefined; } @memoize() @@ -147,21 +136,8 @@ export class GitBranch implements GitBranchReference { const remoteName = this.getRemoteName(); if (remoteName == null) return undefined; - const remotes = await Container.instance.git.getRemotes(this.repoPath); - if (remotes.length === 0) return undefined; - - return remotes.find(r => r.name === remoteName); - } - - @memoize() - async getRemoteWithProvider(): Promise | undefined> { - const remoteName = this.getRemoteName(); - if (remoteName == null) return undefined; - - const remotes = await Container.instance.git.getRemotesWithProviders(this.repoPath); - if (remotes.length === 0) return undefined; - - return remotes.find(r => r.name === remoteName); + const remotes = await this.container.git.getRemotes(this.repoPath); + return remotes.length ? remotes.find(r => r.name === remoteName) : undefined; } @memoize() @@ -184,10 +160,9 @@ export class GitBranch implements GitBranchReference { return GitBranchStatus.UpToDate; } - const remotes = await Container.instance.git.getRemotesWithProviders(this.repoPath); - if (remotes.length > 0) return GitBranchStatus.Unpublished; - - return GitBranchStatus.Local; + // If there are any remotes then say this is unpublished, otherwise local + const remotes = await this.container.git.getRemotes(this.repoPath); + return remotes.length ? GitBranchStatus.Unpublished : GitBranchStatus.Local; } getTrackingStatus(options?: { @@ -203,16 +178,16 @@ export class GitBranch implements GitBranchReference { } get starred() { - const starred = Container.instance.storage.getWorkspace('starred:branches'); + const starred = this.container.storage.getWorkspace('starred:branches'); return starred !== undefined && starred[this.id] === true; } star() { - return Container.instance.git.getRepository(this.repoPath)?.star(this); + return this.container.git.getRepository(this.repoPath)?.star(this); } unstar() { - return Container.instance.git.getRepository(this.repoPath)?.unstar(this); + return this.container.git.getRepository(this.repoPath)?.unstar(this); } } diff --git a/src/git/parsers/branchParser.ts b/src/git/parsers/branchParser.ts index 8886255862df4..96bc889c90688 100644 --- a/src/git/parsers/branchParser.ts +++ b/src/git/parsers/branchParser.ts @@ -1,3 +1,4 @@ +import type { Container } from '../../container'; import { debug } from '../../system/decorators/log'; import { GitBranch } from '../models/branch'; @@ -20,7 +21,7 @@ export class GitBranchParser { ].join(''); @debug({ args: false, singleLine: true }) - static parse(data: string, repoPath: string): GitBranch[] { + static parse(container: Container, data: string, repoPath: string): GitBranch[] { const branches: GitBranch[] = []; if (!data) return branches; @@ -57,6 +58,7 @@ export class GitBranchParser { branches.push( new GitBranch( + container, repoPath, name, remote, diff --git a/src/plus/github/githubGitProvider.ts b/src/plus/github/githubGitProvider.ts index 2f6b43e2913c2..d1169e7aa7161 100644 --- a/src/plus/github/githubGitProvider.ts +++ b/src/plus/github/githubGitProvider.ts @@ -917,11 +917,28 @@ export class GitHubGitProvider implements GitProvider, Disposable { const ref = branch.target.oid; branches.push( - new GitBranch(repoPath!, branch.name, false, branch.name === current, date, ref, { - name: `origin/${branch.name}`, - missing: false, - }), - new GitBranch(repoPath!, `origin/${branch.name}`, true, false, date, ref), + new GitBranch( + this.container, + repoPath!, + branch.name, + false, + branch.name === current, + date, + ref, + { + name: `origin/${branch.name}`, + missing: false, + }, + ), + new GitBranch( + this.container, + repoPath!, + `origin/${branch.name}`, + true, + false, + date, + ref, + ), ); } diff --git a/src/views/nodes/repositoryNode.ts b/src/views/nodes/repositoryNode.ts index 19100a268076e..2c4ed9fae3aa5 100644 --- a/src/views/nodes/repositoryNode.ts +++ b/src/views/nodes/repositoryNode.ts @@ -83,6 +83,7 @@ export class RepositoryNode extends SubscribeableViewNode const status = await this._status; if (status != null) { const branch = new GitBranch( + this.view.container, status.repoPath, status.branch, false, diff --git a/src/views/nodes/viewNode.ts b/src/views/nodes/viewNode.ts index 95935c2a43ff1..834c322a63837 100644 --- a/src/views/nodes/viewNode.ts +++ b/src/views/nodes/viewNode.ts @@ -592,7 +592,7 @@ export abstract class RepositoryFolderNode< ); providerName = providers?.length ? providers[0].name : undefined; } else { - const remote = await branch.getRemoteWithProvider(); + const remote = await branch.getRemote(); providerName = remote?.provider?.name; }