Fix review notes

Author: nzaytsev

CHANGELOG.md

@@ -6,6 +6,10 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p
 
 ## [Unreleased]
 
+### Added
+
+- Adds the ability to get autolinks for branches using branch name [#3547](https://github.com/gitkraken/vscode-gitlens/issues/3547)
+
 ## [16.0.2] - 2024-11-18
 
 ### Changed
@@ -132,7 +136,6 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p
 - Adds new ability to force push from the _Commit Graph_ toolbar — closes [#3493](https://github.com/gitkraken/vscode-gitlens/issues/3493)
 - Adds a new `gitlens.launchpad.includedOrganizations` setting to specify which organizations to include in _Launchpad_ — closes [#3550](https://github.com/gitkraken/vscode-gitlens/issues/3550)
 - Adds repository owner/name and code suggest to hovers on the experimental Launchpad view
-- Adds getBranchAutolinks method to Autolinks class [#3547](https://github.com/gitkraken/vscode-gitlens/issues/3547)
 
 ### Changed
 

package.json

@@ -19602,7 +19602,7 @@
 		"update-dts:main": "pushd \"src/@types\" && pnpx  @vscode/dts main && popd",
 		"update-emoji": "node ./scripts/generateEmojiShortcodeMap.mjs",
 		"update-licenses": "node ./scripts/generateLicenses.mjs",
-		"pretest": "pnpm run build:tests",
+		"//pretest": "pnpm run build:tests",
 		"vscode:prepublish": "pnpm run bundle"
 	},
 	"dependencies": {

src/autolinks/__tests__/autolinks.test.ts

@@ -1,6 +1,7 @@
 import * as assert from 'assert';
 import { suite, test } from 'mocha';
-import type { RefSet } from '../autolinks';
+import { map } from '../../system/iterable';
+import type { Autolink, RefSet } from '../autolinks';
 import { Autolinks } from '../autolinks';
 
 const mockRefSets = (prefixes: string[] = ['']): RefSet[] =>
@@ -18,54 +19,33 @@ const mockRefSets = (prefixes: string[] = ['']): RefSet[] =>
 		],
 	]);
 
+function assertAutolinks(actual: Map<string, Autolink>, expected: Array<string>): void {
+	assert.deepEqual([...map(actual.values(), x => x.url)], expected);
+}
+
 suite('Autolinks Test Suite', () => {
 	test('Branch name autolinks', () => {
-		assert.deepEqual(
-			Autolinks._getBranchAutolinks('123', mockRefSets()).map(x => x.url),
-			['test/123'],
-		);
-		assert.deepEqual(
-			Autolinks._getBranchAutolinks('feature/123', mockRefSets()).map(x => x.url),
-			['test/123'],
-		);
-		assert.deepEqual(
-			Autolinks._getBranchAutolinks('feature/PRE-123', mockRefSets()).map(x => x.url),
-			['test/123'],
-		);
-		assert.deepEqual(
-			Autolinks._getBranchAutolinks('123.2', mockRefSets()).map(x => x.url),
-			['test/123', 'test/2'],
-		);
-		assert.deepEqual(
-			Autolinks._getBranchAutolinks('123', mockRefSets(['PRE-'])).map(x => x.url),
-			[],
-		);
-		assert.deepEqual(
-			Autolinks._getBranchAutolinks('feature/123', mockRefSets(['PRE-'])).map(x => x.url),
-			[],
-		);
-		assert.deepEqual(
-			Autolinks._getBranchAutolinks('feature/PRE-123', mockRefSets(['PRE-'])).map(x => x.url),
-			['test/123'],
-		);
-		assert.deepEqual(
-			Autolinks._getBranchAutolinks('feature/PRE-123.2', mockRefSets(['PRE-'])).map(x => x.url),
-			['test/123'],
-		);
-		assert.deepEqual(
-			Autolinks._getBranchAutolinks('feature/3-123-PRE-123', mockRefSets(['PRE-'])).map(x => x.url),
-			['test/123'],
-		);
-		assert.deepEqual(
-			Autolinks._getBranchAutolinks('feature/3-123-PRE-123', mockRefSets(['', 'PRE-'])).map(x => x.url),
+		assertAutolinks(Autolinks._getBranchAutolinks('123', mockRefSets()), ['test/123']);
+		assertAutolinks(Autolinks._getBranchAutolinks('feature/123', mockRefSets()), ['test/123']);
+		assertAutolinks(Autolinks._getBranchAutolinks('feature/PRE-123', mockRefSets()), ['test/123']);
+		assertAutolinks(Autolinks._getBranchAutolinks('123.2', mockRefSets()), ['test/123', 'test/2']);
+		assertAutolinks(Autolinks._getBranchAutolinks('123', mockRefSets(['PRE-'])), []);
+		assertAutolinks(Autolinks._getBranchAutolinks('feature/123', mockRefSets(['PRE-'])), []);
+		assertAutolinks(Autolinks._getBranchAutolinks('feature/2-fa/123', mockRefSets([''])), ['test/123', 'test/2']);
+		assertAutolinks(Autolinks._getBranchAutolinks('feature/2-fa/123', mockRefSets([''])), ['test/123', 'test/2']);
+		// incorrectly solved case, maybe it worths to compare the blocks length so that the less block size (without possible link) is more likely a link
+		assertAutolinks(Autolinks._getBranchAutolinks('feature/2-fa/3', mockRefSets([''])), ['test/2', 'test/3']);
+		assertAutolinks(Autolinks._getBranchAutolinks('feature/PRE-123', mockRefSets(['PRE-'])), ['test/123']);
+		assertAutolinks(Autolinks._getBranchAutolinks('feature/PRE-123.2', mockRefSets(['PRE-'])), ['test/123']);
+		assertAutolinks(Autolinks._getBranchAutolinks('feature/3-123-PRE-123', mockRefSets(['PRE-'])), ['test/123']);
+		assertAutolinks(
+			Autolinks._getBranchAutolinks('feature/3-123-PRE-123', mockRefSets(['', 'PRE-'])),
+
 			['test/123', 'test/3'],
 		);
 	});
 
 	test('Commit message autolinks', () => {
-		assert.deepEqual(
-			[...Autolinks._getAutolinks('test message 123 sd', mockRefSets()).values()].map(x => x.url),
-			['test/123'],
-		);
+		assertAutolinks(Autolinks._getAutolinks('test message 123 sd', mockRefSets()), ['test/123']);
 	});
 });

src/autolinks/autolinks.ts

@@ -46,6 +46,7 @@ export interface AutolinkReference {
 export interface Autolink extends AutolinkReference {
 	provider?: ProviderReference;
 	id: string;
+	index: number;
 
 	tokenize?:
 		| ((
@@ -80,6 +81,7 @@ export function serializeAutolink(value: Autolink): Autolink {
 			  }
 			: undefined,
 		id: value.id,
+		index: value.index,
 		prefix: value.prefix,
 		url: value.url,
 		alphanumeric: value.alphanumeric,
@@ -126,7 +128,7 @@ export interface DynamicAutolinkReference {
 
 export const supportedAutolinkIntegrations = [IssueIntegrationId.Jira];
 
-export function isDynamic(ref: AutolinkReference | DynamicAutolinkReference): ref is DynamicAutolinkReference {
+function isDynamic(ref: AutolinkReference | DynamicAutolinkReference): ref is DynamicAutolinkReference {
 	return !('prefix' in ref) && !('url' in ref);
 }
 
@@ -139,14 +141,6 @@ export type RefSet = [
 	(AutolinkReference | DynamicAutolinkReference)[] | CacheableAutolinkReference[],
 ];
 
-type ComparingAutolinkSet = {
-	/** the place where the autolink is found from start-like symbol (/|_) */
-	index: number;
-	/** the place where the autolink is found from start */
-	startIndex: number;
-	autolink: Autolink;
-};
-
 export class Autolinks implements Disposable {
 	protected _disposable: Disposable | undefined;
 	private _references: CacheableAutolinkReference[] = [];
@@ -223,7 +217,7 @@ export class Autolinks implements Disposable {
 	/**
 	 * it should always return non-0 result that means a probability of the autolink `b` is more relevant of the autolink `a`
 	 */
-	private static compareAutolinks(a: ComparingAutolinkSet, b: ComparingAutolinkSet) {
+	private static compareAutolinks(a: Autolink, b: Autolink) {
 		// consider that if the number is in the start, it's the most relevant link
 		if (b.index === 0) {
 			return 1;
@@ -232,34 +226,35 @@ export class Autolinks implements Disposable {
 			return -1;
 		}
 		// maybe it worths to use some weight function instead.
-		return (
-			b.autolink.prefix.length - a.autolink.prefix.length ||
-			-(b.startIndex - a.startIndex) ||
-			-(b.index - a.index)
-		);
+		return b.prefix.length - a.prefix.length || b.id.length - a.id.length || -(b.index - a.index);
 	}
 
-	/**
-	 * returns sorted list of autolinks. the first is matched as the most relevant
-	 */
-	async getBranchAutolinks(
-		branchName: string,
-		remote?: GitRemote,
-		options?: { excludeCustom: boolean },
-	): Promise<undefined | Autolink[]> {
+	private async getRefsets(remote?: GitRemote, options?: { excludeCustom?: boolean }) {
 		const refsets: RefSet[] = [];
 		await this.collectIntegrationAutolinks(refsets);
 		await this.collectRemoteAutolinks(remote, refsets);
 		if (!options?.excludeCustom) {
 			this.collectCustomAutolinks(remote, refsets);
 		}
-		if (refsets.length === 0) return undefined;
+		return refsets;
+	}
+
+	/**
+	 * returns sorted list of autolinks. the first is matched as the most relevant
+	 */
+	async getBranchAutolinks(
+		branchName: string,
+		remote?: GitRemote,
+		options?: { excludeCustom?: boolean },
+	): Promise<Map<string, Autolink>> {
+		const refsets = await this.getRefsets(remote, options);
+		if (refsets.length === 0) return emptyAutolinkMap;
 
 		return Autolinks._getBranchAutolinks(branchName, refsets);
 	}
 
 	static _getBranchAutolinks(branchName: string, refsets: Readonly<RefSet[]>) {
-		const autolinks = new Map<string, ComparingAutolinkSet>();
+		const autolinks = new Map<string, Autolink>();
 
 		let match;
 		let num;
@@ -286,28 +281,20 @@ export class Autolinks implements Disposable {
 						index = Math.min(index, autolinks.get(linkUrl)!.index);
 					}
 					autolinks.set(linkUrl, {
+						...ref,
+						provider: provider,
+						id: num,
 						index: index,
-						// TODO: calc the distance from the nearest start-like symbol
-						startIndex: 0,
-						autolink: {
-							...ref,
-							provider: provider,
-							id: num,
-
-							url: linkUrl,
-							title: ref.title?.replace(numRegex, num),
-							description: ref.description?.replace(numRegex, num),
-							descriptor: ref.descriptor,
-						},
+						url: linkUrl,
+						title: ref.title?.replace(numRegex, num),
+						description: ref.description?.replace(numRegex, num),
+						descriptor: ref.descriptor,
 					});
 				} while (!match.done);
 			}
 		}
 
-		return [...autolinks.values()]
-			.flat()
-			.sort(this.compareAutolinks)
-			.map(x => x.autolink);
+		return new Map([...autolinks.entries()].sort((a, b) => this.compareAutolinks(a[1], b[1])));
 	}
 
 	async getAutolinks(message: string, remote?: GitRemote): Promise<Map<string, Autolink>>;
@@ -326,14 +313,9 @@ export class Autolinks implements Disposable {
 	async getAutolinks(
 		message: string,
 		remote?: GitRemote,
-		options?: { excludeCustom?: boolean; isBranchName?: boolean },
+		options?: { excludeCustom?: boolean },
 	): Promise<Map<string, Autolink>> {
-		const refsets: RefSet[] = [];
-		await this.collectIntegrationAutolinks(refsets);
-		await this.collectRemoteAutolinks(remote, refsets);
-		if (!options?.excludeCustom) {
-			this.collectCustomAutolinks(remote, refsets);
-		}
+		const refsets = await this.getRefsets(remote, options);
 		if (refsets.length === 0) return emptyAutolinkMap;
 
 		return Autolinks._getAutolinks(message, refsets);
@@ -363,6 +345,7 @@ export class Autolinks implements Disposable {
 					autolinks.set(num, {
 						provider: provider,
 						id: num,
+						index: match.index,
 						prefix: ref.prefix,
 						url: ref.url?.replace(numRegex, num),
 						alphanumeric: ref.alphanumeric,