diff --git a/src/autolinks/__tests__/autolinks.test.ts b/src/autolinks/__tests__/autolinks.test.ts index 19d4d67d5621e..14a1510fa9e75 100644 --- a/src/autolinks/__tests__/autolinks.test.ts +++ b/src/autolinks/__tests__/autolinks.test.ts @@ -24,24 +24,34 @@ function assertAutolinks(actual: Map, expected: Array) suite('Autolinks Test Suite', () => { test('Branch name autolinks', () => { - assertAutolinks(getBranchAutolinks('123', mockRefSets()), ['test/123']); - assertAutolinks(getBranchAutolinks('feature/123', mockRefSets()), ['test/123']); + // Matches under rule 1 (prefixed 2+ digit number followed by end of string) + assertAutolinks(getBranchAutolinks('feature/PRE-12', mockRefSets(['PRE-'])), ['test/12']); + // Matches under rule 1 (prefixed 2+ digit number followed by a separator) + assertAutolinks(getBranchAutolinks('feature/PRE-12.2', mockRefSets(['PRE-'])), ['test/12']); + // Matches under rule 2 (feature/ followed by a 2+ digit number and nothing after it) + assertAutolinks(getBranchAutolinks('feature/12', mockRefSets()), ['test/12']); + // Matches under rule 2 (feature/ followed by a 2+ digit number and a separator after it) + assertAutolinks(getBranchAutolinks('feature/12.test-bug', mockRefSets()), ['test/12']); + // Matches under rule 3 (3+ digit issue number preceded by at least two non-slash, non-digit characters) assertAutolinks(getBranchAutolinks('feature/PRE-123', mockRefSets()), ['test/123']); - assertAutolinks(getBranchAutolinks('123.2', mockRefSets()), ['test/123', 'test/2']); + // Matches under rule 3 (3+ digit issue number followed by at least two non-slash, non-digit characters) + assertAutolinks(getBranchAutolinks('123abc', mockRefSets()), ['test/123']); + // Matches under rule 3 (3+ digit issue number is the entire branch name) + assertAutolinks(getBranchAutolinks('123', mockRefSets()), ['test/123']); + // Fails all rules because it is a 1 digit number. + assertAutolinks(getBranchAutolinks('feature/3', mockRefSets([''])), []); + // Fails all rules because it is a 1 digit number. + assertAutolinks(getBranchAutolinks('feature/3', mockRefSets(['PRE-'])), []); + // Fails all rules. In rule 3, fails because one of the two following characters is a number. + assertAutolinks(getBranchAutolinks('123.2', mockRefSets()), []); + // Fails all rules. In rule 3, fails because the issue is a full section (a slash before and after it). + assertAutolinks(getBranchAutolinks('improvement/123/ui-fix', mockRefSets()), []); + // Fails all rules. 2&3 because the ref is prefixed, and 1 because the branch name doesn't have the ref's prefix. assertAutolinks(getBranchAutolinks('123', mockRefSets(['PRE-'])), []); - assertAutolinks(getBranchAutolinks('feature/123', mockRefSets(['PRE-'])), []); - assertAutolinks(getBranchAutolinks('feature/2-fa/123', mockRefSets([''])), ['test/123', 'test/2']); - assertAutolinks(getBranchAutolinks('feature/2-fa/123', mockRefSets([''])), ['test/123', 'test/2']); - // incorrectly solved cat worths to compare the blocks length so that the less block size (without possible link) is more likely a link - assertAutolinks(getBranchAutolinks('feature/2-fa/3', mockRefSets([''])), ['test/2', 'test/3']); - assertAutolinks(getBranchAutolinks('feature/PRE-123', mockRefSets(['PRE-'])), ['test/123']); - assertAutolinks(getBranchAutolinks('feature/PRE-123.2', mockRefSets(['PRE-'])), ['test/123']); - assertAutolinks(getBranchAutolinks('feature/3-123-PRE-123', mockRefSets(['PRE-'])), ['test/123']); - assertAutolinks( - getBranchAutolinks('feature/3-123-PRE-123', mockRefSets(['', 'PRE-'])), - - ['test/123', 'test/3'], - ); + // Fails all rules. 2 because the issue is not immediately following the feature/ section, and 3 because it is a full section. + assertAutolinks(getBranchAutolinks('feature/2-fa/123', mockRefSets([''])), []); + // Fails all rules. 2 because of non-separator character after issue number, and 3 because it has end-of-string two character after. + assertAutolinks(getBranchAutolinks('feature/123a', mockRefSets(['PRE-'])), []); }); test('Commit message autolinks', () => { diff --git a/src/autolinks/autolinksProvider.ts b/src/autolinks/autolinksProvider.ts index a25d0a0a50929..19dcb65d829bf 100644 --- a/src/autolinks/autolinksProvider.ts +++ b/src/autolinks/autolinksProvider.ts @@ -122,9 +122,13 @@ export class AutolinksProvider implements Disposable { } /** Collects remote provider autolink references into @param refsets */ - private collectRemoteAutolinks(remote: GitRemote | undefined, refsets: RefSet[]): void { + private collectRemoteAutolinks(remote: GitRemote | undefined, refsets: RefSet[], forBranch?: boolean): void { if (remote?.provider?.autolinks.length) { - refsets.push([remote.provider, remote.provider.autolinks]); + let autolinks = remote.provider.autolinks; + if (forBranch) { + autolinks = autolinks.filter(autolink => !isDynamic(autolink) && autolink.referenceType === 'branch'); + } + refsets.push([remote.provider, autolinks]); } } @@ -135,12 +139,12 @@ export class AutolinksProvider implements Disposable { } } - private async getRefSets(remote?: GitRemote) { + private async getRefSets(remote?: GitRemote, forBranch?: boolean) { return this._refsetCache.get(remote?.remoteKey, async () => { const refsets: RefSet[] = []; - await this.collectIntegrationAutolinks(remote, refsets); - this.collectRemoteAutolinks(remote, refsets); + await this.collectIntegrationAutolinks(forBranch ? undefined : remote, refsets); + this.collectRemoteAutolinks(remote, refsets, forBranch); this.collectCustomAutolinks(refsets); return refsets; @@ -149,7 +153,7 @@ export class AutolinksProvider implements Disposable { /** @returns A sorted list of autolinks. the first match is the most relevant */ async getBranchAutolinks(branchName: string, remote?: GitRemote): Promise> { - const refsets = await this.getRefSets(remote); + const refsets = await this.getRefSets(remote, true); if (refsets.length === 0) return emptyAutolinkMap; return getBranchAutolinks(branchName, refsets); diff --git a/src/autolinks/models/autolinks.ts b/src/autolinks/models/autolinks.ts index ee1ea2ad5c781..a506c4e6b7de2 100644 --- a/src/autolinks/models/autolinks.ts +++ b/src/autolinks/models/autolinks.ts @@ -26,7 +26,6 @@ export interface AutolinkReference { export interface Autolink extends Omit { provider?: ProviderReference; id: string; - index?: number; } export type EnrichedAutolink = [ @@ -56,7 +55,7 @@ export interface CacheableAutolinkReference extends AutolinkReference { messageHtmlRegex?: RegExp; messageMarkdownRegex?: RegExp; messageRegex?: RegExp; - branchNameRegex?: RegExp; + branchNameRegexes?: RegExp[]; } export interface DynamicAutolinkReference { diff --git a/src/autolinks/utils/-webview/autolinks.utils.ts b/src/autolinks/utils/-webview/autolinks.utils.ts index 7313392e5dbec..a0d6a39198c6c 100644 --- a/src/autolinks/utils/-webview/autolinks.utils.ts +++ b/src/autolinks/utils/-webview/autolinks.utils.ts @@ -20,7 +20,6 @@ export function serializeAutolink(value: Autolink): Autolink { } : undefined, id: value.id, - index: value.index, prefix: value.prefix, url: value.url, alphanumeric: value.alphanumeric, @@ -43,23 +42,6 @@ function isCacheable(ref: AutolinkReference | DynamicAutolinkReference): ref is return 'prefix' in ref && ref.prefix != null && 'url' in ref && ref.url != null; } -/** - * Compares autolinks - * @returns non-0 result that means a probability of the autolink `b` is more relevant of the autolink `a` - */ -function compareAutolinks(a: Autolink, b: Autolink): number { - // consider that if the number is in the start, it's the most relevant link - if (b.index === 0) return 1; - if (a.index === 0) return -1; - - // maybe it worths to use some weight function instead. - return ( - b.prefix.length - a.prefix.length || - b.id.length - a.id.length || - (b.index != null && a.index != null ? -(b.index - a.index) : 0) - ); -} - export function ensureCachedRegex( ref: Autolink | CacheableAutolinkReference, outputFormat: 'html', @@ -97,15 +79,29 @@ export function ensureCachedRegex( } } -export function ensureCachedBranchNameRegex( +export function ensureCachedBranchNameRegexes( ref: CacheableAutolinkReference, -): asserts ref is RequireSome { - ref.branchNameRegex ??= new RegExp( - `(^|\\-|_|\\.|\\/)(?${ref.prefix})(?${ - ref.alphanumeric ? '\\w' : '\\d' - }+)(?=$|\\-|_|\\.|\\/)`, - 'gi', - ); +): asserts ref is RequireSome { + if (ref.prefix?.length > 0) { + ref.branchNameRegexes ??= [ + // Rule 1: Any prefixed ref followed by a 2+ digit number and either a connector or end-of-string after it + new RegExp(`(?${ref.prefix})(?\\d{2,})(?:[\\/\\-\\_\\.]|$)`, 'i'), + ]; + } else { + ref.branchNameRegexes ??= [ + // Rule 2: Any 2+ digit number preceded by feature|feat|fix|bug|bugfix|hotfix|issue|ticket with a connector before it, and either a connector or end-of-string after it + new RegExp( + `(?:feature|feat|fix|bug|bugfix|hotfix|issue|ticket)(?:\\/#|-#|_#|\\.#|[\\/\\-\\_\\.#])(?\\d{2,})(?:[\\/\\-\\_\\.]|$)`, + 'i', + ), + // Rule 3.1: Any 3+ digit number preceded by at least two non-slash, non-numeric characters + new RegExp(`(?:[^\\d/]{2})(?\\d{3,})`, 'i'), + // Rule 3.2: Any 3+ digit number followed by at least two non-slash, non-numeric characters + new RegExp(`(?\\d{3,})(?:[^\\d/]{2})`, 'i'), + // Rule 3.3: A 3+ digit number is the entire branch name + new RegExp(`^(?\\d{3,})$`, 'i'), + ]; + } } export const numRegex = //g; @@ -135,7 +131,6 @@ export function getAutolinks(message: string, refsets: Readonly): Map< autolinks.set(num, { provider: provider, id: num, - index: match.index, prefix: ref.prefix, url: ref.url?.replace(numRegex, num), alphanumeric: ref.alphanumeric, @@ -155,9 +150,16 @@ export function getAutolinks(message: string, refsets: Readonly): Map< export function getBranchAutolinks(branchName: string, refsets: Readonly): Map { const autolinks = new Map(); - let match; let num; - for (const [provider, refs] of refsets) { + let match; + // Sort refsets so that issue integrations are checked first for matches + const sortedRefSets = [...refsets].sort((a, b) => { + if (a[0]?.id === IssueIntegrationId.Jira || a[0]?.id === IssueIntegrationId.Trello) return -1; + if (b[0]?.id === IssueIntegrationId.Jira || b[0]?.id === IssueIntegrationId.Trello) return 1; + return 0; + }); + + for (const [provider, refs] of sortedRefSets) { for (const ref of refs) { if ( !isCacheable(ref) || @@ -167,33 +169,27 @@ export function getBranchAutolinks(branchName: string, refsets: Readonly compareAutolinks(a[1], b[1]))); + return autolinks; } diff --git a/src/plus/integrations/providers/jira.ts b/src/plus/integrations/providers/jira.ts index bb0d7a0710cd1..9b7e674babc6e 100644 --- a/src/plus/integrations/providers/jira.ts +++ b/src/plus/integrations/providers/jira.ts @@ -57,16 +57,29 @@ export class JiraIntegration extends IssueIntegration { const projects = this._projects.get(`${this._session.accessToken}:${organization.id}`); if (projects != null) { for (const project of projects) { - const prefix = `${project.key}-`; + const dashedPrefix = `${project.key}-`; + const underscoredPrefix = `${project.key}_`; autolinks.push({ - prefix: prefix, - url: `${organization.url}/browse/${prefix}`, + prefix: dashedPrefix, + url: `${organization.url}/browse/${dashedPrefix}`, alphanumeric: false, ignoreCase: false, - title: `Open Issue ${prefix} on ${organization.name}`, + title: `Open Issue ${dashedPrefix} on ${organization.name}`, type: 'issue', - description: `${organization.name} Issue ${prefix}`, + description: `${organization.name} Issue ${dashedPrefix}`, + descriptor: { ...organization }, + }); + autolinks.push({ + prefix: underscoredPrefix, + url: `${organization.url}/browse/${dashedPrefix}`, + alphanumeric: false, + ignoreCase: false, + referenceType: 'branch', + title: `Open Issue ${dashedPrefix} on ${organization.name}`, + + type: 'issue', + description: `${organization.name} Issue ${dashedPrefix}`, descriptor: { ...organization }, }); }