Fixes get previous commit nav for GitHub

src/env/node/git/localGitProvider.ts

@@ -2659,14 +2659,14 @@ export class LocalGitProvider implements GitProvider, Disposable {
 		skip: number = 0,
 	): Promise<{ current: GitUri; next: GitUri | undefined; deleted?: boolean } | undefined> {
 		// If we have no ref (or staged ref) there is no next commit
-		if (ref == null || ref.length === 0) return undefined;
+		if (!ref) return undefined;
 
-		const path = this.getRelativePath(uri, repoPath);
+		const relativePath = this.getRelativePath(uri, repoPath);
 
 		if (GitRevision.isUncommittedStaged(ref)) {
 			return {
-				current: GitUri.fromFile(path, repoPath, ref),
-				next: GitUri.fromFile(path, repoPath, undefined),
+				current: GitUri.fromFile(relativePath, repoPath, ref),
+				next: GitUri.fromFile(relativePath, repoPath, undefined),
 			};
 		}
 
@@ -2677,22 +2677,22 @@ export class LocalGitProvider implements GitProvider, Disposable {
 				// If the file is staged, diff with the staged version
 				if (status.indexStatus != null) {
 					return {
-						current: GitUri.fromFile(path, repoPath, ref),
-						next: GitUri.fromFile(path, repoPath, GitRevision.uncommittedStaged),
+						current: GitUri.fromFile(relativePath, repoPath, ref),
+						next: GitUri.fromFile(relativePath, repoPath, GitRevision.uncommittedStaged),
 					};
 				}
 			}
 
 			return {
-				current: GitUri.fromFile(path, repoPath, ref),
-				next: GitUri.fromFile(path, repoPath, undefined),
+				current: GitUri.fromFile(relativePath, repoPath, ref),
+				next: GitUri.fromFile(relativePath, repoPath, undefined),
 			};
 		}
 
 		return {
 			current:
 				skip === 0
-					? GitUri.fromFile(path, repoPath, ref)
+					? GitUri.fromFile(relativePath, repoPath, ref)
 					: (await this.getNextUri(repoPath, uri, ref, skip - 1))!,
 			next: next,
 		};
