From ceb5739e5a696e9c90b16d314f1c2e0edd07e738 Mon Sep 17 00:00:00 2001 From: Eric Amodio Date: Mon, 18 Sep 2023 00:49:05 -0400 Subject: [PATCH] Optimizes best remote detection (still wip) Fixes leak with rich remote providers Avoids needing repo remote subscriptions --- src/annotations/lineAnnotationController.ts | 5 +- src/container.ts | 39 ++--- src/env/node/git/localGitProvider.ts | 16 ++ src/git/gitProviderService.ts | 253 +++++++++------------------- src/git/models/repository.ts | 61 ++++--- src/git/parsers/remoteParser.ts | 4 + src/git/remotes/remoteProviderService.ts | 11 ++ src/git/remotes/remoteProviders.ts | 1 + src/git/remotes/richRemoteProvider.ts | 27 ++- src/hovers/hovers.ts | 60 +++---- src/views/nodes/commitNode.ts | 11 +- src/views/nodes/fileRevisionAsCommitNode.ts | 6 +- src/views/nodes/rebaseStatusNode.ts | 6 +- 13 files changed, 232 insertions(+), 268 deletions(-) diff --git a/src/annotations/lineAnnotationController.ts b/src/annotations/lineAnnotationController.ts index cb2a742..b050eb9 100644 --- a/src/annotations/lineAnnotationController.ts +++ b/src/annotations/lineAnnotationController.ts @@ -15,6 +15,7 @@ import { detailsMessage } from '../hovers/hovers'; import { configuration } from '../system/configuration'; import { debug, log } from '../system/decorators/log'; import { once } from '../system/event'; +import { debounce } from '../system/function'; import { count, every, filter } from '../system/iterable'; import { Logger } from '../system/logger'; import type { LogScope } from '../system/logger.scope'; @@ -45,7 +46,9 @@ export class LineAnnotationController implements Disposable { once(container.onReady)(this.onReady, this), configuration.onDidChange(this.onConfigurationChanged, this), container.fileAnnotations.onDidToggleAnnotations(this.onFileAnnotationsToggled, this), - container.richRemoteProviders.onDidChangeConnectionState(() => void this.refresh(window.activeTextEditor)), + container.richRemoteProviders.onAfterDidChangeConnectionState( + debounce(() => void this.refresh(window.activeTextEditor), 250), + ), ); } diff --git a/src/container.ts b/src/container.ts index 8e80f67..0eebddd 100644 --- a/src/container.ts +++ b/src/container.ts @@ -190,8 +190,6 @@ export class Container { configuration.onDidChangeAny(this.onAnyConfigurationChanged, this), ]; - this._richRemoteProviders = new RichRemoteProviderService(this); - this._disposables.push((this._connection = new ServerConnection(this))); this._disposables.push( @@ -533,14 +531,6 @@ export class Container { return this._lineTracker; } - private _repositoryPathMapping: RepositoryPathMappingProvider | undefined; - get repositoryPathMapping() { - if (this._repositoryPathMapping == null) { - this._disposables.push((this._repositoryPathMapping = getSupportedRepositoryPathMappingProvider(this))); - } - return this._repositoryPathMapping; - } - private readonly _prerelease; get prerelease() { return this._prerelease; @@ -566,21 +556,27 @@ export class Container { return this._repositoriesView; } - private readonly _searchAndCompareView: SearchAndCompareView; - get searchAndCompareView() { - return this._searchAndCompareView; - } - - private _subscription: SubscriptionService; - get subscription() { - return this._subscription; + private _repositoryPathMapping: RepositoryPathMappingProvider | undefined; + get repositoryPathMapping() { + if (this._repositoryPathMapping == null) { + this._disposables.push((this._repositoryPathMapping = getSupportedRepositoryPathMappingProvider(this))); + } + return this._repositoryPathMapping; } - private readonly _richRemoteProviders: RichRemoteProviderService; + private _richRemoteProviders: RichRemoteProviderService | undefined; get richRemoteProviders(): RichRemoteProviderService { + if (this._richRemoteProviders == null) { + this._richRemoteProviders = new RichRemoteProviderService(this); + } return this._richRemoteProviders; } + private readonly _searchAndCompareView: SearchAndCompareView; + get searchAndCompareView() { + return this._searchAndCompareView; + } + private readonly _stashesView: StashesView; get stashesView() { return this._stashesView; @@ -596,6 +592,11 @@ export class Container { return this._storage; } + private _subscription: SubscriptionService; + get subscription() { + return this._subscription; + } + private readonly _tagsView: TagsView; get tagsView() { return this._tagsView; diff --git a/src/env/node/git/localGitProvider.ts b/src/env/node/git/localGitProvider.ts index 10c56a5..4d3835b 100644 --- a/src/env/node/git/localGitProvider.ts +++ b/src/env/node/git/localGitProvider.ts @@ -291,6 +291,8 @@ export class LocalGitProvider implements GitProvider, Disposable { } if (e.changed(RepositoryChange.Remotes, RepositoryChange.RemoteProviders, RepositoryChangeComparisonMode.Any)) { + const remotes = this._remotesCache.get(repo.path); + void disposeRemotes([remotes]); this._remotesCache.delete(repo.path); } @@ -1091,6 +1093,8 @@ export class LocalGitProvider implements GitProvider, Disposable { } if (caches.length === 0 || caches.includes('remotes')) { + const remotes = this._remotesCache.get(repoPath); + void disposeRemotes([remotes]); this._remotesCache.delete(repoPath); } @@ -1124,6 +1128,7 @@ export class LocalGitProvider implements GitProvider, Disposable { } if (caches.length === 0 || caches.includes('remotes')) { + void disposeRemotes([...this._remotesCache.values()]); this._remotesCache.clear(); } @@ -5288,3 +5293,14 @@ async function getEncoding(uri: Uri): Promise { const encodingExists = (await import(/* webpackChunkName: "encoding" */ 'iconv-lite')).encodingExists; return encodingExists(encoding) ? encoding : 'utf8'; } + +async function disposeRemotes(remotes: (Promise | undefined)[]) { + const remotesResults = await Promise.allSettled(remotes); + for (const remotes of remotesResults) { + for (const remote of getSettledValue(remotes) ?? []) { + if (remote.hasRichProvider()) { + remote.provider?.dispose(); + } + } + } +} diff --git a/src/git/gitProviderService.ts b/src/git/gitProviderService.ts index 4260f03..097c09a 100644 --- a/src/git/gitProviderService.ts +++ b/src/git/gitProviderService.ts @@ -35,6 +35,7 @@ import { Logger } from '../system/logger'; import { getLogScope, setLogScopeExit } from '../system/logger.scope'; import { getBestPath, getScheme, isAbsolute, maybeUri, normalizePath } from '../system/path'; import { asSettled, cancellable, defer, getSettledValue, isPromise, PromiseCancelledError } from '../system/promise'; +import { sortCompare } from '../system/string'; import { VisitedPathsTrie } from '../system/trie'; import type { GitCaches, @@ -198,13 +199,14 @@ export class GitProviderService implements Disposable { readonly supportedSchemes = new Set(); + private readonly _bestRemotesCache = new Map< + RepoComparisonKey, + Promise[]> + >(); private readonly _disposable: Disposable; private readonly _pendingRepositories = new Map>(); private readonly _providers = new Map(); private readonly _repositories = new Repositories(); - 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) { @@ -213,7 +215,7 @@ export class GitProviderService implements Disposable { window.onDidChangeWindowState(this.onWindowStateChanged, this), workspace.onDidChangeWorkspaceFolders(this.onWorkspaceFoldersChanged, this), configuration.onDidChange(this.onConfigurationChanged, this), - container.richRemoteProviders.onDidChangeConnectionState(e => { + container.richRemoteProviders.onAfterDidChangeConnectionState(e => { if (e.reason === 'connected') { resetAvatarCache('failed'); } @@ -2113,189 +2115,108 @@ export class GitProviderService implements Disposable { return provider.getIncomingActivity(path, options); } + @log() async getBestRemoteWithProvider( - repoPath: string | Uri | undefined, - ): Promise | undefined>; - async getBestRemoteWithProvider( - remotes: GitRemote[], - ): Promise | undefined>; - @gate( - remotesOrRepoPath => - `${ - remotesOrRepoPath == null || typeof remotesOrRepoPath === 'string' - ? remotesOrRepoPath - : remotesOrRepoPath instanceof Uri - ? remotesOrRepoPath.toString() - : `${remotesOrRepoPath.length}:${remotesOrRepoPath[0]?.repoPath ?? ''}` - }`, - ) - @log({ - args: { - 0: remotesOrRepoPath => - Array.isArray(remotesOrRepoPath) ? remotesOrRepoPath.map(r => r.name).join(',') : remotesOrRepoPath, - }, - }) - async getBestRemoteWithProvider( - remotesOrRepoPath: GitRemote[] | string | Uri | undefined, + repoPath: string | Uri, ): 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; - } + const remotes = await this.getBestRemotesWithProviders(repoPath); + return remotes[0]; + } + @log() + async getBestRemotesWithProviders( + repoPath: string | Uri, + ): Promise[]> { + if (repoPath == null) return []; if (typeof repoPath === 'string') { repoPath = this.getAbsoluteUri(repoPath); } const cacheKey = asRepoComparisonKey(repoPath); - let remote = this._bestRemotesCache.get(cacheKey); - if (remote !== undefined) return remote ?? undefined; - - remotes = (remotes ?? (await this.getRemotesWithProviders(repoPath))).filter( - (r: GitRemote): r is GitRemote => r.provider != null, - ); - - if (remotes.length === 0) return undefined; - - if (remotes.length === 1) { - remote = remotes[0]; - } else { - const weightedRemotes = new Map([ - ['upstream', 15], - ['origin', 10], - ]); - - const branch = await this.getBranch(remotes[0].repoPath); - const branchRemote = branch?.getRemoteName(); - - if (branchRemote != null) { - weightedRemotes.set(branchRemote, 100); - } - - let bestRemote; - let weight = 0; - for (const r of remotes) { - if (r.default) { - bestRemote = r; - break; - } - - // Don't choose a remote unless its weighted above - let matchedWeight = weightedRemotes.get(r.name) ?? -1; + let remotes = this._bestRemotesCache.get(cacheKey); + if (remotes == null) { + async function getBest(this: GitProviderService) { + const remotes = await this.getRemotesWithProviders(repoPath, { sort: true }); + if (remotes.length === 0) return []; + if (remotes.length === 1) return [...remotes]; + + const defaultRemote = remotes.find(r => r.default)?.name; + const currentBranchRemote = (await this.getBranch(remotes[0].repoPath))?.getRemoteName(); + + const weighted: [number, GitRemote][] = []; + + let originalFound = false; + + for (const remote of remotes) { + let weight; + switch (remote.name) { + case defaultRemote: + weight = 1000; + break; + case currentBranchRemote: + weight = 6; + break; + case 'upstream': + weight = 5; + break; + case 'origin': + weight = 4; + break; + default: + weight = 0; + } - const p = r.provider; - if (p.hasRichIntegration() && p.maybeConnected) { - const m = await p.getRepositoryMetadata(); - if (m?.isFork === false) { - matchedWeight += 101; + // Only check remotes that have extra weighting and less than the default + if (weight > 0 && weight < 1000 && !originalFound) { + const p = remote.provider; + if ( + p.hasRichIntegration() && + (p.maybeConnected || + (p.maybeConnected === undefined && p.shouldConnect && (await p.isConnected()))) + ) { + const repo = await p.getRepositoryMetadata(); + if (repo != null) { + weight += repo.isFork ? -3 : 3; + // Once we've found the "original" (not a fork) don't bother looking for more + originalFound = !repo.isFork; + } + } } - } - if (matchedWeight > weight) { - bestRemote = r; - weight = matchedWeight; + weighted.push([weight, remote]); } + + // Sort by the weight, but if both are 0 (no weight) then sort by name + weighted.sort(([aw, ar], [bw, br]) => (bw === 0 && aw === 0 ? sortCompare(ar.name, br.name) : bw - aw)); + return weighted.map(wr => wr[1]); } - remote = bestRemote ?? null; + remotes = getBest.call(this); + this._bestRemotesCache.set(cacheKey, remotes); } - this._bestRemotesCache.set(cacheKey, remote); - - return remote ?? undefined; + return [...(await remotes)]; } + @log() 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, + repoPath: string | Uri, 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._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._bestRemotesCache.set(`rich|${cacheKey}`, remote); - this._bestRemotesCache.set(`rich+connected|${cacheKey}`, remote); - } else { - this._bestRemotesCache.set(`rich|${cacheKey}`, remote); - this._bestRemotesCache.set(`rich+connected|${cacheKey}`, null); + const remotes = await this.getBestRemotesWithProviders(repoPath); - if (!options?.includeDisconnected) return undefined; + const includeDisconnected = options?.includeDisconnected ?? false; + for (const r of remotes) { + if (r.hasRichProvider() && (includeDisconnected || r.provider.maybeConnected === true)) { + return r; + } } - return remote; + return undefined; } - @log({ args: { 1: false } }) - async getRemotes(repoPath: string | Uri | undefined, options?: { sort?: boolean }): Promise { + @log() + async getRemotes(repoPath: string | Uri, options?: { sort?: boolean }): Promise { if (repoPath == null) return []; const { provider, path } = this.getProvider(repoPath); @@ -2304,16 +2225,10 @@ export class GitProviderService implements Disposable { @log() async getRemotesWithProviders( - repoPath: string | Uri | undefined, + repoPath: string | Uri, options?: { sort?: boolean }, ): Promise[]> { - if (repoPath == null) return []; - - const repository = this.container.git.getRepository(repoPath); - const remotes = await (repository != null - ? repository.getRemotes(options) - : this.getRemotes(repoPath, options)); - + const remotes = await this.getRemotes(repoPath, options); return remotes.filter( (r: GitRemote): r is GitRemote => r.provider != null, ); diff --git a/src/git/models/repository.ts b/src/git/models/repository.ts index 971adca..aecc2a9 100644 --- a/src/git/models/repository.ts +++ b/src/git/models/repository.ts @@ -9,7 +9,7 @@ import type { Container } from '../../container'; import type { FeatureAccess, Features, PlusFeatures } from '../../features'; import { showCreatePullRequestPrompt, showGenericErrorMessage } from '../../messages'; import { asRepoComparisonKey } from '../../repositories'; -import { filterMap, groupByMap } from '../../system/array'; +import { groupByMap } from '../../system/array'; import { executeActionCommand, executeCoreGitCommand } from '../../system/command'; import { configuration } from '../../system/configuration'; import { formatDate, fromNow } from '../../system/date'; @@ -214,8 +214,6 @@ export class Repository implements Disposable { private _fsWatcherDisposable: Disposable | undefined; private _pendingFileSystemChange?: RepositoryFileSystemChangeEvent; private _pendingRepoChange?: RepositoryChangeEvent; - private _remotes: Promise | undefined; - private _remotesDisposable: Disposable | undefined; private _suspended: boolean; constructor( @@ -256,6 +254,19 @@ export class Repository implements Disposable { this._disposable = Disposable.from( this.setupRepoWatchers(), configuration.onDidChange(this.onConfigurationChanged, this), + // Sending this event in the `'git:cache:reset'` below to avoid unnecessary work. While we will refresh more than needed, this doesn't happen often + // container.richRemoteProviders.onAfterDidChangeConnectionState(async e => { + // const uniqueKeys = new Set(); + // for (const remote of await this.getRemotes()) { + // if (remote.provider?.hasRichIntegration()) { + // uniqueKeys.add(remote.provider.key); + // } + // } + + // if (uniqueKeys.has(e.key)) { + // this.fireChange(RepositoryChange.RemoteProviders); + // } + // }), ); this.onConfigurationChanged(); @@ -281,6 +292,10 @@ export class Repository implements Disposable { this.container.events.on('git:cache:reset', e => { if (!e.data.repoPath || e.data.repoPath === this.path) { this.resetCaches(...(e.data.caches ?? emptyArray)); + + if (e.data.caches?.includes('providers')) { + this.fireChange(RepositoryChange.RemoteProviders); + } } }), ); @@ -323,7 +338,6 @@ export class Repository implements Disposable { dispose() { this.stopWatchingFileSystem(); - this._remotesDisposable?.dispose(); this._disposable.dispose(); } @@ -685,32 +699,15 @@ export class Repository implements Disposable { } async getRemotes(options?: { filter?: (remote: GitRemote) => boolean; sort?: boolean }): Promise { - if (this._remotes == null) { - // Since we are caching the results, always sort - this._remotes = this.container.git.getRemotes(this.uri, { sort: true }); - void this.subscribeToRemotes(this._remotes); - } - - return options?.filter != null ? (await this._remotes).filter(options.filter) : this._remotes; + const remotes = await this.container.git.getRemotes( + this.uri, + options?.sort != null ? { sort: options.sort } : undefined, + ); + return options?.filter != null ? remotes.filter(options.filter) : remotes; } async getRichRemote(connectedOnly: boolean = false): Promise | undefined> { - return this.container.git.getBestRemoteWithRichProvider(await this.getRemotes(), { - includeDisconnected: !connectedOnly, - }); - } - - private async subscribeToRemotes(remotes: Promise) { - this._remotesDisposable?.dispose(); - this._remotesDisposable = undefined; - - this._remotesDisposable = Disposable.from( - ...filterMap(await remotes, r => { - if (!r.provider?.hasRichIntegration()) return undefined; - - return r.provider.onDidChange(() => this.fireChange(RepositoryChange.RemoteProviders)); - }), - ); + return this.container.git.getBestRemoteWithRichProvider(this.uri, { includeDisconnected: !connectedOnly }); } getStash(): Promise { @@ -955,11 +952,11 @@ export class Repository implements Disposable { this._branch = undefined; } - if (caches.length === 0 || caches.includes('remotes')) { - this._remotes = undefined; - this._remotesDisposable?.dispose(); - this._remotesDisposable = undefined; - } + // if (caches.length === 0 || caches.includes('remotes')) { + // this._remotes = undefined; + // this._remotesDisposable?.dispose(); + // this._remotesDisposable = undefined; + // } } resume() { diff --git a/src/git/parsers/remoteParser.ts b/src/git/parsers/remoteParser.ts index a37faa4..7f0059d 100644 --- a/src/git/parsers/remoteParser.ts +++ b/src/git/parsers/remoteParser.ts @@ -55,6 +55,10 @@ export class GitRemoteParser { remote.urls.push({ url: url, type: type as GitRemoteType }); if (remote.provider != null && type !== 'push') continue; + if (remote.provider?.hasRichIntegration()) { + remote.provider.dispose(); + } + const provider = remoteProviderMatcher(url, domain, path); if (provider == null) continue; diff --git a/src/git/remotes/remoteProviderService.ts b/src/git/remotes/remoteProviderService.ts index 272a86d..a92d036 100644 --- a/src/git/remotes/remoteProviderService.ts +++ b/src/git/remotes/remoteProviderService.ts @@ -13,6 +13,11 @@ export class RichRemoteProviderService { return this._onDidChangeConnectionState.event; } + private readonly _onAfterDidChangeConnectionState = new EventEmitter(); + get onAfterDidChangeConnectionState(): Event { + return this._onAfterDidChangeConnectionState.event; + } + private readonly _connectedCache = new Set(); constructor(private readonly container: Container) {} @@ -25,6 +30,7 @@ export class RichRemoteProviderService { this.container.telemetry.sendEvent('remoteProviders/connected', { 'remoteProviders.key': key }); this._onDidChangeConnectionState.fire({ key: key, reason: 'connected' }); + setTimeout(() => this._onAfterDidChangeConnectionState.fire({ key: key, reason: 'connected' }), 250); } disconnected(key: string): void { @@ -34,5 +40,10 @@ export class RichRemoteProviderService { this.container.telemetry.sendEvent('remoteProviders/disconnected', { 'remoteProviders.key': key }); this._onDidChangeConnectionState.fire({ key: key, reason: 'disconnected' }); + setTimeout(() => this._onAfterDidChangeConnectionState.fire({ key: key, reason: 'disconnected' }), 250); + } + + isConnected(key?: string): boolean { + return key == null ? this._connectedCache.size !== 0 : this._connectedCache.has(key); } } diff --git a/src/git/remotes/remoteProviders.ts b/src/git/remotes/remoteProviders.ts index 1eb0500..794c82b 100644 --- a/src/git/remotes/remoteProviders.ts +++ b/src/git/remotes/remoteProviders.ts @@ -171,6 +171,7 @@ function createBestRemoteProvider( return undefined; } catch (ex) { + debugger; Logger.error(ex, 'createRemoteProvider'); return undefined; } diff --git a/src/git/remotes/richRemoteProvider.ts b/src/git/remotes/richRemoteProvider.ts index 488e05c..5f034d2 100644 --- a/src/git/remotes/richRemoteProvider.ts +++ b/src/git/remotes/richRemoteProvider.ts @@ -1,5 +1,5 @@ import type { AuthenticationSession, AuthenticationSessionsChangeEvent, Event, MessageItem } from 'vscode'; -import { authentication, EventEmitter, window } from 'vscode'; +import { authentication, Disposable, EventEmitter, window } from 'vscode'; import { wrapForForcedInsecureSSL } from '@env/fetch'; import { isWeb } from '@env/platform'; import type { Container } from '../../container'; @@ -22,7 +22,7 @@ import { RemoteProvider } from './remoteProvider'; // TODO@eamodio revisit how once authenticated, all remotes are always connected, even after a restart -export abstract class RichRemoteProvider extends RemoteProvider { +export abstract class RichRemoteProvider extends RemoteProvider implements Disposable { override readonly type: 'simple' | 'rich' = 'rich'; private readonly _onDidChange = new EventEmitter(); @@ -30,6 +30,8 @@ export abstract class RichRemoteProvider extends RemoteProvider { return this._onDidChange.event; } + private readonly _disposable: Disposable; + constructor( protected readonly container: Container, domain: string, @@ -40,7 +42,7 @@ export abstract class RichRemoteProvider extends RemoteProvider { ) { super(domain, path, protocol, name, custom); - container.context.subscriptions.push( + this._disposable = Disposable.from( configuration.onDidChange(e => { if (configuration.changed(e, 'remotes')) { this._ignoreSSLErrors.clear(); @@ -58,6 +60,20 @@ export abstract class RichRemoteProvider extends RemoteProvider { }), authentication.onDidChangeSessions(this.onAuthenticationSessionsChanged, this), ); + + container.context.subscriptions.push(this._disposable); + + // If we think we should be connected, try to + if (this.shouldConnect) { + void this.isConnected(); + } + } + + disposed = false; + + dispose() { + this._disposable.dispose(); + this.disposed = true; } abstract get apiBaseUrl(): string; @@ -78,6 +94,11 @@ export abstract class RichRemoteProvider extends RemoteProvider { return this._session === undefined ? undefined : this._session !== null; } + // This is a hack for now, since providers come and go with remotes + get shouldConnect(): boolean { + return this.container.richRemoteProviders.isConnected(this.key); + } + protected _session: AuthenticationSession | null | undefined; protected session() { if (this._session === undefined) { diff --git a/src/hovers/hovers.ts b/src/hovers/hovers.ts index 7380aff..e668cdf 100644 --- a/src/hovers/hovers.ts +++ b/src/hovers/hovers.ts @@ -213,33 +213,35 @@ export async function detailsMessage( if (options?.cancellationToken?.isCancellationRequested) return new MarkdownString(); } - - const remotes = await Container.instance.git.getRemotesWithProviders(commit.repoPath, { sort: true }); - - if (options?.cancellationToken?.isCancellationRequested) return new MarkdownString(); - - const [previousLineComparisonUrisResult, autolinkedIssuesOrPullRequestsResult, prResult, presenceResult] = - await Promise.allSettled([ - commit.isUncommitted ? commit.getPreviousComparisonUrisForLine(editorLine, uri.sha) : undefined, - getAutoLinkedIssuesOrPullRequests(message, remotes), - options?.pullRequests?.pr ?? - getPullRequestForCommitOrBestRemote(commit.ref, remotes, { - pullRequests: - options?.pullRequests?.enabled !== false && - CommitFormatter.has( - options.format, - 'pullRequest', - 'pullRequestAgo', - 'pullRequestAgoOrDate', - 'pullRequestDate', - 'pullRequestState', - ), - }), - Container.instance.vsls.maybeGetPresence(commit.author.email), - ]); + const [ + remotesResult, + previousLineComparisonUrisResult, + autolinkedIssuesOrPullRequestsResult, + prResult, + presenceResult, + ] = await Promise.allSettled([ + Container.instance.git.getRemotesWithProviders(commit.repoPath, { sort: true }), + commit.isUncommitted ? commit.getPreviousComparisonUrisForLine(editorLine, uri.sha) : undefined, + getAutoLinkedIssuesOrPullRequests(message, commit.repoPath), + options?.pullRequests?.pr ?? + getPullRequestForCommitOrBestRemote(commit.ref, commit.repoPath, { + pullRequests: + options?.pullRequests?.enabled !== false && + CommitFormatter.has( + options.format, + 'pullRequest', + 'pullRequestAgo', + 'pullRequestAgoOrDate', + 'pullRequestDate', + 'pullRequestState', + ), + }), + Container.instance.vsls.maybeGetPresence(commit.author.email), + ]); if (options?.cancellationToken?.isCancellationRequested) return new MarkdownString(); + const remotes = getSettledValue(remotesResult); const previousLineComparisonUris = getSettledValue(previousLineComparisonUrisResult); const autolinkedIssuesOrPullRequests = getSettledValue(autolinkedIssuesOrPullRequestsResult); const pr = getSettledValue(prResult); @@ -286,7 +288,7 @@ function getDiffFromHunkLine(hunkLine: GitDiffHunkLine, diffStyle?: 'line' | 'hu }\n\`\`\``; } -async function getAutoLinkedIssuesOrPullRequests(message: string, remotes: GitRemote[]) { +async function getAutoLinkedIssuesOrPullRequests(message: string, repoPath: string) { const scope = getNewLogScope('Hovers.getAutoLinkedIssuesOrPullRequests'); Logger.debug(scope, `${GlyphChars.Dash} message=`); @@ -303,7 +305,7 @@ async function getAutoLinkedIssuesOrPullRequests(message: string, remotes: GitRe return undefined; } - const remote = await Container.instance.git.getBestRemoteWithRichProvider(remotes); + const remote = await Container.instance.git.getBestRemoteWithRichProvider(repoPath); if (remote?.provider == null) { Logger.debug(scope, `completed [${getDurationMilliseconds(start)}ms]`); @@ -362,7 +364,7 @@ async function getAutoLinkedIssuesOrPullRequests(message: string, remotes: GitRe async function getPullRequestForCommitOrBestRemote( ref: string, - remotes: GitRemote[], + repoPath: string, options?: { pullRequests?: boolean; }, @@ -378,9 +380,7 @@ async function getPullRequestForCommitOrBestRemote( return undefined; } - const remote = await Container.instance.git.getBestRemoteWithRichProvider(remotes, { - includeDisconnected: true, - }); + const remote = await Container.instance.git.getBestRemoteWithRichProvider(repoPath, { includeDisconnected: true }); if (remote?.provider == null) { Logger.debug(scope, `completed [${getDurationMilliseconds(start)}ms]`); diff --git a/src/views/nodes/commitNode.ts b/src/views/nodes/commitNode.ts index bf4ab76..f0948f7 100644 --- a/src/views/nodes/commitNode.ts +++ b/src/views/nodes/commitNode.ts @@ -236,13 +236,8 @@ export class CommitNode extends ViewRefNode (a === remote ? -1 : b === remote ? 1 : 0)); - } + const remotes = await this.view.container.git.getBestRemotesWithProviders(this.commit.repoPath); + const [remote] = remotes; if (this.commit.message == null) { await this.commit.ensureFullDetails(); @@ -251,7 +246,7 @@ export class CommitNode extends ViewRefNode