From 217c5749a0ff03408b8e772b9d6b6707f914db88 Mon Sep 17 00:00:00 2001 From: Eric Amodio Date: Fri, 10 Jun 2022 14:44:58 -0400 Subject: [PATCH] Reworks getting the "best" remote with a provider Improves best remote caching --- src/annotations/lineAnnotationController.ts | 2 +- src/avatars.ts | 2 +- src/commands/openPullRequestOnRemote.ts | 2 +- src/commands/remoteProviders.ts | 4 +- src/git/gitProviderService.ts | 133 ++++++++++++++++++++-------- src/git/models/commit.ts | 2 +- src/git/models/repository.ts | 2 +- src/hovers/hovers.ts | 6 +- src/statusbar/statusBarController.ts | 2 +- src/views/nodes/autolinkedItemNode.ts | 64 ++++++++++--- src/views/nodes/commitNode.ts | 2 +- src/views/nodes/fileRevisionAsCommitNode.ts | 2 +- src/views/nodes/rebaseStatusNode.ts | 2 +- src/views/nodes/resultsCommitsNode.ts | 6 +- 14 files changed, 165 insertions(+), 66 deletions(-) diff --git a/src/annotations/lineAnnotationController.ts b/src/annotations/lineAnnotationController.ts index 99024fd..78c2480 100644 --- a/src/annotations/lineAnnotationController.ts +++ b/src/annotations/lineAnnotationController.ts @@ -162,7 +162,7 @@ export class LineAnnotationController implements Disposable { ) { if (lines.length === 0) return undefined; - const remote = await this.container.git.getRichRemoteProvider(repoPath); + const remote = await this.container.git.getBestRemoteWithRichProvider(repoPath); if (remote?.provider == null) return undefined; const refs = new Set(); diff --git a/src/avatars.ts b/src/avatars.ts index bd56770..2fc12ab 100644 --- a/src/avatars.ts +++ b/src/avatars.ts @@ -190,7 +190,7 @@ async function getAvatarUriFromRemoteProvider( // account = await remote?.provider.getAccountForEmail(email, { avatarSize: size }); // } else { if (typeof repoPathOrCommit !== 'string') { - const remote = await Container.instance.git.getRichRemoteProvider(repoPathOrCommit.repoPath); + const remote = await Container.instance.git.getBestRemoteWithRichProvider(repoPathOrCommit.repoPath); account = await remote?.provider.getAccountForCommit(repoPathOrCommit.ref, { avatarSize: size }); } } diff --git a/src/commands/openPullRequestOnRemote.ts b/src/commands/openPullRequestOnRemote.ts index d2daaac..bd99df1 100644 --- a/src/commands/openPullRequestOnRemote.ts +++ b/src/commands/openPullRequestOnRemote.ts @@ -34,7 +34,7 @@ export class OpenPullRequestOnRemoteCommand extends Command { if (args?.pr == null) { if (args?.repoPath == null || args?.ref == null) return; - const remote = await this.container.git.getRichRemoteProvider(args.repoPath); + const remote = await this.container.git.getBestRemoteWithRichProvider(args.repoPath); if (remote?.provider == null) return; const pr = await this.container.git.getPullRequestForCommit(args.ref, remote.provider); diff --git a/src/commands/remoteProviders.ts b/src/commands/remoteProviders.ts index fd926e0..8a06a3a 100644 --- a/src/commands/remoteProviders.ts +++ b/src/commands/remoteProviders.ts @@ -75,7 +75,7 @@ export class ConnectRemoteProviderCommand extends Command { } else if (args?.remote == null) { repoPath = args.repoPath; - remote = await this.container.git.getRichRemoteProvider(repoPath, { includeDisconnected: true }); + remote = await this.container.git.getBestRemoteWithRichProvider(repoPath, { includeDisconnected: true }); if (remote == null) return false; } else { repoPath = args.repoPath; @@ -166,7 +166,7 @@ export class DisconnectRemoteProviderCommand extends Command { } else if (args?.remote == null) { repoPath = args.repoPath; - remote = await this.container.git.getRichRemoteProvider(repoPath, { includeDisconnected: false }); + remote = await this.container.git.getBestRemoteWithRichProvider(repoPath, { includeDisconnected: false }); if (remote == null) return undefined; } else { repoPath = args.repoPath; diff --git a/src/git/gitProviderService.ts b/src/git/gitProviderService.ts index 6576a3d..6cfcc9f 100644 --- a/src/git/gitProviderService.ts +++ b/src/git/gitProviderService.ts @@ -166,7 +166,9 @@ export class GitProviderService implements Disposable { private readonly _pendingRepositories = new Map>(); private readonly _providers = new Map(); private readonly _repositories = new Repositories(); - private readonly _richRemotesCache = new Map | null>(); + private readonly _bestRemotesCache: Map | null> & + Map<`rich|${RepoComparisonKey}`, GitRemote | null> & + Map<`rich+connected|${RepoComparisonKey}`, GitRemote | null> = new Map(); private readonly _visitedPaths = new VisitedPathsTrie(); constructor(private readonly container: Container) { @@ -358,7 +360,7 @@ export class GitProviderService implements Disposable { RepositoryChangeComparisonMode.Any, ) ) { - this._richRemotesCache.clear(); + this._bestRemotesCache.clear(); } if (e.changed(RepositoryChange.Closed, RepositoryChangeComparisonMode.Any)) { @@ -968,7 +970,7 @@ export class GitProviderService implements Disposable { ...affects: ('branches' | 'contributors' | 'providers' | 'remotes' | 'stashes' | 'status' | 'tags')[] ): void { if (affects.length === 0 || affects.includes('providers')) { - this._richRemotesCache.clear(); + this._bestRemotesCache.clear(); } const repoAffects = affects.filter((c): c is 'branches' | 'remotes' => c === 'branches' || c === 'remotes'); @@ -1661,60 +1663,53 @@ export class GitProviderService implements Disposable { return provider.getIncomingActivity(path, options); } - async getRichRemoteProvider( + async getBestRemoteWithProvider( repoPath: string | Uri | undefined, - options?: { includeDisconnected?: boolean }, - ): Promise | undefined>; - async getRichRemoteProvider( + ): Promise | undefined>; + async getBestRemoteWithProvider( remotes: GitRemote[], - options?: { includeDisconnected?: boolean }, - ): Promise | undefined>; - @gate( - (remotesOrRepoPath, options) => + ): Promise | undefined>; + @gate( + remotesOrRepoPath => `${ remotesOrRepoPath == null || typeof remotesOrRepoPath === 'string' ? remotesOrRepoPath : remotesOrRepoPath instanceof Uri ? remotesOrRepoPath.toString() - : `${remotesOrRepoPath[0]?.repoPath}|${remotesOrRepoPath?.map(r => r.id).join(',') ?? ''}` - }|${options?.includeDisconnected ?? false}`, + : `${remotesOrRepoPath.length}:${remotesOrRepoPath[0]?.repoPath ?? ''}` + }`, ) - @log({ + @log({ args: { 0: remotesOrRepoPath => Array.isArray(remotesOrRepoPath) ? remotesOrRepoPath.map(r => r.name).join(',') : remotesOrRepoPath, }, }) - async getRichRemoteProvider( + async getBestRemoteWithProvider( remotesOrRepoPath: GitRemote[] | string | Uri | undefined, - options?: { includeDisconnected?: boolean }, - ): Promise | undefined> { + ): Promise | undefined> { if (remotesOrRepoPath == null) return undefined; let remotes; + let repoPath; if (Array.isArray(remotesOrRepoPath)) { if (remotesOrRepoPath.length === 0) return undefined; remotes = remotesOrRepoPath; - remotesOrRepoPath = remotesOrRepoPath[0].repoPath; + repoPath = remotesOrRepoPath[0].repoPath; + } else { + repoPath = remotesOrRepoPath; } - if (typeof remotesOrRepoPath === 'string') { - remotesOrRepoPath = this.getAbsoluteUri(remotesOrRepoPath); + if (typeof repoPath === 'string') { + repoPath = this.getAbsoluteUri(repoPath); } - const cacheKey = asRepoComparisonKey(remotesOrRepoPath); - - let richRemote = this._richRemotesCache.get(cacheKey); - if (richRemote != null) return richRemote; - if (richRemote === null && !options?.includeDisconnected) return undefined; - - if (options?.includeDisconnected) { - richRemote = this._richRemotesCache.get(`disconnected|${cacheKey}`); - if (richRemote !== undefined) return richRemote ?? undefined; - } + const cacheKey = asRepoComparisonKey(repoPath); + let remote = this._bestRemotesCache.get(cacheKey); + if (remote !== undefined) return remote ?? undefined; - remotes = (remotes ?? (await this.getRemotesWithProviders(remotesOrRepoPath))).filter( + remotes = (remotes ?? (await this.getRemotesWithProviders(repoPath))).filter( ( r: GitRemote, ): r is GitRemote => r.provider != null, @@ -1722,7 +1717,6 @@ export class GitProviderService implements Disposable { if (remotes.length === 0) return undefined; - let remote; if (remotes.length === 1) { remote = remotes[0]; } else { @@ -1757,18 +1751,85 @@ export class GitProviderService implements Disposable { remote = bestRemote ?? null; } + this._bestRemotesCache.set(cacheKey, remote); + + return remote ?? undefined; + } + + async getBestRemoteWithRichProvider( + repoPath: string | Uri | undefined, + options?: { includeDisconnected?: boolean }, + ): Promise | undefined>; + async getBestRemoteWithRichProvider( + remotes: GitRemote[], + options?: { includeDisconnected?: boolean }, + ): Promise | undefined>; + @gate( + (remotesOrRepoPath, options) => + `${ + remotesOrRepoPath == null || typeof remotesOrRepoPath === 'string' + ? remotesOrRepoPath + : remotesOrRepoPath instanceof Uri + ? remotesOrRepoPath.toString() + : `${remotesOrRepoPath.length}:${remotesOrRepoPath[0]?.repoPath ?? ''}` + }|${options?.includeDisconnected ?? false}`, + ) + @log({ + args: { + 0: remotesOrRepoPath => + Array.isArray(remotesOrRepoPath) ? remotesOrRepoPath.map(r => r.name).join(',') : remotesOrRepoPath, + }, + }) + async getBestRemoteWithRichProvider( + remotesOrRepoPath: GitRemote[] | string | Uri | undefined, + options?: { includeDisconnected?: boolean }, + ): Promise | undefined> { + if (remotesOrRepoPath == null) return undefined; + + let remotes; + let repoPath; + if (Array.isArray(remotesOrRepoPath)) { + if (remotesOrRepoPath.length === 0) return undefined; + + remotes = remotesOrRepoPath; + repoPath = remotesOrRepoPath[0].repoPath; + } else { + repoPath = remotesOrRepoPath; + } + + if (typeof repoPath === 'string') { + repoPath = this.getAbsoluteUri(repoPath); + } + + const cacheKey = asRepoComparisonKey(repoPath); + + let richRemote = this._bestRemotesCache.get(`rich+connected|${cacheKey}`); + if (richRemote != null) return richRemote; + if (richRemote === null && !options?.includeDisconnected) return undefined; + + if (options?.includeDisconnected) { + richRemote = this._bestRemotesCache.get(`rich|${cacheKey}`); + if (richRemote !== undefined) return richRemote ?? undefined; + } + + const remote = await (remotes != null + ? this.getBestRemoteWithProvider(remotes) + : this.getBestRemoteWithProvider(repoPath)); + if (!remote?.hasRichProvider()) { - this._richRemotesCache.set(cacheKey, null); + this._bestRemotesCache.set(`rich|${cacheKey}`, null); + this._bestRemotesCache.set(`rich+connected|${cacheKey}`, null); return undefined; } const { provider } = remote; const connected = provider.maybeConnected ?? (await provider.isConnected()); if (connected) { - this._richRemotesCache.set(cacheKey, remote); + this._bestRemotesCache.set(`rich|${cacheKey}`, remote); + this._bestRemotesCache.set(`rich+connected|${cacheKey}`, remote); } else { - this._richRemotesCache.set(cacheKey, null); - this._richRemotesCache.set(`disconnected|${cacheKey}`, remote); + this._bestRemotesCache.set(`rich|${cacheKey}`, remote); + this._bestRemotesCache.set(`rich+connected|${cacheKey}`, null); if (!options?.includeDisconnected) return undefined; } diff --git a/src/git/models/commit.ts b/src/git/models/commit.ts index a23ab8a..da2f75c 100644 --- a/src/git/models/commit.ts +++ b/src/git/models/commit.ts @@ -392,7 +392,7 @@ export class GitCommit implements GitRevisionReference { async getAssociatedPullRequest(options?: { timeout?: number }): Promise { if (this._pullRequest == null) { async function getCore(this: GitCommit): Promise { - const remote = await this.container.git.getRichRemoteProvider(this.repoPath); + const remote = await this.container.git.getBestRemoteWithRichProvider(this.repoPath); if (remote?.provider == null) return undefined; return this.container.git.getPullRequestForCommit(this.ref, remote, options); diff --git a/src/git/models/repository.ts b/src/git/models/repository.ts index e25e5b7..78da2c2 100644 --- a/src/git/models/repository.ts +++ b/src/git/models/repository.ts @@ -588,7 +588,7 @@ export class Repository implements Disposable { } async getRichRemote(connectedOnly: boolean = false): Promise | undefined> { - return this.container.git.getRichRemoteProvider(await this.getRemotes(), { + return this.container.git.getBestRemoteWithRichProvider(await this.getRemotes(), { includeDisconnected: !connectedOnly, }); } diff --git a/src/hovers/hovers.ts b/src/hovers/hovers.ts index f57550e..bd11532 100644 --- a/src/hovers/hovers.ts +++ b/src/hovers/hovers.ts @@ -297,7 +297,7 @@ export namespace Hovers { return undefined; } - const remote = await Container.instance.git.getRichRemoteProvider(remotes); + const remote = await Container.instance.git.getBestRemoteWithRichProvider(remotes); if (remote?.provider == null) { Logger.debug(cc, `completed ${GlyphChars.Dot} ${getDurationMilliseconds(start)} ms`); @@ -372,7 +372,9 @@ export namespace Hovers { return undefined; } - const remote = await Container.instance.git.getRichRemoteProvider(remotes, { includeDisconnected: true }); + const remote = await Container.instance.git.getBestRemoteWithRichProvider(remotes, { + includeDisconnected: true, + }); if (remote?.provider == null) { Logger.debug(cc, `completed ${GlyphChars.Dot} ${getDurationMilliseconds(start)} ms`); diff --git a/src/statusbar/statusBarController.ts b/src/statusbar/statusBarController.ts index decc228..a189828 100644 --- a/src/statusbar/statusBarController.ts +++ b/src/statusbar/statusBarController.ts @@ -341,7 +341,7 @@ export class StatusBarController implements Disposable { commit: GitCommit, { timeout }: { timeout?: number } = {}, ): Promise> | undefined> { - const remote = await this.container.git.getRichRemoteProvider(commit.repoPath); + const remote = await this.container.git.getBestRemoteWithRichProvider(commit.repoPath); if (remote?.provider == null) return undefined; const { provider } = remote; diff --git a/src/views/nodes/autolinkedItemNode.ts b/src/views/nodes/autolinkedItemNode.ts index 823e51f..1e481a6 100644 --- a/src/views/nodes/autolinkedItemNode.ts +++ b/src/views/nodes/autolinkedItemNode.ts @@ -1,4 +1,5 @@ -import { MarkdownString, TreeItem, TreeItemCollapsibleState } from 'vscode'; +import { MarkdownString, ThemeIcon, TreeItem, TreeItemCollapsibleState } from 'vscode'; +import { Autolink } from '../../annotations/autolinks'; import { GitUri } from '../../git/gitUri'; import { GitFile, IssueOrPullRequest, IssueOrPullRequestType } from '../../git/models'; import { fromNow } from '../../system/date'; @@ -19,17 +20,17 @@ export class AutolinkedItemNode extends ViewNode { view: ViewsWithCommits, parent: ViewNode, public readonly repoPath: string, - public readonly issue: IssueOrPullRequest, + public readonly item: Autolink | IssueOrPullRequest, ) { super(GitUri.fromRepoPath(repoPath), view, parent); } override toClipboard(): string { - return this.issue.url; + return this.item.url; } override get id(): string { - return `${this.parent!.id!}:item(${this.issue.id})`; + return `${this.parent!.id!}:item(${this.item.id})`; } getChildren(): ViewNode[] { @@ -37,24 +38,55 @@ export class AutolinkedItemNode extends ViewNode { } getTreeItem(): TreeItem { - const relativeTime = fromNow(this.issue.closedDate ?? this.issue.date); + if (!isIssueOrPullRequest(this.item)) { + const { provider } = this.item; - const item = new TreeItem(`${this.issue.id}: ${this.issue.title}`, TreeItemCollapsibleState.None); + const item = new TreeItem( + `${provider?.name ? `${provider.name} ` : ''}${this.item.prefix}${this.item.id}`, + TreeItemCollapsibleState.None, + ); + item.iconPath = new ThemeIcon( + this.item.type == null + ? 'link' + : this.item.type === IssueOrPullRequestType.PullRequest + ? 'git-pull-request' + : 'issues', + ); + item.contextValue = ContextValues.AutolinkedItem; + item.tooltip = new MarkdownString( + `${ + this.item.description + ? `Autolink: ${this.item.description}` + : `${ + this.item.type == null + ? 'Autolink' + : this.item.type === IssueOrPullRequestType.PullRequest + ? 'Autolinked Pull Request' + : 'Autolinked Issue' + }: **${this.item.prefix}${this.item.id}**` + } \\\n[${this.item.url}](${this.item.url}${this.item.title != null ? ` "${this.item.title}"` : ''})`, + ); + return item; + } + + const relativeTime = fromNow(this.item.closedDate ?? this.item.date); + + const item = new TreeItem(`${this.item.id}: ${this.item.title}`, TreeItemCollapsibleState.None); item.description = relativeTime; - item.iconPath = IssueOrPullRequest.getThemeIcon(this.issue); + item.iconPath = IssueOrPullRequest.getThemeIcon(this.item); item.contextValue = - this.issue.type === IssueOrPullRequestType.PullRequest + this.item.type === IssueOrPullRequestType.PullRequest ? ContextValues.PullRequest : ContextValues.AutolinkedIssue; const linkTitle = ` "Open ${ - this.issue.type === IssueOrPullRequestType.PullRequest ? 'Pull Request' : 'Issue' - } \\#${this.issue.id} on ${this.issue.provider.name}"`; + this.item.type === IssueOrPullRequestType.PullRequest ? 'Pull Request' : 'Issue' + } \\#${this.item.id} on ${this.item.provider.name}"`; const tooltip = new MarkdownString( - `${IssueOrPullRequest.getMarkdownIcon(this.issue)} [**${this.issue.title.trim()}**](${ - this.issue.url - }${linkTitle}) \\\n[#${this.issue.id}](${this.issue.url}${linkTitle}) was ${ - this.issue.closed ? 'closed' : 'opened' + `${IssueOrPullRequest.getMarkdownIcon(this.item)} [**${this.item.title.trim()}**](${ + this.item.url + }${linkTitle}) \\\n[#${this.item.id}](${this.item.url}${linkTitle}) was ${ + this.item.closed ? 'closed' : 'opened' } ${relativeTime}`, true, ); @@ -66,3 +98,7 @@ export class AutolinkedItemNode extends ViewNode { return item; } } + +function isIssueOrPullRequest(item: Autolink | IssueOrPullRequest): item is IssueOrPullRequest { + return 'closed' in item && typeof item.closed === 'boolean'; +} diff --git a/src/views/nodes/commitNode.ts b/src/views/nodes/commitNode.ts index cfd327b..4f6b257 100644 --- a/src/views/nodes/commitNode.ts +++ b/src/views/nodes/commitNode.ts @@ -131,7 +131,7 @@ export class CommitNode extends ViewRefNode