@@ -2707,7 +2707,7 @@ export class LocalGitProvider implements GitProvider, Disposable {
 		// editorLine?: number
 	): Promise<GitUri | undefined> {
 		// If we have no ref (or staged ref) there is no next commit
-		if (ref == null || ref.length === 0 || GitRevision.isUncommittedStaged(ref)) return undefined;
+		if (!ref || GitRevision.isUncommittedStaged(ref)) return undefined;
 
 		let filters: GitDiffFilter[] | undefined;
 		if (ref === GitRevision.deletedOrMissing) {
@@ -2764,7 +2764,7 @@ export class LocalGitProvider implements GitProvider, Disposable {
 	): Promise<{ current: GitUri; previous: GitUri | undefined } | undefined> {
 		if (ref === GitRevision.deletedOrMissing) return undefined;
 
-		const path = this.getRelativePath(uri, repoPath);
+		const relativePath = this.getRelativePath(uri, repoPath);
 
 		// If we are at the working tree (i.e. no ref), we need to dig deeper to figure out where to go
 		if (!ref) {
@@ -2782,20 +2782,20 @@ export class LocalGitProvider implements GitProvider, Disposable {
 					if (skip === 0) {
 						// Diff working with staged
 						return {
-							current: GitUri.fromFile(path, repoPath, undefined),
-							previous: GitUri.fromFile(path, repoPath, GitRevision.uncommittedStaged),
+							current: GitUri.fromFile(relativePath, repoPath, undefined),
+							previous: GitUri.fromFile(relativePath, repoPath, GitRevision.uncommittedStaged),
 						};
 					}
 
 					return {
 						// Diff staged with HEAD (or prior if more skips)
-						current: GitUri.fromFile(path, repoPath, GitRevision.uncommittedStaged),
+						current: GitUri.fromFile(relativePath, repoPath, GitRevision.uncommittedStaged),
 						previous: await this.getPreviousUri(repoPath, uri, ref, skip - 1, undefined, firstParent),
 					};
 				} else if (status.workingTreeStatus != null) {
 					if (skip === 0) {
 						return {
-							current: GitUri.fromFile(path, repoPath, undefined),
+							current: GitUri.fromFile(relativePath, repoPath, undefined),
 							previous: await this.getPreviousUri(repoPath, uri, undefined, skip, undefined, firstParent),
 						};
 					}
@@ -2808,7 +2808,7 @@ export class LocalGitProvider implements GitProvider, Disposable {
 		else if (GitRevision.isUncommittedStaged(ref)) {
 			const current =
 				skip === 0
-					? GitUri.fromFile(path, repoPath, ref)
+					? GitUri.fromFile(relativePath, repoPath, ref)
 					: (await this.getPreviousUri(repoPath, uri, undefined, skip - 1, undefined, firstParent))!;
 			if (current == null || current.sha === GitRevision.deletedOrMissing) return undefined;
 
@@ -2821,7 +2821,7 @@ export class LocalGitProvider implements GitProvider, Disposable {
 		// If we are at a commit, diff commit with previous
 		const current =
 			skip === 0
-				? GitUri.fromFile(path, repoPath, ref)
+				? GitUri.fromFile(relativePath, repoPath, ref)
 				: (await this.getPreviousUri(repoPath, uri, ref, skip - 1, undefined, firstParent))!;
 		if (current == null || current.sha === GitRevision.deletedOrMissing) return undefined;
 
@@ -2841,7 +2841,7 @@ export class LocalGitProvider implements GitProvider, Disposable {
 	): Promise<{ current: GitUri; previous: GitUri | undefined; line: number } | undefined> {
 		if (ref === GitRevision.deletedOrMissing) return undefined;
 
-		let path = this.getRelativePath(uri, repoPath);
+		let relativePath = this.getRelativePath(uri, repoPath);
 
 		let previous;
 
@@ -2868,8 +2868,8 @@ export class LocalGitProvider implements GitProvider, Disposable {
 						if (status.indexStatus != null) {
 							// Diff working with staged
 							return {
-								current: GitUri.fromFile(path, repoPath, undefined),
-								previous: GitUri.fromFile(path, repoPath, GitRevision.uncommittedStaged),
+								current: GitUri.fromFile(relativePath, repoPath, undefined),
+								previous: GitUri.fromFile(relativePath, repoPath, GitRevision.uncommittedStaged),
 								line: editorLine,
 							};
 						}
@@ -2877,7 +2877,7 @@ export class LocalGitProvider implements GitProvider, Disposable {
 
 					// Diff working with HEAD (or prior if more skips)
 					return {
-						current: GitUri.fromFile(path, repoPath, undefined),
+						current: GitUri.fromFile(relativePath, repoPath, undefined),
 						previous: await this.getPreviousUri(repoPath, uri, undefined, skip, editorLine),
 						line: editorLine,
 					};
@@ -2899,19 +2899,19 @@ export class LocalGitProvider implements GitProvider, Disposable {
 			// If line is committed, diff with line ref with previous
 			else {
 				ref = blameLine.commit.sha;
-				path = blameLine.commit.file?.path ?? blameLine.commit.file?.originalPath ?? path;
-				uri = this.getAbsoluteUri(path, repoPath);
+				relativePath = blameLine.commit.file?.path ?? blameLine.commit.file?.originalPath ?? relativePath;
+				uri = this.getAbsoluteUri(relativePath, repoPath);
 				editorLine = blameLine.line.from.line - 1;
 
 				if (skip === 0 && blameLine.commit.file?.previousSha) {
-					previous = GitUri.fromFile(path, repoPath, blameLine.commit.file.previousSha);
+					previous = GitUri.fromFile(relativePath, repoPath, blameLine.commit.file.previousSha);
 				}
 			}
 		} else {
 			if (GitRevision.isUncommittedStaged(ref)) {
 				const current =
 					skip === 0
-						? GitUri.fromFile(path, repoPath, ref)
+						? GitUri.fromFile(relativePath, repoPath, ref)
 						: (await this.getPreviousUri(repoPath, uri, undefined, skip - 1, editorLine))!;
 				if (current.sha === GitRevision.deletedOrMissing) return undefined;
 
@@ -2928,18 +2928,18 @@ export class LocalGitProvider implements GitProvider, Disposable {
 
 			// Diff with line ref with previous
 			ref = blameLine.commit.sha;
-			path = blameLine.commit.file?.path ?? blameLine.commit.file?.originalPath ?? path;
-			uri = this.getAbsoluteUri(path, repoPath);
+			relativePath = blameLine.commit.file?.path ?? blameLine.commit.file?.originalPath ?? relativePath;
+			uri = this.getAbsoluteUri(relativePath, repoPath);
 			editorLine = blameLine.line.from.line - 1;
 
 			if (skip === 0 && blameLine.commit.file?.previousSha) {
-				previous = GitUri.fromFile(path, repoPath, blameLine.commit.file.previousSha);
+				previous = GitUri.fromFile(relativePath, repoPath, blameLine.commit.file.previousSha);
 			}
 		}
 
 		const current =
 			skip === 0
-				? GitUri.fromFile(path, repoPath, ref)
+				? GitUri.fromFile(relativePath, repoPath, ref)
 				: (await this.getPreviousUri(repoPath, uri, ref, skip - 1, editorLine))!;
 		if (current.sha === GitRevision.deletedOrMissing) return undefined;
 
@@ -2967,12 +2967,12 @@ export class LocalGitProvider implements GitProvider, Disposable {
 			ref = undefined;
 		}
 
-		const path = this.getRelativePath(uri, repoPath);
+		const relativePath = this.getRelativePath(uri, repoPath);
 
 		// TODO: Add caching
 		let data;
 		try {
-			data = await this.git.log__file(repoPath, path, ref, {
+			data = await this.git.log__file(repoPath, relativePath, ref, {
 				argsOrFormat: GitLogParser.simpleFormat,
 				fileMode: 'simple',
 				firstParent: firstParent,
@@ -2987,14 +2987,14 @@ export class LocalGitProvider implements GitProvider, Disposable {
 				if (ref == null) {
 					const status = await this.getStatusForFile(repoPath, uri);
 					if (status?.indexStatus != null) {
-						return GitUri.fromFile(path, repoPath, GitRevision.uncommittedStaged);
+						return GitUri.fromFile(relativePath, repoPath, GitRevision.uncommittedStaged);
 					}
 				}
 
-				ref = await this.git.log__file_recent(repoPath, path, {
+				ref = await this.git.log__file_recent(repoPath, relativePath, {
 					ordering: this.container.config.advanced.commitOrdering,
 				});
-				return GitUri.fromFile(path, repoPath, ref ?? GitRevision.deletedOrMissing);
+				return GitUri.fromFile(relativePath, repoPath, ref ?? GitRevision.deletedOrMissing);
 			}
 
 			Logger.error(ex, cc);
@@ -3006,7 +3006,7 @@ export class LocalGitProvider implements GitProvider, Disposable {
 		// If the previous ref matches the ref we asked for assume we are at the end of the history
 		if (ref != null && ref === previousRef) return undefined;
 
-		return GitUri.fromFile(file ?? path, repoPath, previousRef ?? GitRevision.deletedOrMissing);
+		return GitUri.fromFile(file ?? relativePath, repoPath, previousRef ?? GitRevision.deletedOrMissing);
 	}
 
 	@log()

src/premium/github/github.ts

@@ -2,6 +2,7 @@ import { Octokit } from '@octokit/core';
 import { GraphqlResponseError } from '@octokit/graphql';
 import { RequestError } from '@octokit/request-error';
 import type { Endpoints, OctokitResponse, RequestParameters } from '@octokit/types';
+import { window } from 'vscode';
 import { fetch } from '@env/fetch';
 import { isWeb } from '@env/platform';
 import {
@@ -14,6 +15,7 @@ import { PagedResult } from '../../git/gitProvider';
 import {
 	type DefaultBranch,
 	GitFileIndexStatus,
+	GitRevision,
 	type GitUser,
 	type IssueOrPullRequest,
 	type IssueOrPullRequestType,
@@ -700,6 +702,9 @@ export class GitHubApi {
 		ref: string,
 		path: string,
 	): Promise<(GitHubCommit & { viewer?: string }) | undefined> {
+		if (GitRevision.isSha(ref)) return this.getCommit(token, owner, repo, ref);
+
+		// TODO: optimize this -- only need to get the sha for the ref
 		const results = await this.getCommits(token, owner, repo, ref, { limit: 1, path: path });
 		if (results.values.length === 0) return undefined;
 
@@ -1079,6 +1084,83 @@ export class GitHubApi {
 		}
 	}
 
+	@debug<GitHubApi['getCommitRefs']>({ args: { 0: '<token>' } })
+	async getCommitRefs(
+		token: string,
+		owner: string,
+		repo: string,
+		ref: string,
+		options?: {
+			after?: string;
+			before?: string;
+			limit?: number;
+			path?: string;
+			since?: Date;
+			until?: Date;
+		},
+	): Promise<string[]> {
+		const cc = Logger.getCorrelationContext();
+
+		interface QueryResult {
+			repository:
+				| {
+						object:
+							| {
+									history: {
+										nodes: { oid: string }[];
+									};
+							  }
+							| null
+							| undefined;
+				  }
+				| null
+				| undefined;
+		}
+
+		try {
+			const query = `query getCommitRefs(
+	$owner: String!
+	$repo: String!
+	$ref: String!
+	$after: String
+	$before: String
+	$limit: Int = 1
+	$path: String
+	$since: GitTimestamp
+	$until: GitTimestamp
+) {
+	repository(name: $repo, owner: $owner) {
+		object(expression: $ref) {
+			... on Commit {
+				history(first: $limit, path: $path, since: $since, until: $until, after: $after, before: $before) {
+					nodes { oid }
+				}
+			}
+		}
+	}
+}`;
+
+			const rsp = await this.graphql<QueryResult>(token, query, {
+				owner: owner,
+				repo: repo,
+				ref: ref,
+				path: options?.path,
+				limit: Math.max(1, options?.limit ?? 1),
+				after: options?.after,
+				before: options?.before,
+				since: options?.since?.toISOString(),
+				until: options?.until?.toISOString(),
+			});
+			const history = rsp?.repository?.object?.history;
+			if (history == null) return [];
+
+			return history.nodes.map(n => n.oid);
+		} catch (ex) {
+			debugger;
+			return this.handleRequestError<string[]>(ex, cc, []);
+		}
+	}
+
 	@debug<GitHubApi['getContributors']>({ args: { 0: '<token>' } })
 	async getContributors(token: string, owner: string, repo: string): Promise<GitHubContributor[]> {
 		const cc = Logger.getCorrelationContext();
@@ -1410,6 +1492,7 @@ export class GitHubApi {
 				}
 			}
 
+			void window.showErrorMessage(`Unable to complete GitHub request: ${ex.message}`);
 			throw ex;
 		}
 	}
@@ -1444,6 +1527,7 @@ export class GitHubApi {
 				}
 			}
 
+			void window.showErrorMessage(`Unable to complete GitHub request: ${ex.message}`);
 			throw ex;
 		}
 	}

src/premium/github/githubGitProvider.ts

@@ -1254,6 +1254,7 @@ export class GitHubGitProvider implements GitProvider, Disposable {
 		_search: SearchPattern,
 		_options?: { limit?: number; ordering?: string | null; skip?: number },
 	): Promise<GitLog | undefined> {
+		// TODO@eamodio try implementing with the commit search api
 		return undefined;
 	}
 
@@ -1640,23 +1641,71 @@ export class GitHubGitProvider implements GitProvider, Disposable {
 
 	@log()
 	async getNextDiffUris(
-		_repoPath: string,
-		_uri: Uri,
-		_ref: string | undefined,
-		_skip: number = 0,
+		repoPath: string,
+		uri: Uri,
+		ref: string | undefined,
+		skip: number = 0,
 	): Promise<{ current: GitUri; next: GitUri | undefined; deleted?: boolean } | undefined> {
-		return undefined;
+		// If we have no ref (or staged ref) there is no next commit
+		if (!ref) return undefined;
+
+		const relativePath = this.getRelativePath(uri, repoPath);
+
+		return {
+			current:
+				skip === 0
+					? GitUri.fromFile(relativePath, repoPath, ref)
+					: (await this.getNextUri(repoPath, uri, ref, skip - 1))!,
+			next: await this.getNextUri(repoPath, uri, ref, skip),
+		};
 	}
 
 	@log()
 	async getNextUri(
 		_repoPath: string,
 		_uri: Uri,
-		_ref?: string,
+		ref?: string,
 		_skip: number = 0,
 		// editorLine?: number
 	): Promise<GitUri | undefined> {
+		// If we have no ref (or staged ref) there is no next commit
+		if (!ref || GitRevision.isUncommittedStaged(ref)) return undefined;
 		return undefined;
+
+		// const cc = Logger.getCorrelationContext();
+
+		// // const relativePath = this.getRelativePath(uri, repoPath);
+
+		// try {
+		// 	const context = await this.ensureRepositoryContext(repoPath);
+		// 	if (context == null) return undefined;
+		// 	const { metadata, github, remotehub, session } = context;
+
+		// 	const relativePath = this.getRelativePath(uri, remotehub.getProviderRootUri(uri));
+
+		// 	const refs = await github.getCommitRefs(
+		// 		session.accessToken,
+		// 		metadata.repo.owner,
+		// 		metadata.repo.name,
+		// 		ref ?? 'HEAD',
+		// 		{
+		// 			path: relativePath,
+		// 			limit: skip + 2,
+		// 			before: `${ref} 0`,
+		// 		},
+		// 	);
+
+		// 	const nextRef = refs[skip];
+		// 	if (nextRef == null) return undefined;
+
+		// 	const next = await this.getBestRevisionUri(repoPath, relativePath, nextRef);
+		// 	return new GitUri(next);
+		// } catch (ex) {
+		// 	Logger.error(ex, cc);
+		// 	debugger;
+
+		// 	throw ex;
+		// }
 	}
 
 	@log()
@@ -1664,17 +1713,24 @@ export class GitHubGitProvider implements GitProvider, Disposable {
 		repoPath: string,
 		uri: Uri,
 		ref: string | undefined,
-		_skip: number = 0,
+		skip: number = 0,
 		firstParent: boolean = false,
 	): Promise<{ current: GitUri; previous: GitUri | undefined } | undefined> {
 		if (ref === GitRevision.deletedOrMissing) return undefined;
 
-		const path = this.getRelativePath(uri, repoPath);
+		const relativePath = this.getRelativePath(uri, repoPath);
+
+		// If we are at a commit, diff commit with previous
+		const current =
+			skip === 0
+				? GitUri.fromFile(relativePath, repoPath, ref)
+				: (await this.getPreviousUri(repoPath, uri, ref, skip - 1, undefined, firstParent))!;
+		if (current == null || current.sha === GitRevision.deletedOrMissing) return undefined;
+
 		return {
-			current: GitUri.fromFile(path, repoPath, undefined),
-			previous: await this.getPreviousUri(repoPath, uri, ref, 0, undefined, firstParent),
+			current: current,
+			previous: await this.getPreviousUri(repoPath, uri, ref, skip, undefined, firstParent),
 		};
-		// return undefined;
 	}
 
 	@log()
@@ -1693,14 +1749,50 @@ export class GitHubGitProvider implements GitProvider, Disposable {
 		repoPath: string,
 		uri: Uri,
 		ref?: string,
-		_skip: number = 0,
+		skip: number = 0,
 		_editorLine?: number,
 		_firstParent: boolean = false,
 	): Promise<GitUri | undefined> {
 		if (ref === GitRevision.deletedOrMissing) return undefined;
 
-		const commit = await this.getCommitForFile(repoPath, uri, { ref: `${ref ?? 'HEAD'}^` });
-		return commit?.getGitUri();
+		const cc = Logger.getCorrelationContext();
+
+		if (ref === GitRevision.uncommitted) {
+			ref = undefined;
+		}
+
+		try {
+			const context = await this.ensureRepositoryContext(repoPath);
+			if (context == null) return undefined;
+			const { metadata, github, remotehub, session } = context;
+
+			const relativePath = this.getRelativePath(uri, remotehub.getProviderRootUri(uri));
+
+			const offset = ref != null ? 1 : 0;
+
+			const refs = await github.getCommitRefs(
+				session.accessToken,
+				metadata.repo.owner,
+				metadata.repo.name,
+				ref ?? 'HEAD',
+				{
+					path: relativePath,
+					limit: offset + skip + 1,
+				},
+			);
+
+			const previous = await this.getBestRevisionUri(
+				repoPath,
+				relativePath,
+				refs[offset + skip] ?? GitRevision.deletedOrMissing,
+			);
+			return new GitUri(previous);
+		} catch (ex) {
+			Logger.error(ex, cc);
+			debugger;
+
+			throw ex;
+		}
 	}
 
 	@log()