Allow commenting on files from earlier commits of a pull request

src/commands.ts

@@ -396,6 +396,34 @@ export function registerCommands(
 		),
 	);
 
+	// Used from the tooltip of a file that has changed since the commit being viewed, to get to the
+	// version of that file that can actually be commented on.
+	context.subscriptions.push(
+		vscode.commands.registerCommand(
+			'pr.openCurrentFileChange',
+			async (args: { rootPath: string; fileName: string } | undefined) => {
+				if (!args?.fileName) {
+					return;
+				}
+
+				const reviewManager = reviewsManager.reviewManagers.find(
+					manager => manager.repository.rootUri.path === args.rootPath,
+				);
+				const fileChange = reviewManager?.reviewModel.localFileChanges.find(
+					change => change.fileName === args.fileName,
+				);
+				if (!fileChange) {
+					return vscode.window.showInformationMessage(
+						vscode.l10n.t('{0} is not part of the changes of the checked out pull request.', args.fileName),
+					);
+				}
+
+				await fileChange.resolve();
+				return vscode.commands.executeCommand(fileChange.command.command, ...(fileChange.command.arguments ?? []));
+			},
+		),
+	);
+
 	context.subscriptions.push(
 		vscode.commands.registerCommand('pr.deleteLocalBranch', async (e: PRNode) => {
 			const folderManager = reposManager.getManagerForIssueModel(e.pullRequestModel);

src/test/view/reviewCommentController.test.ts

@@ -356,6 +356,89 @@ describe('ReviewCommentController', function () {
 		});
 	});
 
+	describe('provideCommentingRanges', function () {
+		const fileName = 'data/products.json';
+		const commitSha = '1111111111111111111111111111111111111111';
+		const patch = ['@@ -1,3 +1,4 @@', ' line one', ' line two', '+added line', ' line three'].join('\n');
+
+		function createController(): TestReviewCommentController {
+			return new TestReviewCommentController(
+				reviewManager,
+				manager,
+				repository,
+				new ReviewModel(),
+				gitApiImpl,
+				telemetry,
+			);
+		}
+
+		/**
+		 * A document as opened from the Commits tree: a `review` uri holding the sha of that commit.
+		 */
+		function createCommitDocument(commit: string): vscode.TextDocument {
+			const uri = toReviewUri(
+				vscode.Uri.parse(`commit~11111111/${fileName}`),
+				fileName,
+				undefined,
+				commit,
+				true,
+				{ base: false },
+				repository.rootUri,
+			);
+			return { uri, lineCount: 4 } as vscode.TextDocument;
+		}
+
+		function stubDiffBetween(changedSinceCommit: string) {
+			const diffBetween = sinon.stub(repository, 'diffBetween');
+			// Whether the file changed between the commit being viewed and the head of the PR.
+			diffBetween.withArgs(commitSha, activePullRequest.head!.sha, fileName).returns(Promise.resolve(changedSinceCommit));
+			// The diff the commenting ranges are taken from.
+			diffBetween.withArgs(activePullRequest.base.sha, commitSha, fileName).returns(Promise.resolve(patch));
+			return diffBetween;
+		}
+
+		it('provides ranges for a file that has not changed since the commit', async function () {
+			stubDiffBetween('');
+
+			const result = await createController().provideCommentingRanges(
+				createCommitDocument(commitSha),
+				new vscode.CancellationTokenSource().token,
+			) as { enableFileComments: boolean; ranges?: vscode.Range[] };
+
+			assert.ok(result, 'ranges should be provided for a file that can be commented on');
+			assert.strictEqual(result.enableFileComments, true);
+			assert.strictEqual(result.ranges?.length, 1);
+			assert.strictEqual(result.ranges![0].start.line, 0);
+			assert.strictEqual(result.ranges![0].end.line, 3);
+		});
+
+		it('does not allow commenting on a file that changed after the commit', async function () {
+			// Comments always go to the head of the pull request, so the line numbers of this
+			// document can no longer be trusted to point at the same code.
+			stubDiffBetween(patch);
+
+			const result = await createController().provideCommentingRanges(
+				createCommitDocument(commitSha),
+				new vscode.CancellationTokenSource().token,
+			) as { enableFileComments: boolean; ranges?: vscode.Range[] };
+
+			assert.ok(result);
+			assert.strictEqual(result.enableFileComments, false);
+			assert.deepStrictEqual(result.ranges, []);
+		});
+
+		it('leaves a document of the head commit to the regular code path', async function () {
+			const diffBetween = stubDiffBetween('');
+
+			await createController().provideCommentingRanges(
+				createCommitDocument(activePullRequest.head!.sha),
+				new vscode.CancellationTokenSource().token,
+			);
+
+			assert.ok(diffBetween.notCalled, 'the head commit is handled by matching against the local file changes');
+		});
+	});
+
 	describe('_findMatchingThread', function () {
 		it('returns the moved thread when an existing workspace thread becomes outdated', function () {
 			const fileName = 'data/products.json';

src/view/reviewCommentController.ts

@@ -15,6 +15,7 @@ import { ReviewManager } from './reviewManager';
 import { ReviewModel } from './reviewModel';
 import { DiffSide, IReviewThread, SubjectType } from '../common/comment';
 import { getCommentingRanges } from '../common/commentingRanges';
+import { parsePatch } from '../common/diffHunk';
 import { mapNewPositionToOld, mapOldPositionToNew } from '../common/diffPositionMapping';
 import { commands, contexts } from '../common/executeCommands';
 import { GitChangeType, InMemFileChange } from '../common/file';
@@ -292,13 +293,20 @@ export class ReviewCommentController extends CommentControllerBase implements Co
 					}
 
 					const index = await arrayFindIndexAsync(this._pendingCommentThreadAdds, async t => {
-						const fileName = this._folderRepoManager.gitRelativeRootPath(t.uri.path);
+						const prCommitDocument = this.prCommitDocument(t.uri);
+						const fileName = prCommitDocument?.fileName ?? this._folderRepoManager.gitRelativeRootPath(t.uri.path);
 						if (fileName !== thread.path) {
 							return false;
 						}
 
-						const diff = await this.getContentDiff(t.uri, fileName);
-						const line = t.range ? mapNewPositionToOld(diff, t.range.end.line) : 0;
+						let line = 0;
+						if (t.range) {
+							if (prCommitDocument) {
+								line = t.range.end.line;
+							} else {
+								line = mapNewPositionToOld(await this.getContentDiff(t.uri, fileName), t.range.end.line);
+							}
+						}
 						const sameLine = line + 1 === thread.endLine;
 						return sameLine;
 					});
@@ -513,6 +521,11 @@ export class ReviewCommentController extends CommentControllerBase implements Co
 				Logger.debug('Found matched file for commenting ranges.', ReviewCommentController.ID);
 				return { ranges: getCommentingRanges(await matchedFile.changeModel.diffHunks(), query.base, ReviewCommentController.ID), enableFileComments: true };
 			}
+
+			const prCommitDocument = this.prCommitDocument(document.uri);
+			if (prCommitDocument) {
+				return this.provideCommitCommentingRanges(prCommitDocument.commit, prCommitDocument.fileName);
+			}
 		}
 
 		const bestRepoForFile = getRepositoryForFile(this._gitApi, document.uri);
@@ -571,6 +584,78 @@ export class ReviewCommentController extends CommentControllerBase implements Co
 
 	// #endregion
 
+	/**
+	 * Commenting ranges for a file shown as of one of the pull request's commits. The ranges are
+	 * taken from the diff of the pull request base against that commit, so the line numbers are the
+	 * ones of the document being shown, and every offered line is part of the pull request's diff.
+	 */
+	private async provideCommitCommentingRanges(commit: string, fileName: string): Promise<{ enableFileComments: boolean; ranges?: vscode.Range[] } | undefined> {
+		const activePullRequest = this._folderRepoManager.activePullRequest;
+		if (!activePullRequest) {
+			return;
+		}
+
+		if (!(await this.isUnchangedSinceCommit(commit, fileName))) {
+			Logger.debug(`No commenting ranges: ${fileName} has changed since commit ${commit.substring(0, 8)}.`, ReviewCommentController.ID);
+			return { ranges: [], enableFileComments: false };
+		}
+
+		try {
+			const patch = await this._repository.diffBetween(activePullRequest.base.sha, commit, fileName);
+			const ranges = getCommentingRanges(parsePatch(patch), false, ReviewCommentController.ID);
+			Logger.debug(`Providing ${ranges.length} commenting ranges for ${fileName} at commit ${commit.substring(0, 8)}.`, ReviewCommentController.ID);
+			return { ranges, enableFileComments: true };
+		} catch (e) {
+			Logger.error(`Failed to get commenting ranges for ${fileName} at commit ${commit.substring(0, 8)}: ${formatError(e)}`, ReviewCommentController.ID);
+			return;
+		}
+	}
+
+	/**
+	 * Detects a `review` document that shows a file as of one of the pull request's commits, as
+	 * opened from the Commits node of the tree. Returns `undefined` for every other document,
+	 * including the diff of the head commit, which is already handled by the regular code paths.
+	 */
+	private prCommitDocument(uri: vscode.Uri): { commit: string; fileName: string } | undefined {
+		if (uri.scheme !== Schemes.Review || !uri.query) {
+			return;
+		}
+
+		let query: ReviewUriParams;
+		try {
+			query = fromReviewUri(uri.query);
+		} catch (e) {
+			return;
+		}
+
+		const headSha = this._folderRepoManager.activePullRequest?.head?.sha;
+		if (!query.commit || !query.path || query.base || !headSha || query.commit === headSha) {
+			return;
+		}
+
+		return { commit: query.commit, fileName: query.path };
+	}
+
+	/**
+	 * Comments are always created against the head of the pull request, so a comment made while
+	 * looking at an older commit is only unambiguous when the file hasn't been touched since. When
+	 * that holds, the line numbers of the document match the head and need no translation.
+	 */
+	private async isUnchangedSinceCommit(commit: string, fileName: string): Promise<boolean> {
+		const headSha = this._folderRepoManager.activePullRequest?.head?.sha;
+		if (!headSha) {
+			return false;
+		}
+
+		try {
+			const diff = await this._repository.diffBetween(commit, headSha, fileName);
+			return diff.length === 0;
+		} catch (e) {
+			Logger.warn(`Unable to tell whether ${fileName} changed between ${commit} and ${headSha}: ${formatError(e)}`, ReviewCommentController.ID);
+			return false;
+		}
+	}
+
 	private async getContentDiff(uri: vscode.Uri, fileName: string, retry: boolean = true): Promise<string> {
 		const matchedEditor = vscode.window.visibleTextEditors.find(
 			editor => editor.document.uri.toString() === uri.toString(),
@@ -686,7 +771,8 @@ export class ReviewCommentController extends CommentControllerBase implements Co
 		try {
 			temporaryCommentId = await this.optimisticallyAddComment(thread, input, true);
 			if (!hasExistingComments) {
-				const fileName = this._folderRepoManager.gitRelativeRootPath(thread.uri.path);
+				const prCommitDocument = this.prCommitDocument(thread.uri);
+				const fileName = prCommitDocument?.fileName ?? this._folderRepoManager.gitRelativeRootPath(thread.uri.path);
 				const side = this.getCommentSide(thread);
 				this._pendingCommentThreadAdds.push(thread);
 
@@ -695,7 +781,12 @@ export class ReviewCommentController extends CommentControllerBase implements Co
 				let startLine: number | undefined = undefined;
 				let endLine: number | undefined = undefined;
 				if (thread.range) {
-					if (side === DiffSide.RIGHT) {
+					if (prCommitDocument) {
+						// Commenting is only offered on a commit's document when the file is unchanged
+						// between that commit and the head, so the lines already match the head diff.
+						startLine = thread.range.start.line;
+						endLine = thread.range.end.line;
+					} else if (side === DiffSide.RIGHT) {
 						const diff = await this.getContentDiff(thread.uri, fileName);
 						startLine = mapNewPositionToOld(diff, thread.range.start.line);
 						endLine = mapNewPositionToOld(diff, thread.range.end.line);
@@ -797,7 +888,8 @@ export class ReviewCommentController extends CommentControllerBase implements Co
 
 		try {
 			if (!hasExistingComments) {
-				const fileName = this._folderRepoManager.gitRelativeRootPath(thread.uri.path);
+				const prCommitDocument = this.prCommitDocument(thread.uri);
+				const fileName = prCommitDocument?.fileName ?? this._folderRepoManager.gitRelativeRootPath(thread.uri.path);
 				this._pendingCommentThreadAdds.push(thread);
 				const side = this.getCommentSide(thread);
 
@@ -806,7 +898,12 @@ export class ReviewCommentController extends CommentControllerBase implements Co
 				let startLine: number | undefined = undefined;
 				let endLine: number | undefined = undefined;
 				if (thread.range) {
-					if (side === DiffSide.RIGHT) {
+					if (prCommitDocument) {
+						// Commenting is only offered on a commit's document when the file is unchanged
+						// between that commit and the head, so the lines already match the head diff.
+						startLine = thread.range.start.line;
+						endLine = thread.range.end.line;
+					} else if (side === DiffSide.RIGHT) {
 						const diff = await this.getContentDiff(thread.uri, fileName);
 						startLine = mapNewPositionToOld(diff, thread.range.start.line);
 						endLine = mapNewPositionToOld(diff, thread.range.end.line);

src/view/treeNodes/commitNode.ts

@@ -51,6 +51,33 @@ export class CommitNode extends TreeNode implements vscode.TreeItem {
 		return this;
 	}
 
+	/**
+	 * Files that have been modified between this commit and the head of the pull request. Comments
+	 * can't be added to those from this view, since comments always go to the head. One git call for
+	 * the whole commit, rather than one per file.
+	 */
+	private async getFilesChangedSinceCommit(): Promise<Set<string>> {
+		const changedFiles = new Set<string>();
+		const headSha = this.pullRequest.head?.sha;
+		if (!headSha || headSha === this.commit.sha) {
+			return changedFiles;
+		}
+
+		try {
+			const changes = await this.pullRequestManager.repository.diffBetween(this.commit.sha, headSha);
+			for (const change of changes) {
+				changedFiles.add(this.pullRequestManager.gitRelativeRootPath(change.uri.path));
+				if (change.originalUri) {
+					changedFiles.add(this.pullRequestManager.gitRelativeRootPath(change.originalUri.path));
+				}
+			}
+		} catch (e) {
+			// Without this the file simply doesn't get marked, which is the pre-existing behavior.
+		}
+
+		return changedFiles;
+	}
+
 	override async getChildren(): Promise<TreeNode[]> {
 		super.getChildren();
 		const fileChanges = (await this.pullRequest.getCommitChangedFiles(this.commit)) ?? [];
@@ -59,6 +86,8 @@ export class CommitNode extends TreeNode implements vscode.TreeItem {
 			return [new LabelOnlyNode(this, 'No changed files')];
 		}
 
+		const changedSinceCommit = await this.getFilesChangedSinceCommit();
+
 		const fileChangeNodes = fileChanges.map(change => {
 			const fileName = change.filename!;
 			const uri = reviewPath(fileName, this.commit.sha);
@@ -99,6 +128,10 @@ export class CommitNode extends TreeNode implements vscode.TreeItem {
 
 			fileChangeNode.useViewChangesCommand();
 
+			if (changedSinceCommit.has(fileName)) {
+				fileChangeNode.markChangedSinceCommit(this.commit.sha);
+			}
+
 			return fileChangeNode;
 		});