diff --git a/src/commands.ts b/src/commands.ts index 79f4303e59..bc55ed0eea 100644 --- a/src/commands.ts +++ b/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); diff --git a/src/test/view/reviewCommentController.test.ts b/src/test/view/reviewCommentController.test.ts index 12e7e87a32..37b07ae8dd 100644 --- a/src/test/view/reviewCommentController.test.ts +++ b/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'; diff --git a/src/view/reviewCommentController.ts b/src/view/reviewCommentController.ts index 36c0641567..44ff5b540f 100644 --- a/src/view/reviewCommentController.ts +++ b/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 { + 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 { 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); diff --git a/src/view/treeNodes/commitNode.ts b/src/view/treeNodes/commitNode.ts index ee95b34a12..3c0b208e5c 100644 --- a/src/view/treeNodes/commitNode.ts +++ b/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> { + const changedFiles = new Set(); + 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 { 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; }); diff --git a/src/view/treeNodes/fileChangeNode.ts b/src/view/treeNodes/fileChangeNode.ts index 4e1e8ac0c6..038784ad93 100644 --- a/src/view/treeNodes/fileChangeNode.ts +++ b/src/view/treeNodes/fileChangeNode.ts @@ -93,7 +93,7 @@ export class FileChangeNode extends TreeNode implements vscode.TreeItem { return this.changeModel.sha; } - get tooltip(): string { + get tooltip(): string | vscode.MarkdownString | undefined { return this.resourceUri.fsPath; } @@ -339,6 +339,59 @@ export class GitFileChangeNode extends FileChangeNode implements vscode.TreeItem this._useViewChangesCommand = true; } + private _changedSinceCommit: string | undefined; + private _changedSinceTooltip: vscode.MarkdownString | undefined; + + /** + * Marks this file as one that has been modified between the commit being viewed and the head of + * the pull request. Comments always land on the head, so they can't be placed from this view. + */ + public markChangedSinceCommit(commit: string) { + this._changedSinceCommit = commit; + this.iconPath = new vscode.ThemeIcon('warning', new vscode.ThemeColor('problemsWarningIcon.foreground')); + } + + override get tooltip(): string | vscode.MarkdownString | undefined { + if (!this._changedSinceCommit) { + return super.tooltip; + } + + // Returning undefined until the tooltip has been built makes VS Code call `resolveTreeItem`, + // so the git lookup only happens for the file that is actually hovered. + return this._changedSinceTooltip; + } + + private async buildChangedSinceTooltip(commit: string): Promise { + const tooltip = new vscode.MarkdownString(undefined, true); + tooltip.isTrusted = true; + tooltip.appendMarkdown(vscode.l10n.t('$(warning) `{0}` has changed since commit {1}. Comments are always added to the latest version of a pull request, so they cannot be added from this view.', this.fileName, commit.substring(0, 8))); + + const headSha = this.pullRequest.head?.sha; + if (headSha) { + try { + const log = await this.pullRequestManager.repository.log({ + path: vscode.Uri.joinPath(this.pullRequestManager.repository.rootUri, this.fileName).fsPath, + range: `${commit}..${headSha}`, + maxEntries: 1, + }); + if (log.length) { + const message = log[0].message.split('\n')[0]; + tooltip.appendMarkdown('\n\n' + vscode.l10n.t('Last changed in {0}: {1}', log[0].hash.substring(0, 8), message)); + } + } catch (e) { + // The commit may not be available locally. The explanation above is still useful. + } + } + + const args = encodeURIComponent(JSON.stringify({ + rootPath: this.pullRequestManager.repository.rootUri.path, + fileName: this.fileName, + })); + tooltip.appendMarkdown(`\n\n[${vscode.l10n.t('Open the latest version of this file')}](command:pr.openCurrentFileChange?${args})`); + + return tooltip; + } + private async alternateCommand(): Promise { if (this.status === GitChangeType.DELETE || this.status === GitChangeType.ADD) { // create an empty `review` uri without any path/commit info. @@ -403,6 +456,10 @@ export class GitFileChangeNode extends FileChangeNode implements vscode.TreeItem } async resolve(): Promise { + if (this._changedSinceCommit && !this._changedSinceTooltip) { + this._changedSinceTooltip = await this.buildChangedSinceTooltip(this._changedSinceCommit); + } + if (this._useViewChangesCommand) { this.command = await this.alternateCommand(); } else {