Fixes #2300 better support for pr scheme uris

- Ensures we handle the authority when parsing uris for repo matching - Prioritizes multiple provider matches by open repositories
gitkraken · Nov 3, 2022 · ffbac6f · ffbac6f
1 parent ee1fbe4
commit ffbac6f
Show file tree

Hide file tree

Showing 5 changed files with 45 additions and 100 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,8 +8,10 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p
 
 ### Fixed
 
+- Fixes [#2300](https://github.com/gitkraken/vscode-gitlens/issues/2300) - extra non-functional toolbar buttons when viewing PR diffs in VSCode web
 - Fixes [#2281](https://github.com/gitkraken/vscode-gitlens/issues/2281) - Push and Pull buttons missing from the commits view w/ integrations disabled
 - Fixes [#2276](https://github.com/gitkraken/vscode-gitlens/issues/2276) - Search commit by Sha not working in Gitlens side bar
+- Fixes issues with PR uris (scheme: `pr`) from not working properly, especially with virtual repositories
 
 ## [13.0.3] - 2022-10-20
 

diff --git a/src/git/gitProviderService.ts b/src/git/gitProviderService.ts
@@ -804,8 +804,6 @@ export class GitProviderService implements Disposable {
 		this._providers.forEach(p => p.updateContext?.());
 	}
 
-	// private _pathToProvider = new Map<string, GitProviderResult>();
-
 	private getProvider(repoPath: string | Uri): GitProviderResult {
 		if (repoPath == null || (typeof repoPath !== 'string' && !this.supportedSchemes.has(repoPath.scheme))) {
 			debugger;
@@ -819,48 +817,30 @@ export class GitProviderService implements Disposable {
 			({ scheme } = repoPath);
 		}
 
-		// const key = repoPath.toString();
-		// let providerResult = this._pathToProvider.get(key);
-		// if (providerResult != null) return providerResult;
+		const possibleResults = new Set<GitProviderResult>();
 
 		for (const provider of this._providers.values()) {
 			const path = provider.canHandlePathOrUri(scheme, repoPath);
 			if (path == null) continue;
 
-			const providerResult: GitProviderResult = { provider: provider, path: path };
-			// this._pathToProvider.set(key, providerResult);
-			return providerResult;
-		}
-
-		debugger;
-		throw new ProviderNotFoundError(repoPath);
-
-		// let id = !isWeb ? GitProviderId.Git : undefined;
-		// if (typeof repoPath !== 'string' && repoPath.scheme === Schemes.Virtual) {
-		// 	if (repoPath.authority.startsWith('github')) {
-		// 		id = GitProviderId.GitHub;
-		// 	} else {
-		// 		throw new ProviderNotFoundError(repoPath);
-		// 	}
-		// }
-		// if (id == null) throw new ProviderNotFoundError(repoPath);
-
-		// const provider = this._providers.get(id);
-		// if (provider == null) throw new ProviderNotFoundError(repoPath);
-
-		// switch (id) {
-		// 	case GitProviderId.Git:
-		// 		return {
-		// 			provider: provider,
-		// 			path: typeof repoPath === 'string' ? repoPath : repoPath.fsPath,
-		// 		};
-
-		// 	default:
-		// 		return {
-		// 			provider: provider,
-		// 			path: typeof repoPath === 'string' ? repoPath : repoPath.toString(),
-		// 		};
-		// }
+			possibleResults.add({ provider: provider, path: path });
+		}
+
+		if (possibleResults.size === 0) {
+			debugger;
+			throw new ProviderNotFoundError(repoPath);
+		}
+
+		// Prefer the provider with an open repository
+		if (possibleResults.size > 1) {
+			for (const result of possibleResults) {
+				if (this.hasOpenRepositories(result.provider.descriptor.id)) {
+					return result;
+				}
+			}
+		}
+
+		return first(possibleResults)!;
 	}
 
 	getAbsoluteUri(pathOrUri: string | Uri, base?: string | Uri): Uri {

diff --git a/src/git/gitUri.ts b/src/git/gitUri.ts
@@ -282,7 +282,7 @@ export class GitUri extends (Uri as any as UriEx) {
 
 				const commitish: GitCommitish = {
 					fileName: data.path,
-					repoPath: repository?.path,
+					repoPath: repository.path,
 					sha: ref,
 				};
 				return new GitUri(uri, commitish);
@@ -306,26 +306,15 @@ export class GitUri extends (Uri as any as UriEx) {
 			} catch {}
 
 			if (data?.fileName) {
-				const repository = await Container.instance.git.getOrOpenRepository(Uri.file(data.fileName));
+				const repository = await Container.instance.git.getOrOpenRepository(uri);
 				if (repository == null) {
 					debugger;
 					throw new Error(`Unable to find repository for uri=${Uri.file(data.fileName).toString(true)}`);
 				}
 
-				let repoPath: string | undefined = normalizePath(uri.fsPath);
-				if (repoPath.endsWith(data.fileName)) {
-					repoPath = repoPath.substr(0, repoPath.length - data.fileName.length - 1);
-				} else {
-					repoPath = (await Container.instance.git.getOrOpenRepository(uri))?.path;
-					if (!repoPath) {
-						debugger;
-						throw new Error(`Unable to find repository for uri=${uri.toString(true)}`);
-					}
-				}
-
 				const commitish: GitCommitish = {
 					fileName: data.fileName,
-					repoPath: repoPath,
+					repoPath: repository.path,
 					sha: data.isBase ? data.baseCommit : data.headCommit,
 				};
 				return new GitUri(uri, commitish);

diff --git a/src/repositories.ts b/src/repositories.ts
@@ -37,18 +37,19 @@ export function normalizeRepoUri(uri: Uri): { path: string; ignoreCase: boolean
 		case Schemes.GitHub: {
 			path = uri.path;
 			if (path.charCodeAt(path.length - 1) === slash) {
-				path = path.slice(0, -1);
+				path = path.slice(1, -1);
+			} else {
+				path = path.slice(1);
 			}
 
 			// TODO@eamodio Revisit this, as we can't strip off the authority details (e.g. metadata) ultimately (since you in theory could have a workspace with more than 1 virtual repo which are the same except for the authority)
 			const authority = uri.authority?.split('+', 1)[0];
-			return { path: authority ? `${authority}${path}` : path.slice(1), ignoreCase: false };
+			return { path: authority ? `${authority}/${path}` : path, ignoreCase: false };
 		}
 		case Schemes.Vsls:
 		case Schemes.VslsScc:
 			// Check if this is a root live share folder, if so add the required prefix (required to match repos correctly)
 			path = addVslsPrefixIfNeeded(uri.path);
-
 			if (path.charCodeAt(path.length - 1) === slash) {
 				path = path.slice(1, -1);
 			} else {
@@ -57,6 +58,21 @@ export function normalizeRepoUri(uri: Uri): { path: string; ignoreCase: boolean
 
 			return { path: path, ignoreCase: false };
 
+		case Schemes.PRs: {
+			path = uri.path;
+			if (path.charCodeAt(path.length - 1) === slash) {
+				path = path.slice(1, -1);
+			} else {
+				path = path.slice(1);
+			}
+
+			const authority = uri.authority?.split('+', 1)[0];
+			if (authority === Schemes.GitHub) {
+				return { path: authority ? `${authority}/${path}` : path, ignoreCase: false };
+			}
+
+			return { path: path, ignoreCase: !isLinux };
+		}
 		default:
 			path = uri.path;
 			if (path.charCodeAt(path.length - 1) === slash) {

diff --git a/src/system/trie.ts b/src/system/trie.ts
@@ -1,54 +1,12 @@
 import type { Uri } from 'vscode';
 import { isLinux } from '@env/platform';
-import { Schemes } from '../constants';
 import { filterMap } from './iterable';
 import { normalizePath as _normalizePath } from './path';
 // TODO@eamodio don't import from string here since it will break the tests because of ESM dependencies
 // import { CharCode } from './string';
 
 const slash = 47; //CharCode.Slash;
 
-function normalizeUri(uri: Uri): { path: string; ignoreCase: boolean } {
-	let path;
-	switch (uri.scheme.toLowerCase()) {
-		case Schemes.File:
-			path = normalizePath(uri.fsPath);
-			return { path: path, ignoreCase: !isLinux };
-
-		case Schemes.Git:
-			path = normalizePath(uri.fsPath);
-			// TODO@eamodio parse the ref out of the query
-			return { path: path, ignoreCase: !isLinux };
-
-		case Schemes.GitLens:
-			path = uri.path;
-			if (path.charCodeAt(path.length - 1) === slash) {
-				path = path.slice(0, -1);
-			}
-
-			if (!isLinux) {
-				path = path.toLowerCase();
-			}
-
-			return { path: uri.authority ? `${uri.authority}${path}` : path.slice(1), ignoreCase: false };
-
-		case Schemes.Virtual:
-		case Schemes.GitHub:
-			path = uri.path;
-			if (path.charCodeAt(path.length - 1) === slash) {
-				path = path.slice(0, -1);
-			}
-			return { path: uri.authority ? `${uri.authority}${path}` : path.slice(1), ignoreCase: false };
-
-		default:
-			path = uri.path;
-			if (path.charCodeAt(path.length - 1) === slash) {
-				path = path.slice(0, -1);
-			}
-			return { path: path.slice(1), ignoreCase: false };
-	}
-}
-
 function normalizePath(path: string): string {
 	path = _normalizePath(path);
 	if (path.charCodeAt(0) === slash) {
@@ -63,7 +21,7 @@ export type UriEntry<T> = PathEntry<T>;
 export class UriEntryTrie<T> {
 	private readonly trie: PathEntryTrie<T>;
 
-	constructor(private readonly normalize: (uri: Uri) => { path: string; ignoreCase: boolean } = normalizeUri) {
+	constructor(private readonly normalize: (uri: Uri) => { path: string; ignoreCase: boolean }) {
 		this.trie = new PathEntryTrie<T>();
 	}
 
@@ -112,7 +70,7 @@ export class UriEntryTrie<T> {
 export class UriTrie<T> {
 	private readonly trie: PathTrie<T>;
 
-	constructor(private readonly normalize: (uri: Uri) => { path: string; ignoreCase: boolean } = normalizeUri) {
+	constructor(private readonly normalize: (uri: Uri) => { path: string; ignoreCase: boolean }) {
 		this.trie = new PathTrie<T>();
 	}