From 179b1bf4896370b5f1d658f86faa7ac83fd47512 Mon Sep 17 00:00:00 2001 From: Eric Amodio Date: Fri, 27 Oct 2023 01:31:26 -0400 Subject: [PATCH] Fixes #2625 adds # escaping to regex Ensures correct repository on GitHub/GitLab enriched autolinks --- CHANGELOG.md | 2 + src/annotations/autolinks.ts | 6 +- src/cache.ts | 13 ++- src/git/remotes/github.ts | 88 ++++++++++++++++++--- src/git/remotes/gitlab.ts | 145 +++++++++++++++++++++++++++++----- src/git/remotes/richRemoteProvider.ts | 14 +++- 6 files changed, 227 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 630a9a6..14cec85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p ### Fixed +- Fixes [#2625](https://github.com/gitkraken/vscode-gitlens/issues/2625) - full issue ref has escape characters that break hover links - Fixes [#2987](https://github.com/gitkraken/vscode-gitlens/issues/2987) - Unable to remove all marks on reviewed files with a single operation - Fixes [#2923](https://github.com/gitkraken/vscode-gitlens/issues/2923) - TypeError: Only absolute URLs are supported - Fixes [#2926](https://github.com/gitkraken/vscode-gitlens/issues/2926) - "Open File at Revision" has incorrect editor label if revision contains path separator @@ -44,6 +45,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p - Fixes intermittent issues where details sometimes get cleared/overwritten when opening the _Commit Details_ view - Fixes issue when clicking on commits in the Visual File History to open the _Commit Details_ view - Fixes issue opening stashes in the _Commit Details_ view from the _Stashes_ view +- Fixes issue where GitHub/GitLab enriched autolinks could incorrectly point to the wrong repository ## [14.4.0] - 2023-10-13 diff --git a/src/annotations/autolinks.ts b/src/annotations/autolinks.ts index 6e7b440..e4e44ee 100644 --- a/src/annotations/autolinks.ts +++ b/src/annotations/autolinks.ts @@ -7,7 +7,7 @@ import type { IssueOrPullRequest } from '../git/models/issue'; import { getIssueOrPullRequestHtmlIcon, getIssueOrPullRequestMarkdownIcon } from '../git/models/issue'; import type { GitRemote } from '../git/models/remote'; import type { RemoteProviderReference } from '../git/models/remoteProvider'; -import type { RichRemoteProvider } from '../git/remotes/richRemoteProvider'; +import type { RepositoryDescriptor, RichRemoteProvider } from '../git/remotes/richRemoteProvider'; import type { MaybePausedResult } from '../system/cancellation'; import { configuration } from '../system/configuration'; import { fromNow } from '../system/date'; @@ -30,6 +30,8 @@ export interface Autolink { type?: AutolinkType; description?: string; + + descriptor?: RepositoryDescriptor; } export type EnrichedAutolink = [ @@ -240,7 +242,7 @@ export class Autolinks implements Disposable { provider != null && link.provider?.id === provider.id && link.provider?.domain === provider.domain - ? provider.getIssueOrPullRequest(id) + ? provider.getIssueOrPullRequest(id, link.descriptor) : undefined, link, ] satisfies EnrichedAutolink, diff --git a/src/cache.ts b/src/cache.ts index 7e77eb7..b797612 100644 --- a/src/cache.ts +++ b/src/cache.ts @@ -7,13 +7,14 @@ import type { PullRequest } from './git/models/pullRequest'; import type { GitRemote } from './git/models/remote'; import type { RepositoryMetadata } from './git/models/repositoryMetadata'; import type { RemoteProvider } from './git/remotes/remoteProvider'; -import type { RichRemoteProvider } from './git/remotes/richRemoteProvider'; +import type { RepositoryDescriptor, RichRemoteProvider } from './git/remotes/richRemoteProvider'; import { isPromise } from './system/promise'; type Caches = { defaultBranch: { key: `repo:${string}`; value: DefaultBranch }; // enrichedAutolinksBySha: { key: `sha:${string}:${string}`; value: Map }; issuesOrPrsById: { key: `id:${string}:${string}`; value: IssueOrPullRequest }; + issuesOrPrsByIdAndRepo: { key: `id:${string}:${string}:${string}`; value: IssueOrPullRequest }; prByBranch: { key: `branch:${string}:${string}`; value: PullRequest }; prsBySha: { key: `sha:${string}:${string}`; value: PullRequest }; repoMetadata: { key: `repo:${string}`; value: RepositoryMetadata }; @@ -73,11 +74,16 @@ export class CacheProvider implements Disposable { getIssueOrPullRequest( id: string, + repo: RepositoryDescriptor | undefined, remoteOrProvider: RichRemoteProvider | GitRemote, cacheable: Cacheable, ): CacheResult { const { key, etag } = getRemoteKeyAndEtag(remoteOrProvider); - return this.get('issuesOrPrsById', `id:${id}:${key}`, etag, cacheable); + + if (repo == null) { + return this.get('issuesOrPrsById', `id:${id}:${key}`, etag, cacheable); + } + return this.get('issuesOrPrsByIdAndRepo', `id:${id}:${key}:${JSON.stringify(repo)}}`, etag, cacheable); } // getEnrichedAutolinks( @@ -182,7 +188,8 @@ function getExpiresAt(cache: T, value: CacheValue | undefine case 'defaultBranch': case 'repoMetadata': return 0; // Never expires - case 'issuesOrPrsById': { + case 'issuesOrPrsById': + case 'issuesOrPrsByIdAndRepo': { if (value == null) return 0; // Never expires // Open issues expire after 1 hour, but closed issues expire after 12 hours unless recently updated and then expire in 1 hour diff --git a/src/git/remotes/github.ts b/src/git/remotes/github.ts index e70badc..ff96164 100644 --- a/src/git/remotes/github.ts +++ b/src/git/remotes/github.ts @@ -1,27 +1,30 @@ import type { AuthenticationSession, Disposable, QuickInputButton, Range } from 'vscode'; import { env, ThemeIcon, Uri, window } from 'vscode'; -import type { Autolink, DynamicAutolinkReference } from '../../annotations/autolinks'; +import type { Autolink, DynamicAutolinkReference, MaybeEnrichedAutolink } from '../../annotations/autolinks'; import type { AutolinkReference } from '../../config'; +import { GlyphChars } from '../../constants'; import type { Container } from '../../container'; import type { IntegrationAuthenticationProvider, IntegrationAuthenticationSessionDescriptor, } from '../../plus/integrationAuthentication'; +import { fromNow } from '../../system/date'; import { log } from '../../system/decorators/log'; import { memoize } from '../../system/decorators/memoize'; import { encodeUrl } from '../../system/encoding'; -import { equalsIgnoreCase } from '../../system/string'; +import { equalsIgnoreCase, escapeMarkdown } from '../../system/string'; import { supportedInVSCodeVersion } from '../../system/utils'; import type { Account } from '../models/author'; import type { DefaultBranch } from '../models/defaultBranch'; import type { IssueOrPullRequest, SearchedIssue } from '../models/issue'; +import { getIssueOrPullRequestMarkdownIcon } from '../models/issue'; import type { PullRequest, PullRequestState, SearchedPullRequest } from '../models/pullRequest'; import { isSha } from '../models/reference'; import type { Repository } from '../models/repository'; import type { RepositoryMetadata } from '../models/repositoryMetadata'; import { ensurePaidPlan, RichRemoteProvider } from './richRemoteProvider'; -const autolinkFullIssuesRegex = /\b(?[^/\s]+\/[^/\s]+)#(?[0-9]+)\b(?!]\()/g; +const autolinkFullIssuesRegex = /\b([^/\s]+\/[^/\s]+?)(?:\\)?#([0-9]+)\b(?!]\()/g; const fileRegex = /^\/([^/]+)\/([^/]+?)\/blob(.+)$/i; const rangeRegex = /^L(\d+)(?:-L(\d+))?$/; @@ -32,7 +35,14 @@ function isGitHubDotCom(domain: string): boolean { return equalsIgnoreCase(domain, 'github.com'); } -export class GitHubRemote extends RichRemoteProvider { +type GitHubRepositoryDescriptor = + | { + owner: string; + name: string; + } + | Record; + +export class GitHubRemote extends RichRemoteProvider { @memoize() protected get authProvider() { return isGitHubDotCom(this.domain) ? authProvider : enterpriseAuthProvider; @@ -77,6 +87,9 @@ export class GitHubRemote extends RichRemoteProvider { text: string, outputFormat: 'html' | 'markdown' | 'plaintext', tokenMapping: Map, + enrichedAutolinks?: Map, + prs?: Set, + footnotes?: Map, ) => { return outputFormat === 'plaintext' ? text @@ -91,28 +104,72 @@ export class GitHubRemote extends RichRemoteProvider { tokenMapping.set(token, `${linkText}`); } + let footnoteIndex: number; + + const issueResult = enrichedAutolinks?.get(num)?.[0]; + if (issueResult?.value != null) { + if (issueResult.paused) { + if (footnotes != null && !prs?.has(num)) { + footnoteIndex = footnotes.size + 1; + footnotes.set( + footnoteIndex, + `[${getIssueOrPullRequestMarkdownIcon()} ${ + this.name + } Issue or Pull Request ${repo}#${num} $(loading~spin)](${url}${title}")`, + ); + } + } else { + const issue = issueResult.value; + const issueTitle = escapeMarkdown(issue.title.trim()); + if (footnotes != null && !prs?.has(num)) { + footnoteIndex = footnotes.size + 1; + footnotes.set( + footnoteIndex, + `[${getIssueOrPullRequestMarkdownIcon( + issue, + )} **${issueTitle}**](${url}${title})\\\n${GlyphChars.Space.repeat( + 5, + )}${linkText} ${issue.state} ${fromNow( + issue.closedDate ?? issue.date, + )}`, + ); + } + } + } else if (footnotes != null && !prs?.has(num)) { + footnoteIndex = footnotes.size + 1; + footnotes.set( + footnoteIndex, + `[${getIssueOrPullRequestMarkdownIcon()} ${ + this.name + } Issue or Pull Request ${repo}#${num}](${url}${title})`, + ); + } + return token; }); }, parse: (text: string, autolinks: Map) => { - let repo: string; + let ownerAndRepo: string; let num: string; let match; do { match = autolinkFullIssuesRegex.exec(text); - if (match?.groups == null) break; + if (match == null) break; - ({ repo, num } = match.groups); + [, ownerAndRepo, num] = match; + const [owner, repo] = ownerAndRepo.split('/', 2); autolinks.set(num, { provider: this, id: num, - prefix: `${repo}#`, - url: `${this.protocol}://${this.domain}/${repo}/issues/${num}`, - title: `Open Issue or Pull Request # from ${repo} on ${this.name}`, + prefix: `${ownerAndRepo}#`, + url: `${this.protocol}://${this.domain}/${ownerAndRepo}/issues/${num}`, + title: `Open Issue or Pull Request # from ${ownerAndRepo} on ${this.name}`, + + description: `${this.name} Issue or Pull Request ${ownerAndRepo}#${num}`, - description: `${this.name} Issue or Pull Request ${repo}#${num}`, + descriptor: { owner: owner, name: repo } satisfies GitHubRepositoryDescriptor, }); } while (true); }, @@ -300,8 +357,15 @@ export class GitHubRemote extends RichRemoteProvider { protected override async getProviderIssueOrPullRequest( { accessToken }: AuthenticationSession, id: string, + descriptor: GitHubRepositoryDescriptor | undefined, ): Promise { - const [owner, repo] = this.splitPath(); + let owner; + let repo; + if (descriptor != null) { + ({ owner, name: repo } = descriptor); + } else { + [owner, repo] = this.splitPath(); + } return (await this.container.github)?.getIssueOrPullRequest(this, accessToken, owner, repo, Number(id), { baseUrl: this.apiBaseUrl, }); diff --git a/src/git/remotes/gitlab.ts b/src/git/remotes/gitlab.ts index 0a31e9c..d05ccaa 100644 --- a/src/git/remotes/gitlab.ts +++ b/src/git/remotes/gitlab.ts @@ -1,33 +1,43 @@ import type { AuthenticationSession, Disposable, QuickInputButton, Range } from 'vscode'; import { env, ThemeIcon, Uri, window } from 'vscode'; -import type { Autolink, DynamicAutolinkReference } from '../../annotations/autolinks'; +import type { Autolink, DynamicAutolinkReference, MaybeEnrichedAutolink } from '../../annotations/autolinks'; import type { AutolinkReference } from '../../config'; +import { GlyphChars } from '../../constants'; import type { Container } from '../../container'; import type { IntegrationAuthenticationProvider, IntegrationAuthenticationSessionDescriptor, } from '../../plus/integrationAuthentication'; +import { fromNow } from '../../system/date'; import { log } from '../../system/decorators/log'; import { encodeUrl } from '../../system/encoding'; -import { equalsIgnoreCase } from '../../system/string'; +import { equalsIgnoreCase, escapeMarkdown } from '../../system/string'; import { supportedInVSCodeVersion } from '../../system/utils'; import type { Account } from '../models/author'; import type { DefaultBranch } from '../models/defaultBranch'; import type { IssueOrPullRequest, SearchedIssue } from '../models/issue'; +import { getIssueOrPullRequestMarkdownIcon } from '../models/issue'; import type { PullRequest, PullRequestState, SearchedPullRequest } from '../models/pullRequest'; import { isSha } from '../models/reference'; import type { Repository } from '../models/repository'; import type { RepositoryMetadata } from '../models/repositoryMetadata'; import { ensurePaidPlan, RichRemoteProvider } from './richRemoteProvider'; -const autolinkFullIssuesRegex = /\b(?[^/\s]+\/[^/\s]+)#(?[0-9]+)\b(?!]\()/g; -const autolinkFullMergeRequestsRegex = /\b(?[^/\s]+\/[^/\s]+)!(?[0-9]+)\b(?!]\()/g; +const autolinkFullIssuesRegex = /\b([^/\s]+\/[^/\s]+?)(?:\\)?#([0-9]+)\b(?!]\()/g; +const autolinkFullMergeRequestsRegex = /\b([^/\s]+\/[^/\s]+?)(?:\\)?!([0-9]+)\b(?!]\()/g; const fileRegex = /^\/([^/]+)\/([^/]+?)\/-\/blob(.+)$/i; const rangeRegex = /^L(\d+)(?:-(\d+))?$/; const authProvider = Object.freeze({ id: 'gitlab', scopes: ['read_api', 'read_user', 'read_repository'] }); -export class GitLabRemote extends RichRemoteProvider { +type GitLabRepositoryDescriptor = + | { + owner: string; + name: string; + } + | Record; + +export class GitLabRemote extends RichRemoteProvider { protected get authProvider() { return authProvider; } @@ -72,6 +82,9 @@ export class GitLabRemote extends RichRemoteProvider { text: string, outputFormat: 'html' | 'markdown' | 'plaintext', tokenMapping: Map, + enrichedAutolinks?: Map, + prs?: Set, + footnotes?: Map, ) => { return outputFormat === 'plaintext' ? text @@ -86,29 +99,68 @@ export class GitLabRemote extends RichRemoteProvider { tokenMapping.set(token, `${linkText}`); } + let footnoteIndex: number; + + const issueResult = enrichedAutolinks?.get(num)?.[0]; + if (issueResult?.value != null) { + if (issueResult.paused) { + if (footnotes != null && !prs?.has(num)) { + footnoteIndex = footnotes.size + 1; + footnotes.set( + footnoteIndex, + `[${getIssueOrPullRequestMarkdownIcon()} GitLab Issue ${repo}#${num} $(loading~spin)](${url}${title}")`, + ); + } + } else { + const issue = issueResult.value; + const issueTitle = escapeMarkdown(issue.title.trim()); + if (footnotes != null && !prs?.has(num)) { + footnoteIndex = footnotes.size + 1; + footnotes.set( + footnoteIndex, + `[${getIssueOrPullRequestMarkdownIcon( + issue, + )} **${issueTitle}**](${url}${title})\\\n${GlyphChars.Space.repeat( + 5, + )}${linkText} ${issue.state} ${fromNow( + issue.closedDate ?? issue.date, + )}`, + ); + } + } + } else if (footnotes != null && !prs?.has(num)) { + footnoteIndex = footnotes.size + 1; + footnotes.set( + footnoteIndex, + `[${getIssueOrPullRequestMarkdownIcon()} GitLab Issue ${repo}#${num}](${url}${title})`, + ); + } + return token; }); }, parse: (text: string, autolinks: Map) => { - let repo: string; + let ownerAndRepo: string; let num: string; let match; do { match = autolinkFullIssuesRegex.exec(text); - if (match?.groups == null) break; + if (match == null) break; - ({ repo, num } = match.groups); + [, ownerAndRepo, num] = match; + const [owner, repo] = ownerAndRepo.split('/', 2); autolinks.set(num, { provider: this, id: num, - prefix: `${repo}#`, - url: `${this.protocol}://${this.domain}/${repo}/-/issues/${num}`, - title: `Open Issue # from ${repo} on ${this.name}`, + prefix: `${ownerAndRepo}#`, + url: `${this.protocol}://${this.domain}/${ownerAndRepo}/-/issues/${num}`, + title: `Open Issue # from ${ownerAndRepo} on ${this.name}`, type: 'issue', - description: `${this.name} Issue ${repo}#${num}`, + description: `${this.name} Issue ${ownerAndRepo}#${num}`, + descriptor: { owner: owner, name: repo } satisfies GitLabRepositoryDescriptor, }); } while (true); }, @@ -118,6 +170,9 @@ export class GitLabRemote extends RichRemoteProvider { text: string, outputFormat: 'html' | 'markdown' | 'plaintext', tokenMapping: Map, + enrichedAutolinks?: Map, + prs?: Set, + footnotes?: Map, ) => { return outputFormat === 'plaintext' ? text @@ -136,30 +191,73 @@ export class GitLabRemote extends RichRemoteProvider { tokenMapping.set(token, `${linkText}`); } + let footnoteIndex: number; + + const issueResult = enrichedAutolinks?.get(num)?.[0]; + if (issueResult?.value != null) { + if (issueResult.paused) { + if (footnotes != null && !prs?.has(num)) { + footnoteIndex = footnotes.size + 1; + footnotes.set( + footnoteIndex, + `[${getIssueOrPullRequestMarkdownIcon()} ${ + this.name + } Merge Request ${repo}!${num} $(loading~spin)](${url}${title}")`, + ); + } + } else { + const issue = issueResult.value; + const issueTitle = escapeMarkdown(issue.title.trim()); + if (footnotes != null && !prs?.has(num)) { + footnoteIndex = footnotes.size + 1; + footnotes.set( + footnoteIndex, + `[${getIssueOrPullRequestMarkdownIcon( + issue, + )} **${issueTitle}**](${url}${title})\\\n${GlyphChars.Space.repeat( + 5, + )}${linkText} ${issue.state} ${fromNow( + issue.closedDate ?? issue.date, + )}`, + ); + } + } + } else if (footnotes != null && !prs?.has(num)) { + footnoteIndex = footnotes.size + 1; + footnotes.set( + footnoteIndex, + `[${getIssueOrPullRequestMarkdownIcon()} ${ + this.name + } Merge Request ${repo}!${num}](${url}${title})`, + ); + } return token; }, ); }, parse: (text: string, autolinks: Map) => { - let repo: string; + let ownerAndRepo: string; let num: string; let match; do { match = autolinkFullMergeRequestsRegex.exec(text); - if (match?.groups == null) break; + if (match == null) break; - ({ repo, num } = match.groups); + [, ownerAndRepo, num] = match; + const [owner, repo] = ownerAndRepo.split('/', 2); autolinks.set(num, { provider: this, id: num, - prefix: `${repo}!`, - url: `${this.protocol}://${this.domain}/${repo}/-/merge_requests/${num}`, - title: `Open Merge Request ! from ${repo} on ${this.name}`, + prefix: `${ownerAndRepo}!`, + url: `${this.protocol}://${this.domain}/${ownerAndRepo}/-/merge_requests/${num}`, + title: `Open Merge Request ! from ${ownerAndRepo} on ${this.name}`, type: 'pullrequest', - description: `Merge Request !${num} from ${repo} on ${this.name}`, + description: `${this.name} Merge Request !${num} from ${ownerAndRepo}`, + + descriptor: { owner: owner, name: repo } satisfies GitLabRepositoryDescriptor, }); } while (true); }, @@ -330,8 +428,15 @@ export class GitLabRemote extends RichRemoteProvider { protected override async getProviderIssueOrPullRequest( { accessToken }: AuthenticationSession, id: string, + descriptor: GitLabRepositoryDescriptor | undefined, ): Promise { - const [owner, repo] = this.splitPath(); + let owner; + let repo; + if (descriptor != null) { + ({ owner, name: repo } = descriptor); + } else { + [owner, repo] = this.splitPath(); + } return (await this.container.gitlab)?.getIssueOrPullRequest(this, accessToken, owner, repo, Number(id), { baseUrl: this.apiBaseUrl, }); diff --git a/src/git/remotes/richRemoteProvider.ts b/src/git/remotes/richRemoteProvider.ts index ed3464d..c3f827f 100644 --- a/src/git/remotes/richRemoteProvider.ts +++ b/src/git/remotes/richRemoteProvider.ts @@ -29,7 +29,12 @@ 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 implements Disposable { +export type RepositoryDescriptor = Record; + +export abstract class RichRemoteProvider + extends RemoteProvider + implements Disposable +{ override readonly type: 'simple' | 'rich' = 'rich'; private readonly _onDidChange = new EventEmitter(); @@ -356,16 +361,16 @@ export abstract class RichRemoteProvider extends RemoteProvider implements Dispo }: AuthenticationSession): Promise; @debug() - async getIssueOrPullRequest(id: string): Promise { + async getIssueOrPullRequest(id: string, repo: T | undefined): Promise { const scope = getLogScope(); const connected = this.maybeConnected ?? (await this.isConnected()); if (!connected) return undefined; - const issueOrPR = this.container.cache.getIssueOrPullRequest(id, this, () => ({ + const issueOrPR = this.container.cache.getIssueOrPullRequest(id, repo, this, () => ({ value: (async () => { try { - const result = await this.getProviderIssueOrPullRequest(this._session!, id); + const result = await this.getProviderIssueOrPullRequest(this._session!, id, repo); this.resetRequestExceptionCount(); return result; } catch (ex) { @@ -379,6 +384,7 @@ export abstract class RichRemoteProvider extends RemoteProvider implements Dispo protected abstract getProviderIssueOrPullRequest( session: AuthenticationSession, id: string, + repo: T | undefined, ): Promise; @debug()