From dc7571ba51a41f382976e76a12aa6e07cd70ec83 Mon Sep 17 00:00:00 2001 From: Patrick Cheung Date: Fri, 25 Sep 2026 17:38:27 +0800 Subject: [PATCH] feat(diff): open files with the external diff tool (#83) --- l10n/bundle.l10n.json | 1 + l10n/bundle.l10n.ko.json | 1 + l10n/bundle.l10n.zh-cn.json | 1 + .../git-service.external-diff.test.ts | 113 ++++++++++++++++++ src/git/git-service.ts | 51 ++++++++ src/panels/MainPanel.ts | 17 +++ src/panels/__tests__/MainPanel.test.ts | 16 +++ src/utils/message-bus.ts | 1 + .../components/commit/CommitDetails.svelte | 11 ++ .../commit/__tests__/CommitDetails.test.ts | 19 +++ webview-ui/src/lib/i18n/en.ts | 1 + webview-ui/src/lib/i18n/ko.ts | 1 + webview-ui/src/lib/i18n/zh.ts | 1 + 13 files changed, 234 insertions(+) create mode 100644 src/git/__tests__/git-service.external-diff.test.ts diff --git a/l10n/bundle.l10n.json b/l10n/bundle.l10n.json index b793b65c..79194869 100644 --- a/l10n/bundle.l10n.json +++ b/l10n/bundle.l10n.json @@ -113,5 +113,6 @@ "remoteBranchDeleted": "Remote branch '{0}/{1}' deleted", "remoteAdded": "Remote '{0}' added", "remoteRemoved": "Remote '{0}' removed", + "externalDiffFailed": "Failed to open external diff tool: {0}", "worktreeRemoved": "Worktree removed" } diff --git a/l10n/bundle.l10n.ko.json b/l10n/bundle.l10n.ko.json index c5dabd1c..2d67075f 100644 --- a/l10n/bundle.l10n.ko.json +++ b/l10n/bundle.l10n.ko.json @@ -113,5 +113,6 @@ "remoteBranchDeleted": "리모트 브랜치 '{0}/{1}'이(가) 삭제되었습니다", "remoteAdded": "리모트 '{0}'이(가) 추가되었습니다", "remoteRemoved": "리모트 '{0}'이(가) 제거되었습니다", + "externalDiffFailed": "외부 diff 도구를 열지 못했습니다: {0}", "worktreeRemoved": "Worktree가 제거되었습니다" } diff --git a/l10n/bundle.l10n.zh-cn.json b/l10n/bundle.l10n.zh-cn.json index 30b988df..cc684a44 100644 --- a/l10n/bundle.l10n.zh-cn.json +++ b/l10n/bundle.l10n.zh-cn.json @@ -113,5 +113,6 @@ "remoteBranchDeleted": "远程分支 '{0}/{1}' 已删除", "remoteAdded": "远程 '{0}' 已添加", "remoteRemoved": "远程 '{0}' 已移除", + "externalDiffFailed": "打开外部 diff 工具失败：{0}", "worktreeRemoved": "工作树已移除" } \ No newline at end of file diff --git a/src/git/__tests__/git-service.external-diff.test.ts b/src/git/__tests__/git-service.external-diff.test.ts new file mode 100644 index 00000000..e10566fc --- /dev/null +++ b/src/git/__tests__/git-service.external-diff.test.ts @@ -0,0 +1,113 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { EventEmitter } from 'events'; +import { GitService, GitError } from '../git-service'; + +// `git difftool` hands the comparison to a GUI tool and git stays alive until +// that tool exits. GitService.openExternalDiff must therefore spawn it detached +// and return as soon as the process has launched, never awaiting 'close'. +// Driving the spawned process by hand (same pattern as +// git-service.exec.test.ts) pins the argument construction, the +// first-parent / empty-tree base resolution, and the non-blocking contract. +import * as childProcess from 'child_process'; +vi.mock('child_process', () => ({ spawn: vi.fn() })); + +function fakeProc() { + const proc = new EventEmitter() as EventEmitter & { + stdout: EventEmitter; + stderr: EventEmitter; + stdin: { write: ReturnType; end: ReturnType }; + kill: ReturnType; + unref: ReturnType; + }; + proc.stdout = new EventEmitter(); + proc.stderr = new EventEmitter(); + proc.stdin = { write: vi.fn(), end: vi.fn() }; + proc.kill = vi.fn(); + proc.unref = vi.fn(); + return proc; +} + +const spawnMock = vi.mocked(childProcess.spawn); + +describe('GitService.openExternalDiff', () => { + let service: GitService; + let proc: ReturnType; + let spawnArgs: string[]; + + beforeEach(() => { + service = new GitService('C:\\repo'); + proc = fakeProc(); + spawnArgs = []; + spawnMock.mockImplementation((_bin: string, args: readonly string[]) => { + spawnArgs = [...args]; + // A real child emits 'spawn' asynchronously, after the caller has + // attached its listeners; emit on a microtask to mirror that. + queueMicrotask(() => proc.emit('spawn')); + return proc as unknown as ReturnType; + }); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it('spawns git difftool detached against the resolved first parent', async () => { + (service as any).exec = vi.fn(async (args: string[]) => { + if (args[0] === 'rev-parse' && args.includes('--verify')) return 'parentsha\n'; + throw new Error(`unexpected git call: ${args.join(' ')}`); + }); + + await service.openExternalDiff('abc123', 'assets/logo.bin'); + + expect(spawnArgs).toEqual([ + '-c', 'core.quotePath=false', + 'difftool', '--no-prompt', 'parentsha', 'abc123', '--', 'assets/logo.bin', + ]); + const opts = spawnMock.mock.calls[0][2] as { detached?: boolean; cwd?: string; stdio?: string }; + expect(opts.detached).toBe(true); + expect(opts.cwd).toBe('C:\\repo'); + expect(opts.stdio).toBe('ignore'); + // The GUI tool may outlive the extension host; the child must be unref'd. + expect(proc.unref).toHaveBeenCalled(); + }); + + it('does not wait for the GUI tool to exit (resolves while the process is still running)', async () => { + (service as any).exec = vi.fn(async () => 'parentsha\n'); + + // No 'close' is ever emitted by the mock, yet the call must resolve. + await expect(service.openExternalDiff('abc123', 'file.bin')).resolves.toBeUndefined(); + expect(proc.unref).toHaveBeenCalledTimes(1); + }); + + it('falls back to the empty tree for a root commit (no parent)', async () => { + (service as any).exec = vi.fn(async (args: string[]) => { + if (args[0] === 'rev-parse' && args.includes('--show-object-format')) return 'sha1\n'; + // The parent probe (`--verify --quiet`) exits non-zero for a root commit. + throw new GitError('', 1, args); + }); + + await service.openExternalDiff('root123', 'file.bin'); + + expect(spawnArgs).toContain('4b825dc642cb6eb9a060e54bf8d69288fbee4904'); + expect(spawnArgs).toContain('root123'); + }); + + it('rejects when the process cannot be spawned (git missing)', async () => { + (service as any).exec = vi.fn(async () => 'parentsha\n'); + spawnMock.mockImplementationOnce((_bin: string, args: readonly string[]) => { + spawnArgs = [...args]; + queueMicrotask(() => proc.emit('error', new Error('spawn git ENOENT'))); + return proc as unknown as ReturnType; + }); + + await expect(service.openExternalDiff('abc123', 'file.bin')).rejects.toThrow('ENOENT'); + expect(proc.unref).not.toHaveBeenCalled(); + }); + + it('rejects unsafe refs and paths before spawning', async () => { + await expect(service.openExternalDiff('--upload-pack=evil', 'file.bin')).rejects.toThrow(); + await expect(service.openExternalDiff('abc123', '../escape.bin')).rejects.toThrow(); + await expect(service.openExternalDiff('abc123', '-f')).rejects.toThrow(); + expect(spawnMock).not.toHaveBeenCalled(); + }); +}); diff --git a/src/git/git-service.ts b/src/git/git-service.ts index 71e65760..61540947 100644 --- a/src/git/git-service.ts +++ b/src/git/git-service.ts @@ -225,6 +225,57 @@ export class GitService { return GitService.EMPTY_TREE[format] ?? GitService.EMPTY_TREE.sha1; } + /** + * Hand a single file's change in a commit to git's configured external diff + * tool (#83) — the built-in editor can't render binary files. The base is the + * same one the built-in diff uses ({@link resolveDiffBaseRef}: first parent, + * empty tree for a root commit). + * + * `git difftool` opens a GUI and stays alive until the user closes it, so + * the process is spawned detached and this method resolves as soon as the + * launch succeeds — callers must not wait for the tool to exit. Spawn + * failures reject so the caller can report them. + */ + async openExternalDiff(hash: string, file: string): Promise { + this.assertSafeRef(hash, 'openExternalDiff'); + this.assertSafePath(file, 'openExternalDiff'); + + const base = await this.resolveDiffBaseRef(hash); + this.assertSafeRef(base, 'openExternalDiff'); + + // `--no-prompt` keeps git from stopping to ask which tool to use; the + // user's diff.tool / GIT_EXTERNAL_DIFF configuration decides the tool. A + // user without a difftool configured gets git's own fallback. + const args = ['difftool', '--no-prompt', base, hash, '--', file]; + + const proc = spawn(getGitBinaryPath(), ['-c', 'core.quotePath=false', ...args], { + cwd: this.repoPath, + env: { ...process.env, ...this.extraEnv, GIT_TERMINAL_PROMPT: '0', LC_ALL: 'C', GIT_MERGE_AUTOEDIT: 'no', GIT_EDITOR: 'true', EDITOR: 'true' }, + // Detach and discard stdio: neither the extension host nor VS Code's + // shutdown may be held open by a GUI the user leaves running, and an + // ignored stdio pipe means no data event can keep the loop alive. + detached: true, + stdio: 'ignore', + windowsHide: true, + }); + + await new Promise((resolve, reject) => { + let launched = false; + proc.once('spawn', () => { launched = true; resolve(); }); + proc.on('error', (err: Error) => { + if (launched) { + // Nothing awaits the process after launch; surface late failures + // (git exiting non-zero once the tool closes) as a warning. + this.warn(`external difftool failed: ${err.message}`); + return; + } + reject(new GitError(err.message, null, args)); + }); + }); + + proc.unref(); + } + /** * Whether `file` exists at `ref` (`ref === ''` checks the index, `:`). * Used to pick a diff side's base: a file that's absent at a ref — added or diff --git a/src/panels/MainPanel.ts b/src/panels/MainPanel.ts index 5869905d..fc98b044 100644 --- a/src/panels/MainPanel.ts +++ b/src/panels/MainPanel.ts @@ -736,6 +736,23 @@ export class MainPanel { } break; } + case 'openExternalDiff': { + // Hand one changed file to git's configured difftool (#83) — binary + // files in particular can't be shown in VS Code's text diff. The + // service spawns the GUI detached and returns once it has launched, + // so the only failure to report here is an invalid path or a failed + // launch (e.g. git missing). + try { + const file = this.assertSafeArgPath(message.payload.file, 'openExternalDiff'); + this.resolveRepoRelativePath(file, 'openExternalDiff'); + await this.gitService.openExternalDiff(message.payload.hash, file); + } catch (err) { + vscode.window.showErrorMessage( + vscode.l10n.t('externalDiffFailed', err instanceof Error ? err.message : String(err)), + ); + } + break; + } case 'openFile': { const fullPath = this.resolveRepoRelativePath(message.payload.file, 'openFile'); const fileUri = vscode.Uri.file(fullPath); diff --git a/src/panels/__tests__/MainPanel.test.ts b/src/panels/__tests__/MainPanel.test.ts index 76051ee3..0a0b482f 100644 --- a/src/panels/__tests__/MainPanel.test.ts +++ b/src/panels/__tests__/MainPanel.test.ts @@ -17,6 +17,7 @@ const H = vi.hoisted(() => { stashPop: vi.fn(async () => {}), showCommitDiff: vi.fn(async () => []), showCommitFiles: vi.fn(async () => []), + openExternalDiff: vi.fn(async () => {}), resolveDiffBaseRef: vi.fn(async () => 'parentsha'), getEmptyTreeRef: vi.fn(async () => '4b825dc642cb6eb9a060e54bf8d69288fbee4904'), fileExistsAtRef: vi.fn(async () => true), @@ -125,6 +126,7 @@ beforeEach(() => { H.git.getConflictFiles.mockResolvedValue([]); H.git.getRemoteUrl.mockResolvedValue(''); H.git.showCommitDiff.mockResolvedValue([]); + H.git.openExternalDiff.mockResolvedValue(undefined); H.git.fileExistsAtRef.mockResolvedValue(true); H.git.getEmptyTreeRef.mockResolvedValue('4b825dc642cb6eb9a060e54bf8d69288fbee4904'); H.repos = [{ path: '/repo', name: 'repo', type: 'root' }]; @@ -233,6 +235,20 @@ describe('MainPanel message routing', () => { expect(rightRef).toBe('2222222'); }); + it('openExternalDiff launches the difftool for the commit file', async () => { + await dispatch({ type: 'openExternalDiff', payload: { hash: 'h1', file: 'assets/logo.bin' } }); + expect(H.git.openExternalDiff).toHaveBeenCalledWith('h1', 'assets/logo.bin'); + }); + + it('openExternalDiff shows an error notification when the launch fails', async () => { + const vscode = await import('vscode'); + H.git.openExternalDiff.mockRejectedValueOnce(new Error('spawn git ENOENT')); + + await dispatch({ type: 'openExternalDiff', payload: { hash: 'h1', file: 'logo.bin' } }); + + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith('externalDiffFailed'); + }); + it('revealInExplorer resolves the repo path and runs revealFileInOS', async () => { const vscode = await import('vscode'); diff --git a/src/utils/message-bus.ts b/src/utils/message-bus.ts index deec2ab7..8713207a 100644 --- a/src/utils/message-bus.ts +++ b/src/utils/message-bus.ts @@ -67,6 +67,7 @@ export type WebviewMessage = | { type: 'addRemote'; payload: { name: string; url: string } } | { type: 'removeRemote'; payload: { name: string } } | { type: 'openDiff'; payload: { file: string; commitHash?: string; ref1?: string; ref2?: string; staged?: boolean } } + | { type: 'openExternalDiff'; payload: { hash: string; file: string } } | { type: 'openFile'; payload: { file: string } } | { type: 'revealInExplorer'; payload: { file: string } } | { type: 'copyFilePath'; payload: { file: string } } diff --git a/webview-ui/src/components/commit/CommitDetails.svelte b/webview-ui/src/components/commit/CommitDetails.svelte index a9eb9b45..67625ca6 100644 --- a/webview-ui/src/components/commit/CommitDetails.svelte +++ b/webview-ui/src/components/commit/CommitDetails.svelte @@ -956,6 +956,17 @@ }, }); + // External diff tool — the escape hatch for binary files the + // built-in text diff can't render (#83). Committed view only: + // the action needs a commit hash to diff against its parent. + if (commit) { + const hash = commit.hash; + items.push({ + label: t('file.openExternalDiff'), + action: () => { vscode.postMessage({ type: 'openExternalDiff', payload: { hash, file: node.path } }); fileContextMenu = null; }, + }); + } + // Reverse this file's change against the working tree. if (commit && canReverseInThisView) { const hash = commit.hash; diff --git a/webview-ui/src/components/commit/__tests__/CommitDetails.test.ts b/webview-ui/src/components/commit/__tests__/CommitDetails.test.ts index 9b04ec14..cb39d228 100644 --- a/webview-ui/src/components/commit/__tests__/CommitDetails.test.ts +++ b/webview-ui/src/components/commit/__tests__/CommitDetails.test.ts @@ -1003,6 +1003,25 @@ describe('CommitDetails — file context menu actions', () => { }); }); + it('"Open with external diff tool" posts openExternalDiff with hash and file', async () => { + const { container } = render(CommitDetails, { commit: commit({ hash: 'h1' }) }); + deliverCommitDiff('h1', [{ path: 'assets/logo.bin', status: 'M' }]); + await openMenu(container); + const item = Array.from(document.querySelectorAll('.context-menu button, .menu-item, [role="menuitem"]')) + .find(b => /external diff/i.test(b.textContent ?? ''))!; + expect(item).not.toBeUndefined(); + globalThis.__postedMessages = []; + await fireEvent.click(item); + const req = globalThis.__postedMessages.find( + (m) => (m.data as { type?: string }).type === 'openExternalDiff' + ); + expect(req).toBeDefined(); + expect((req!.data as { payload: unknown }).payload).toMatchObject({ + hash: 'h1', + file: 'assets/logo.bin', + }); + }); + it('LFS unlocked file shows Lock action and posts lfsLock', async () => { const { container } = render(CommitDetails, { commit: commit({ hash: 'h1' }) }); deliverCommitDiff('h1', [{ path: 'a.bin', status: 'M' }]); diff --git a/webview-ui/src/lib/i18n/en.ts b/webview-ui/src/lib/i18n/en.ts index 716013a7..1da9f151 100644 --- a/webview-ui/src/lib/i18n/en.ts +++ b/webview-ui/src/lib/i18n/en.ts @@ -633,6 +633,7 @@ export const en: Record = { // File context menu 'file.open': 'Open File', 'file.openChanges': 'Open Changes', + 'file.openExternalDiff': 'Open with External Diff Tool', 'file.revealInExplorer': 'Reveal in File Explorer', 'file.copyPath': 'Copy Path', 'file.copyRelativePath': 'Copy Relative Path', diff --git a/webview-ui/src/lib/i18n/ko.ts b/webview-ui/src/lib/i18n/ko.ts index 11378934..c91e910f 100644 --- a/webview-ui/src/lib/i18n/ko.ts +++ b/webview-ui/src/lib/i18n/ko.ts @@ -634,6 +634,7 @@ export const ko: Record = { 'lfs.locked': '{owner}가 잠금', 'file.open': '파일 열기', 'file.openChanges': '변경 내용 열기', + 'file.openExternalDiff': '외부 Diff 도구로 열기', 'file.revealInExplorer': '파일 탐색기에 표시', 'file.copyPath': '경로 복사', 'file.copyRelativePath': '상대 경로 복사', diff --git a/webview-ui/src/lib/i18n/zh.ts b/webview-ui/src/lib/i18n/zh.ts index 8d15a66e..eb8b18c5 100644 --- a/webview-ui/src/lib/i18n/zh.ts +++ b/webview-ui/src/lib/i18n/zh.ts @@ -634,6 +634,7 @@ export const zh: Record = { 'lfs.locked': '由 {owner} 锁定', 'file.open': '打开文件', 'file.openChanges': '打开更改', + 'file.openExternalDiff': '使用外部 Diff 工具打开', 'file.revealInExplorer': '在文件资源管理器中显示', 'file.copyPath': '复制路径', 'file.copyRelativePath': '复制相对路径',