Performs specific PR URL match for each connected provider

(#3788, #3795)

src/git/models/__tests__/pullRequest.utils.test.ts

@@ -1,62 +1,30 @@
 import * as assert from 'assert';
 import { suite, test } from 'mocha';
-import { getPullRequestIdentityValuesFromSearch } from '../pullRequest.utils';
+import { getPullRequestIdentityFromMaybeUrl } from '../pullRequest.utils';
 
-suite('Test GitHub PR URL parsing to identity: getPullRequestIdentityValuesFromSearch()', () => {
+suite('Test PR URL parsing to identity: getPullRequestIdentityFromMaybeUrl()', () => {
 	function t(message: string, query: string, prNumber: string | undefined, ownerAndRepo?: string) {
 		assert.deepStrictEqual(
-			getPullRequestIdentityValuesFromSearch(query),
-			{
-				ownerAndRepo: ownerAndRepo,
-				prNumber: prNumber,
-			},
+			getPullRequestIdentityFromMaybeUrl(query),
+			prNumber == null
+				? undefined
+				: {
+						ownerAndRepo: ownerAndRepo,
+						prNumber: prNumber,
+						provider: undefined,
+				  },
 			`${message} (${JSON.stringify(query)})`,
 		);
 	}
 
-	test('full URL or without protocol but with domain, should parse to ownerAndRepo and prNumber', () => {
-		t('full URL', 'https://github.com/eamodio/vscode-gitlens/pull/1', '1', 'eamodio/vscode-gitlens');
-		t(
-			'with suffix',
-			'https://github.com/eamodio/vscode-gitlens/pull/1/files?diff=unified#hello',
-			'1',
-			'eamodio/vscode-gitlens',
-		);
-		t(
-			'with query',
-			'https://github.com/eamodio/vscode-gitlens/pull/1?diff=unified#hello',
-			'1',
-			'eamodio/vscode-gitlens',
-		);
-
-		t('with anchor', 'https://github.com/eamodio/vscode-gitlens/pull/1#hello', '1', 'eamodio/vscode-gitlens');
-		t('a weird suffix', 'https://github.com/eamodio/vscode-gitlens/pull/1-files', '1', 'eamodio/vscode-gitlens');
-		t('numeric repo name', 'https://github.com/sergeibbb/1/pull/16', '16', 'sergeibbb/1');
-
-		t('no protocol with leading slash', '/github.com/sergeibbb/1/pull/16?diff=unified', '16', 'sergeibbb/1');
-		t('no protocol without leading slash', 'github.com/sergeibbb/1/pull/16/files', '16', 'sergeibbb/1');
-	});
-
-	test('no domain, should parse to ownerAndRepo and prNumber', () => {
-		t('with leading slash', '/sergeibbb/1/pull/16#hello', '16', 'sergeibbb/1');
-		t('words in repo name', 'eamodio/vscode-gitlens/pull/1?diff=unified#hello', '1', 'eamodio/vscode-gitlens');
-		t('numeric repo name', 'sergeibbb/1/pull/16/files', '16', 'sergeibbb/1');
-	});
-
-	test('domain vs. no domain', () => {
-		t(
-			'with anchor',
-			'https://github.com/eamodio/vscode-gitlens/pull/1#hello/sergeibbb/1/pull/16',
-			'1',
-			'eamodio/vscode-gitlens',
-		);
-	});
+	test('cannot recognize GitHub or GitLab URLs, sees only numbers', () => {
+		t('full URL', 'https://github.com/eamodio/vscode-gitlens/pull/16', '16');
+		t('numeric repo name', 'https://github.com/sergeibbb/1/pull/16', '1');
 
-	test('has "pull/" fragment', () => {
-		t('with leading slash', '/pull/16/files#hello', '16');
-		t('without leading slash', 'pull/16?diff=unified#hello', '16');
-		t('with numeric repo name', '1/pull/16?diff=unified#hello', '16');
-		t('with double slash', '1//pull/16?diff=unified#hello', '16');
+		t('no protocol', '/github.com/sergeibbb/1/pull/16?diff=unified', '1');
+		t('no domain', '/sergeibbb/1/pull/16#hello', '1');
+		t('domain vs. no domain', 'https://github.com/eamodio/vscode-gitlens/pull/1#hello/sergeibbb/2/pull/16', '1');
+		t('has "pull/" fragment', '/pull/16/files#hello', '16');
 	});
 
 	test('has "/<num>" fragment', () => {

src/git/models/pullRequest.ts

@@ -2,12 +2,10 @@ import { Uri, window } from 'vscode';
 import { Schemes } from '../../constants';
 import { Container } from '../../container';
 import type { RepositoryIdentityDescriptor } from '../../gk/models/repositoryIdentities';
-import type { EnrichablePullRequest } from '../../plus/integrations/providers/models';
 import { formatDate, fromNow } from '../../system/date';
 import { memoize } from '../../system/decorators/memoize';
 import type { LeftRightCommitCountResult } from '../gitProvider';
 import type { IssueOrPullRequest, IssueRepository, IssueOrPullRequestState as PullRequestState } from './issue';
-import type { PullRequestUrlIdentity } from './pullRequest.utils';
 import type { ProviderReference } from './remoteProvider';
 import type { Repository } from './repository';
 import { createRevisionRange, shortenRevision } from './revision.utils';
@@ -417,21 +415,3 @@ export async function getOpenedPullRequestRepo(
 	const repo = await getOrOpenPullRequestRepository(container, pr, { promptIfNeeded: true });
 	return repo;
 }
-
-export function doesPullRequestSatisfyRepositoryURLIdentity(
-	pr: EnrichablePullRequest | undefined,
-	{ ownerAndRepo, prNumber }: PullRequestUrlIdentity,
-): boolean {
-	if (pr == null) {
-		return false;
-	}
-	const satisfiesPrNumber = prNumber != null && pr.number === parseInt(prNumber, 10);
-	if (!satisfiesPrNumber) {
-		return false;
-	}
-	const satisfiesOwnerAndRepo = ownerAndRepo != null && pr.repoIdentity.name === ownerAndRepo;
-	if (!satisfiesOwnerAndRepo) {
-		return false;
-	}
-	return true;
-}

src/git/models/pullRequest.utils.ts

@@ -8,31 +8,19 @@ export interface PullRequestUrlIdentity {
 	provider?: HostingIntegrationId;
 
 	ownerAndRepo?: string;
-	prNumber?: string;
+	prNumber: string;
 }
 
-export function getPullRequestIdentityValuesFromSearch(search: string): PullRequestUrlIdentity {
-	let ownerAndRepo: string | undefined = undefined;
+export function isMaybeNonSpecificPullRequestSearchUrl(search: string): boolean {
+	return getPullRequestIdentityFromMaybeUrl(search) != null;
+}
+
+export function getPullRequestIdentityFromMaybeUrl(search: string): PullRequestUrlIdentity | undefined {
 	let prNumber: string | undefined = undefined;
 
-	let match = search.match(/([^/]+\/[^/]+)\/(?:pull|-\/merge_requests)\/(\d+)/); // with org and rep name
+	let match = search.match(/(?:\/)(\d+)/); // any number starting with "/"
 	if (match != null) {
-		ownerAndRepo = match[1];
-		prNumber = match[2];
-	}
-
-	if (prNumber == null) {
-		match = search.match(/(?:\/|^)(?:pull|-\/merge_requests)\/(\d+)/); // without repo name
-		if (match != null) {
-			prNumber = match[1];
-		}
-	}
-
-	if (prNumber == null) {
-		match = search.match(/(?:\/)(\d+)/); // any number starting with "/"
-		if (match != null) {
-			prNumber = match[1];
-		}
+		prNumber = match[1];
 	}
 
 	if (prNumber == null) {
@@ -42,5 +30,5 @@ export function getPullRequestIdentityValuesFromSearch(search: string): PullRequ
 		}
 	}
 
-	return { ownerAndRepo: ownerAndRepo, prNumber: prNumber };
+	return prNumber == null ? undefined : { ownerAndRepo: undefined, prNumber: prNumber, provider: undefined };
 }

src/plus/integrations/integration.ts

@@ -16,6 +16,7 @@ import type {
 	PullRequestState,
 	SearchedPullRequest,
 } from '../../git/models/pullRequest';
+import type { PullRequestUrlIdentity } from '../../git/models/pullRequest.utils';
 import type { RepositoryMetadata } from '../../git/models/repositoryMetadata';
 import { showIntegrationDisconnectedTooManyFailedRequestsWarningMessage } from '../../messages';
 import { gate } from '../../system/decorators/gate';
@@ -1361,4 +1362,6 @@ export abstract class HostingIntegration<
 		repos?: T[],
 		cancellation?: CancellationToken,
 	): Promise<PullRequest[] | undefined>;
+
+	getPullRequestIdentityFromMaybeUrl?(search: string): PullRequestUrlIdentity | undefined;
 }

src/plus/integrations/providers/github.ts

@@ -11,6 +11,7 @@ import type {
 	PullRequestState,
 	SearchedPullRequest,
 } from '../../../git/models/pullRequest';
+import type { PullRequestUrlIdentity } from '../../../git/models/pullRequest.utils';
 import type { RepositoryMetadata } from '../../../git/models/repositoryMetadata';
 import { log } from '../../../system/decorators/log';
 import { ensurePaidPlan } from '../../utils';
@@ -20,6 +21,7 @@ import type {
 } from '../authentication/integrationAuthentication';
 import type { RepositoryDescriptor, SupportedIntegrationIds } from '../integration';
 import { HostingIntegration } from '../integration';
+import { getGitHubPullRequestIdentityFromMaybeUrl } from './github/github.utils';
 import { providersMetadata } from './models';
 import type { ProvidersApi } from './providersApi';
 
@@ -292,6 +294,10 @@ export class GitHubIntegration extends GitHubIntegrationBase<HostingIntegrationI
 			super.refresh();
 		}
 	}
+
+	override getPullRequestIdentityFromMaybeUrl(search: string): PullRequestUrlIdentity | undefined {
+		return getGitHubPullRequestIdentityFromMaybeUrl(search);
+	}
 }
 
 export class GitHubEnterpriseIntegration extends GitHubIntegrationBase<SelfHostedIntegrationId.GitHubEnterprise> {

src/plus/integrations/providers/github/__tests__/github.utils.test.ts

@@ -0,0 +1,87 @@
+import * as assert from 'assert';
+import { suite, test } from 'mocha';
+//import { getGitHubPullRequestIdentityFromMaybeUrl } from '../github/models';
+import { getGitHubPullRequestIdentityFromMaybeUrl, isMaybeGitHubPullRequestUrl } from '../github.utils';
+
+suite('Test GitHub PR URL parsing to identity: getPullRequestIdentityFromMaybeUrl()', () => {
+	function t(message: string, query: string, prNumber: string | undefined, ownerAndRepo?: string) {
+		assert.deepStrictEqual(
+			getGitHubPullRequestIdentityFromMaybeUrl(query),
+			prNumber == null
+				? undefined
+				: {
+						ownerAndRepo: ownerAndRepo,
+						prNumber: prNumber,
+						provider: 'github',
+				  },
+			`Parse: ${message} (${JSON.stringify(query)})`,
+		);
+		assert.equal(
+			isMaybeGitHubPullRequestUrl(query),
+			prNumber != null,
+			`Check: ${message} (${JSON.stringify(query)})`,
+		);
+	}
+
+	test('full URL or without protocol but with domain, should parse to ownerAndRepo and prNumber', () => {
+		t('full URL', 'https://github.com/eamodio/vscode-gitlens/pull/1', '1', 'eamodio/vscode-gitlens');
+		t(
+			'with suffix',
+			'https://github.com/eamodio/vscode-gitlens/pull/1/files?diff=unified#hello',
+			'1',
+			'eamodio/vscode-gitlens',
+		);
+		t(
+			'with query',
+			'https://github.com/eamodio/vscode-gitlens/pull/1?diff=unified#hello',
+			'1',
+			'eamodio/vscode-gitlens',
+		);
+
+		t('with anchor', 'https://github.com/eamodio/vscode-gitlens/pull/1#hello', '1', 'eamodio/vscode-gitlens');
+		t('a weird suffix', 'https://github.com/eamodio/vscode-gitlens/pull/1-files', '1', 'eamodio/vscode-gitlens');
+		t('numeric repo name', 'https://github.com/sergeibbb/1/pull/16', '16', 'sergeibbb/1');
+
+		t('no protocol with leading slash', '/github.com/sergeibbb/1/pull/16?diff=unified', '16', 'sergeibbb/1');
+		t('no protocol without leading slash', 'github.com/sergeibbb/1/pull/16/files', '16', 'sergeibbb/1');
+	});
+
+	test('no domain, should parse to ownerAndRepo and prNumber', () => {
+		t('with leading slash', '/sergeibbb/1/pull/16#hello', '16', 'sergeibbb/1');
+		t('words in repo name', 'eamodio/vscode-gitlens/pull/1?diff=unified#hello', '1', 'eamodio/vscode-gitlens');
+		t('numeric repo name', 'sergeibbb/1/pull/16/files', '16', 'sergeibbb/1');
+	});
+
+	test('domain vs. no domain', () => {
+		t(
+			'with anchor',
+			'https://github.com/eamodio/vscode-gitlens/pull/1#hello/sergeibbb/1/pull/16',
+			'1',
+			'eamodio/vscode-gitlens',
+		);
+	});
+
+	test('numbers', () => {
+		t('has "pull/" fragment', '/pull/16/files#hello', '16');
+		t('has "pull/" fragment with double slash', '1//pull/16?diff=unified#hello', '16');
+		t('with leading slash', '/16/files#hello', undefined);
+		t('just a number', '16', undefined);
+		t('with a hash', '#16', undefined);
+	});
+
+	test('does not match', () => {
+		t('without leading slash', '16?diff=unified#hello', undefined);
+		t('with leading hash', '/#16/files#hello', undefined);
+		t('number is a part of a word', 'hello16', undefined);
+		t('number is a part of a word', '16hello', undefined);
+
+		t('GitLab', 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/16', undefined);
+
+		t('with a number', '1/16?diff=unified#hello', undefined);
+		t('with a number and slash', '/1/16?diff=unified#hello', undefined);
+		t('with a word', 'anything/16?diff=unified#hello', undefined);
+
+		t('with a wrong character leading to pull', 'sergeibbb/1/-pull/16?diff=unified#hello', undefined);
+		t('with a wrong character leading to pull', 'sergeibbb/1-pull/16?diff=unified#hello', undefined);
+	});
+});

src/plus/integrations/providers/github/github.utils.ts

@@ -0,0 +1,36 @@
+// GitHub provider: github.ts pulls many dependencies through Container and some of them break the unit tests.
+// That's why this file has been created that can collect more simple functions which
+// don't require Container and can be tested.
+
+import { HostingIntegrationId } from '../../../../constants.integrations';
+import type { PullRequestUrlIdentity } from '../../../../git/models/pullRequest.utils';
+
+export function isMaybeGitHubPullRequestUrl(url: string): boolean {
+	if (url == null) return false;
+
+	return getGitHubPullRequestIdentityFromMaybeUrl(url) != null;
+}
+
+export function getGitHubPullRequestIdentityFromMaybeUrl(
+	search: string,
+): (PullRequestUrlIdentity & { provider: HostingIntegrationId.GitHub }) | undefined {
+	let ownerAndRepo: string | undefined = undefined;
+	let prNumber: string | undefined = undefined;
+
+	let match = search.match(/([^/]+\/[^/]+)\/(?:pull)\/(\d+)/); // with org and rep name
+	if (match != null) {
+		ownerAndRepo = match[1];
+		prNumber = match[2];
+	}
+
+	if (prNumber == null) {
+		match = search.match(/(?:\/|^)(?:pull)\/(\d+)/); // without repo name
+		if (match != null) {
+			prNumber = match[1];
+		}
+	}
+
+	return prNumber != null
+		? { ownerAndRepo: ownerAndRepo, prNumber: prNumber, provider: HostingIntegrationId.GitHub }
+		: undefined;
+}

src/plus/integrations/providers/github/models.ts

@@ -1,5 +1,4 @@
 import type { Endpoints } from '@octokit/types';
-import { HostingIntegrationId } from '../../../../constants.integrations';
 import { GitFileIndexStatus } from '../../../../git/models/file';
 import type { IssueLabel } from '../../../../git/models/issue';
 import { Issue, RepositoryAccessLevel } from '../../../../git/models/issue';
@@ -11,7 +10,6 @@ import {
 	PullRequestReviewState,
 	PullRequestStatusCheckRollupState,
 } from '../../../../git/models/pullRequest';
-import type { PullRequestUrlIdentity } from '../../../../git/models/pullRequest.utils';
 import type { Provider } from '../../../../git/models/remoteProvider';
 
 export interface GitHubBlame {
@@ -511,20 +509,3 @@ export function fromCommitFileStatus(
 	}
 	return undefined;
 }
-
-const prUrlRegex = /^(?:https?:\/\/)?(?:github\.com\/)?([^/]+\/[^/]+)\/pull\/(\d+)/i;
-
-export function isMaybeGitHubPullRequestUrl(url: string): boolean {
-	if (url == null) return false;
-
-	return prUrlRegex.test(url);
-}
-
-export function getGitHubPullRequestIdentityFromMaybeUrl(url: string): RequireSome<PullRequestUrlIdentity, 'provider'> {
-	if (url == null) return { prNumber: undefined, ownerAndRepo: undefined, provider: HostingIntegrationId.GitHub };
-
-	const match = prUrlRegex.exec(url);
-	if (match == null) return { prNumber: undefined, ownerAndRepo: undefined, provider: HostingIntegrationId.GitHub };
-
-	return { prNumber: match[2], ownerAndRepo: match[1], provider: HostingIntegrationId.GitHub };
-}

src/plus/integrations/providers/gitlab.ts

@@ -12,6 +12,7 @@ import type {
 	PullRequestState,
 	SearchedPullRequest,
 } from '../../../git/models/pullRequest';
+import type { PullRequestUrlIdentity } from '../../../git/models/pullRequest.utils';
 import type { RepositoryMetadata } from '../../../git/models/repositoryMetadata';
 import { log } from '../../../system/decorators/log';
 import { uniqueBy } from '../../../system/iterable';
@@ -22,6 +23,7 @@ import type {
 } from '../authentication/integrationAuthentication';
 import type { RepositoryDescriptor } from '../integration';
 import { HostingIntegration } from '../integration';
+import { getGitLabPullRequestIdentityFromMaybeUrl } from './gitlab/gitlab.utils';
 import { fromGitLabMergeRequestProvidersApi } from './gitlab/models';
 import type { ProviderPullRequest } from './models';
 import { ProviderPullRequestReviewState, providersMetadata, toSearchedIssue } from './models';
@@ -393,6 +395,10 @@ abstract class GitLabIntegrationBase<
 			username: currentUser.username || undefined,
 		};
 	}
+
+	override getPullRequestIdentityFromMaybeUrl(search: string): PullRequestUrlIdentity | undefined {
+		return getGitLabPullRequestIdentityFromMaybeUrl(search);
+	}
 }
 
 export class GitLabIntegration extends GitLabIntegrationBase<HostingIntegrationId.GitLab> {